No reviewers
Labels
No milestone
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
schaefera/utStudyBIble!82
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/module-manager"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #30. Part 2b of #10 (part 2c is not in this PR).
What
Details by round:
count()and per-row invokables, re-asked whenrevisionchanges.core/src/bible/locale/table.tsv(about 25 KB, generated bytools/langnames/langnames.py,--checkverifies it; CLDR 47.0.0 and the SIL ISO 639-3 table, inputs pinned by SHA-256), 3-letter code folding.Kind::usable()is true only for Bible; an installed unusable module is listed greyed and can be removed; the kind filter is hidden while one kind is usable.PageBar.qmlis the bottom bar of every page (Back inside it, centred title, trailing slot), which fixes the Back overflow. Per-row logs trimmed. NOTICES.md added.module-managercase, older cases ported to the manager and Settings Display.Repos not in the enabled set are not offered; an installed module from one of them is listed and can only be removed.
Blast radius
Tier: trunk
Trunk paths touched:
qt/glue,qml/pages/ReadingPane.qml,qml/Main.qml,core/src/bible*,tools/e2e/,tools/mem-check/,tools/ci/,Makefile.toml,.forgejo/workflows/Testing tier: elevated (also: UI, e2e harness, generated data)
Gate / rollback: no flag; revert this merge. No schema change, so no migration or
app.sqlite3backup is involved.Proof
3499449(all jobs).cargo make app-dockerpassed: window loaded after 122 ms, text on screen after 318 ms, peak PSS 161280 kB normal profile and 163729 kB slow profile, both under the 165000 kB budget.E2E_CASES="module-manager display-settings"e2e passed 2/2. I looked at the shots in day (module-manager-01-menu.png) and night (display-settings-04-menu-night.png): both icons show next to the entries, and in night mode they are tinted light like the text. origin/main was already an ancestor of HEAD, so there was no merge. One push, no force. I did not wait for CI.display-settings display-settings-kill red-letters red-letters-greyed red-letters-kill switch-bible-red-letters module-manager picker-back typed-reference starter-install add-bible11 passed, 0 failed; shots of Modules, Settings and the picker looked at.Kind::usablealways true and the label returning the autonym only, 7 core tests fail (for exampleonly_usable_kinds_are_offered_but_an_installed_one_is_listed_to_be_removed,the_label_has_both_names_unless_they_are_the_same_or_missing). With Remove's pending-task refusal removed,removing_a_module_with_a_queued_task_is_refusedandremoving_a_module_that_is_downloading_is_refused_and_the_download_goes_onfail.tools/langnames/langnames.py --check: up to date.3499449, installed over USB): the owner's checklist passed.Budgets
3499449, Modules open on the full index): peak 84,689 kB, within the 165,000 kB budget. The desktop figure above counts desktop Qt and Mesa, so the phone number is the one that applies.Review
Reviewer: fresh-context
code-review(claude-review): high at00c99d1(QML, qt, e2e, sword; core) and a delta atff1d852Review-Confidence: 55 (audit at
3499449; one deduction was for the empty-CipherKey gap, now deferred to #83)Confidence by part
Open items
🤖 Generated with Claude Code
Level: high. Blast radius: trunk. Reviewed commit:
b5d51e0980. Range: origin/main...HEAD.Files read in full or around the hunks: core/src/tasks.rs, core/src/tasks/exec.rs, core/src/db/tasks.rs and 003_task_repo.sql, core/src/repos.rs, core/src/catalog.rs and catalog/load.rs, sword/src/install.rs and install/http.rs (download resume), qt/src/installer.rs, qt/src/module_manager.rs, qml/Main.qml, qml/pages/ReadingPane.qml, qml/pages/ModulesPage.qml (top). Not read in depth: the e2e cases, po/ and docs.
No high or medium defects found. Checked and found sound: migration 003 is additive and NULL means the default repo; a partial download resumed against a different repo's URL is discarded because the .state file is checked against the URL; the Remove-while-pending refusal is surfaced through Refused and Failed(Remove) to removeRefused.
Low findings (inline, not blocking):
@ -106,0 +105,4 @@/// Queues a removal, unless the module has a task of its own going:/// an install may be unpacking it, or a removal already be on its way./// The caller cancels first if that is what it wants.fn remove(&self, id: u64, module: &str) {Low: the behaviour changed from cancel-then-remove to refuse-if-pending. The Remove command's only caller is the module manager, which must cancel first; this is documented, but nothing in the UI path cancels an update install before Remove, so the user gets a refusal text and has to cancel by hand.
@ -0,0 +36,4 @@if (page.reading && name === page.reading.moduleName) {for (let i = 0; i < page.reading.bibleCount(); i++) {if (page.reading.bibleName(i) !== name) {page.reading.switchBible(page.reading.bibleName(i))Low: the reading Bible is switched away before removeModule is accepted. If the core refuses (the module has a queued or running task), the reader has moved to another Bible for no reason. Scenario: reading Bible X while X has a pending update install, then Remove X: the Bible switches, the removal is refused. Consider switching on moduleRemoved instead.
@ -0,0 +316,4 @@}self.as_mut().notify();let thread = self.qt_thread();std::thread::spawn(move || {Low: Refresh spawns a new loading thread without stopping the previous one (the stop flag is shared per object). Two threads can fetch and write the same index and ETag files at once; each file write is atomic, but the index/ETag pair can end up mismatched, costing one extra full fetch. Only wasted work, no corruption.
Round 1 answers (head
5210486):Review level: medium (blast radius: trunk).
Reviewed commit:
97e1875387Range: b5d51e0980e4d98048e2b9915f6adb741eaa2db7..HEAD (re-review of the fixes)
Files: qml/pages/ModulesPage.qml, qt/src/module_manager.rs
Findings: none. Checked that phaseOf returns "" for a module without a task row (so the switch still happens for an idle module), that its phase names match the QML comparison, and that replacing and signalling the stop flag per loading leaves the generation guard and Drop intact.
Not covered here: CI results, and the phone check, which is still pending.
Tests added for the two post-review fixes (
2a72e9b):Board::holds(module)in core (queued, downloading or retrying).Installer.holds()exposes it andModulesPage.remove()asks it. Unit test:only_a_queued_downloading_or_retrying_module_holds_back_its_removal.catalog::Handover, outside the Qt thread code. Unit tests:a_new_load_stops_the_previous_one_and_only_that,stopping_stops_the_load_in_flight, anda_stopped_load_fetches_and_writes_nothing(a stoppedIndexCache::loadfetches nothing and creates no index dir; an unstopped one does).Each test fails when its fix is broken (Handover::begin not stopping the previous flag;
holdsignoring Queued) and passes once restored.Local:
cargo makepasses,cargo make app-dockerpasses, and themodule-managere2e case passes. CI not awaited.Level: high. Blast radius: trunk. Commit:
2a72e9b2e7. Range: origin/main...HEAD, limited to the files below plus the test commit2a72e9b(git log 97e1875..HEAD).Files reviewed: qt/build.rs, qt/src/lib.rs, qt/src/module_manager.rs (registration only), core/src/db.rs, core/src/db/003_task_repo.sql, core/src/db/tasks.rs, core/src/db/tasks/tests.rs, tools/e2e/lib.sh, tools/e2e/repo-server.py, sword/src/install.rs and install/index.rs (index format check), qml/pages/ModulesPage.qml and ReadingPane.qml and Main.qml (against the e2e hooks), and the changed cases add-bible-fail.sh, add-bible.sh, install-queue-cancel.sh, install-queue-fail.sh, module-manager.sh, typed-reference.sh.
No high or medium defects found.
Checked and sound:
mods.d/<name>.conf, USTAR, gzip) matches what sword::install::index::parse accepts. The pathraw/mods.d.tar.gzmatches Repo::new's default layout.Finding outside the diff (low, test flakiness):
e2e_seento the count of rows lines. Then e2e_menu (line 313) overwrites the same global with the count of "utsb: menu " lines. The finale2e_wait ... $((e2e_seen + 1))therefore waits against the menu count, not the rows count.Re:
e2e_menuoverwriting the globale2e_seenthate2e_open_moduleshad just set (low). Confirmed, and fixed inb412734. Bothe2e_menuande2e_open_modulesnow declarelocal e2e_seen, so the final wait compares against the rows count again. I checked that no caller relies one2e_menusetting the global; the cases set their owne2e_seen.E2E_CASES="module-manager install-queue-cancel" cargo make e2e-dockerpasses (2 passed, 0 failed). I did not wait for CI.Fresh-context review. Level: high. Blast radius: trunk. Reviewed commit:
00c99d1826. Range: origin/main...HEAD.Findings: none with a concrete failure scenario (no high, medium or low).
Checked in context: qt/src/module_manager.rs (generation guard, stop handover, notify/revision flow), qt/src/installer.rs (new invokables, Done/Refused/Failed event mapping, board.add replacing an existing row on retry), sword/src/install.rs (Repo layouts; the default stays the old paths), ReadingPane.qml menu and footer, Main.qml wiring, PageBar.qml (Back is Layout.fillHeight inside the bar, so it cannot exceed the bar's height; all pages now use PageBar as a bottom footer, matching the reading pane), ModulesPage.qml remove/switch flow, StarterPage/SettingsPage footers, mem-check and e2e renames (no stale aa-sheet / QuickSettings / e2e_tap_aa references remain).
Files reviewed: CLAUDE.md, Makefile.toml, qml/Main.qml, qml/pages/ModulesPage.qml, qml/pages/PageBar.qml, qml/pages/ReadingPane.qml, qml/pages/SettingsPage.qml, qml/pages/StarterPage.qml, qml/pages/PickerPage.qml, qt/src/module_manager.rs, qt/src/installer.rs, qt/src/reading_model.rs, sword/src/install.rs, tools/ci/e2e.sh, tools/mem-check/run.sh, tools/e2e/lib.sh, tools/e2e/cases (renames and references).
Level: high (trunk). Reviewed commit:
00c99d1826. Range: origin/main...00c99d1, plus a read of core/src/bible/locale*, core/src/catalog*, core/src/tasks/{progress,exec,tests}.rs, tools/langnames/, and the docs/04 and docs/06 hunks. No commits since00c99d1. This complements the earlier review of the QML, qt/, tools/e2e and sword files.Files reviewed: core/src/bible/locale.rs, core/src/bible/locale/table.rs, core/src/bible/locale/table.tsv (spot checks: de, en, el, grc, hbo, he, la, aai, cop, nb, no, sr, zh), core/src/catalog.rs, core/src/catalog/{browser,handover,kind,language,load,query,version,tests}.rs, core/src/tasks/progress.rs, core/src/tasks/tests.rs, core/src/tasks/exec.rs, core/src/db/tasks.rs, tools/langnames/langnames.py, tools/langnames/index-codes.txt, docs/04-features.md, docs/06-sources.md, core/src/bible/installed.rs and sword/src/module.rs (callers), qml/pages/ModulesPage.qml (the remove path only).
Verified fine:
langnames.py --checkreproduces table.tsv from the pinned inputs. The table is sorted, so the binary search is valid. The de, en, grc, hbo and aai rows give the labels the tests expect. The load handover and generation guard hold: a stopped load fetches and writes nothing. Startup cost is fine, because the table is parsed lazily on first use and the indexes are read only when the manager opens.Findings outside the diff lines:
medium · core/src/catalog/kind.rs:21 and core/src/catalog.rs:61 · Usable-kind leak by driver.
Kind::ofclassifies by category and ignores the driver, andinstallable()andlisted()check only the kind. The app opens onlyModDrv=zTextwithCompressType=ZIPand no cipher (sword/src/module.rsfrom_conf). A "Biblical Texts" module withModDrv=RawText,zText4orRawText4, or an LZSS-compressed one, therefore appears under Bibles with an Install button and no note. The same happens for a category-less conf with aRawTextorzText4driver, which the driver fallback maps to Bible. It installs, then never appears in the Bible chooser (BibleInfo::installeddrops it throughModule::from_conf), so the user installed a Bible that cannot be read. Makeusable()orinstallable()depend on the driver (case-insensitive zText only). CompressType would need anIndexEntryfield for the LZSS case. Tell the user why the others are not offered, and add a test with a Biblical Texts entry whose driver is RawText or zText4.medium · docs/06-sources.md:122-140, core/src/bible/locale/table.tsv · Licence omission. table.tsv is CLDR-derived data compiled into the shipped binary. The Unicode License v3 requires its copyright and permission notice to accompany the data in copies and in the associated documentation. The SIL ISO 639-3 names need attribution. The doc says the licence text "travels with the data in the table's source packages", but the click and the repo contain neither the notice nor the SIL attribution: grep for CLDR, Unicode and SIL finds only docs/06-sources.md, and the app has no licences or About page. Add the Unicode notice and the SIL attribution to a shipped notices file (for example THIRD-PARTY-NOTICES, packaged in the click) or to the table header.
low · tools/langnames/langnames.py:3, Makefile.toml, .forgejo/workflows/ci.yml ·
--checkis wired nowhere. Only docs/06-sources.md claims it "fails when the committed table differs". The UI columns come frompo/*.po, so adding po/fr.po without regenerating leaves the table without a French column. The French names then silently fall back to English (Table::column). Wire it in as a cargo make or CI step (it needs the cached inputs) or drop the claim.low · core/src/bible/locale.rs:27 (
parse) · A 3-letter code with a 2-letter equivalent ("deu", "eng") is never folded into the 2-letter one. The generator emits alias rows only for codes present in the 2026-10-27 indexes, and there are none now, so table.tsv has no=rows. A hand-installed or future module withLang=deushows the raw code, or, if an alias row exists, a second "German · Deutsch" entry. The filter comparescode()strings, so "deu" and "de" are separate filter entries. Normalise through the alias inLocale::parse/language(). Four indexed codes (cug, daf, dud, plj) have no table row and show as raw codes.low · core/src/tasks/progress.rs:175 (
Board::holds) ·holdsreads only the starter Board, which is filled byaddand the live events. After a restart, a queued or running task resumed from the DB has no Board row, soholdsis false. ModulesPage.qml:37 then switches the reader away from that module beforeremoveModule, and the runner'shas_pending_taskrefuses the removal. The user ends up switched off their Bible with the module still installed. Ask the DB (has_pending_task) before switching, or seed the Board from the queue at start.Author response to the review, fixes in
6447313:compress. Test added.cargo make passes. Not run: app-docker (no qt/QML/C++ change; CMake only gains an install line).
Level: high (trunk). Reviewed commit:
ff1d8527c4. Range: 00c99d18261b3dfa40b1ffc969349186bbc61f8c..HEAD (6447313review fixes,ff1d852menu icons).Files reviewed: CMakeLists.txt, NOTICES.md, assets/icons/.svg, core/src/bible/locale.rs, core/src/bible/locale/table.rs, core/src/bible/locale/table.tsv (alias rows), core/src/catalog/kind.rs, core/src/catalog/tests.rs, docs/06-sources.md, qml/icons/.png, qml/pages/ReadingPane.qml, qml/qml.qrc, sword/src/install/index.rs, sword/src/install/index/tests.rs, sword/src/module.rs (from_conf, for comparison), tools/e2e/cases/display-settings.sh, tools/icons/render.sh, tools/langnames/langnames.py.
Checked and sound: alias folding in Locale::parse (deu, ger, eng, heb, nor, nob, zho, chi now fold; real rows like he are untouched; the new tests cover deu and ger-DE); the po-column test; NOTICES.md installed next to the binary; icon paths resolve from the /qml qrc prefix (pages/ to ../icons), icon.color follows Theme.text. The Board::holds refutation is not re-litigated here.
Findings outside the diff lines (all low, none blocking):
low · core/src/catalog/kind.rs:44-45 · The "benefit of the doubt" for an empty CompressType is wrong for compress: Module::from_conf (sword/src/module.rs:92-95) refuses a missing CompressType (SWORD's default is LZSS). A "Biblical Texts" zText conf with no CompressType is still classed Bible, offered with Install, installs, and never appears in the Bible chooser. Only the driver deserves the doubt (and even that is refused by from_conf if empty); require compress to equal ZIP.
low · core/src/catalog/kind.rs:29-34 · The category-less driver fallback still maps RawText, zText4 and RawText4 to Bible without the biblical() check, so a conf without a Category and with one of those drivers is offered and unreadable (the scenario of the earlier review's second half). Also enciphered Bibles (a CipherKey line, which from_conf refuses) are not filtered; IndexEntry has no cipher field.
low · tools/e2e/cases/display-settings.sh (new menu-night block) · The shot is taken but nothing asserts the icon tint; a regression of icon.color only shows in the PNG. Acceptable if shots are looked at before the phone install.
Review round answers (fixed in
3499449):locked(listed, not installable). An empty CipherKey is deliberately notlockedin sword/install/index (test a_cipher_key_means_locked), while from_conf refuses it; changing that is a trunk sword change outside this PR, so left open.Installed
3499449on the phone (USB, 18:08). Phone checklist:Comments on @claude-bot wrote in #82 (comment):
Comments:
All checklist items passed other than the above comments.
WIP: Module manager and a footer menu for Modules and Settings (#10 part 2b, #30)to Module manager and a footer menu for Modules and Settings (#10 part 2b, #30)Review level: high (trunk). Reviewed commit:
3499449add.Range: ff1d852..3499449 in depth, plus a skim of origin/main...HEAD (73 files, already reviewed at
00c99d1andff1d852).Files reviewed: core/src/catalog/kind.rs, core/src/catalog/tests.rs, qml/pages/ReadingPane.qml, tools/e2e/cases/display-settings.sh.
Findings:
bible()test helper now sets ZIP, so the existing catalog tests still reflect the new rule. No failure scenario found.CI: success on
3499449(combined status success; rust job green, remaining contexts also success).Gate decision: APPROVED. No high or medium findings, CI green, phone checklist passed by the owner.