PR 1255: make the ignore rule dialog and the ignored checks clearer
Your PR, reviewed twice at 1ca1526: Cody's process (codyrancher/ai-skills c04c11a) and ours, then merged and re-checked. Nothing was posted to GitHub.
Preview:harvesterignoreuxr, built from 1ca1526, backend Harvester .105 (v1.10.0-rc1 with the resource health controller). Its certificate is still waiting on Let's Encrypt's rate limit, so the browser warns. Baseline: harvester-main at f3ca9e4, same backend.
Where it stands: everything the issue asks for works, 3 things to fix first
Every point of issue 12277 works on the preview in both themes, and lint, unit tests and CI are green.
Every point of the issue on the preview against .105, in light and dark, with fake ignored states in the browser and every write aborted. The contrast drop, measured the same way (backgrounds composited) and landing on the same numbers.
Only one pass did
Before/after demo videos. A sourced acceptance table. His a11y probe, over CDP, with a control run on an active rule to set shell findings aside. The docs and test-steps side effects. Copilot's duplicate reproduced in jest.
The main-vs-PR contrast table in both themes. The lost hover colour. Copilot's duplicate in the browser in both data shapes. Full lint and test:ci. A coverage grid.
The yellow is the warning banner's background, but the rows keep the greys picked for a plain row. The banner itself puts body text on it, not muted text. Both passes measured it, with the row colour composited over what is behind it:
Text on a yellow row
Theme
main
PR
Muted: API version; name and message of a check ignored as a whole
dark
4.62
3.6
Same
light
5.23
4.58
Label: resource kind ("Upgrades")
light
5.03
4.41
Same
dark
7.35
5.73
All of it is 12-14px normal weight, so WCAG 1.4.3 asks for 4.5:1. The dashed ignored chips are disabled tags, which are exempt.
Dark, main on top, the PR below: the same grey text is fainter on the yellow.
Ways out, for you or the designer: body text instead of muted text on those rows (what the banner does), a lighter tint, or the yellow on a narrower element than the whole row. Each changes the look, so I haven't picked one.
The new rule wins over the table's hover colour. On main every check row turns light grey under the pointer; on the PR the yellow rows stay yellow, so they are the only ones with no hover feedback. Pass B saw it from the computed colours; Pass A's probe measured the same background at rest and on hover.
3
Chips show twice when the check becomes ignored as a whole
Copilot's point holds. Open "+ N ignored" on a check, then widen its rule to cover every resource: the controller marks the check ignored as a whole and keeps each name's own flag, the row then lists all names and appends the ignored ones again, and the link that would close the list is gone. Vue also warns about duplicate keys.
Pass B dropped it, reading that this shape needs a second rule on the check. It doesn't: the dialog's default Create is "Selected resources" with every resource selected, and for such a rule the controller sets the check ignored once every resource is (ignore.go, "a check is ignored as a whole once every affected resource of it is ignored"). Pass A reproduced it in jest, Pass B in the browser by changing the store.
hvst-upgrade-xv5nq twice, four chips under a count of 3, no link left.
Copilot's other point. The list of ignored names checks every name against the list of reported ones, so its cost grows with the square of the names, and only while that check's list is open. Pass A: 79 ms at 2,000 names, 640 ms at 10,000, 4.2 s at 30,000 (drawing the chips included). Pass B: 0.36 ms at 1,000 for the filter alone. .105's checks have at most 5 names. It sits in the same function as 3, so it costs nothing extra to tidy while fixing that.
After picking 30 days the banner still says "To make any changes, set a new expiration", although that has just been done and "Will expire on ..." shows under the radios. I kept it because your note said the banner should always show; the issue only describes it when the dialog opens. Hiding it once a new expiration is picked is one line.
6
The expired-rule dialog scrolls at 1280x720
Found by Pass A (Cody)
Designer
The banner makes the dialog taller, so at a 720px-high window Reason is cut off until you scroll inside the dialog. On main the same dialog fits. Layout only.
7
Docs PR 1172 describes the old dialog and table
Found by Pass A (Cody)
Follow-up
harvester/docs PR 1172 is still open and says "Entire check", "Ignore for", "+ 6 ignored", that the Show ignored label counts checks and resources, and that changing an expired rule without a duration shows a warning. Its dialog screenshot shows the old labels. It needs those lines and that image updated once this merges. PR 1225's Test Steps, which QA follows for issue 9747, also say "Entire check".
Pass A: Cody's process
Followed my-pr-full-review in this session, mapped to our setup: the PR preview for his dev server, harvester-main (rebuilt at f3ca9e4) for his master build, local files instead of his harness, our recorder for the demos, his a11y-probe.mjs run from the clone over CDP (axe not run: the probe looks for it under /workspace only). The ignored states were added in the browser on top of .105's real checks and your two real rules; every write was aborted.
Before this change: main.With this change: the PR.
Acceptance, from issue 12277 (written by us from your message, so a weaker standard)
Criterion
Verdict
AC1
"Ignore for" reads "Expiration"
Met
AC2
"Entire check" reads "All resources"
Met
AC3
Expired rule in Edit: the warning first in the dialog, with the issue's text
Met
AC4
Only Expiration enabled, Save too, until a new one is set
Met
AC5
"+ N ignored" is a link like "+ N more", right after it, and shows the ignored resources
Met
AC6
"Show ignored checks" with a badge of the ignored checks
Met
AC7
Checks with anything ignored get the warning banner's background
Met, but see 1 and 2
AC8
Create, View and non-expired rules unchanged apart from the labels
Met
Simplify: nothing beyond Copilot's filter point.
Side effects: the docs PR and 1225's test steps (finding 7).
Component reuse, design fidelity: nothing in scope.
Accessibility: finding 1. The probe's other reports (14px checkbox and radios, LabeledSelect structure, the tab panel) are the same on an active rule and on main, so they come from shell components.
Pass B: our process
A separate agent that didn't see Pass A. Verdict: ready once the contrast is fixed; 0 blockers, 1 suggestion (findings 1 and 2), 0 nits, 1 question (finding 5). It read Copilot's two points as not worth posting; the merge keeps the duplicate (finding 3) because the shape it needs comes from the dialog's default rule.
Also seen
.105 now has a third real rule, harvester-controller-volume.replicaschedulingfailure ("review fixture: an expired rule"), besides your two. Neither pass touched any of them.
Pass B ran read-only git log and git show in the PR's working clone, which its brief ruled out. Nothing changed there.
What I'd do
It's your PR, so nothing to post as a review. I'd fix 1 to 3 (and 4 with 3) in one more commit, once you pick how the yellow rows should look, then reply to Copilot's two threads and resolve them, and update the docs PR after the merge.