fix: [AH-2866]: Deletion statements updated at all 3 registry,package and version level#89
fix: [AH-2866]: Deletion statements updated at all 3 registry,package and version level#89abhinavcode wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughAdds HTML-entity decoding for displayed identifiers, uses decoded values as modal confirmation inputs, exposes a decode utility, extends version deletion to accept OCI digests, and updates the delete modal component to accept an optional input label value for augmented labels. Changes
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/ar/pages/version-details/components/VersionActions/DeleteVersionMenuItem.tsx (1)
28-45:⚠️ Potential issue | 🟡 MinorAvoid
as any— use precise types forpackageTypeanddigest.Two type-safety gaps here:
- Line 44:
data?.packageType as anyshould bedata?.packageType as RepositoryPackageType(which is already imported byuseDeleteVersionModal). Usingas anysilently bypasses the enum constraint.- Line 28 /
VersionActionProps:digest?: anyshould bedigest?: stringto match theuseDeleteVersionModalprop signature, preventing non-string values from slipping through at the call site.🛠️ Proposed fix
In
types.ts(VersionActionProps):- digest?: any + digest?: stringIn
DeleteVersionMenuItem.tsx:- packageType: data?.packageType as any, + packageType: data?.packageType as RepositoryPackageType,
RepositoryPackageTypecan be imported from@ar/common/types(already used inuseDeleteVersionModal).🤖 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/DeleteVersionMenuItem.tsx` around lines 28 - 45, Update the types and call-site to remove unsafe any casts: change VersionActionProps.digest from digest?: any to digest?: string, and in DeleteVersionMenuItem replace data?.packageType as any with data?.packageType typed as RepositoryPackageType (import RepositoryPackageType from `@ar/common/types` if needed) when calling useDeleteVersionModal; ensure useDeleteVersionModal invocation matches its prop types (artifactKey, repoKey, versionKey, artifactType, packageType: RepositoryPackageType, digest: string | undefined) so the enum constraint and string-typed digest are enforced.
🧹 Nitpick comments (1)
web/src/ar/pages/repository-details/hooks/useDeleteRepositoryModal/useDeleteRepositoryModal.tsx (1)
26-31: ExtractdecodeHtmlEntitiesto a shared utility.This function is copy-pasted verbatim in
useDeleteArtifactModal.tsxanduseDeleteVersionModal.tsx. It should live in a single shared location (e.g.,@ar/utils/htmlUtils.ts) and be imported where needed.♻️ Proposed refactor
Create
web/src/ar/utils/htmlUtils.ts:+/** + * Decodes HTML entities in a string using a temporary textarea element. + * Safe for browser environments only. + */ +export const decodeHtmlEntities = (text: string): string => { + const textarea = document.createElement('textarea') + textarea.innerHTML = text + return textarea.value +}Then in all three hook files, replace the local definition with:
-// Helper function to decode HTML entities -const decodeHtmlEntities = (text: string): string => { - const textarea = document.createElement('textarea') - textarea.innerHTML = text - return textarea.value -} +import { decodeHtmlEntities } from '@ar/utils/htmlUtils'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/ar/pages/repository-details/hooks/useDeleteRepositoryModal/useDeleteRepositoryModal.tsx` around lines 26 - 31, The local helper decodeHtmlEntities is duplicated across useDeleteRepositoryModal, useDeleteArtifactModal, and useDeleteVersionModal; extract it into a single shared utility (e.g., create and export decodeHtmlEntities(text: string): string from a new module named htmlUtils.ts under your AR utils), remove the local definitions in those three hooks, and import the exported function (e.g., import { decodeHtmlEntities } from '@ar/utils/htmlUtils') where needed, preserving the original TypeScript signature and behavior so compilation and existing callers remain unchanged.
🤖 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/hooks/useDeleteVersionModal.tsx`:
- Around line 58-62: The variable confirmationValue is inferred as string |
undefined because digest isn't narrowed through shouldUseDigest; change the
assignment to use an inline conditional that directly narrows digest (e.g. const
confirmationValue = isOCIPackage && digest ? digest : versionKey) so TypeScript
knows the truthy branch is string, then pass confirmationValue to
decodeHtmlEntities(confirmationValue); if you cannot guarantee a string, provide
a safe fallback or explicit non-null assertion where used.
---
Outside diff comments:
In
`@web/src/ar/pages/version-details/components/VersionActions/DeleteVersionMenuItem.tsx`:
- Around line 28-45: Update the types and call-site to remove unsafe any casts:
change VersionActionProps.digest from digest?: any to digest?: string, and in
DeleteVersionMenuItem replace data?.packageType as any with data?.packageType
typed as RepositoryPackageType (import RepositoryPackageType from
`@ar/common/types` if needed) when calling useDeleteVersionModal; ensure
useDeleteVersionModal invocation matches its prop types (artifactKey, repoKey,
versionKey, artifactType, packageType: RepositoryPackageType, digest: string |
undefined) so the enum constraint and string-typed digest are enforced.
---
Duplicate comments:
In
`@web/src/ar/pages/artifact-details/hooks/useDeleteArtifactModal/useDeleteArtifactModal.tsx`:
- Around line 27-32: The decodeHtmlEntities function is duplicated; remove the
local copy from useDeleteArtifactModal (the function named decodeHtmlEntities in
that file) and instead import a single shared implementation (create or use the
existing decodeHtmlEntities in the shared utilities module used by
useDeleteRepositoryModal) so both hooks reuse the same function; update the hook
to import decodeHtmlEntities from the shared util and delete the duplicated
local implementation.
In `@web/src/ar/pages/version-details/hooks/useDeleteVersionModal.tsx`:
- Around line 28-33: The file defines a duplicate helper function
decodeHtmlEntities (third occurrence); remove this local copy and replace its
uses with a single shared implementation: extract decodeHtmlEntities into a
common utility module (or use the existing shared helper if one already exists),
export it, then import and use that exported function in useDeleteVersionModal
(replace the local decodeHtmlEntities definition). Ensure the symbol name
decodeHtmlEntities is used consistently and delete the duplicate declaration
from this file.
---
Nitpick comments:
In
`@web/src/ar/pages/repository-details/hooks/useDeleteRepositoryModal/useDeleteRepositoryModal.tsx`:
- Around line 26-31: The local helper decodeHtmlEntities is duplicated across
useDeleteRepositoryModal, useDeleteArtifactModal, and useDeleteVersionModal;
extract it into a single shared utility (e.g., create and export
decodeHtmlEntities(text: string): string from a new module named htmlUtils.ts
under your AR utils), remove the local definitions in those three hooks, and
import the exported function (e.g., import { decodeHtmlEntities } from
'@ar/utils/htmlUtils') where needed, preserving the original TypeScript
signature and behavior so compilation and existing callers remain unchanged.
| const shouldUseDigest = isOCIPackage && digest | ||
| const confirmationValue = shouldUseDigest ? digest : versionKey | ||
|
|
||
| // Decode HTML entities to display properly | ||
| const decodedConfirmationValue = decodeHtmlEntities(confirmationValue) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n web/src/ar/pages/version-details/hooks/useDeleteVersionModal.tsx | head -100Repository: abhinavcode/harness
Length of output: 4524
🏁 Script executed:
find . -name "tsconfig.json" -o -name "tsconfig*.json" | head -5Repository: abhinavcode/harness
Length of output: 172
🏁 Script executed:
cat web/tsconfig.jsonRepository: abhinavcode/harness
Length of output: 862
confirmationValue has type string | undefined — potential TypeScript error under strict mode.
TypeScript does not narrow digest through the intermediate shouldUseDigest variable. When shouldUseDigest ? digest : versionKey is evaluated, TS sees digest as string | undefined in the truthy branch, making confirmationValue: string | undefined. Passing it to decodeHtmlEntities(text: string) violates the type contract under strict mode.
Use an inline condition so TypeScript can narrow digest directly:
🛠️ Proposed fix
- const shouldUseDigest = isOCIPackage && digest
- const confirmationValue = shouldUseDigest ? digest : versionKey
+ const confirmationValue = (isOCIPackage && digest) ? digest : versionKey📝 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.
| const shouldUseDigest = isOCIPackage && digest | |
| const confirmationValue = shouldUseDigest ? digest : versionKey | |
| // Decode HTML entities to display properly | |
| const decodedConfirmationValue = decodeHtmlEntities(confirmationValue) | |
| const confirmationValue = (isOCIPackage && digest) ? digest : versionKey | |
| // Decode HTML entities to display properly | |
| const decodedConfirmationValue = decodeHtmlEntities(confirmationValue) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/ar/pages/version-details/hooks/useDeleteVersionModal.tsx` around
lines 58 - 62, The variable confirmationValue is inferred as string | undefined
because digest isn't narrowed through shouldUseDigest; change the assignment to
use an inline conditional that directly narrows digest (e.g. const
confirmationValue = isOCIPackage && digest ? digest : versionKey) so TypeScript
knows the truthy branch is string, then pass confirmationValue to
decodeHtmlEntities(confirmationValue); if you cannot guarantee a string, provide
a safe fallback or explicit non-null assertion where used.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/ar/pages/version-details/hooks/useDeleteVersionModal.tsx (1)
58-67:⚠️ Potential issue | 🟠 MajorPass
digestinstead ofversionKeyto the delete API for OCI packages.The confirmation UI correctly uses
digestfor OCI packages (Docker/Helm) at lines 52–56, buthandleDeleteVersion(lines 58–67) always sendsversion: versionKeyregardless of package type. The comment at line 46 indicates this was intended, but the implementation is incomplete. For OCI packages, the API call should pass the digest received in props to the mutation instead of versionKey.Update the API call to:
const deleteParam = (isOCIPackage && digest) ? digest : versionKey // Then use: version: deleteParamThis ensures the API receives the same value that the user confirmed in the modal.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/ar/pages/version-details/hooks/useDeleteVersionModal.tsx` around lines 58 - 67, In useDeleteVersionModal's handleDeleteVersion, the delete API always passes version: versionKey even for OCI packages; change it to compute a deleteParam that uses digest when isOCIPackage and digest are present (e.g. const deleteParam = (isOCIPackage && digest) ? digest : versionKey) and pass version: deleteParam to the deleteVersion call so the mutation receives the same value confirmed in the modal; update references to versionKey in handleDeleteVersion accordingly.
🧹 Nitpick comments (1)
web/src/ar/common/utils/decodeHtmlEntities.ts (1)
39-39: Preferdoc.body.textContentoverdoc.documentElement.textContent.
doc.documentElement.textContentincludes text from<head>(e.g. a decoded<title>element the parser hoisted out of body).doc.body.textContentis more targeted and avoids accidentally including head-level content.♻️ Proposed refactor
- return doc.documentElement.textContent || text + return doc.body?.textContent || text🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/ar/common/utils/decodeHtmlEntities.ts` at line 39, The function decodeHtmlEntities currently returns doc.documentElement.textContent which can include head-level text; change it to prefer doc.body.textContent (falling back to doc.documentElement.textContent or the original input `text` if body is null) so decoding only pulls visible body content; update the return in decodeHtmlEntities to use doc.body.textContent || doc.documentElement.textContent || text and keep the rest of the function unchanged.
🤖 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/common/utils/decodeHtmlEntities.ts`:
- Around line 25-27: Update the JSDoc for decodeHtmlEntities to correct the
second example and clarify behavior: change the example input
'<script>alert(1)</script>' to show that the function returns the
script element's textContent (e.g. 'alert(1)') rather than the literal
'<script>...</script>', and add a brief note that any decoded input that forms
real HTML elements will have tags stripped because the function uses
DOMParser/documentElement.textContent; reference the decodeHtmlEntities function
name so the comment sits next to the implementation.
---
Outside diff comments:
In `@web/src/ar/pages/version-details/hooks/useDeleteVersionModal.tsx`:
- Around line 58-67: In useDeleteVersionModal's handleDeleteVersion, the delete
API always passes version: versionKey even for OCI packages; change it to
compute a deleteParam that uses digest when isOCIPackage and digest are present
(e.g. const deleteParam = (isOCIPackage && digest) ? digest : versionKey) and
pass version: deleteParam to the deleteVersion call so the mutation receives the
same value confirmed in the modal; update references to versionKey in
handleDeleteVersion accordingly.
---
Duplicate comments:
In `@web/src/ar/pages/version-details/hooks/useDeleteVersionModal.tsx`:
- Around line 52-56: confirmationValue is inferred as string | undefined because
digest isn't narrowed via shouldUseDigest, so pass a guaranteed string to
decodeHtmlEntities: replace the two-step logic (shouldUseDigest +
confirmationValue) with a single inline conditional that directly narrows digest
(e.g., set confirmationValue using isOCIPackage && digest ? digest : versionKey)
so decodeHtmlEntities receives a string; reference: shouldUseDigest,
confirmationValue, decodeHtmlEntities, digest, versionKey.
---
Nitpick comments:
In `@web/src/ar/common/utils/decodeHtmlEntities.ts`:
- Line 39: The function decodeHtmlEntities currently returns
doc.documentElement.textContent which can include head-level text; change it to
prefer doc.body.textContent (falling back to doc.documentElement.textContent or
the original input `text` if body is null) so decoding only pulls visible body
content; update the return in decodeHtmlEntities to use doc.body.textContent ||
doc.documentElement.textContent || text and keep the rest of the function
unchanged.
| * decodeHtmlEntities('github.com/rs/xid') // returns 'github.com/rs/xid' | ||
| * decodeHtmlEntities('<script>alert(1)</script>') // returns '<script>alert(1)</script>' (safe, no execution) | ||
| */ |
There was a problem hiding this comment.
JSDoc second example has an incorrect return value.
When '<script>alert(1)</script>' is passed to DOMParser.parseFromString, the HTML tokenizer decodes the entities first, producing <script>alert(1)</script>, which the parser materialises as an actual <script> DOM element. doc.documentElement.textContent then returns 'alert(1)' — the text content of the script node — not '<script>alert(1)</script>' as the comment claims.
The security note ("safe, no execution") is correct, but the stated return value is misleading. More broadly, any decoded string that forms valid HTML elements (e.g. <b>text</b>) would have the tag markup stripped from the result, which could cause a display mismatch if package keys ever contain <, > encoded as HTML entities.
📝 Corrected JSDoc example
- * decodeHtmlEntities('<script>alert(1)</script>') // returns '<script>alert(1)</script>' (safe, no execution)
+ * decodeHtmlEntities('<script>alert(1)</script>') // returns 'alert(1)' (HTML tags stripped; safe, no execution)📝 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.
| * decodeHtmlEntities('github.com/rs/xid') // returns 'github.com/rs/xid' | |
| * decodeHtmlEntities('<script>alert(1)</script>') // returns '<script>alert(1)</script>' (safe, no execution) | |
| */ | |
| * decodeHtmlEntities('github.com/rs/xid') // returns 'github.com/rs/xid' | |
| * decodeHtmlEntities('<script>alert(1)</script>') // returns 'alert(1)' (HTML tags stripped; safe, no execution) | |
| */ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/ar/common/utils/decodeHtmlEntities.ts` around lines 25 - 27, Update
the JSDoc for decodeHtmlEntities to correct the second example and clarify
behavior: change the example input '<script>alert(1)</script>' to
show that the function returns the script element's textContent (e.g.
'alert(1)') rather than the literal '<script>...</script>', and add a brief note
that any decoded input that forms real HTML elements will have tags stripped
because the function uses DOMParser/documentElement.textContent; reference the
decodeHtmlEntities function name so the comment sits next to the implementation.
2 participants
Summary by CodeRabbit
Bug Fixes
New Features
UI/UX