Error displayed while fetching rollout revisions details (#10132)#255
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #255 +/- ##
==========================================
- Coverage 11.92% 11.83% -0.10%
==========================================
Files 154 155 +1
Lines 6272 6330 +58
Branches 2028 2110 +82
==========================================
+ Hits 748 749 +1
- Misses 5524 5581 +57
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughA new shared Loading spinner component module was added, exporting ChangesShared Loading component adoption
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
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.
🧹 Nitpick comments (3)
src/gitops/utils/components/LoadingSpinner/Loading.tsx (1)
4-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReorder declarations for readability.
LoadingInline(Line 4) referencesLoadingbefore it is declared (Line 7). It works because the reference is inside a deferred function body, but declaringLoadingfirst improves readability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/gitops/utils/components/LoadingSpinner/Loading.tsx` around lines 4 - 14, Reorder the component declarations in Loading.tsx so Loading is defined before LoadingInline for readability. Keep the behavior the same by leaving LoadingInline as a thin wrapper around Loading and preserving LoadingInline.displayName, but move the Loading function declaration above LoadingInline to make the dependency order explicit.src/gitops/components/rollout/RolloutRevisionsTab.tsx (1)
67-68: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNested ternary flagged by ESLint.
Consider refactoring to an early-return/if-else style for readability, per the static analysis hint (
no-nested-ternary).♻️ Suggested refactor
- {loadError || podsloadError ? ( - <div> - ... - </div> - ) : !loaded || !podsloaded ? ( - <Loading /> - ) : ( - <Revisions - rollout={rollout} - replicaSets={resourceAsArray(replicaSets) as ReplicaSetKind[]} - pods={resourceAsArray(pods) as PodKind[]} - /> - )} + {(() => { + if (loadError || podsloadError) { + return ( + <div> + ... + </div> + ); + } + if (!loaded || !podsloaded) { + return <Loading />; + } + return ( + <Revisions + rollout={rollout} + replicaSets={resourceAsArray(replicaSets) as ReplicaSetKind[]} + pods={resourceAsArray(pods) as PodKind[]} + /> + ); + })()}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/gitops/components/rollout/RolloutRevisionsTab.tsx` around lines 67 - 68, The conditional rendering in RolloutRevisionsTab is using a nested ternary that triggers the no-nested-ternary lint rule. Refactor the JSX in RolloutRevisionsTab to use early returns or a simple if/else sequence instead of chaining ternary branches, keeping the same loading/empty/main render behavior while making the render logic easier to read and maintain.Source: Linters/SAST tools
src/gitops/topology/console/pod-traffic.tsx (1)
27-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove dead commented-out code instead of leaving it in-place.
The old
LoadingInline/Loading/LoadingPropsimplementations are commented out rather than deleted now that they've been moved to the shared module. Leaving this dead code adds clutter.🧹 Suggested cleanup
-// export const LoadingInline: React.FunctionComponent = () => <Loading isInline />; -// LoadingInline.displayName = 'LoadingInline'; - -// export const Loading: React.FunctionComponent<LoadingProps> = ({ className, isInline }) => ( -// <div -// className={classNames('co-m-loader', { 'co-m-loader--inline': isInline }, className)} -// data-test="loading-indicator" -// > -// <Spinner aria-live="polite" aria-busy="true" isInline={isInline} size="lg" /> -// </div> -// ); - -// Loading.displayName = 'Loading'; - -// type LoadingProps = { -// className?: string; -// isInline?: boolean; -// };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/gitops/topology/console/pod-traffic.tsx` around lines 27 - 44, Remove the commented-out LoadingInline, Loading, and LoadingProps block from pod-traffic.tsx since those implementations now live in the shared module. Clean up the dead code in this area and keep the file focused on the active pod traffic logic and any remaining references to LoadingInline/Loading that are still in use.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/gitops/components/rollout/RolloutRevisionsTab.tsx`:
- Around line 67-68: The conditional rendering in RolloutRevisionsTab is using a
nested ternary that triggers the no-nested-ternary lint rule. Refactor the JSX
in RolloutRevisionsTab to use early returns or a simple if/else sequence instead
of chaining ternary branches, keeping the same loading/empty/main render
behavior while making the render logic easier to read and maintain.
In `@src/gitops/topology/console/pod-traffic.tsx`:
- Around line 27-44: Remove the commented-out LoadingInline, Loading, and
LoadingProps block from pod-traffic.tsx since those implementations now live in
the shared module. Clean up the dead code in this area and keep the file focused
on the active pod traffic logic and any remaining references to
LoadingInline/Loading that are still in use.
In `@src/gitops/utils/components/LoadingSpinner/Loading.tsx`:
- Around line 4-14: Reorder the component declarations in Loading.tsx so Loading
is defined before LoadingInline for readability. Keep the behavior the same by
leaving LoadingInline as a thin wrapper around Loading and preserving
LoadingInline.displayName, but move the Loading function declaration above
LoadingInline to make the dependency order explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 84746ebe-8c19-4071-adb8-c25b235c871e
📒 Files selected for processing (3)
src/gitops/components/rollout/RolloutRevisionsTab.tsxsrc/gitops/topology/console/pod-traffic.tsxsrc/gitops/utils/components/LoadingSpinner/Loading.tsx
Signed-off-by: Keith Chong <kykchong@redhat.com>
keithchong
force-pushed
the
10132-LoadingIsNotError
branch
from
cacdace to
2c0d401
Compare
aali309
left a comment
aali309
left a comment
There was a problem hiding this comment.
LGTM! Thanks @keithchong
3 participants
See GITOPS-10132 for details. I've pulled out the Loading into its own component