feat: [AH-2731]: modify based on api schema changes#77
Open
abhinavcode wants to merge 2 commits into
Open
Conversation
📝 WalkthroughWalkthroughThis PR updates the artifact registry components to use V3 API types and functions instead of V2 variants, following a dependency upgrade to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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: 1
🧹 Nitpick comments (2)
web/src/ar/pages/version-details/components/VersionActions/ReEvaluateMenuItem.tsx (1)
57-59:handleAfterReScanruns unconditionally — confirmonCloseon error is intentional.The
finallyblock callshandleAfterReScan()on both success and failure paths. This means:
- Three query invalidations fire even when the re-evaluation API call fails (triggering unnecessary network refetches).
onClose?.()dismisses the menu while the error toast is still being shown, preventing the user from retrying without reopening the context menu.If closing on error is intentional, query invalidations should be moved to the
.then()block only:♻️ Suggested adjustment if close-on-error is unintended
- .catch((err: ErrorV3) => { + .catch((err: ErrorV3) => { clear() showError(err?.error?.message ?? getString('versionList.messages.reEvaluateFailed')) }) - .finally(() => { - handleAfterReScan() - }) + .then(() => { + handleAfterReScan() + })🤖 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/ReEvaluateMenuItem.tsx` around lines 57 - 59, The current .finally(() => { handleAfterReScan() }) causes handleAfterReScan (which invalidates queries and calls onClose) to run on both success and failure; move the handleAfterReScan() call into the promise .then() handler of the re-evaluation mutation so it only runs after a successful re-eval, and ensure onClose?.() is likewise only invoked on success; if you intended to close on error instead, keep onClose in the error path but do not call handleAfterReScan there—adjust inside the reEvaluateMutation promise handlers accordingly, referencing handleAfterReScan, onClose, and the re-evaluate mutation call.web/src/ar/pages/violations-list/components/ViolationDetailsContent/ViolationDetails.tsx (1)
64-77: Pre-existing:keyshould be on the fragment, not onCollapse.The
keyprop is on<Collapse>(line 67) but the outermost element in the.map()is a<>fragment (line 65), which doesn't acceptkey. This causes a React warning. Use<React.Fragment key={...}>instead.Suggested fix
{props.data.policySetFailureDetails.map(policySet => ( - <> + <React.Fragment key={policySet.policySetRef}> <Collapse - key={policySet.policySetRef} expandedIcon="chevron-down" collapsedIcon="chevron-right" collapseClassName={css.collapseMain} isOpen heading={<PolicySetCollapseTitle data={policySet} />}> <ViolationFailureDetails data={policySet} fixVersionDetails={props.data.fixVersionDetails} /> </Collapse> <Separator /> - </> + </React.Fragment> ))}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/ar/pages/violations-list/components/ViolationDetailsContent/ViolationDetails.tsx` around lines 64 - 77, The key prop is currently applied to the inner Collapse but the outermost element returned by props.data.policySetFailureDetails.map is a fragment (<>), causing a React key warning; change the fragment to an explicit React.Fragment and move the key to it using the unique identifier policySet.policySetRef so that React can track the list items (i.e., wrap Collapse, Separator and related children in React.Fragment with key={policySet.policySetRef} and remove the key from the Collapse component), referencing the map over props.data.policySetFailureDetails and the components Collapse, PolicySetCollapseTitle, ViolationFailureDetails, and Separator.
🤖 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/violations-list/components/ViolationDetailsContent/InformationMetrics.tsx`:
- Line 22: InformationMetrics now passes ArtifactScanV3['scanStatus'] into
ScanBadge but ScanBadge's prop types still expect
ArtifactVersionSummary['scanStatus'], causing a TypeScript mismatch; update
ScanBadge's prop/interface to accept the V3 scanStatus type
(ArtifactScanV3['scanStatus']) or a union that includes it, adjust the ScanBadge
component signature/props and any related prop typings/usage to reference
ArtifactScanV3 (or ArtifactScanV3 | ArtifactVersionSummary) so
InformationMetrics can pass its value without type errors while
ScanCategoryBadge can remain unchanged.
---
Nitpick comments:
In
`@web/src/ar/pages/version-details/components/VersionActions/ReEvaluateMenuItem.tsx`:
- Around line 57-59: The current .finally(() => { handleAfterReScan() }) causes
handleAfterReScan (which invalidates queries and calls onClose) to run on both
success and failure; move the handleAfterReScan() call into the promise .then()
handler of the re-evaluation mutation so it only runs after a successful
re-eval, and ensure onClose?.() is likewise only invoked on success; if you
intended to close on error instead, keep onClose in the error path but do not
call handleAfterReScan there—adjust inside the reEvaluateMutation promise
handlers accordingly, referencing handleAfterReScan, onClose, and the
re-evaluate mutation call.
In
`@web/src/ar/pages/violations-list/components/ViolationDetailsContent/ViolationDetails.tsx`:
- Around line 64-77: The key prop is currently applied to the inner Collapse but
the outermost element returned by props.data.policySetFailureDetails.map is a
fragment (<>), causing a React key warning; change the fragment to an explicit
React.Fragment and move the key to it using the unique identifier
policySet.policySetRef so that React can track the list items (i.e., wrap
Collapse, Separator and related children in React.Fragment with
key={policySet.policySetRef} and remove the key from the Collapse component),
referencing the map over props.data.policySetFailureDetails and the components
Collapse, PolicySetCollapseTitle, ViolationFailureDetails, and Separator.
| import { Layout, Text } from '@harnessio/uicore' | ||
| import { Color, FontVariation } from '@harnessio/design-system' | ||
| import type { ArtifactScan, PolicyFailureDetailCategory } from '@harnessio/react-har-service-client' | ||
| import type { ArtifactScanV3, PolicyFailureDetailCategoryV3 } from '@harnessio/react-har-service-client' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "(ScanBadge|ScanCategoryBadge)" --extension tsx --extension tsRepository: abhinavcode/harness
Length of output: 154
🏁 Script executed:
cat -n web/src/ar/components/Badge/ScanBadge.tsxRepository: abhinavcode/harness
Length of output: 3795
🏁 Script executed:
cat -n web/src/ar/components/Badge/ScanCategoryBadge.tsxRepository: abhinavcode/harness
Length of output: 2030
🏁 Script executed:
cat -n web/src/ar/pages/violations-list/components/ViolationDetailsContent/InformationMetrics.tsxRepository: abhinavcode/harness
Length of output: 5879
🏁 Script executed:
# Verify what types these are in the service client to understand compatibility
rg "type ArtifactScanV3|interface ArtifactScanV3" -A 3 --type ts --type tsx 2>/dev/null || echo "Checking in node_modules or package..."
# Also check ArtifactVersionSummary
rg "type ArtifactVersionSummary|interface ArtifactVersionSummary" -A 3 --type ts --type tsx 2>/dev/null || echo "Not found in repo"Repository: abhinavcode/harness
Length of output: 119
Type mismatch in ScanBadge: incompatible with V3 migration
The downstream component ScanBadge expects ArtifactVersionSummary['scanStatus'] but InformationMetrics is now passing ArtifactScanV3['scanStatus'] (line 138). This type mismatch will cause TypeScript compilation errors. Update ScanBadge to accept the V3 type.
ScanCategoryBadge correctly accepts PolicyFailureDetailCategoryV3 and is compatible.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@web/src/ar/pages/violations-list/components/ViolationDetailsContent/InformationMetrics.tsx`
at line 22, InformationMetrics now passes ArtifactScanV3['scanStatus'] into
ScanBadge but ScanBadge's prop types still expect
ArtifactVersionSummary['scanStatus'], causing a TypeScript mismatch; update
ScanBadge's prop/interface to accept the V3 scanStatus type
(ArtifactScanV3['scanStatus']) or a union that includes it, adjust the ScanBadge
component signature/props and any related prop typings/usage to reference
ArtifactScanV3 (or ArtifactScanV3 | ArtifactVersionSummary) so
InformationMetrics can pass its value without type errors while
ScanCategoryBadge can remain unchanged.
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
2 participants
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