yonlu/yellowjacket
1
1
Fork 0
Code Issues 55 Pull Requests 6 Actions Packages Projects Releases 12 Wiki Activity

make ui-visual has been failing on main and nothing runs it #196

New Issue
Closed
opened 2026-08-22 02:43:52 +00:00 by logan · 3 comments
logan commented 2026-08-22 02:43:52 +00:00
Collaborator
Copy Link
Copy Source

Report

make ui-visual fails on main (52038dc) with four stored
screenshots out of date, and nothing runs it, so nobody found out. CI's
check job runs make ui-test, which is the same suite with the
comparisons off (render.ts's visualEnabled gate) -- so the visual
tier is the only one in this repo that is never executed by anything
but a person choosing to.

FAIL chrome.test.ts       <app-sidebar>   expected 180x366, got 180x335
FAIL now-playing.test.ts  <now-playing>   expected 1152x58, got 1152x65
FAIL page-header.test.ts  title-count-and-sort    expected 810x45, got 810x61
FAIL page-header.test.ts  filtered-by-search      expected 810x45, got 810x61

Findings

  • All four are stale references, not regressions, and each records
    a change that shipped deliberately. Read side by side:
    • app-sidebar's reference still lists Jobs, the destination
      #27 retired. That reference has been wrong since #27 merged.
    • now-playing gained a third line ("Playing from a dynamic mix")
      and the reference has two.
    • both page-header shots are #195's taller sort control, merged
      last session as the first half of #186.
  • So the failures span at least three separate pieces of work,
    which is the actual finding: this is not one forgotten refresh, it
    is a tier that has been red long enough to accumulate unrelated
    debris. git log on test/components/__screenshots__/ has three
    commits, the most recent being 862e8a0.
  • The suite is not merely unrun, it is unrunnable as a gate in its
    current state    : a person who runs it today gets four failures that
    are nothing to do with what they changed, which is the strongest
    possible incentive to stop running it. That is how the sidebar
    reference survived #27.
  • Found while measuring #186's second pass -- the Settings work is
    clean, but make ui-visual was one of the tiers that pass was meant
    to be judged in, and it could not be read until these were separated
    out.
  • Worth holding beside CLAUDE.md's own note on this tier: make ui-visual renders the fallback colours, not the theme, because the
    theme only reaches :root in the real app. So it is a layout tier,
    and layout is exactly what these four are recording.

Direction

Two halves, and the first is nearly free.

Refresh the three references that are behind shipped work, having read
each diff rather than blessing them -- #196 refreshes the two
page-header shots, since those are #186's own debris and it is the
issue that found this. app-sidebar and now-playing are deliberately
left, because blessing an unrelated screenshot is how this happened.

The second half is whether this tier gates. Three options, and the
choice wants an argument rather than a default:

  1. Run make ui-visual in CI's check job instead of
    make ui-test. It is the same 1,029 tests plus 30-odd comparisons
    and costs nothing extra in time -- but a screenshot comparison
    across machines is the classic flake, and the references here were
    taken on a developer's Arch box while CI is ubuntu:24.04, so this
    probably fails on font rendering alone until the references are
    regenerated in the container.
  2. Keep it local but make it loud -- a pre-push hook, or a line in
    the skill saying to run it after any stylesheet change.
  3. Delete the stored screenshots and keep the suite as a
    render-without-crashing tier, which is what make ui-test already
    is. That is the honest option if nobody is going to look at the
    images, and this issue is evidence that nobody has.

.pi/skills/yellowjacket-dev/references/ui-tier.md is where whichever
answer wins gets written down.

**Report** `make ui-visual` fails on `main` (`52038dc`) with four stored screenshots out of date, and nothing runs it, so nobody found out. CI's `check` job runs `make ui-test`, which is the same suite with the comparisons off (`render.ts`'s `visualEnabled` gate) -- so the visual tier is the only one in this repo that is never executed by anything but a person choosing to. ``` FAIL chrome.test.ts <app-sidebar> expected 180x366, got 180x335 FAIL now-playing.test.ts <now-playing> expected 1152x58, got 1152x65 FAIL page-header.test.ts title-count-and-sort expected 810x45, got 810x61 FAIL page-header.test.ts filtered-by-search expected 810x45, got 810x61 ``` **Findings** - **All four are stale references, not regressions**, and each records a change that shipped deliberately. Read side by side: - `app-sidebar`'s reference still lists **Jobs**, the destination #27 retired. That reference has been wrong since #27 merged. - `now-playing` gained a third line ("Playing from a dynamic mix") and the reference has two. - both `page-header` shots are #195's taller sort control, merged last session as the first half of #186. - **So the failures span at least three separate pieces of work**, which is the actual finding: this is not one forgotten refresh, it is a tier that has been red long enough to accumulate unrelated debris. `git log` on `test/components/__screenshots__/` has three commits, the most recent being `862e8a0`. - **The suite is not merely unrun, it is unrunnable as a gate in its current state**: a person who runs it today gets four failures that are nothing to do with what they changed, which is the strongest possible incentive to stop running it. That is how the sidebar reference survived #27. - Found while measuring #186's second pass -- the Settings work is clean, but `make ui-visual` was one of the tiers that pass was meant to be judged in, and it could not be read until these were separated out. - Worth holding beside CLAUDE.md's own note on this tier: **`make ui-visual` renders the fallback colours, not the theme**, because the theme only reaches `:root` in the real app. So it is a layout tier, and layout is exactly what these four are recording. **Direction** Two halves, and the first is nearly free. Refresh the three references that are behind shipped work, having read each diff rather than blessing them -- #196 refreshes the two `page-header` shots, since those are #186's own debris and it is the issue that found this. `app-sidebar` and `now-playing` are deliberately left, because blessing an unrelated screenshot is how this happened. The second half is whether this tier gates. Three options, and the choice wants an argument rather than a default: 1. **Run `make ui-visual` in CI's `check` job** instead of `make ui-test`. It is the same 1,029 tests plus 30-odd comparisons and costs nothing extra in time -- but a screenshot comparison across machines is the classic flake, and the references here were taken on a developer's Arch box while CI is `ubuntu:24.04`, so this probably fails on font rendering alone until the references are regenerated *in the container*. 2. **Keep it local but make it loud** -- a pre-push hook, or a line in the skill saying to run it after any stylesheet change. 3. **Delete the stored screenshots** and keep the suite as a render-without-crashing tier, which is what `make ui-test` already is. That is the honest option if nobody is going to look at the images, and this issue is evidence that nobody has. `.pi/skills/yellowjacket-dev/references/ui-tier.md` is where whichever answer wins gets written down.
logan added the Area/Design
Priority
Medium
3
Reviewed
Confirmed
1
Kind/Testing labels 2026-08-22 02:43:52 +00:00
logan commented 2026-08-22 02:44:00 +00:00
Author
Collaborator
Copy Link
Copy Source

