feat: [AH-2867]: Frontend support to add tag to docker and helm artifact#93
Open
abhinavcode wants to merge 5 commits into
Open
feat: [AH-2867]: Frontend support to add tag to docker and helm artifact#93abhinavcode wants to merge 5 commits into
abhinavcode wants to merge 5 commits into
Conversation
📝 WalkthroughWalkthroughAdds a new VersionAction.AddTag and enables it for Docker and Helm versions; implements an RBAC-protected Add Tag menu item and Formik modal that normalizes/validates tags, calls the add-tags mutation, shows success/error toasts, invalidates queries, and closes the modal on success. (35 words) Changes
Sequence DiagramsequenceDiagram
participant UI as UI (VersionActions)
participant Modal as AddTagModalContent
participant API as AddTags API
participant Cache as QueryClient
UI->>UI: User selects "Add Tag"
UI->>Modal: showAddTagModal(artifactKey, repoKey, versionKey)
Modal->>Modal: User enters tags -> normalize & validate
alt valid
Modal->>API: POST add-tags (package, version, tags, queryParams)
API-->>Modal: success
Modal->>Cache: invalidate(GetAllHarnessArtifacts, ListVersions)
Modal-->>UI: hideModal()/onClose
Modal->>UI: show success toast
else invalid
Modal->>Modal: show validation error / show error toast
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
web/src/ar/pages/artifact-list/ArtifactListPage.tsx (1)
92-96: Extract the event name to a shared constant to prevent silent cross-file mismatch.The event string
'ar-refresh-artifact-list'is duplicated across two files (ArtifactListPage.tsx and AddTagMenuItem.tsx). Any typo in either location silently breaks the feature with no runtime error. Add to the existingweb/src/ar/constants.ts:export const AR_EVENTS = { REFRESH_ARTIFACT_LIST: 'ar-refresh-artifact-list', } as constThen update both the listener and emitter to use this constant. Additionally, the redundant handler wrapper can be removed and
refetchpassed directly to the event listener.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/ar/pages/artifact-list/ArtifactListPage.tsx` around lines 92 - 96, Extract the event name into the shared constant AR_EVENTS. Add the suggested AR_EVENTS constant (REFRESH_ARTIFACT_LIST) to web/src/ar/constants.ts, then in ArtifactListPage.tsx replace the literal 'ar-refresh-artifact-list' with AR_EVENTS.REFRESH_ARTIFACT_LIST and simplify the useEffect by passing refetch directly as the listener (remove the wrapper handler). Also update AddTagMenuItem.tsx to emit the event using AR_EVENTS.REFRESH_ARTIFACT_LIST instead of the hard-coded string so both emitter and listener use the same constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@web/src/ar/pages/version-details/components/VersionActions/AddTagMenuItem.module.scss`:
- Around line 17-22: The Stylelint rule selector-pseudo-class-no-unknown is
tripping on the :global(.bp3-input) selector inside the .addTagInputWrapper
block; add a local suppression comment immediately before the offending line —
e.g. place /* stylelint-disable-next-line selector-pseudo-class-no-unknown */
above :global(.bp3-input) — so the CSS module global selector is preserved while
the linter is skipped for that line.
In
`@web/src/ar/pages/version-details/components/VersionActions/VersionActions.tsx`:
- Around line 214-225: The Add Tag action is not currently hidden for public
sessions; wrap the AddTagMenuItem render with the same public-session guard used
for other mutating actions so it is not shown to public users. Update the JSX
condition to require both isAllowed(VersionAction.AddTag) and
!isCurrentSessionPublic (or the existing helper used elsewhere) before rendering
AddTagMenuItem (keeping artifactKey, repoKey, versionKey, data, pageType,
readonly, onClose, openAddTagModal props unchanged).
---
Nitpick comments:
In `@web/src/ar/pages/artifact-list/ArtifactListPage.tsx`:
- Around line 92-96: Extract the event name into the shared constant AR_EVENTS.
Add the suggested AR_EVENTS constant (REFRESH_ARTIFACT_LIST) to
web/src/ar/constants.ts, then in ArtifactListPage.tsx replace the literal
'ar-refresh-artifact-list' with AR_EVENTS.REFRESH_ARTIFACT_LIST and simplify the
useEffect by passing refetch directly as the listener (remove the wrapper
handler). Also update AddTagMenuItem.tsx to emit the event using
AR_EVENTS.REFRESH_ARTIFACT_LIST instead of the hard-coded string so both emitter
and listener use the same constant.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
web/src/ar/pages/artifact-list/ArtifactListPage.tsxweb/src/ar/pages/version-details/DockerVersion/DockerVersionType.tsxweb/src/ar/pages/version-details/HelmVersion/HelmVersionType.tsxweb/src/ar/pages/version-details/components/VersionActions/AddTagMenuItem.module.scssweb/src/ar/pages/version-details/components/VersionActions/AddTagMenuItem.tsxweb/src/ar/pages/version-details/components/VersionActions/VersionActions.tsxweb/src/ar/pages/version-details/components/VersionActions/types.tsweb/src/ar/pages/version-list/strings/strings.en.yamlweb/src/ar/strings/types.ts
Comment on lines
+17
to
+22
| /* Fixed width for the tag input bar only when rendered inside Add Tag modal */ | ||
| .addTagInputWrapper { | ||
| :global(.bp3-input) { | ||
| min-width: 400px; | ||
| max-width: 400px; | ||
| } |
There was a problem hiding this comment.
Stylelint error for :global needs a fix.
The current selector trips selector-pseudo-class-no-unknown, so the lint step will fail unless the rule is configured to allow :global.
✅ One quick fix (local suppression)
.addTagInputWrapper {
+ /* stylelint-disable-next-line selector-pseudo-class-no-unknown */
:global(.bp3-input) {
min-width: 400px;
max-width: 400px;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /* Fixed width for the tag input bar only when rendered inside Add Tag modal */ | |
| .addTagInputWrapper { | |
| :global(.bp3-input) { | |
| min-width: 400px; | |
| max-width: 400px; | |
| } | |
| /* Fixed width for the tag input bar only when rendered inside Add Tag modal */ | |
| .addTagInputWrapper { | |
| /* stylelint-disable-next-line selector-pseudo-class-no-unknown */ | |
| :global(.bp3-input) { | |
| min-width: 400px; | |
| max-width: 400px; | |
| } | |
| } |
🧰 Tools
🪛 Stylelint (17.3.0)
[error] 19-19: Unexpected unknown pseudo-class selector ":global" (selector-pseudo-class-no-unknown)
(selector-pseudo-class-no-unknown)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@web/src/ar/pages/version-details/components/VersionActions/AddTagMenuItem.module.scss`
around lines 17 - 22, The Stylelint rule selector-pseudo-class-no-unknown is
tripping on the :global(.bp3-input) selector inside the .addTagInputWrapper
block; add a local suppression comment immediately before the offending line —
e.g. place /* stylelint-disable-next-line selector-pseudo-class-no-unknown */
above :global(.bp3-input) — so the CSS module global selector is preserved while
the linter is skipped for that line.
Comment on lines
+214
to
+225
| {isAllowed(VersionAction.AddTag) && ( | ||
| <AddTagMenuItem | ||
| artifactKey={artifactKey} | ||
| repoKey={repoKey} | ||
| versionKey={versionKey} | ||
| data={data} | ||
| pageType={pageType} | ||
| readonly={readonly} | ||
| onClose={closeMenu} | ||
| openAddTagModal={showAddTagModal} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
Hide Add Tag in public sessions for consistency.
Other mutating actions are gated by isCurrentSessionPublic, but Add Tag is currently visible. This can expose an action that will fail for public users.
Suggested guard
- {isAllowed(VersionAction.AddTag) && (
+ {!isCurrentSessionPublic && isAllowed(VersionAction.AddTag) && (
<AddTagMenuItem
artifactKey={artifactKey}
repoKey={repoKey}
versionKey={versionKey}
data={data}
pageType={pageType}
readonly={readonly}
onClose={closeMenu}
openAddTagModal={showAddTagModal}
/>
)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {isAllowed(VersionAction.AddTag) && ( | |
| <AddTagMenuItem | |
| artifactKey={artifactKey} | |
| repoKey={repoKey} | |
| versionKey={versionKey} | |
| data={data} | |
| pageType={pageType} | |
| readonly={readonly} | |
| onClose={closeMenu} | |
| openAddTagModal={showAddTagModal} | |
| /> | |
| )} | |
| {!isCurrentSessionPublic && isAllowed(VersionAction.AddTag) && ( | |
| <AddTagMenuItem | |
| artifactKey={artifactKey} | |
| repoKey={repoKey} | |
| versionKey={versionKey} | |
| data={data} | |
| pageType={pageType} | |
| readonly={readonly} | |
| onClose={closeMenu} | |
| openAddTagModal={showAddTagModal} | |
| /> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@web/src/ar/pages/version-details/components/VersionActions/VersionActions.tsx`
around lines 214 - 225, The Add Tag action is not currently hidden for public
sessions; wrap the AddTagMenuItem render with the same public-session guard used
for other mutating actions so it is not shown to public users. Update the JSX
condition to require both isAllowed(VersionAction.AddTag) and
!isCurrentSessionPublic (or the existing helper used elsewhere) before rendering
AddTagMenuItem (keeping artifactKey, repoKey, versionKey, data, pageType,
readonly, onClose, openAddTagModal props unchanged).
There was a problem hiding this comment.
♻️ Duplicate comments (2)
web/src/ar/pages/version-details/components/VersionActions/AddTagMenuItem.module.scss (1)
18-25:⚠️ Potential issue | 🟠 MajorFix Stylelint violations in the global input selector block.
Both the
:globalselector and the blank line beforebox-sizingwill fail Stylelint. Add a local suppression for:globaland remove the empty line.🔧 Proposed fix
.addTagInputWrapper { + /* stylelint-disable-next-line selector-pseudo-class-no-unknown */ :global(.bp3-input) { width: min(400px, 90vw); min-width: min(400px, 90vw); max-width: 100%; - box-sizing: border-box; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/ar/pages/version-details/components/VersionActions/AddTagMenuItem.module.scss` around lines 18 - 25, The Stylelint errors come from the `:global(.bp3-input)` selector and the stray blank line before `box-sizing` inside the `.addTagInputWrapper` block; update the `.addTagInputWrapper` ruleset by adding a local Stylelint suppression comment for the `:global` selector (e.g., /* stylelint-disable-next-line selector-pseudo-class-no-unknown */) immediately above the `:global(.bp3-input)` line and remove the empty line so `box-sizing: border-box;` sits directly after the other declarations, keeping the selector and declarations otherwise unchanged.web/src/ar/pages/version-details/components/VersionActions/VersionActions.tsx (1)
214-225:⚠️ Potential issue | 🟡 MinorHide Add Tag in public sessions to match other mutating actions.
Add Tag is still shown when
isCurrentSessionPublicis true, while other mutating actions are hidden.✅ Suggested guard
- {isAllowed(VersionAction.AddTag) && ( + {!isCurrentSessionPublic && isAllowed(VersionAction.AddTag) && ( <AddTagMenuItem artifactKey={artifactKey} repoKey={repoKey} versionKey={versionKey} data={data} pageType={pageType} readonly={readonly} onClose={closeMenu} openAddTagModal={showAddTagModal} /> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/ar/pages/version-details/components/VersionActions/VersionActions.tsx` around lines 214 - 225, The AddTagMenuItem is still rendered even for public sessions; update the render guard in VersionActions (around the isAllowed(VersionAction.AddTag) check) to also check that the session is not public (e.g., add a "!isCurrentSessionPublic" condition) so it matches the other mutating actions' behavior; locate the JSX that renders AddTagMenuItem in VersionActions.tsx and add the same public-session guard used for other mutating menu items.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@web/src/ar/pages/version-details/components/VersionActions/AddTagMenuItem.module.scss`:
- Around line 18-25: The Stylelint errors come from the `:global(.bp3-input)`
selector and the stray blank line before `box-sizing` inside the
`.addTagInputWrapper` block; update the `.addTagInputWrapper` ruleset by adding
a local Stylelint suppression comment for the `:global` selector (e.g., /*
stylelint-disable-next-line selector-pseudo-class-no-unknown */) immediately
above the `:global(.bp3-input)` line and remove the empty line so `box-sizing:
border-box;` sits directly after the other declarations, keeping the selector
and declarations otherwise unchanged.
In
`@web/src/ar/pages/version-details/components/VersionActions/VersionActions.tsx`:
- Around line 214-225: The AddTagMenuItem is still rendered even for public
sessions; update the render guard in VersionActions (around the
isAllowed(VersionAction.AddTag) check) to also check that the session is not
public (e.g., add a "!isCurrentSessionPublic" condition) so it matches the other
mutating actions' behavior; locate the JSX that renders AddTagMenuItem in
VersionActions.tsx and add the same public-session guard used for other mutating
menu items.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
web/src/ar/pages/version-details/components/VersionActions/AddTagMenuItem.module.scssweb/src/ar/pages/version-details/components/VersionActions/AddTagMenuItem.tsxweb/src/ar/pages/version-details/components/VersionActions/VersionActions.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@web/src/ar/pages/version-details/components/VersionActions/AddTagMenuItem.tsx`:
- Around line 70-77: The code is overwriting the base path and leaving debug
logs; replace the current URL construction so it preserves any API base prefix
and remove console.log lines. Instead of creating baseUrl from getApiBaseUrl('')
and then assigning baseUrl.pathname = path, call getApiBaseUrl with the full
path (e.g., const baseUrl = new URL(getApiBaseUrl(path),
window.location.origin)) so existing prefixes like /gateway or /api/v1 are
preserved, then set search params on baseUrl and call baseUrl.toString() to get
url; also delete the console.log('baseUrl', ...) and console.log('baseUrl after
adding path', ...) statements.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web/src/ar/pages/version-details/components/VersionActions/AddTagMenuItem.tsxweb/src/ar/pages/version-details/components/VersionActions/VersionActions.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/ar/pages/version-details/components/VersionActions/VersionActions.tsx
Comment on lines
+70
to
+77
| const path = '/har/api/v2/oci/tags' | ||
| const baseUrl = new URL(getApiBaseUrl(''), window.location.origin) | ||
| console.log('baseUrl', baseUrl); | ||
| baseUrl.pathname = path | ||
| console.log('baseUrl after adding path', baseUrl); | ||
| baseUrl.searchParams.set('account_identifier', accountId || '') | ||
| baseUrl.searchParams.set('registry_identifier', repoKey) | ||
| const url = baseUrl.toString() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify getApiBaseUrl signature and intended base path behavior
rg -n "getApiBaseUrl" -C3Repository: abhinavcode/harness
Length of output: 9384
🏁 Script executed:
#!/bin/bash
# View the actual file content around the problematic code
cat -n web/src/ar/pages/version-details/components/VersionActions/AddTagMenuItem.tsx | sed -n '60,85p'Repository: abhinavcode/harness
Length of output: 1333
🏁 Script executed:
#!/bin/bash
# Check how getApiBaseUrl is used elsewhere in codebase to understand intended pattern
rg "getApiBaseUrl\(" -A2 | head -60Repository: abhinavcode/harness
Length of output: 1934
Remove debug logs and preserve base path when building the tags URL.
Assigning baseUrl.pathname = path discards any base prefix from getApiBaseUrl (e.g., /gateway, /api/v1), causing requests to route to the wrong endpoint in non-root deployments. The pattern used elsewhere in the codebase (e.g., useGetDownloadFileURL.ts, useOpenApiClient.ts) is to pass the full path directly to getApiBaseUrl. Additionally, the console.log calls on lines 72 and 74 are debug statements that should not ship.
🔧 Suggested fix
- const path = '/har/api/v2/oci/tags'
- const baseUrl = new URL(getApiBaseUrl(''), window.location.origin)
- console.log('baseUrl', baseUrl);
- baseUrl.pathname = path
- console.log('baseUrl after adding path', baseUrl);
- baseUrl.searchParams.set('account_identifier', accountId || '')
- baseUrl.searchParams.set('registry_identifier', repoKey)
- const url = baseUrl.toString()
+ const path = 'har/api/v2/oci/tags'
+ const baseUrl = new URL(getApiBaseUrl(''), window.location.origin)
+ if (!baseUrl.pathname.endsWith('/')) baseUrl.pathname += '/'
+ const urlObj = new URL(path, baseUrl)
+ urlObj.searchParams.set('account_identifier', accountId || '')
+ urlObj.searchParams.set('registry_identifier', repoKey)
+ const url = urlObj.toString()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const path = '/har/api/v2/oci/tags' | |
| const baseUrl = new URL(getApiBaseUrl(''), window.location.origin) | |
| console.log('baseUrl', baseUrl); | |
| baseUrl.pathname = path | |
| console.log('baseUrl after adding path', baseUrl); | |
| baseUrl.searchParams.set('account_identifier', accountId || '') | |
| baseUrl.searchParams.set('registry_identifier', repoKey) | |
| const url = baseUrl.toString() | |
| const path = 'har/api/v2/oci/tags' | |
| const baseUrl = new URL(getApiBaseUrl(''), window.location.origin) | |
| if (!baseUrl.pathname.endsWith('/')) baseUrl.pathname += '/' | |
| const urlObj = new URL(path, baseUrl) | |
| urlObj.searchParams.set('account_identifier', accountId || '') | |
| urlObj.searchParams.set('registry_identifier', repoKey) | |
| const url = urlObj.toString() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@web/src/ar/pages/version-details/components/VersionActions/AddTagMenuItem.tsx`
around lines 70 - 77, The code is overwriting the base path and leaving debug
logs; replace the current URL construction so it preserves any API base prefix
and remove console.log lines. Instead of creating baseUrl from getApiBaseUrl('')
and then assigning baseUrl.pathname = path, call getApiBaseUrl with the full
path (e.g., const baseUrl = new URL(getApiBaseUrl(path),
window.location.origin)) so existing prefixes like /gateway or /api/v1 are
preserved, then set search params on baseUrl and call baseUrl.toString() to get
url; also delete the console.log('baseUrl', ...) and console.log('baseUrl after
adding path', ...) statements.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
web/src/ar/pages/version-details/components/VersionActions/AddTagMenuItem.tsx (3)
25-26: Consolidate duplicate imports from the same module.
ResourceTypeandPermissionIdentifierare imported from@ar/common/permissionTypeson two separate lines.♻️ Proposed fix
-import { ResourceType } from '@ar/common/permissionTypes' -import { PermissionIdentifier } from '@ar/common/permissionTypes' +import { ResourceType, PermissionIdentifier } from '@ar/common/permissionTypes'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/ar/pages/version-details/components/VersionActions/AddTagMenuItem.tsx` around lines 25 - 26, The imports for ResourceType and PermissionIdentifier are duplicated from the same module; consolidate them into a single import statement by combining the two separate import lines into one that imports both ResourceType and PermissionIdentifier from '@ar/common/permissionTypes' (locate the import statements referencing ResourceType and PermissionIdentifier in AddTagMenuItem.tsx and merge them).
104-104:enableReinitializehas no effect here — remove it.
initialValuesis a static literal ({ tagNames: [] as string[] }) that never changes between renders, soenableReinitializeis a no-op and may mislead future readers into thinking re-initialization is intentional.♻️ Proposed fix
- <Formik initialValues={{ tagNames: [] as string[] }} enableReinitialize onSubmit={() => undefined}> + <Formik initialValues={{ tagNames: [] as string[] }} onSubmit={() => undefined}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/ar/pages/version-details/components/VersionActions/AddTagMenuItem.tsx` at line 104, Remove the unnecessary enableReinitialize prop from the Formik usage in the AddTagMenuItem component: the Formik call in AddTagMenuItem.tsx currently sets initialValues to a static literal ({ tagNames: [] as string[] }) so enableReinitialize is a no-op; delete the enableReinitialize attribute from the Formik element (the component that wraps the form and currently has initialValues and onSubmit) to avoid misleading readers.
115-119: Fragile DOM query to capture uncommittedPatternInputvalue.Reading the raw
<input>value viainputWrapperRef.current?.querySelector('input')?.valuebypasses React and Formik. This pattern is brittle and will break ifSimpleTagInput's internal DOM structure changes. SincePatternInputwrapsMultiTagsInputwhich wrapsSimpleTagInput(from@harnessio/uicore), the uncommitted input state is inaccessible through the component API. Consider refactoringPatternInputto expose the uncommitted input value either as a forwarded ref callback or a controlled prop, so the complete form state stays within React's data flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/ar/pages/version-details/components/VersionActions/AddTagMenuItem.tsx` around lines 115 - 119, The onClick handler in AddTagMenuItem currently reads the uncommitted PatternInput value via a fragile DOM query (inputWrapperRef.current?.querySelector('input')?.value), which bypasses React/Formik; refactor PatternInput (the component wrapping MultiTagsInput/SimpleTagInput) to expose the uncommitted input either via a forwarded ref method (e.g., getUncommittedValue) or a controlled prop (e.g., uncommittedValue + onUncommittedChange), then update the AddTagMenuItem onClick to read that value from the new ref/mapped prop and pass it to handleSubmit(formik.values.tagNames, uncommittedValue) instead of querying the DOM; ensure PatternInput forwards the API through MultiTagsInput/SimpleTagInput so the component tree remains controlled by React/Formik.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@web/src/ar/pages/version-details/components/VersionActions/AddTagMenuItem.tsx`:
- Line 68: The file contains a leftover debug statement: remove the
console.log('allTags', allTags); from the AddTagMenuItem component so it no
longer prints to the console; search for the AddTagMenuItem component or the
allTags variable in AddTagMenuItem.tsx and delete that single console.log line
(or replace it with appropriate debug-level logging via existing logger if
needed).
---
Nitpick comments:
In
`@web/src/ar/pages/version-details/components/VersionActions/AddTagMenuItem.tsx`:
- Around line 25-26: The imports for ResourceType and PermissionIdentifier are
duplicated from the same module; consolidate them into a single import statement
by combining the two separate import lines into one that imports both
ResourceType and PermissionIdentifier from '@ar/common/permissionTypes' (locate
the import statements referencing ResourceType and PermissionIdentifier in
AddTagMenuItem.tsx and merge them).
- Line 104: Remove the unnecessary enableReinitialize prop from the Formik usage
in the AddTagMenuItem component: the Formik call in AddTagMenuItem.tsx currently
sets initialValues to a static literal ({ tagNames: [] as string[] }) so
enableReinitialize is a no-op; delete the enableReinitialize attribute from the
Formik element (the component that wraps the form and currently has
initialValues and onSubmit) to avoid misleading readers.
- Around line 115-119: The onClick handler in AddTagMenuItem currently reads the
uncommitted PatternInput value via a fragile DOM query
(inputWrapperRef.current?.querySelector('input')?.value), which bypasses
React/Formik; refactor PatternInput (the component wrapping
MultiTagsInput/SimpleTagInput) to expose the uncommitted input either via a
forwarded ref method (e.g., getUncommittedValue) or a controlled prop (e.g.,
uncommittedValue + onUncommittedChange), then update the AddTagMenuItem onClick
to read that value from the new ref/mapped prop and pass it to
handleSubmit(formik.values.tagNames, uncommittedValue) instead of querying the
DOM; ensure PatternInput forwards the API through MultiTagsInput/SimpleTagInput
so the component tree remains controlled by React/Formik.
| const committed = normalizeTagNames(tagNamesRaw) | ||
| const current = currentInputValue?.trim() ?? '' | ||
| const allTags = current ? [...committed, current] : committed | ||
| console.log('allTags', allTags); |
There was a problem hiding this comment.
Remove leftover console.log debug statement.
🔧 Proposed fix
- console.log('allTags', allTags);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| console.log('allTags', allTags); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@web/src/ar/pages/version-details/components/VersionActions/AddTagMenuItem.tsx`
at line 68, The file contains a leftover debug statement: remove the
console.log('allTags', allTags); from the AddTagMenuItem component so it no
longer prints to the console; search for the AddTagMenuItem component or the
allTags variable in AddTagMenuItem.tsx and delete that single console.log line
(or replace it with appropriate debug-level logging via existing logger if
needed).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Localization
Style