Modules redesign (#148 step 4) #154

Merged
claude-bot merged 4 commits from feat/redesign-modules into main 2026-11-03 15:27:48 +00:00
Collaborator

What

The module manager in the Lapis style (board E8): rows with a thin progress bar, installed modules first, filter chips that open a choice sheet, and a sheet per module with its details and actions.
The Remove and Import dialogs are restyled through a new PageDialog, which lays itself out because Suru broke the stock layout on the phone.

Differences from E8: a sheet instead of a detail page (the chevron or any row opens it, a disabled Install too); Remove on update rows is in the sheet; Cancel stays in the row's 72 px bar; the note stays in the row. E8 doesn't draw the ⋮ menu (Import, Refresh), which is unchanged here and gets its own PR next.

Blast radius

Tier: trunk
Trunk paths touched: qt/src/module_manager.rs, tools/e2e/lib.sh, tools/e2e/cases/module-manager.sh, tools/e2e/cases/typed-reference.sh
Testing tier: standard (also: ship on sight, for the styling)
Gate / rollback: revert this merge. No schema or stored data changes.
Budgets (docs/07): no measurable change. app-docker passed at 173.6/176.4 MB (normal/slow). A/B over six rounds, base vs branch, sorted MB: normal 186 186 187 195 197 199 vs 182 182 184 197 198 200; slow 181 182 189 196 199 201 vs 185 189 195 197 197 202. The swing is the same binary's noise (#156).

Proof

  • CI on the final commit 7b319ff (run 321): changes, rust, audit, app, e2e and click all green.
  • Two earlier CI failures, on 033b7b5, one case each:
    • scroll-chapters, first attempt: a step of 18 verses against a limit of 15. Ten local runs passed with steps of at most 9, and it passed in the two later CI runs. Load on the runner is the likely cause, not proven: #157 is open and this could fail again.
    • typed-reference, re-run: a close tap straight after a line logged when the page is made, so it could be dropped during the transition. Fixed in 7b319ff. Not reproduced locally (8 of 8 passed before the fix, 5 of 5 after), so the cause comes from the failed log and the code; the fix passed its one CI run.
  • picker-current also fails intermittently, on base too (#155). It passed in all these CI runs.
  • Fails without the change, by breaking core/src/catalog on purpose: no installed-first sort fails the first new test; no bidi isolate fails the second; if false for the update branch fails the second.
  • E2E on the module code: module-manager, import-url, modules-offline, install-queue-cancel pass locally and in CI.
  • Phone (Suru differs from e2e's style): installed and checked with ute2e screenshots: page, sheet, Import and Remove dialogs. The Remove dialog's buttons overlapped its text there, fixed in 033b7b5 and checked again. Owner's phone check: "Works, go for it". That check was on 033b7b5; 7b319ff changes only an e2e case.

Review

Reviewer: fresh-context review at high on 0f73646: 3 low findings, 3 fixed (aa951f5), 0 declined, 0 deferred. Re-review at medium of 0f73646..7b319ff: no findings, by reading the code only.
Review-Confidence: 100/100, every changed trunk file was reviewed, all findings are fixed, CI is green on 7b319ff and the new tests fail without the change. It scores the review, not the code.

Confidence by part

  • core/src/catalog (sort, meta line): high, covered by tests shown to fail without the change.
  • Module page and sheet: high, e2e plus the owner's phone check.
  • PageDialog: medium. Its layout works around Suru's behaviour, checked on the phone for the two dialogs that use it; the next dialog built on it should be checked there too.
  • typed-reference fix: medium. The cause is inferred, not reproduced.

🤖 Generated with Claude Code

## What The module manager in the Lapis style (board E8): rows with a thin progress bar, installed modules first, filter chips that open a choice sheet, and a sheet per module with its details and actions. The Remove and Import dialogs are restyled through a new `PageDialog`, which lays itself out because Suru broke the stock layout on the phone. Differences from E8: a sheet instead of a detail page (the chevron or any row opens it, a disabled Install too); Remove on update rows is in the sheet; Cancel stays in the row's 72 px bar; the note stays in the row. E8 doesn't draw the ⋮ menu (Import, Refresh), which is unchanged here and gets its own PR next. ## Blast radius **Tier:** trunk **Trunk paths touched:** `qt/src/module_manager.rs`, `tools/e2e/lib.sh`, `tools/e2e/cases/module-manager.sh`, `tools/e2e/cases/typed-reference.sh` **Testing tier:** standard (also: ship on sight, for the styling) **Gate / rollback:** revert this merge. No schema or stored data changes. **Budgets (docs/07):** no measurable change. `app-docker` passed at 173.6/176.4 MB (normal/slow). A/B over six rounds, base vs branch, sorted MB: normal 186 186 187 195 197 199 vs 182 182 184 197 198 200; slow 181 182 189 196 199 201 vs 185 189 195 197 197 202. The swing is the same binary's noise (#156). ## Proof - CI on the final commit `7b319ff` (run 321): `changes`, `rust`, `audit`, `app`, `e2e` and `click` all green. - **Two earlier CI failures, on `033b7b5`, one case each:** - `scroll-chapters`, first attempt: a step of 18 verses against a limit of 15. Ten local runs passed with steps of at most 9, and it passed in the two later CI runs. Load on the runner is the likely cause, not proven: #157 is open and this could fail again. - `typed-reference`, re-run: a close tap straight after a line logged when the page is made, so it could be dropped during the transition. Fixed in `7b319ff`. Not reproduced locally (8 of 8 passed before the fix, 5 of 5 after), so the cause comes from the failed log and the code; the fix passed its one CI run. - `picker-current` also fails intermittently, on base too (#155). It passed in all these CI runs. - Fails without the change, by breaking `core/src/catalog` on purpose: no installed-first sort fails the first new test; no bidi isolate fails the second; `if false` for the update branch fails the second. - E2E on the module code: `module-manager`, `import-url`, `modules-offline`, `install-queue-cancel` pass locally and in CI. - Phone (Suru differs from e2e's style): installed and checked with `ute2e` screenshots: page, sheet, Import and Remove dialogs. The Remove dialog's buttons overlapped its text there, fixed in `033b7b5` and checked again. Owner's phone check: "Works, go for it". That check was on `033b7b5`; `7b319ff` changes only an e2e case. ## Review **Reviewer:** fresh-context review at high on `0f73646`: 3 low findings, 3 fixed (`aa951f5`), 0 declined, 0 deferred. Re-review at medium of `0f73646..7b319ff`: no findings, by reading the code only. **Review-Confidence:** 100/100, every changed trunk file was reviewed, all findings are fixed, CI is green on `7b319ff` and the new tests fail without the change. It scores the review, not the code. ## Confidence by part - `core/src/catalog` (sort, meta line): high, covered by tests shown to fail without the change. - Module page and sheet: high, e2e plus the owner's phone check. - `PageDialog`: medium. Its layout works around Suru's behaviour, checked on the phone for the two dialogs that use it; the next dialog built on it should be checked there too. - `typed-reference` fix: medium. The cause is inferred, not reproduced. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
modules: the module manager in Lapis, with a sheet per module (#148)
Some checks failed
CI / changes (pull_request) Successful in 18s
CI / rust (pull_request) Has been cancelled
CI / audit (pull_request) Has been cancelled
CI / app (pull_request) Has been cancelled
CI / e2e (pull_request) Has been cancelled
CI / click (pull_request) Has been cancelled
0f7364636a
Board E8: a search row, filter chips that open a sheet of choices, INSTALLED
first, rows with a one-line title and a meta line, an outlined action and a
thin progress bar. The language name sits in a bidi isolate, so a Hebrew name
no longer moves the size round to its left (Aleppo).

There is no detail page, so a tap on a row or its chevron opens a sheet with
the full description, language, version, size and Remove, which still asks
first. Cancel stays beside the bar of a running download.

BottomSheet now parents to the window's overlay: declared in a page with a
bar, it sat that bar's height too low and cut off its last row.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude-review left a comment

Review by claude-review. Level: high. Blast radius: trunk. Commit 0f73646. Range: origin/main...HEAD.

Files reviewed: core/src/catalog.rs, core/src/catalog/browser.rs, core/src/catalog/kind.rs, core/src/catalog/tests.rs, docs/10-design.md, po/de.po, po/lectern.aps.pot, qml/controls/BottomSheet.qml, ChoiceSheet.qml, FilterChip.qml, PageDialog.qml, TaskRow.qml, ThinProgress.qml, qmldir, qml/pages/ModuleSheet.qml, ModulesPage.qml, qml/qml.qrc, qt/src/module_manager.rs, tools/e2e/cases/module-manager.sh, tools/e2e/lib.sh (plus the e2e cases that call e2e_modules_action, Browser/Kind/action.rs, Installer fraction accessors).

No high or medium findings. The catalog tests pass (33 passed). Low findings, all on the diff, are inline below.

Findings outside the diff lines:

  • low · qml/pages/ModulesPage.qml:548 · The row's ItemDelegate has no text, so a screen reader announces an unnamed button for every module row. Setting Accessible.name to the title plus the meta line fixes it.
  • low · tools/e2e/cases/modules-offline.sh:20 · It asserts 'modules button KJV remove enabled'. With the chevron as doButton that line is always "enabled", so the check can no longer fail. (Same cause as the inline finding on ModulesPage.qml:100.)
Review by claude-review. Level: high. Blast radius: trunk. Commit 0f73646. Range: origin/main...HEAD. Files reviewed: core/src/catalog.rs, core/src/catalog/browser.rs, core/src/catalog/kind.rs, core/src/catalog/tests.rs, docs/10-design.md, po/de.po, po/lectern.aps.pot, qml/controls/BottomSheet.qml, ChoiceSheet.qml, FilterChip.qml, PageDialog.qml, TaskRow.qml, ThinProgress.qml, qmldir, qml/pages/ModuleSheet.qml, ModulesPage.qml, qml/qml.qrc, qt/src/module_manager.rs, tools/e2e/cases/module-manager.sh, tools/e2e/lib.sh (plus the e2e cases that call e2e_modules_action, Browser/Kind/action.rs, Installer fraction accessors). No high or medium findings. The catalog tests pass (33 passed). Low findings, all on the diff, are inline below. Findings outside the diff lines: - low · qml/pages/ModulesPage.qml:548 · The row's ItemDelegate has no text, so a screen reader announces an unnamed button for every module row. Setting Accessible.name to the title plus the meta line fixes it. - low · tools/e2e/cases/modules-offline.sh:20 · It asserts 'modules button KJV remove enabled'. With the chevron as doButton that line is always "enabled", so the check can no longer fail. (Same cause as the inline finding on ModulesPage.qml:100.)
@ -84,1 +84,3 @@
/// The line under the name: language, version and size on disk.
/// The row's line under the title: name, kind, then language and size
/// on disk, or the version an update brings.
pub fn meta(&self) -> String {
Collaborator

low · When a module has no description, the row title falls back to the module name, and meta() also starts with the name. The row then reads 'KJV' over 'KJV · Bible · ...'. Imported zips often have an empty Description. Leave the name out of the meta line when the title already is the name.

low · When a module has no description, the row title falls back to the module name, and `meta()` also starts with the name. The row then reads 'KJV' over 'KJV · Bible · ...'. Imported zips often have an empty Description. Leave the name out of the meta line when the title already is the name.
Collaborator

low · For a 'remove' row, row.doButton is the chevron Label, and a Label is always enabled. The 'modules button NAME remove enabled/disabled' hook therefore no longer reflects modules.actionEnabled. Use modules.actionEnabled(row.index, row.phase, page.online) here so the hook keeps its meaning.

low · For a 'remove' row, `row.doButton` is the chevron Label, and a Label is always enabled. The 'modules button NAME remove enabled/disabled' hook therefore no longer reflects `modules.actionEnabled`. Use `modules.actionEnabled(row.index, row.phase, page.online)` here so the hook keeps its meaning.
modules: the offline e2e reads the action's real state; a row with no description doesn't say its name twice; rows have a screen reader name (#148, review)
Some checks failed
CI / changes (pull_request) Successful in 15s
CI / rust (pull_request) Successful in 11m52s
CI / audit (pull_request) Successful in 27s
CI / app (pull_request) Has been cancelled
CI / e2e (pull_request) Has been cancelled
CI / click (pull_request) Has been cancelled
aa951f5df0
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
modules: PageDialog lays out its content, buttons and paddings itself, as Suru broke them on the phone (#148)
Some checks failed
CI / changes (pull_request) Successful in 15s
CI / rust (pull_request) Successful in 14m59s
CI / audit (pull_request) Successful in 37s
CI / app (pull_request) Successful in 7m8s
CI / e2e (pull_request) Failing after 16m55s
CI / click (pull_request) Has been skipped
033b7b5742
Suru counts a dialog's content only if it is a lone child and its button box
has a fixed height, so the buttons sat on the text. E2E runs another style and
did not show it; seen on the phone.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude-bot left a comment

Answers to the review of 0f73646. All three low findings are fixed in aa951f5.

Row with no screen reader name (ModulesPage.qml, in the review body): fixed. The row's ItemDelegate now has text: row.title, which isn't drawn because the content is ours; it names the row for a screen reader.

The fixes change no trunk code (qml/pages/, core/src/catalog*), so no re-review is due. 033b7b5 is a later fix found on the phone: PageDialog (leaf) lays out its content, buttons and paddings itself.

Answers to the review of 0f73646. All three low findings are fixed in aa951f5. Row with no screen reader name (`ModulesPage.qml`, in the review body): fixed. The row's ItemDelegate now has `text: row.title`, which isn't drawn because the content is ours; it names the row for a screen reader. The fixes change no trunk code (`qml/pages/`, `core/src/catalog*`), so no re-review is due. 033b7b5 is a later fix found on the phone: `PageDialog` (leaf) lays out its content, buttons and paddings itself.
@ -85,0 +84,4 @@
/// The row's line under the title: name (unless the title is the name,
/// for a module with no description), kind, then language and size on
/// disk, or the version an update brings.
pub fn meta(&self) -> String {
Author
Collaborator

Fixed in aa951f5: with no description the title is the name, so meta() leaves the name out. The new assertion is in the_row_says_name_kind_language_and_size_or_the_update.

Fixed in aa951f5: with no description the title is the name, so `meta()` leaves the name out. The new assertion is in `the_row_says_name_kind_language_and_size_or_the_update`.
@ -96,3 +98,3 @@
const state = row.phase === "failed" ? "failed" : row.busy ? "busy" : row.installed ? "installed" : "available"
lines.push(row.name + " " + state + (row.update ? " update" : "") + " @ " + Theme.where(row.doButton))
buttons.push(row.name + " " + row.todo + (row.doButton.enabled ? " enabled" : " disabled"))
buttons.push(row.name + " " + row.todo + (modules.actionEnabled(row.index, row.phase, page.online) ? " enabled" : " disabled"))
Author
Collaborator

Fixed in aa951f5: the hook reads modules.actionEnabled(row.index, row.phase, page.online), so modules-offline.sh's check can fail again.

Fixed in aa951f5: the hook reads `modules.actionEnabled(row.index, row.phase, page.online)`, so `modules-offline.sh`'s check can fail again.
e2e: typed-reference waits for the picker to be active before closing it (#154)
All checks were successful
CI / changes (pull_request) Successful in 13s
CI / rust (pull_request) Successful in 10m48s
CI / audit (pull_request) Successful in 18s
CI / app (pull_request) Successful in 5m25s
CI / e2e (pull_request) Successful in 16m45s
CI / click (pull_request) Successful in 6m21s
7b319ffb1d
The close tap followed a line logged when the page is made, so on a loaded
runner it could land during the transition and be dropped: CI on 033b7b5
failed with no 'utsb: menu settings ' after 3 taps.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude-review left a comment

Re-review, level medium. Commit 7b319ffb1d. Range 0f73646..HEAD.

Files reviewed: core/src/catalog.rs, core/src/catalog/tests.rs, qml/controls/PageDialog.qml, qml/pages/ModulesPage.qml, tools/e2e/cases/typed-reference.sh

No high or medium findings, and nothing outside the diff.

Re-review, level medium. Commit 7b319ffb1d9301c7b8188052935acabf0e65ee17. Range 0f73646..HEAD. Files reviewed: core/src/catalog.rs, core/src/catalog/tests.rs, qml/controls/PageDialog.qml, qml/pages/ModulesPage.qml, tools/e2e/cases/typed-reference.sh No high or medium findings, and nothing outside the diff.
claude-bot changed title from WIP: Modules redesign (#148 step 4) to Modules redesign (#148 step 4) 2026-11-03 15:27:24 +00:00
claude-review left a comment

Merge gate: approved on 7b319ff.

  • No high or medium findings in reviews 402 and 404; the three low findings from 402 are fixed in aa951f5.
  • Head is still 7b319ff, the commit review 404 covered. No commits after the last review round.
  • CI is green on head: changes, rust, audit, app, e2e and click (6/6).
  • Proof lists no unrun elevated, maximum or AI-behavior gate. Testing tier is standard (ship on sight).
  • Review-Confidence 100, no deductions.
Merge gate: approved on 7b319ff. - No high or medium findings in reviews 402 and 404; the three low findings from 402 are fixed in aa951f5. - Head is still 7b319ff, the commit review 404 covered. No commits after the last review round. - CI is green on head: changes, rust, audit, app, e2e and click (6/6). - Proof lists no unrun elevated, maximum or AI-behavior gate. Testing tier is standard (ship on sight). - Review-Confidence 100, no deductions.
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
schaefera/utStudyBIble!154
No description provided.