Correction to the Direction above: the two page-header references are
refreshed in #186's second-pass branch (186-touch-targets-settings),
not in this issue -- the body's "#196 refreshes" is self-referential and
wrong. This issue is the tier, not the refresh.

The two left standing are app-sidebar and now-playing, deliberately.

Correction to the Direction above: the two `page-header` references are refreshed in **#186**'s second-pass branch (`186-touch-targets-settings`), not in this issue -- the body's "#196 refreshes" is self-referential and wrong. This issue is the tier, not the refresh. The two left standing are `app-sidebar` and `now-playing`, deliberately.
logan self-assigned this 2026-08-23 07:34:07 +00:00
logan added the
Status
In Progress
 label 2026-08-23 07:34:08 +00:00
logan commented 2026-08-23 07:34:08 +00:00
Author
Collaborator
Copy Link
Copy Source

Picking this up. Branch test/196-visual-tier-gates.

Reproduced on main (ee1d8b3): the two page-header shots the
comment above assigns to #186 now pass — they were refreshed in
2100f00 — so exactly the two this issue keeps are red:

FAIL chrome.test.ts       <app-sidebar>  expected 180x366, got 180x335
FAIL now-playing.test.ts  <now-playing>  expected 1152x58, got 1152x65

Approach: read each diff before refreshing it, then answer the second
half — whether the tier gates — with a measurement rather than a
preference. The whole question hangs on whether a baseline recorded on
Arch survives CI's ubuntu:24.04, so I am going to run the visual
suite in that container against the committed baselines and let the
number decide between options 1, 2 and 3. Whichever wins is written
into .pi/skills/yellowjacket-dev/references/ui-tier.md.

Picking this up. Branch `test/196-visual-tier-gates`. Reproduced on `main` (`ee1d8b3`): the two `page-header` shots the comment above assigns to #186 now pass — they were refreshed in `2100f00` — so exactly the two this issue keeps are red: ``` FAIL chrome.test.ts <app-sidebar> expected 180x366, got 180x335 FAIL now-playing.test.ts <now-playing> expected 1152x58, got 1152x65 ``` Approach: read each diff before refreshing it, then answer the second half — whether the tier gates — with a measurement rather than a preference. The whole question hangs on whether a baseline recorded on Arch survives CI's `ubuntu:24.04`, so I am going to run the visual suite in that container against the committed baselines and let the number decide between options 1, 2 and 3. Whichever wins is written into `.pi/skills/yellowjacket-dev/references/ui-tier.md`.
logan commented 2026-08-23 07:58:49 +00:00
Author
Collaborator
Copy Link
Copy Source

PR: #205 — CI green (check and e2e, run 544). Not merged; leaving
Status/In Progress on.

Both halves are answered in it.

