Repository navigation
Draw the next-action cue so it shows on a warning notice, and use design system tokens for the window's remaining values - #675
Conversation
…ng notice The cue was a ring in the warning surface stroke, which is the border a warning notice already has: on the "code is old" notice, the block it points at most often, the ring was that border again and could not be seen. It also said "warning" about what is guidance, and was made of a 3px ring and a 14px glow mixed with color-mix(), neither of them on the design system's scales. It is now an outline in the brand surface stroke at the system's medium border width. Brand, because the next step is guidance. Not the strong brand stroke: that is the focus ring's colour, and at the focus ring's width and offset the cue would be keyboard focus to look at, on a block or around a header button that does not have it. An outline, so it follows the block's own corners and leaves its border alone, which also fits it to the header's buttons. The journey checked only that the class was there. It now checks that the ring is drawn, is not the colour of the notice's border, and is not the focus colour; red on both earlier rules. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…oles What shell.css still wrote by hand where the design system has a token: - The status dots of the sites list and the processes are the 4xs size, and are painted in the foreground role an icon takes, filled with currentColor, instead of a surface stroke used as a fill. The values are the same. - Hairlines and the conflict rule are the xs and sm border widths, and the tabs bar's -1px is minus the xs width. - The step marks are the 2xs size and the feedback popover the sm surface width. - Controls that are not links take the control cursor. - In the dark theme the older library's popovers keep only the surface hairline, as the design system's own popover draws its edge, and lose the glow mixed from the text colour. What is left untokenised is layout the system has no token for: breakpoints, column bases, the content width, pane heights and the toast stack's place. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughShell styles replace fixed values with design-system tokens for status indicators, borders, dimensions, and interactive cursors. The next-action cue now uses a brand-colored outline, and the dark-mode popover shadow is removed. The site-page test checks the notice outline’s style, width, and color against its border and the focus color token. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The shell updates use design-system tokens, and the site-page test targets the updated notice cue. No actionable regression is established; custom themes may make the cue resemble a semantic notice border. 🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 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 review |
✅ Action performedReview finished.
|
…dited (#676) ## Why Since #660, editing a pull request's title or description turns its six required checks (`unit`, `Journeys` and `Packaged smoke` on macOS and Windows) into "Expected — Waiting for status to be reported", even though they already passed on that commit. The merge box shows them as pending until someone pushes or re-runs. Seen on #675: the runs at 07:59 passed, and the edits at 08:18 and 08:45 started runs 37904200109/37904200065 and 37906918098/37906918097 with every job skipped. ## What changes **Root cause:** #660 made `unit-tests.yml` and `e2e.yml` listen for `edited`, so that a base move would re-run their `changes` job. For any other edit, `changes` is skipped and every job after it is skipped too. GitHub treats the newest run of a workflow on a commit as that commit's checks. A skipped matrix job is reported with its name unexpanded (`unit (${{ matrix.os }})`), so the required `unit (macos-latest)` and the rest disappear from the newest run. **Fix:** both workflows stop listening for `edited`, so a title or description edit starts nothing in them and the earlier results stand. A new `.github/workflows/base-change.yml` handles a base move instead. It finds the latest Unit tests and E2E runs for the PR's head and re-runs them with `gh run rerun`; a run that is still in progress is cancelled first and re-run once it stops. A re-run is a new attempt of the same run rather than a newer run, so it replaces those checks directly. The `changes` job reads the PR's file list from the API at run time, so the re-run sees the new base: a Markdown-only stacked PR moved onto trunk is evaluated as code again, which is the guarantee #660 added. Deliberately not in it: re-running anything for a fork PR. A fork's token is read-only, so the workflow posts a warning instead (see Risks). ## How to test this Platforms: none. This is CI only, and it runs on GitHub's Linux runners. The workflows only take effect once this is on trunk: `edited` runs use the workflow files from the PR's merge commit, so a PR opened after this merges, or one that is pushed to after it merges, picks them up. **Starting state:** this branch merged to trunk. You need a pull request against trunk whose head was pushed after the merge, with all six required checks passed. 1. Edit that PR's description and save. Then edit its title. Expected: no new run of **Unit tests** or **E2E** appears in the Actions tab. **Re-run on base change** shows a run in which the `rerun` and `fork` jobs are both skipped. In the merge box, all six required checks are still ✓, not "Expected — Waiting for status to be reported". 2. Make a stacked pair: PR A (any code change) against trunk, and PR B with one Markdown-only commit on top of A's branch, opened against A's branch. Wait for B's checks. Expected: B passes the six checks through the stub jobs ("Only Markdown changed"). 3. On PR B, choose **Edit** next to the title and change the base from A's branch to `trunk`. Expected: a **Re-run on base change** run whose `rerun` job prints `re-running run <id>` for `unit-tests.yml` and `e2e.yml`. Unit tests and E2E each show a second attempt of their earlier run. In it, `changes` reports `code=true` (B now carries A's code), and the real macOS and Windows jobs run in place of the stubs. 4. Optional: repeat step 3 while B's Unit tests run is still in progress (push a Markdown commit to B, then retarget it straight away). Expected: the in-progress run is cancelled and then re-run, and the job log shows both. **What must not have happened:** - The six required checks going to "Expected" after step 1. This is the bug: on trunk today, step 1 reproduces it. - The stub passes surviving step 3. That would mean B could merge A's code without the suites having run on it. - A push made right after a retarget having its new run cancelled. The job compares the PR's live head with the event's head immediately before each re-run, and stops if they differ. **Cannot be tested here:** none of this runs before merge (see Platforms), and there is no unit test for workflow files. In particular, it is unverified that `GITHUB_TOKEN` with `actions: write` may cancel and re-run a `pull_request` run in this repository. Steps 3 and 4 are the check for that, and the first stacked retarget after merge should be watched. The workflows pass `actionlint` (with shellcheck), and the `gh run list` lookup was run against #675's real runs. ## Risks and limitations - **A re-run tests the old merge commit.** A re-run keeps the original event, so the suites check out the head merged into the *old* base. Only the Markdown-only decision uses the new base. That is what #660's guarantee needs, and it matches every PR run once its base branch has moved on. #660's `edited` runs did check out a fresh merge with the new base. - **Fork PRs:** a fork PR whose base moves is not re-run. A `::warning::` asks for a manual re-run or a push. `pull_request_target` would have a write token, but re-running a fork's run with it might bypass the approval that a first-time contributor's run waits for, so I did not use it. It is rare: a fork PR based on an upstream branch other than trunk. - **Runs older than 30 days:** GitHub refuses to re-run them. The job fails with an error that says to push, or to close and reopen the PR. That check is not required, so it does not block the merge on its own. - **PRs already open when this merges:** where the newest run on the head is one of the old skipped `edited` runs (as on #675 now), re-running it replays that skip. A push, or re-running the earlier passing run from the Actions tab, clears it once. - Review: 6 [fix here] · 1 [follow-up]. Five fixed; the remaining [fix here] (no live run yet) is covered by How to test steps 3–4. The follow-up is the open-PR transition above, noted rather than coded. ## Related Follow-up to #660. Seen on #675. --- <details> <summary>Design decisions and alternatives considered</summary> - **Keep `edited`, but give the matrix jobs expanded names when skipped:** not possible. GitHub does not expand a matrix for a job that is skipped at the job level, which is why #660 added stub jobs in the first place. - **Keep `edited` and run the stubs/real jobs again on every edit:** every title edit would cost two Linux jobs at best and the full macOS/Windows suites at worst, and it would cancel or duplicate runs. - **`workflow_dispatch` from the edit workflow:** this would start a fresh run on the head with the new base. But it is a different event, so the `pull_request`-only logic in `changes` would need a second branch, and the checks would come from a run nobody pushed. A re-run reuses everything that already exists. - **Concurrency:** base moves share the group `base-change-<PR>`, so a second move cancels the first and re-runs whatever the first left behind. Any other edit gets a group per run (`-<run_id>`), so it cannot cancel a base move. The concurrency group is evaluated before the job's `if:`, so without this a description edit made during a base move would have cancelled it. </details> <details> <summary>Review outcome</summary> 6 [fix here] · 1 [follow-up]: 5 fixed, 1 covered by How to test, 1 follow-up recorded in Risks. `npm run lint` clean; unit suite 2096 pass, 0 fail, 2 skipped (local, macOS). `actionlint` clean on all workflows. - **Review:** completed. Reviewer: a separate agent context, read-only. Head `da2dbfa`, base `7ee3ead`. Findings: - 🟡 A description edit could cancel a base-move run, because the concurrency group is evaluated before `if:`. Fixed. - 🟡 Re-running the old head's run after a push would cancel the new head's run through the shared concurrency group. Fixed. - 🟡 The 30-day re-run limit failed silently and aborted before the second workflow. Fixed. - 🔵 The fork regression was unannounced. Fixed with a warning job. - 🔵 [follow-up] PRs open across the merge. Recorded in Risks. - 🔵 The GitHub semantics are unverified in practice. Covered by How to test steps 3–4. - **Since review:** `da2dbfa → 667bd95` re-reviewed by the same separate context. #1–#4 resolved. One new 🔵: a push during the cancel-and-wait loop could still be cancelled. Fixed in `667bd95 → 8b5ea25`, which moves the head check to just before each re-run. I checked that last change myself; it was not reviewed again. - **CodeRabbit:** not requested yet. </details> <details> <summary>Implementation notes</summary> - The run lookup is `gh run list --workflow <file> --event pull_request --commit <head sha> --branch <head ref> --limit 1`. `--commit` matches the run's head SHA, not the merge commit's, and `--event` leaves out trunk push runs. - The job needs `actions: write` (cancel, re-run) and `pull-requests: read` (the head check). The workflow's default permissions are `{}`. Every value from the event reaches the script through `env:`, never interpolated into it. - `if ! gh run rerun` keeps a failure for one workflow from skipping the other under `bash -e`. The job then exits non-zero. - The `changes` jobs lose their `if:`, and the concurrency groups lose their `-edit` suffix, since neither workflow sees `edited` any more. </details> Screenshots: nothing on screen changed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Why
The old "next thing to do" used hardcoded values that had no space in the WDS, along with other page elements. What the user needs to do should "stand out" on the page, which in CSS is an outline. In our case, it's in the
--wpds-color-stroke-surface-brandcolor.The cue that points a contributor at the next thing to do (#252) could not be seen on the block it points at most often. On the "This site's WordPress code is N days old" notice, the ring was drawn in
--wpds-color-stroke-surface-warning, which is the border a warning notice already has, so the ring was just that border again. The rest ofshell.cssstill had a handful of values written by hand where the WordPress Design System has a token, or used a token for a job it is not meant for.What changes
--wpds-color-stroke-surface-brandat--wpds-border-width-md, replacing the 3px warning-coloured ring plus a 14px glow mixed withcolor-mix().size-4xsand are painted in the foreground role an icon takes (background: currentColor), instead of a surface stroke used as a fill.--wpds-cursor-control.Deliberately left out: layout sizes the design system has no token for (breakpoints, column widths, the content width, pane heights, the toast stack's position). Also left out are the overrides of the older
@wordpress/componentsdialogs and popovers, which only go away by moving those dialogs to@wordpress/ui.How to test this
Platforms: any. Nothing here touches paths, spawning or signing.
Covered by a journey:
npx playwright test --project=journeys site-page.spec.js, run from the repository root. The test "a site whose code is old…" now checks that the cue's ring is drawn, is not the colour of the warning notice's border, and is not the focus colour. It is red on trunk's rule and on this branch's first attempt (the strong brand stroke), and green now.What the journey cannot judge is whether the band looks right, so this is a by-eye check on the current head's Buildkite build.
Starting state: a site whose setup is finished, with its dev server stopped.
What must not have happened: the band must not be cut off at any edge, and the cued block must not move or change size when the band appears or goes. The status dots in the sites list and the header must look the same as before, in both themes.
Risks and limitations
outline-offsetwould separate them whatever the colours; it is not done here.#936300,#cc1818). For the dark theme I relied on the reviewer's reading of@wordpress/theme's colour ramps, which says the new tokens have at least the old ones' contrast; I did not take a screenshot.Related
Follow-up to #252.
Design decisions and alternatives considered
stroke-surface-brand-strongandstroke-focusare the same step of the same ramp in every theme), so a keyboard user would see two focus rings.border-radiusitself so the shadow would curve, which forced the header buttons into 8px corners. A shadow also disappears in forced-colours mode; an outline does not.Review outcome (required — see AGENTS.md)
0 [fix here] open · 1 [follow-up]. The two earlier [fix here] findings are fixed.
.github/instructions/code-review.instructions.md. Head e5cef6f, base 7ee3ead. 2 [fix here]:git diff e5cef6f 58f7fd1plus the clipping and placement checks again. Both earlier findings are resolved. 1 new [follow-up]: 🔵 the custom-theme case above, deferred because it is narrow and still an improvement on trunk.eslint . --ignore-pattern '.claude/worktrees/**'andstylelint "src/renderer/**/*.css"are clean. Plainnpm run lintalso lints the local.claude/worktreescheckouts, which are not part of the repository.npm test: 2096 passed, 2 skipped.@coderabbitai review. No actionable comments. It also noted the custom-theme case already listed under Risks.Screenshots or recording
The visible change is the cue's ring, shown on the warning notice with the notice's own button focused. I'll attach the screenshots in a comment, since images cannot be added to a PR description from the command line.
🤖 Generated with Claude Code