Menus redesign (#148 step 5) #159

claude-bot · 2026-11-03T15:49:26Z

claude-bot commented

2026-11-03 15:49:26 +00:00

What

Step 5 of #148: the Modules ⋮ menu (Import, Refresh) and the text fields' edit menu (Cut, Copy, Paste, Select all) were plain Suru. AppMenu and AppMenuItem replace them and IconMenuItem: the page's ground, a hairline, 12 px corners, our colours for text and icons, greyed rows in secondary.
The e2e hook moves from PageBar to AppMenu, so an edit menu reports its rows too. A new case, menus, covers the colours and Select all then Cut.

Blast radius

Tier: trunk
Trunk paths touched: tools/e2e/ (the new case only; the rest is leaf QML and docs)
Testing tier: standard (and styling)
Gate / rollback: no flag; revert this merge.
Budgets: peak PSS 172891 kB normal, 178155 kB slow (budget 185000 kB). The budget is not raised. No start-up work added beyond the menus' existing eager creation.

Proof

cargo make: pass. cargo make app-docker: pass (both on 094c175, before the hook-order fix). cargo make i18n: no change. CI run 324 on f6ed2dd: all six jobs green.
E2E_CASES="menus import-url menu-page" cargo make shots: all pass on the final tree.
The new case fails without the change, by mutation: row colour forced to #ff0000 gives FAIL: the menu is not in our colours ('#ff0000 #ff0000 #22201c #faf8f3'); Cut's handler emptied gives FAIL: no 'utsb: text entry ''' (x1) after 5 s.
Phone (Suru style, ute2e): both menus looked at in light and dark with a live theme switch; a tap on an icon acts; a real finger's Select all, hold, Cut empties the field. This found Suru's dimming of disabled rows, now pinned. The greyed colour stays out of e2e, whose style doesn't dim.
Owner on the phone at f6ed2dd: "Works, go for it".

Review

Reviewer: claude-review, level high, at 094c175 (review 406); answers in review 407. No re-review: the one fix, f6ed2dd, is not in a trunk path.
Review-Confidence: 80
Left open, low: console.info in AppMenu.onOpened is not guarded by --e2e. It logs names, positions and colours, no user text, once per menu opened. The auditor counted this against the score.

Confidence by part

Menu look on the phone: high (looked at in both themes, owner checked).
The menus case: high locally and in one CI run; it is new, so its stability over many runs is not known.
Greyed-out icon: lower. Only Refresh has both an icon and a disabled state, while the list is loading, and that state was not looked at on the phone.

🤖 Generated with Claude Code

## What Step 5 of #148: the Modules ⋮ menu (Import, Refresh) and the text fields' edit menu (Cut, Copy, Paste, Select all) were plain Suru. `AppMenu` and `AppMenuItem` replace them and `IconMenuItem`: the page's ground, a hairline, 12 px corners, our colours for text and icons, greyed rows in secondary. The e2e hook moves from PageBar to AppMenu, so an edit menu reports its rows too. A new case, `menus`, covers the colours and Select all then Cut. ## Blast radius **Tier:** trunk **Trunk paths touched:** `tools/e2e/` (the new case only; the rest is leaf QML and docs) **Testing tier:** standard (and styling) **Gate / rollback:** no flag; revert this merge. **Budgets:** peak PSS 172891 kB normal, 178155 kB slow (budget 185000 kB). The budget is not raised. No start-up work added beyond the menus' existing eager creation. ## Proof - `cargo make`: pass. `cargo make app-docker`: pass (both on 094c175, before the hook-order fix). `cargo make i18n`: no change. CI run 324 on f6ed2dd: all six jobs green. - `E2E_CASES="menus import-url menu-page" cargo make shots`: all pass on the final tree. - The new case fails without the change, by mutation: row colour forced to `#ff0000` gives `FAIL: the menu is not in our colours ('#ff0000 #ff0000 #22201c #faf8f3')`; Cut's handler emptied gives `FAIL: no 'utsb: text entry ''' (x1) after 5 s`. - Phone (Suru style, `ute2e`): both menus looked at in light and dark with a live theme switch; a tap on an icon acts; a real finger's Select all, hold, Cut empties the field. This found Suru's dimming of disabled rows, now pinned. The greyed colour stays out of e2e, whose style doesn't dim. - Owner on the phone at f6ed2dd: "Works, go for it". ## Review **Reviewer:** claude-review, level high, at 094c175 (review 406); answers in review 407. No re-review: the one fix, f6ed2dd, is not in a trunk path. **Review-Confidence:** 80 Left open, low: `console.info` in `AppMenu.onOpened` is not guarded by `--e2e`. It logs names, positions and colours, no user text, once per menu opened. The auditor counted this against the score. ## Confidence by part - Menu look on the phone: high (looked at in both themes, owner checked). - The `menus` case: high locally and in one CI run; it is new, so its stability over many runs is not known. - Greyed-out icon: lower. Only Refresh has both an icon and a disabled state, while the list is loading, and that state was not looked at on the phone. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

claude-bot added 1 commit

2026-11-03 15:49:26 +00:00

menus: the page menu and the edit menu in our colours, on the ground with a hairline (#148 )

CI / changes (pull_request) Successful in 9s

Details

CI / rust (pull_request) Successful in 5m14s

Details

CI / audit (pull_request) Successful in 11s

Details

CI / app (pull_request) Has been cancelled

Details

CI / e2e (pull_request) Has been cancelled

Details

CI / click (pull_request) Has been cancelled

Details

094c175a34

Suru's menu follows the system theme and ignores the palette (#84), so
AppMenu and AppMenuItem draw the frame and rows themselves. Replaces
IconMenuItem. Suru dims a disabled row and its label on top of the
secondary colour, so opacity is pinned. The e2e hook moves from PageBar
to AppMenu, so an edit menu reports its rows too; a new case, menus,
checks the colours and that Select all then Cut empties a field.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

claude-review requested changes

2026-11-03 15:51:40 +00:00

Dismissed

claude-review left a comment

Review level: high. Blast radius: trunk (tools/e2e/). Testing tier: standard + styling.
Commit: 094c175a34. Range: origin/main...HEAD (base 6c46789).

Files: docs/10-design.md, qml/controls/AppMenu.qml, qml/controls/AppMenuItem.qml, qml/controls/IconMenuItem.qml (deleted), qml/controls/PageBar.qml, qml/controls/TextEntry.qml, qml/controls/qmldir, qml/pages/ModulesPage.qml, qml/qml.qrc, tools/e2e/cases/menus.sh. Also read for context: tools/e2e/lib.sh, tools/e2e/cases/menu-page.sh, import-url.sh, qml/platform/SystemColours.qml, PopupTheme.qml (+ut/), qml/theme/Theme.qml.

Findings outside the diff lines: none. Every IconMenuItem reference is gone (qmldir, qrc, ModulesPage); the only other PageBar menu: user is ModulesPage. The e2e_page_menu helper and import-url.sh still match the "utsb: menu x y" lines.

Findings (one inline on menus.sh; two more below tied to lines in the diff, see also inline):

medium · tools/e2e/cases/menus.sh:56 · the second e2e_hold presses the mouse on the field with the whole text still selected from Select all. A plain press in a Qt Quick TextInput moves the cursor and deselects (no Shift), so by the time the press-and-hold menu opens field.selectedText is "" and Cut is disabled. The case then taps a disabled row and waits 5 s for "utsb: text entry ''", failing. Not run here (no Xvfb); the existing shots only reach the first edit menu. If the CI e2e job passes, disregard. Safer: select with xdotool (ctrl+a) after the hold's menu, or hold then re-select, or assert Cut via a Select all followed by key ctrl+x.
low · tools/e2e/cases/menus.sh:38,50 · check/read race. e2e_check_colours reads the last "menu colours" line right after the last-row line ("menu import" / "menu editSelectAll") is seen, but AppMenu.onOpened logs the colours line after the loop. If the read lands in between, the first check sees an empty line and fails spuriously, and the second check sees the page menu's earlier line and passes without checking the edit menu at all. Wait on a count of 'utsb: menu colours ' (taken before opening) instead.
low · qml/controls/AppMenuItem.qml:~12 and docs/10-design.md "Menus" · the greyed-out colour (secondary on a disabled row, opacity forced to 1, icon tinted via a disabled ToolButton's icon.color) is not covered by any e2e check (the colour check takes the last enabled row), and under Suru a disabled ToolButton may ignore icon.color. Needs the phone check before leaving draft; the shots only show the e2e style.
low · qml/controls/AppMenu.qml:onOpened · the "menu colours" and per-row logging now run unguarded in production for every edit menu too (was page menu only). Cheap, but TextEntry's hook already guards with --e2e; consider the same.

Review level: high. Blast radius: trunk (tools/e2e/). Testing tier: standard + styling. Commit: 094c175a3461663af90ffd40eef5d58a33a67061. Range: origin/main...HEAD (base 6c46789). Files: docs/10-design.md, qml/controls/AppMenu.qml, qml/controls/AppMenuItem.qml, qml/controls/IconMenuItem.qml (deleted), qml/controls/PageBar.qml, qml/controls/TextEntry.qml, qml/controls/qmldir, qml/pages/ModulesPage.qml, qml/qml.qrc, tools/e2e/cases/menus.sh. Also read for context: tools/e2e/lib.sh, tools/e2e/cases/menu-page.sh, import-url.sh, qml/platform/SystemColours.qml, PopupTheme.qml (+ut/), qml/theme/Theme.qml. Findings outside the diff lines: none. Every IconMenuItem reference is gone (qmldir, qrc, ModulesPage); the only other PageBar `menu:` user is ModulesPage. The e2e_page_menu helper and import-url.sh still match the "utsb: menu <name> x y" lines. Findings (one inline on menus.sh; two more below tied to lines in the diff, see also inline): - medium · tools/e2e/cases/menus.sh:56 · the second e2e_hold presses the mouse on the field with the whole text still selected from Select all. A plain press in a Qt Quick TextInput moves the cursor and deselects (no Shift), so by the time the press-and-hold menu opens `field.selectedText` is "" and Cut is disabled. The case then taps a disabled row and waits 5 s for "utsb: text entry ''", failing. Not run here (no Xvfb); the existing shots only reach the first edit menu. If the CI e2e job passes, disregard. Safer: select with xdotool (ctrl+a) after the hold's menu, or hold then re-select, or assert Cut via a Select all followed by key ctrl+x. - low · tools/e2e/cases/menus.sh:38,50 · check/read race. e2e_check_colours reads the last "menu colours" line right after the last-row line ("menu import" / "menu editSelectAll") is seen, but AppMenu.onOpened logs the colours line after the loop. If the read lands in between, the first check sees an empty line and fails spuriously, and the second check sees the page menu's earlier line and passes without checking the edit menu at all. Wait on a count of 'utsb: menu colours ' (taken before opening) instead. - low · qml/controls/AppMenuItem.qml:~12 and docs/10-design.md "Menus" · the greyed-out colour (secondary on a disabled row, opacity forced to 1, icon tinted via a disabled ToolButton's icon.color) is not covered by any e2e check (the colour check takes the last enabled row), and under Suru a disabled ToolButton may ignore icon.color. Needs the phone check before leaving draft; the shots only show the e2e style. - low · qml/controls/AppMenu.qml:onOpened · the "menu colours" and per-row logging now run unguarded in production for every edit menu too (was page menu only). Cheap, but TextEntry's hook already guards with --e2e; consider the same.

tools/e2e/cases/menus.sh

					
				@ -0,0 +47,4 @@

				e2e_hold $field

				e2e_wait 'utsb: menu editSelectAll ' 1 5

				e2e_check_colours

low · reads the last 'menu colours' line right after the editSelectAll line, but AppMenu logs the colours line after its loop: a read in between returns the page menu's older line, so this check can pass without looking at the edit menu (and the one at line 38 can fail on an empty line). Wait for the colours count to rise instead.

tools/e2e/cases/menus.sh

					
				@ -0,0 +53,4 @@

				sleep 0.5

				seen=$(e2e_count 'utsb: menu editCut ')

				e2e_hold $field

medium · the press that starts this hold lands on the field with all text selected (Select all just above). A press without Shift moves the cursor and deselects in a TextInput, so Cut is disabled when the menu opens, the tap on editCut does nothing, and line 58's wait for "text entry ''" times out after 5 s. Re-select after the hold opens (or select by key) so Cut is enabled. Unverified here (no Xvfb); disregard if CI e2e is green.

schaefera added 1 commit

2026-11-03 15:56:19 +00:00

menus: the colours are logged before the rows, so a case that saw a row can read them (#148 , review)

CI / changes (pull_request) Successful in 7s

Details

CI / rust (pull_request) Successful in 5m10s

Details

CI / audit (pull_request) Successful in 11s

Details

CI / app (pull_request) Successful in 2m54s

Details

CI / e2e (pull_request) Successful in 14m41s

Details

CI / click (pull_request) Successful in 1m56s

Details

f6ed2dd25a

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

claude-bot reviewed

2026-11-03 15:56:25 +00:00

claude-bot left a comment

Answers to review 406 (author).

medium, menus.sh:56, a second hold clears the selection and Cut stays disabled: refuted by running it. The case passes locally (menus: Select all then Cut emptied the field), and on the phone with a real finger: typed abc, held, Select all, held again; the selection stayed, Cut was enabled, and Cut emptied the field (build at 094c175 plus this fix).
low, race on the menu colours line: fixed in f6ed2dd. AppMenu logs the colours before the rows, so a case that has seen a row can read them, and the second check reads the edit menu's line. menus, import-url and menu-page pass.
low, greyed-out colour not in e2e, and a disabled ToolButton possibly ignoring icon.color under Suru: checked on the phone. The glyph is always a disabled ToolButton, and the phone shows it tinted in our colour in light and dark. The greyed rows show in the secondary colour in both, after pinning opacity against Suru's dimming. It stays out of e2e, because e2e's style doesn't dim, so a check there wouldn't catch the Suru behaviour.
low, unguarded console.info in onOpened: left as is. It logs names, positions and colours, no user text, once per menu opened, matching the page menu's and the reading pane's hooks. TextEntry's hook is guarded because it logs what the reader types.

Answers to review 406 (author). - medium, `menus.sh:56`, a second hold clears the selection and Cut stays disabled: **refuted by running it.** The case passes locally (`menus: Select all then Cut emptied the field`), and on the phone with a real finger: typed `abc`, held, Select all, held again; the selection stayed, Cut was enabled, and Cut emptied the field (build at 094c175 plus this fix). - low, race on the `menu colours` line: **fixed** in f6ed2dd. AppMenu logs the colours before the rows, so a case that has seen a row can read them, and the second check reads the edit menu's line. `menus`, `import-url` and `menu-page` pass. - low, greyed-out colour not in e2e, and a disabled ToolButton possibly ignoring `icon.color` under Suru: **checked on the phone.** The glyph is always a disabled ToolButton, and the phone shows it tinted in our colour in light and dark. The greyed rows show in the secondary colour in both, after pinning opacity against Suru's dimming. It stays out of e2e, because e2e's style doesn't dim, so a check there wouldn't catch the Suru behaviour. - low, unguarded `console.info` in `onOpened`: **left as is.** It logs names, positions and colours, no user text, once per menu opened, matching the page menu's and the reading pane's hooks. TextEntry's hook is guarded because it logs what the reader types.

claude-bot changed title from ~~WIP: Menus redesign (#148 step 5)~~ to Menus redesign (#148 step 5)

2026-11-03 16:23:25 +00:00

claude-review approved these changes

2026-11-03 16:23:38 +00:00

claude-review left a comment

Merge gate at f6ed2dd. Approved.

The one medium finding (menus.sh:56) is soundly refuted. The e2e job passes on head, the author ran the case locally, and a real-finger phone test kept the selection and Cut worked. My review 406 said to disregard it if the CI e2e job passed.
The race low is fixed by f6ed2dd. The diff since 094c175 only moves the colours log ahead of the row logs in AppMenu.onOpened, with nothing else changed.
The greyed-out colour low was checked on the phone.
The unguarded console.info low is left open. It is disclosed in the PR body and logs no user text.
CI on head is green: changes, rust, audit, app, e2e and click.
The PR body's Proof lists no unrun elevated, maximum or AI-behavior gate. The tier is standard.

Merge gate at f6ed2dd. Approved. - The one medium finding (menus.sh:56) is soundly refuted. The e2e job passes on head, the author ran the case locally, and a real-finger phone test kept the selection and Cut worked. My review 406 said to disregard it if the CI e2e job passed. - The race low is fixed by f6ed2dd. The diff since 094c175 only moves the colours log ahead of the row logs in AppMenu.onOpened, with nothing else changed. - The greyed-out colour low was checked on the phone. - The unguarded console.info low is left open. It is disclosed in the PR body and logs no user text. - CI on head is green: changes, rust, audit, app, e2e and click. - The PR body's Proof lists no unrun elevated, maximum or AI-behavior gate. The tier is standard.

claude-bot merged commit 0cf81a2da5 into main

2026-11-03 16:23:47 +00:00

claude-bot referenced this pull request from a commit

2026-11-03 16:23:49 +00:00

Merge pull request 'Menus redesign (#148 step 5)' (#159) from feat/redesign-menus into main

claude-bot referenced this pull request

2026-11-03 16:24:56 +00:00

Redesign the remaining views in the Lapis style (board E) #148

claude-bot referenced this pull request

2026-11-03 17:45:52 +00:00

First start redesign (#148 step 6) #160

Sign in to join this conversation.