The refresh: the two references this issue kept were red, and reading
each diff turned up something the issue could not have known — both
recorded two changes, the shipped one and a leak. app-sidebar lost
Jobs (#27) and moved its highlight Home → Tracks; now-playing
gained the source line and was playing from "a dynamic mix". The
stores are singletons and neither case sets one, so each shot
photographed whatever the case above it left behind. --update alone
would have pinned each file's own test ordering into a PNG. Both cases
state their world now, and then the references were re-recorded.

The gate: answered with a container run rather than a preference. The
committed baselines replayed in a bare ubuntu:24.04 — CI's check
image — fail three ways that have nothing to do with any component
(track-info and page-header filtered-by-search at ratio 0.03
against a 0.02 allowance, seek-bar at 1152×17 against a stored
1152×18), and the two stale ones disagree about their new height
between the machines (now-playing 1152×65 here, 1152×64 there). So
option 1 costs a second baseline set, which every local run would
then fail against — the same trap with the machines swapped — and a
pre-push hook is that fault again. Option 3 is declined on the one
regression this tier has caught that nothing else could (the <span>
that lost box-sizing and grew a badge 36→38px, NOTES.md).

So option 2, written where a person meets it rather than as a line
nobody reads: references/ui-tier.md carries the table and what the
tier is and is not worth, SKILL.md's "rules about climbing" carries
the obligation, CLAUDE.md carries the constraint. The rule is a change
that moves a component's geometry refreshes that component's baseline
in the same commit, having read the image, and never one it did not
cause — plus a visual case states the world it photographs.

Filed while here: #204 — make ui-visual-update UI_ARGS=<path>
re-records every baseline, because vitest's --update takes the
following positional as its value. That is this issue's own hazard
living in the tool meant to fix it; it bit this session and is
deliberately left out of the diff.

Not done, and it is the door left open: a container-recorded second
baseline set is what option 1 would have to pay for. The measurement
above is the price.

PR: #205 — CI green (`check` and `e2e`, run 544). Not merged; leaving `Status/In Progress` on. **Both halves are answered in it.** The refresh: the two references this issue kept were red, and reading each diff turned up something the issue could not have known — both recorded *two* changes, the shipped one and a leak. `app-sidebar` lost Jobs (#27) **and** moved its highlight Home → Tracks; `now-playing` gained the source line **and** was playing from "a dynamic mix". The stores are singletons and neither case sets one, so each shot photographed whatever the case above it left behind. `--update` alone would have pinned each file's own test ordering into a PNG. Both cases state their world now, and then the references were re-recorded. The gate: answered with a container run rather than a preference. The committed baselines replayed in a bare `ubuntu:24.04` — CI's `check` image — fail **three** ways that have nothing to do with any component (`track-info` and `page-header` filtered-by-search at ratio 0.03 against a 0.02 allowance, `seek-bar` at 1152×17 against a stored 1152×18), and the two stale ones disagree about their *new* height between the machines (`now-playing` 1152×65 here, 1152×64 there). So **option 1 costs a second baseline set**, which every local run would then fail against — the same trap with the machines swapped — and a pre-push hook is that fault again. **Option 3 is declined** on the one regression this tier has caught that nothing else could (the `<span>` that lost `box-sizing` and grew a badge 36→38px, `NOTES.md`). So **option 2**, written where a person meets it rather than as a line nobody reads: `references/ui-tier.md` carries the table and what the tier is and is not worth, `SKILL.md`'s "rules about climbing" carries the obligation, CLAUDE.md carries the constraint. The rule is *a change that moves a component's geometry refreshes that component's baseline in the same commit, having read the image, and never one it did not cause* — plus *a visual case states the world it photographs*. **Filed while here:** #204 — `make ui-visual-update UI_ARGS=<path>` re-records **every** baseline, because vitest's `--update` takes the following positional as its value. That is this issue's own hazard living in the tool meant to fix it; it bit this session and is deliberately left out of the diff. **Not done, and it is the door left open:** a container-recorded second baseline set is what option 1 would have to pay for. The measurement above is the price.
logan closed this issue 2026-08-25 16:39:33 +00:00
gitea-actions bot removed the
Status
In Progress
 label 2026-08-25 16:42:34 +00:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
Area/Design
Area/Downloads
Area/Explore
Area/Library-UI
Area/Metadata
Area/Packaging
Area/Player
Area/Queue
Area/Settings
Area/Shell-Nav
Compat/Breaking
Breaking change that won't be backward compatible
 Kind/Bug
Something is not working
 Kind/Documentation
Documentation changes
 Kind/Enhancement
Improve existing functionality
 Kind/Feature
New functionality
 Kind/Security
This is security issue
 Kind/Testing
Issue or pull request related to testing
 Platform/Android
Platform/Desktop
Priority
Critical
1
The priority is critical
Priority
High
2
The priority is high
Priority
Medium
3
The priority is medium
Priority
Low
4
The priority is low
Reviewed
Confirmed
1
Issue has been confirmed
Reviewed
Duplicate
2
This issue or pull request already exists
Reviewed
Invalid
3
Invalid issue
Reviewed
Won't Fix
3
This issue won't be fixed
Status
Blocked
1
Something is blocking this issue or pull request
Status
Need More Info
2
Feedback is required to reproduce issue or to continue work
Status
Abandoned
3
Somebody has started to work on this but abandoned work
Status
In Progress
Somebody is actively working on this right now
No labels Area/Design Kind/Testing
Priority
Medium
3
Reviewed
Confirmed
1
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
logan yonlu
No Assignees logan
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: yonlu/yellowjacket#196