fix: [AH-2312]: client setup details is giving 500 at account and org level when you select the project level registry scope should be account, org and project#97
Open
abhinavcode wants to merge 4 commits into
Conversation
… level when you select the project level registry scope should be account, org and project
![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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThreaded an optional Changes
Sequence DiagramsequenceDiagram
participant UI as SetupClientMenuItem
participant Modal as useSetupClientModal
participant Widget as RepositorySetupClientWidget
participant Content as SetupClientContent
participant API as ClientSetupAPI
UI->>Modal: open(registryRef?)
activate Modal
Modal->>Widget: render(registryRef?)
activate Widget
Widget->>Content: clone element injecting registryRef
activate Content
Content->>Content: resolve registryRef (prop || useGetSpaceRef)
Content->>API: fetch setup details with registry_ref
activate API
API-->>Content: setup details
deactivate API
Content-->>Widget: rendered content
deactivate Content
Widget-->>Modal: rendered widget
deactivate Widget
Modal-->>UI: modal ready
deactivate Modal
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 |
![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 (1)
web/src/ar/frameworks/RepositoryStep/RepositorySetupClientWidget.tsx (1)
52-61: Add a validity guard beforecloneElement.
React.cloneElementwill throw ifrenderSetupClientever returnsnullor a non-element value. A lightweightReact.isValidElementguard makes this path safer.Proposed hardening
const content = repositoryType.renderSetupClient({ onClose, repoKey, artifactKey, versionKey }) - return React.cloneElement(content as React.ReactElement, { + if (!React.isValidElement(content)) { + return <Text intent="warning">{getString('stepNotFound')}</Text> + } + return React.cloneElement(content, { isSentFromRegistryPage, registryPath })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/ar/frameworks/RepositoryStep/RepositorySetupClientWidget.tsx` around lines 52 - 61, The code calls React.cloneElement on the value returned by repositoryType.renderSetupClient without checking it's a valid React element; add a guard using React.isValidElement(content) before cloning to avoid runtime throws, and if it's not valid return null or fallback UI. Locate the variable content (from repositoryType.renderSetupClient) and the clone call (React.cloneElement(..., { isSentFromRegistryPage, registryPath })) and wrap the clone in a validity check so only valid React elements are cloned.
🤖 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/repository-details/components/Actions/SetupClient.tsx`:
- Around line 33-34: The current object blindly sets isSentFromRegistryPage:
true while registryPath may be undefined; change to compute registryPath first
from (data as { path?: string }).path and set isSentFromRegistryPage to a
boolean based on its presence (e.g., !!registryPath) so registry mode is only
enabled when registryPath exists; update any uses of
registryPath/isSentFromRegistryPage accordingly in SetupClient.tsx.
---
Nitpick comments:
In `@web/src/ar/frameworks/RepositoryStep/RepositorySetupClientWidget.tsx`:
- Around line 52-61: The code calls React.cloneElement on the value returned by
repositoryType.renderSetupClient without checking it's a valid React element;
add a guard using React.isValidElement(content) before cloning to avoid runtime
throws, and if it's not valid return null or fallback UI. Locate the variable
content (from repositoryType.renderSetupClient) and the clone call
(React.cloneElement(..., { isSentFromRegistryPage, registryPath })) and wrap the
clone in a validity check so only valid React elements are cloned.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
web/src/ar/frameworks/RepositoryStep/RepositorySetupClientWidget.tsxweb/src/ar/hooks/index.tsweb/src/ar/pages/repository-details/components/Actions/SetupClient.tsxweb/src/ar/pages/repository-details/components/SetupClientContent/SetupClientContent.tsxweb/src/ar/pages/repository-details/hooks/useSetupClientModal/useSetupClientModal.tsx
… level when you select the project level registry scope should be account, org and project
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web/src/ar/frameworks/RepositoryStep/RepositorySetupClientWidget.tsx (1)
61-64: Consider adding guards for intrinsic/Fragment elements and improving prop typing forcloneElement.While the abstract method signature constrains
renderSetupClientto returnJSX.Elementand all current implementations return theSetupClientContentcomponent (never intrinsic elements or fragments), adding defensive checks would prevent future implementations from accidentally returning unsupported element types. Additionally, the broad type cast on line 64 can be tightened.Suggested refactor
- if (!content || !React.isValidElement(content)) { + if ( + !content || + !React.isValidElement(content) || + typeof content.type === 'string' || + content.type === React.Fragment + ) { return content } - return React.cloneElement(content, { + type RegistryInjectedProps = { + isSentFromRegistryPage?: boolean + registryPath?: string + } + return React.cloneElement(content as React.ReactElement<RegistryInjectedProps>, { isSentFromRegistryPage, registryPath - } as React.Attributes & { isSentFromRegistryPage?: boolean; registryPath?: string }) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/ar/frameworks/RepositoryStep/RepositorySetupClientWidget.tsx` around lines 61 - 64, The cloneElement call in RepositorySetupClientWidget should defensively ensure renderSetupClient() returns a valid React element of the expected component type and tighten the prop typing instead of a broad cast; update RepositorySetupClientWidget to check that content is a React.isValidElement and not a Fragment/intrinsic (e.g., typeof content.type === 'string' or content.type === React.Fragment) and only call React.cloneElement when content is a component element (matching SetupClientContent or a ComponentType), otherwise render content as-is or wrap it; replace the loose cast on the cloneElement props with a stricter generic typed props/interface for the added props (isSentFromRegistryPage?: boolean; registryPath?: string) so cloneElement<SetupClientContentProps>(...) enforces types and avoids unsafe casting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@web/src/ar/frameworks/RepositoryStep/RepositorySetupClientWidget.tsx`:
- Around line 61-64: The cloneElement call in RepositorySetupClientWidget should
defensively ensure renderSetupClient() returns a valid React element of the
expected component type and tighten the prop typing instead of a broad cast;
update RepositorySetupClientWidget to check that content is a
React.isValidElement and not a Fragment/intrinsic (e.g., typeof content.type ===
'string' or content.type === React.Fragment) and only call React.cloneElement
when content is a component element (matching SetupClientContent or a
ComponentType), otherwise render content as-is or wrap it; replace the loose
cast on the cloneElement props with a stricter generic typed props/interface for
the added props (isSentFromRegistryPage?: boolean; registryPath?: string) so
cloneElement<SetupClientContentProps>(...) enforces types and avoids unsafe
casting.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web/src/ar/frameworks/RepositoryStep/RepositorySetupClientWidget.tsxweb/src/ar/pages/repository-details/components/Actions/SetupClient.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/ar/pages/repository-details/components/Actions/SetupClient.tsx
… level when you select the project level registry scope should be account, org and project
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/repository-details/hooks/useSetupClientModal/useSetupClientModal.tsx (1)
38-67:⚠️ Potential issue | 🟠 MajorAdd dependency array to
useModalHookcall to ensure modal reflects prop changes.The
useModalHookmemoizes the component factory based on aninputsdependency array (second parameter).useSetupClientModalomits this parameter, causing all captured props—repoKey,artifactKey,versionKey,packageType,onClose, andregistryRef—to become stale closures. If any prop changes after the hook is called but before the modal is opened, the change is not reflected.Pass
[packageType, repoKey, artifactKey, versionKey, onClose, registryRef]as the second argument touseModalHook, matching the pattern used inuseCreateLabelModalanduseCreateRepositoryModal.🤖 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/useSetupClientModal/useSetupClientModal.tsx` around lines 38 - 67, The modal factory passed into useModalHook inside useSetupClientModal currently omits the inputs dependency array, causing captured props (repoKey, artifactKey, versionKey, packageType, onClose, registryRef) to become stale; fix it by passing an inputs array as the second argument to useModalHook — e.g. useModalHook(() => { ... }, [packageType, repoKey, artifactKey, versionKey, onClose, registryRef]) — so the Drawer/RepositorySetupClientWidget sees updated props when opened.
🧹 Nitpick comments (2)
web/src/ar/pages/repository-details/components/SetupClientContent/SetupClientContent.tsx (1)
41-58:artifactKey/versionKeynot destructured alongside other props.Line 42 destructures most props but leaves
artifactKeyandversionKeyonprops, accessed asprops.artifactKeyandprops.versionKeyon lines 55–56. Minor consistency nit.♻️ Proposed cleanup
- const { onClose, packageType, repoKey, registryRef: registryRefProp } = props + const { onClose, packageType, repoKey, artifactKey, versionKey, registryRef: registryRefProp } = props ... queryParams: { - artifact: props.artifactKey, - version: props.versionKey + artifact: artifactKey, + version: versionKey }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/ar/pages/repository-details/components/SetupClientContent/SetupClientContent.tsx` around lines 41 - 58, Destructure artifactKey and versionKey from props in SetupClientContent alongside onClose, packageType, repoKey, and registryRefProp so the call to useGetClientSetupDetailsQuery uses artifactKey and versionKey directly; update the useGetClientSetupDetailsQuery call (and any other usages) to reference the local variables artifactKey and versionKey instead of props.artifactKey / props.versionKey to keep prop access consistent and concise.web/src/ar/pages/repository-list/components/RepositoryListTreeView/RepositoryListTreeView.tsx (1)
154-167: Redundant optional chain on the type assertion.
(metadata as { path?: string })?.path— the optional chain (?.) after the cast is a no-op because a type assertion never returnsnullorundefined;metadataitself is already in scope. Thepath?: stringin the cast already communicates that the property may be absent.♻️ Minor simplification
- const path = (metadata as { path?: string })?.path + const path = (metadata as { path?: string }).path🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/ar/pages/repository-list/components/RepositoryListTreeView/RepositoryListTreeView.tsx` around lines 154 - 167, In the TreeNodeEntityEnum.REGISTRY case simplify the redundant optional chaining on the type assertion: replace the expression (metadata as { path?: string })?.path with a direct property access on the asserted type so that registryRef is derived from (metadata as { path?: string }).path (or pull that asserted type into a small const named path) before calling encodeRef; update references around registryRef/encodeRef/RepositoryActionsWidget accordingly.
🤖 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/upstream-proxy-details/components/UpstreamProxyActions/UpstreamProxyActions.tsx`:
- Line 32: Create a new RegistryRefContext module that exports a React context,
a RegistryRefProvider component, and the useRegistryRef hook; implement
RegistryRefContext to hold the registry reference state (e.g., registryRef value
and a setter), ensure RegistryRefProvider accepts children and provides the
context value, and implement useRegistryRef to read the context and throw a
clear error if used outside the provider; export both RegistryRefProvider and
useRegistryRef so RepositoryActionsWidget.tsx and UpstreamProxyActions.tsx can
import them.
---
Outside diff comments:
In
`@web/src/ar/pages/repository-details/hooks/useSetupClientModal/useSetupClientModal.tsx`:
- Around line 38-67: The modal factory passed into useModalHook inside
useSetupClientModal currently omits the inputs dependency array, causing
captured props (repoKey, artifactKey, versionKey, packageType, onClose,
registryRef) to become stale; fix it by passing an inputs array as the second
argument to useModalHook — e.g. useModalHook(() => { ... }, [packageType,
repoKey, artifactKey, versionKey, onClose, registryRef]) — so the
Drawer/RepositorySetupClientWidget sees updated props when opened.
---
Nitpick comments:
In
`@web/src/ar/pages/repository-details/components/SetupClientContent/SetupClientContent.tsx`:
- Around line 41-58: Destructure artifactKey and versionKey from props in
SetupClientContent alongside onClose, packageType, repoKey, and registryRefProp
so the call to useGetClientSetupDetailsQuery uses artifactKey and versionKey
directly; update the useGetClientSetupDetailsQuery call (and any other usages)
to reference the local variables artifactKey and versionKey instead of
props.artifactKey / props.versionKey to keep prop access consistent and concise.
In
`@web/src/ar/pages/repository-list/components/RepositoryListTreeView/RepositoryListTreeView.tsx`:
- Around line 154-167: In the TreeNodeEntityEnum.REGISTRY case simplify the
redundant optional chaining on the type assertion: replace the expression
(metadata as { path?: string })?.path with a direct property access on the
asserted type so that registryRef is derived from (metadata as { path?: string
}).path (or pull that asserted type into a small const named path) before
calling encodeRef; update references around
registryRef/encodeRef/RepositoryActionsWidget accordingly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
web/src/ar/frameworks/RepositoryStep/RepositoryActionsWidget.tsxweb/src/ar/frameworks/RepositoryStep/RepositorySetupClientWidget.tsxweb/src/ar/pages/repository-details/components/Actions/RepositoryActions.tsxweb/src/ar/pages/repository-details/components/Actions/SetupClient.tsxweb/src/ar/pages/repository-details/components/Actions/types.tsweb/src/ar/pages/repository-details/components/SetupClientContent/SetupClientContent.tsxweb/src/ar/pages/repository-details/hooks/useSetupClientModal/useSetupClientModal.tsxweb/src/ar/pages/repository-list/components/RepositoryListTable/RepositoryListCells.tsxweb/src/ar/pages/repository-list/components/RepositoryListTreeView/RepositoryListTreeView.tsxweb/src/ar/pages/upstream-proxy-details/components/UpstreamProxyActions/UpstreamProxyActions.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/ar/frameworks/RepositoryStep/RepositorySetupClientWidget.tsx
… level when you select the project level registry scope should be account, org and project
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