fix: [AH-2859]: Re-direction issue for OCI overview cards resolved#86
fix: [AH-2859]: Re-direction issue for OCI overview cards resolved#86abhinavcode wants to merge 3 commits into
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThe changes propagate a tag query parameter from the Docker version detail parent component into the OverviewCards component and include the tag when constructing redirect URLs alongside the digest parameter. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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.
🧹 Nitpick comments (1)
web/src/ar/pages/version-details/components/OverviewCards/OverviewCards.tsx (1)
85-105: Consider memoizinghandleRedirectToTabwithuseCallback.The function is recreated on every render and captures
tag,digest,routes,pathParams, andhistory. Wrapping it inuseCallbackwith the relevant dependencies would avoid unnecessary re-creation on unrelated renders, and is idiomatic for event-handler helpers in React.🤖 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/OverviewCards/OverviewCards.tsx` around lines 85 - 105, Memoize the handleRedirectToTab function using React's useCallback to avoid recreating it on every render; wrap the existing handleRedirectToTab implementation in useCallback and supply a dependency array including tag, digest, routes, pathParams (or its individual properties: versionIdentifier, artifactIdentifier, repositoryIdentifier, artifactType), and history so it updates only when those inputs change; ensure the function signature (tab: VersionDetailsTab, options: RedirectToTabOptions = {}) and its internal URLSearchParams logic remain unchanged.
🤖 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/pages/version-details/components/OverviewCards/OverviewCards.tsx`:
- Around line 85-105: Memoize the handleRedirectToTab function using React's
useCallback to avoid recreating it on every render; wrap the existing
handleRedirectToTab implementation in useCallback and supply a dependency array
including tag, digest, routes, pathParams (or its individual properties:
versionIdentifier, artifactIdentifier, repositoryIdentifier, artifactType), and
history so it updates only when those inputs change; ensure the function
signature (tab: VersionDetailsTab, options: RedirectToTabOptions = {}) and its
internal URLSearchParams logic remain unchanged.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web/src/ar/pages/version-details/components/OverviewCards/OverviewCards.tsx (1)
95-104: Refactor to either passtaganddigestto the route helper, or use a location descriptor to avoid manual URL concatenation.Two related concerns:
Design inconsistency:
routes.toARVersionDetailsTab()is designed to serializetaganddigestas query parameters—they're part ofVersionDetailsTabPathParamsand the route helper explicitly handles them (lines 102–118 in RouteDefinitions.ts). OverviewCards bypasses this design and manually serializes them afterward. Passtaganddigestin the options object instead, or use React Router v5's cleaner location descriptor pattern ({ pathname, search }) to separate path and query concerns.Inefficiency:
queryParams.toString()is called twice—once in the condition (line 100) and once in the template (line 101). Cache it.♻️ Proposed refactor (location descriptor approach)
- // Build query params to preserve both tag and digest - const queryParams = new URLSearchParams() - if (tag) queryParams.append('tag', tag) - if (digest) queryParams.append('digest', digest) - - if (queryParams.toString()) { - url = `${url}?${queryParams.toString()}` - } - - history.push(url) + // Build query params to preserve both tag and digest + const queryParams = new URLSearchParams() + if (tag) queryParams.append('tag', tag) + if (digest) queryParams.append('digest', digest) + + const queryString = queryParams.toString() + history.push(queryString ? { pathname: url, search: `?${queryString}` } : url)🤖 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/OverviewCards/OverviewCards.tsx` around lines 95 - 104, OverviewCards is manually appending tag/digest to the URL and calling history.push(url) instead of using the route helper that already knows how to serialize VersionDetailsTabPathParams; update the navigation to either call routes.toARVersionDetailsTab(id, { tab: activeTab, tag, digest }) so the helper emits the query params, or call history.push({ pathname: routes.toARVersionDetailsTab(id, { tab: activeTab }), search: cachedSearch }) where you build and cache the query string once (const cachedSearch = queryParams.toString()) before the if-check; ensure you stop calling queryParams.toString() twice and remove the manual string interpolation.
🤖 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/pages/version-details/components/OverviewCards/OverviewCards.tsx`:
- Around line 95-104: OverviewCards is manually appending tag/digest to the URL
and calling history.push(url) instead of using the route helper that already
knows how to serialize VersionDetailsTabPathParams; update the navigation to
either call routes.toARVersionDetailsTab(id, { tab: activeTab, tag, digest }) so
the helper emits the query params, or call history.push({ pathname:
routes.toARVersionDetailsTab(id, { tab: activeTab }), search: cachedSearch })
where you build and cache the query string once (const cachedSearch =
queryParams.toString()) before the if-check; ensure you stop calling
queryParams.toString() twice and remove the manual string interpolation.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes