Follow-ups from the Argo Rollouts review: two bugs, a blue-green verdict, a collision test - #1767
Merged
Merged
Conversation
PR Summary by QodoFix Rollout analysis history, ordering, and timeline verdicts
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d3be39d. Configure here.
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+185
to
+188
| className={clsx('badge-sm hover:underline', healthColors[level])} | ||
| title={phase.analysis.name} | ||
| > | ||
| {phase.analysis.status} |
There was a problem hiding this comment.
2. Verdict badges bypass shared styling 📘 Rule violation ⚙ Maintainability
CanaryStepTimeline and BlueGreenTimeline render analysis verdicts as custom button and span elements with badge-sm and healthColors instead of the shared Badge component. Both linked and unlinked verdict paths therefore maintain their own status-badge markup rather than inheriting the component's semantic tone and interaction behavior.
Agent Prompt
## Issue description
The new analysis verdict controls recreate badge appearance with custom elements and color classes instead of using the shared semantic badge component.
## Fix Focus Areas
- packages/k8s-ui/src/components/resources/renderers/rollout/CanaryStepTimeline.tsx[89-103]
- packages/k8s-ui/src/components/resources/renderers/rollout/CanaryStepTimeline.tsx[179-192]
## Recommended Fix
Import `Badge` and render each verdict through it, passing the normalized analysis level through the semantic `tone` prop. Use `Badge`'s `onClick` and `title` support for navigable verdicts rather than constructing separate buttons.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+752
to
+754
| // Collapsed when it holds history, because that is backward-looking. | ||
| // Open when it could not be read, so the reason does not need a click. | ||
| defaultExpanded={!!analysisRunHistoryError} |
There was a problem hiding this comment.
3. Analysis history starts collapsed 📘 Rule violation ⚙ Maintainability
RolloutRenderer sets defaultExpanded from analysisRunHistoryError, so a populated history section is collapsed whenever the fetch succeeds. The section has non-empty normal-priority content in that path but no explicit low-priority prop, reaching every successful analysis-history render.
Agent Prompt
## Issue description
The populated AnalysisRun History renderer section explicitly starts collapsed even though it contains content and cannot be marked as low priority through the section API.
## Fix Focus Areas
- packages/k8s-ui/src/components/resources/renderers/RolloutRenderer.tsx[748-760]
## Recommended Fix
Set `defaultExpanded={true}` for the AnalysisRun History section so successful history and lookup failures both satisfy the renderer-section expansion requirement.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
hisco
force-pushed
the
radar-rollouts-followups
branch
from
September 15, 2026 13:12
d3be39d to
c2861c9
Compare
Katib ships an Experiment at kubeflow.org sharing the plural with Argo Rollouts, and only an apiVersion gate keeps the Argo status mapping off it. Nothing failed if that gate was removed. Two cases now cover it, alongside the other colliding plurals in this file.
The history sorted on creation time alone. Argo creates a step run and a background run on the same reconcile, so both carry the same second and their order was left to chance, which reshuffled the list between loads. Ties now fall back to step index, then to runs that have one at all, then to name. A step-triggered run is what an operator is looking for, so it leads, and a later step leads an earlier one. That makes the step index decide what an operator sees, so reading it got stricter in the same change. Sscanf accepted "3abc" as 3 and reported no error, which would have sorted a run under a step it does not belong to.
The canary step badge said an analysis was Inconclusive but could not take you to the run, and a blue-green phase could not say anything at all, so reading either meant going back to the Analysis section and matching by name. The step badge now links to the AnalysisRun, and a blue-green phase carries its verdict and link the same way, with the dot picking up a failed or inconclusive tone exactly as a canary step does. Nothing is removed. The Analysis section still lists every populated slot, because the timeline only shows the step run while the current step is an analysis step, and an aborted canary is not: the controller moves the index off that step while the failed run's status stays behind.
A caller without `list analysisruns` got no AnalysisRun History section at all, which looks exactly like a Rollout that has never run an analysis. The section now appears either way, and a refused read says so through the same LookupFailureNote the other reverse lookups use. It opens by default only in that case, so the reason does not need a click, and stays collapsed when it actually holds history. The note is marked partial when rows are already on screen: React Query keeps the last good data through a failed refetch, so a session that loses its grant mid-way would otherwise print a flat refusal above a visible list.
hisco
force-pushed
the
radar-rollouts-followups
branch
from
September 16, 2026 08:28
c2861c9 to
ff77e5f
Compare
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.
Four follow-ups from reviewing #1625, which is already merged. Two bugs, one
addition, one test. They are independent, so any can be dropped.
A refused analysis-history read looked like an empty one. A caller without
list analysisrunsgot no AnalysisRun History section at all, which is exactlywhat a Rollout that never ran an analysis looks like. The section now renders
either way and a refusal says so, through the
LookupFailureNotethe otherreverse lookups already use. It is marked partial when rows are on screen,
because React Query keeps the last good data through a failed refetch, so a
session that loses its grant mid-way would otherwise print a flat refusal above
a visible list.
The history list reshuffled between loads. It sorted on creation time only,
and Argo creates a step run and a background run on the same reconcile, so their
order was left to chance. Ties now fall back to step index, then to runs that
have one, then to name. That makes the step index decide what an operator sees,
so
parseInt64got stricter in the same commit:Sscanfread"3abc"as3and reported no error, which would sort a run under a step it does not belong to.
A blue-green phase could not show its own verdict. Reading one meant going
back to the Analysis section and matching by name. The phase now carries its
verdict and links the run, the canary step badge links its run too, and the
blue-green dot picks up a failed or inconclusive tone the way a canary step
already did. Nothing is removed. The Analysis section still lists every
populated slot.
The Experiment plural collision had no test. Katib ships
Experimentatkubeflow.orgsharing the plural with Argo Rollouts, and only an apiVersion gatekeeps the Argo status mapping off it. Removing that gate failed nothing.
Worth a careful look
The third one deliberately stops short. An earlier version also filtered the
Analysis section down to slots no timeline had shown, and that dropped the step
run on an aborted canary: the controller moves
currentStepIndexoff theanalysis step while
currentStepAnalysisRunStatuskeeps the failed run, so therun that caused the abort had nowhere left to appear. Consolidating those
surfaces is still worth doing, but it needs the filter to derive from the same
condition the timeline renders on, and it is a product call about a section that
arrived four weeks ago in #1380.
Testing
Ran against a kind cluster with Argo Rollouts and the
rollouts-demofixtures,covering canary, blue-green, aborted and workloadRef Rollouts, plus a Katib
ExperimentCRD for the collision. Confirmed the aborted canary rendersidentically to main.
Each new test was checked against the old behaviour: the partial-read case fails
without the
incompleteflag, the ordering case fails with the step-indextiebreak removed, the malformed-label case fails with
Sscanfrestored, and thecollision case fails with the apiVersion gate removed.
go test ./pkg/rollouts ./internal/server,npx vitest runinpackages/k8s-ui(3731 passing),
tsc --noEmitinweb, andgofmt -lall clean.Note
Low Risk
UI and list-sorting changes in Rollouts surfaces with targeted tests; no auth or data-mutation paths touched.
Overview
Argo Rollouts follow-ups: clearer failure handling for analysis history, stabler backend ordering, and richer progression UI.
AnalysisRun history no longer disappears when the list API fails (e.g. missing
list analysisruns).RolloutRendereracceptsanalysisRunHistoryError, shows the existingLookupFailureNote, auto-expands the section on error, and marks data as possibly incomplete when React Query still has cached rows. The web shell forwards the fetch error fromuseRolloutAnalysisRuns.History ordering in
ListAnalysisRunsuses deterministic tie-breaks (step index, then name) when runs share a creation timestamp, and rejects malformedstep-indexlabels via strictstrconv.ParseIntinstead of permissive parsing.Progression timelines surface analysis verdicts inline: blue-green phases carry pre/post-promotion analysis status with navigable badges and dot health tones; canary step analysis badges link to the
AnalysisRunwhen navigation is available.Collision coverage: tests lock Argo
experimentsstatus to AnalysisPhase vocabulary while Katibexperimentsatkubeflow.orgdo not get Argo health levels.Reviewed by Cursor Bugbot for commit ff77e5f. Bugbot is set up for automated code reviews on this repo. Configure here.