Red letters: words of Jesus in red, through core::render (#12 part 2) #37

claude-bot · 2026-10-22T12:42:41Z

claude-bot commented

2026-10-22 12:42:41 +00:00

What

Words of Jesus show in red (dark red by day, light red at night), through core::render (sword filter runs -> render -> styled text).
A switch in the Aa sheet ("Words of Jesus in red", on by default) turns it off; it is greyed out, with the reason, for a Bible that does not mark them. The setting is saved per user.
Merged with main after the e2e suite (#40): the flow checks this PR had added to tools/mem-check are now e2e cases (red-letters, red-letters-kill, red-letters-greyed); mem-check measures only.

Blast radius

Tier: trunk
Trunk paths touched: sword/src/filter*, core/src/bible*, core/src/worker*, core/src/render/, qt/ glue, qml/pages/ReadingPane.qml, tools/e2e/, tools/mem-check/
Testing tier: standard (feature) (also: CI tooling changes in tools/e2e and tools/mem-check)
Gate / rollback: the setting text.red_letters (Aa sheet switch, default on) turns the feature off at runtime. Full undo: revert the merge commit. No migration in this PR.

Proof

Final commit 08541ce: CI green on all five jobs: rust (fmt, clippy, tests with coverage, release check, deny), audit, app (C++/QML lint, -Werror build, clang-tidy, PSS mem-check), e2e (isolated cases incl. red-letters, red-letters-kill, red-letters-greyed, normal and slow profiles), click.
Local e2e: 8 passed in the normal profile, 4 passed in the slow profile.
Old mem-check check -> e2e case: red letters off in KJV, no list reset, top verse stays, 0 verses red off and >0 on -> red-letters; setting survives kill -9 -> red-letters-kill; switch greyed in a Bible without OSISRedLetterWords (CI has KJV only) -> red-letters-greyed; peak PSS with red on -> mem-check itself, which now jumps to Matthew 5 (e2e_type_reference) before its fling so the measured chapters carry red runs.
Each new e2e case fails without the change (implementation broken in a scratch copy): red-letters ("red letters on showed no verse in red"), red-letters-kill ("setting didn't come back ... after a kill -9"), red-letters-greyed ("switch is on in a Bible without the words of Jesus marked").
Rust tests fail on the old code: render (4 core), runs collapse (2 sword), osis runs and golden kjv_runs_match_the_plain_text_and_red_words, no re-render on a setting change (worker), module support (reader).
Review 242 round tests fail on the old code: sword overlapping_milestones_stay_red_until_the_last_ends (sID q1, sID q2, eID q2, eID q1 gave red "b c", now "b c d"); core a_setting_that_changes_the_text_arrives_with_it (was Setting then a later Rerendered, now one Rerendered carrying the setting).
mem-check reaches Matt 5 and flings through Matthew (e2e_wait 'utsb: showing Matt 5' fails the run otherwise).
StyledText wraps like RichText for the strings render.rs emits (qml6 check: equal height and line count on a wrapped verse with , ,  , ", <, >, &).
docs/07 budgets: PSS peak, KJV only, Matthew 5 then fling through Matthew (budget 165000 kB), RichText vs StyledText, one run each: normal 152130 vs 148321 kB, slow 148036 vs 149194 kB, within run-to-run spread (about 140000-152000 seen). StyledText is taken because it needs no QTextDocument per verse. Text on screen 317 ms in both (budget 3000 ms). The Aa sheet's red-count Timer runs only after a text change in place, nothing at start-up.
Contrast: red #b71c1c on day window #fff 6.57:1; red #ff8a80 on night window #121212 8.21:1.
On the phone (docs/09 "On the phone"): the click built from dbd77eb was installed on the owner's Volla/Plinius. Journal: "build 0.0.1 phase1/red-letters@dbd77eb8b", "window loaded after 345 ms", "restored KJV 27380" (John 7) and a save. The owner confirmed "Got the new app". 08541ce after dbd77eb changes only shell comments in test tooling, so the installed app equals head.
Not yet done: the owner's touch check of red letters on screen (red Matthew 5, Aa switch off/on, night mode, kill and reopen, BSB greyed, fast scroll). It moves to the post-merge phone QC checklist on #12, which stays open until the owner signs off.

Review

Reviewer: fresh-context code-review, four rounds:

Review 235 (high, a2f7723): no high or medium, 3 lows, all fixed in 2299823 (skip the re-render when the Bible has no red letters; red_letters_available on open failure; row compare on (verse, ordinal)). Re-review 236 (medium, 2299823): no findings.
Review 242 (high, 876cfb5, after the merge with main): no high or medium, 3 lows listed (red-letters PSS no longer measured in KJV; q_milestone single slot; light red for a frame on a night toggle), all fixed in dbd77eb: mem-check jumps to Matt 5 and states its numbers above; q_milestones (overlapping sIDs stay red until the last ends); Event::Rerendered carries the setting so the glue applies it with the rows; StyledText taken for rendering.
Re-review 243 (medium, dbd77eb): no high or medium, 2 lows, answered in PR comment 1658 and resolved in 08541ce (comment-only): e2e_type_reference is a toggle, so it is documented as once per sandbox in tools/e2e/lib.sh (a code fix was tried and failed: a second call in one sandbox opens no picker); mem-check's chapter count is since the jump (tools/mem-check/run.sh).
Total: 8 lows over the rounds, 8 fixed or answered with evidence, 0 declined, 0 left open.
Known follow-up, not in this PR: #41 (night mode: follow the system theme and dim it; found on the phone).

Review-Confidence: 100/100, trunk files all covered by the HIGH review 242 (callers traced) and the MEDIUM re-review 243 of the dbd77eb fixes; all lows fixed or refuted with evidence; fails-before evidence for the new tests; 08541ce is comment-only (verified by diff); CI green on all five jobs on the final commit; proof items exercise the behaviour; riskiest hunk (Event::Rerendered with key/value in worker.rs and reading_model.rs) re-read independently: setting applied even when rows are stale, rows only if id > shown; consistent. No missed defect.

Confidence by part

sword filter runs (osis.rs, runs.rs, thml.rs): high. Golden test against diatheke output, overlapping-milestone test that fails before the fix, traced by review 242.
core render and worker (Event::Rerendered, settings): high. Fails-before tests, two reviews with callers traced.
qt/ glue and ReadingPane.qml / Aa sheet: medium. Covered by e2e cases and reviews; the on-screen touch behaviour is not yet checked by the owner on the phone.
tools/e2e and tools/mem-check: medium. CI-tooling only; e2e_type_reference is once per sandbox by design (documented). Read these if touching the cases.
Night-mode colours: medium. Contrast computed, not seen on the phone; follow-up #41.

🤖 Generated with Claude Code

## What Words of Jesus show in red (dark red by day, light red at night), through core::render (sword filter runs -> render -> styled text). A switch in the Aa sheet ("Words of Jesus in red", on by default) turns it off; it is greyed out, with the reason, for a Bible that does not mark them. The setting is saved per user. Merged with main after the e2e suite (#40): the flow checks this PR had added to tools/mem-check are now e2e cases (`red-letters`, `red-letters-kill`, `red-letters-greyed`); mem-check measures only. ## Blast radius **Tier:** trunk **Trunk paths touched:** sword/src/filter*, core/src/bible*, core/src/worker*, core/src/render/, qt/ glue, qml/pages/ReadingPane.qml, tools/e2e/, tools/mem-check/ **Testing tier:** standard (feature) (also: CI tooling changes in tools/e2e and tools/mem-check) **Gate / rollback:** the setting `text.red_letters` (Aa sheet switch, default on) turns the feature off at runtime. Full undo: revert the merge commit. No migration in this PR. ## Proof - Final commit 08541ce: CI green on all five jobs: rust (fmt, clippy, tests with coverage, release check, deny), audit, app (C++/QML lint, -Werror build, clang-tidy, PSS mem-check), e2e (isolated cases incl. `red-letters`, `red-letters-kill`, `red-letters-greyed`, normal and slow profiles), click. - Local e2e: 8 passed in the normal profile, 4 passed in the slow profile. - Old mem-check check -> e2e case: red letters off in KJV, no list reset, top verse stays, 0 verses red off and >0 on -> `red-letters`; setting survives kill -9 -> `red-letters-kill`; switch greyed in a Bible without OSISRedLetterWords (CI has KJV only) -> `red-letters-greyed`; peak PSS with red on -> mem-check itself, which now jumps to Matthew 5 (`e2e_type_reference`) before its fling so the measured chapters carry red runs. - Each new e2e case fails without the change (implementation broken in a scratch copy): `red-letters` ("red letters on showed no verse in red"), `red-letters-kill` ("setting didn't come back ... after a kill -9"), `red-letters-greyed` ("switch is on in a Bible without the words of Jesus marked"). - Rust tests fail on the old code: render (4 core), runs collapse (2 sword), osis runs and golden `kjv_runs_match_the_plain_text_and_red_words`, no re-render on a setting change (worker), module support (reader). - Review 242 round tests fail on the old code: sword `overlapping_milestones_stay_red_until_the_last_ends` (sID q1, sID q2, eID q2, eID q1 gave red "b c", now "b c d"); core `a_setting_that_changes_the_text_arrives_with_it` (was `Setting` then a later `Rerendered`, now one `Rerendered` carrying the setting). - mem-check reaches Matt 5 and flings through Matthew (`e2e_wait 'utsb: showing Matt 5'` fails the run otherwise). - StyledText wraps like RichText for the strings render.rs emits (qml6 check: equal height and line count on a wrapped verse with ``, ``, ` `, `"`, `<`, `>`, `&`). - docs/07 budgets: PSS peak, KJV only, Matthew 5 then fling through Matthew (budget 165000 kB), RichText vs StyledText, one run each: normal 152130 vs 148321 kB, slow 148036 vs 149194 kB, within run-to-run spread (about 140000-152000 seen). StyledText is taken because it needs no QTextDocument per verse. Text on screen 317 ms in both (budget 3000 ms). The Aa sheet's red-count Timer runs only after a text change in place, nothing at start-up. - Contrast: red #b71c1c on day window #fff 6.57:1; red #ff8a80 on night window #121212 8.21:1. - On the phone (docs/09 "On the phone"): the click built from dbd77eb was installed on the owner's Volla/Plinius. Journal: "build 0.0.1 phase1/red-letters@dbd77eb8b", "window loaded after 345 ms", "restored KJV 27380" (John 7) and a save. The owner confirmed "Got the new app". 08541ce after dbd77eb changes only shell comments in test tooling, so the installed app equals head. - Not yet done: the owner's touch check of red letters on screen (red Matthew 5, Aa switch off/on, night mode, kill and reopen, BSB greyed, fast scroll). It moves to the post-merge phone QC checklist on #12, which stays open until the owner signs off. ## Review **Reviewer:** fresh-context `code-review`, four rounds: - Review 235 (high, a2f7723): no high or medium, 3 lows, all fixed in 2299823 (skip the re-render when the Bible has no red letters; red_letters_available on open failure; row compare on (verse, ordinal)). Re-review 236 (medium, 2299823): no findings. - Review 242 (high, 876cfb5, after the merge with main): no high or medium, 3 lows listed (red-letters PSS no longer measured in KJV; `q_milestone` single slot; light red for a frame on a night toggle), all fixed in dbd77eb: mem-check jumps to Matt 5 and states its numbers above; `q_milestones` (overlapping sIDs stay red until the last ends); `Event::Rerendered` carries the setting so the glue applies it with the rows; StyledText taken for rendering. - Re-review 243 (medium, dbd77eb): no high or medium, 2 lows, answered in PR comment 1658 and resolved in 08541ce (comment-only): `e2e_type_reference` is a toggle, so it is documented as once per sandbox in tools/e2e/lib.sh (a code fix was tried and failed: a second call in one sandbox opens no picker); mem-check's chapter count is since the jump (tools/mem-check/run.sh). - Total: 8 lows over the rounds, 8 fixed or answered with evidence, 0 declined, 0 left open. - Known follow-up, not in this PR: #41 (night mode: follow the system theme and dim it; found on the phone). **Review-Confidence:** 100/100, trunk files all covered by the HIGH review 242 (callers traced) and the MEDIUM re-review 243 of the dbd77eb fixes; all lows fixed or refuted with evidence; fails-before evidence for the new tests; 08541ce is comment-only (verified by diff); CI green on all five jobs on the final commit; proof items exercise the behaviour; riskiest hunk (`Event::Rerendered` with key/value in worker.rs and reading_model.rs) re-read independently: setting applied even when rows are stale, rows only if id > shown; consistent. No missed defect. ## Confidence by part - sword filter runs (osis.rs, runs.rs, thml.rs): high. Golden test against diatheke output, overlapping-milestone test that fails before the fix, traced by review 242. - core render and worker (`Event::Rerendered`, settings): high. Fails-before tests, two reviews with callers traced. - qt/ glue and ReadingPane.qml / Aa sheet: medium. Covered by e2e cases and reviews; the on-screen touch behaviour is not yet checked by the owner on the phone. - tools/e2e and tools/mem-check: medium. CI-tooling only; `e2e_type_reference` is once per sandbox by design (documented). Read these if touching the cases. - Night-mode colours: medium. Contrast computed, not seen on the phone; follow-up #41. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

claude-bot added 1 commit

2026-10-22 12:42:41 +00:00

Red letters: words of Jesus in red, through core::render (#12 part 2)

CI / rust (pull_request) Successful in 3m9s

Details

CI / audit (pull_request) Successful in 9s

Details

CI / app (pull_request) Failing after 1m5s

Details

CI / click (pull_request) Has been skipped

Details

a2f77232ea

sword gives opt-in display runs (red, italic); core::render turns them into
Qt rich text for the reading pane with the red for day or night; a
text.red_letters setting (on by default) re-renders the loaded chapters in
place; the Aa sheet's switch is greyed out for a Bible whose conf lacks
GlobalOptionFilter=OSISRedLetterWords. mem-check covers both cases.

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

claude-review reviewed

2026-10-22 12:55:34 +00:00

claude-review left a comment

claude-review · level HIGH · commit a2f77232ea · range origin/main...HEAD (blast radius: trunk, testing tier: standard)

Files reviewed: core/src/bible/chapter.rs, core/src/bible/reader.rs, core/src/lib.rs, core/src/render.rs, core/src/settings.rs, core/src/worker.rs, core/src/worker/session.rs, core/src/worker/session/jump.rs, po/de.po, po/utstudybible.schaefera.pot, qml/pages/QuickSettings.qml, qml/pages/ReadingPane.qml, qt/src/reading_model.rs, sword/src/filter.rs, sword/src/filter/osis.rs, sword/src/filter/runs.rs, sword/src/filter/thml.rs, sword/src/lib.rs, sword/tests/golden.rs, tools/mem-check/run.sh

No high or medium findings. I traced the display-run offsets in osis.rs (divine-name upper-casing, the " * " italic marks that stay out of the runs, space tags), Run::collapse's style rule for spaces, the escaping in render.rs, the ordering of Setting, Rerendered, Loaded and Slid events against shown in the glue, and the start-up order of settings() before the first read. All hold.

Low findings (outside the diff):

low · core/src/worker/session/jump.rs:~100 · Options::apply reports a change for text.night and text.red_letters even when the open Bible has no red letters (BSB). Toggling night mode then re-reads and re-renders the whole loaded window on the worker, and the glue emits dataChanged over every row, though no text changes. This is wasted work on the low-memory phones the app targets. Skip the rerender when !reader.has_red_letters().
low · qt/src/reading_model.rs (Event::Features handling) · red_letters_available is only set when a Bible opens. If a later open fails after the old reader is dropped, or no Bible is open, the sheet keeps the previous Bible's value. The red letters switch then stays enabled with nothing to colour.
low · qt/src/reading_model.rs (rerendered) · The "same rows" check compares only ordinals. A module's missing verses have ordinal 0, so two different windows of such rows could look equal. Unlikely, since a jump or slide carries a newer id, and the effect would be one stale text refresh at worst.

claude-review · level HIGH · commit a2f77232ea116e54879458dd290b89e367248005 · range origin/main...HEAD (blast radius: trunk, testing tier: standard) Files reviewed: core/src/bible/chapter.rs, core/src/bible/reader.rs, core/src/lib.rs, core/src/render.rs, core/src/settings.rs, core/src/worker.rs, core/src/worker/session.rs, core/src/worker/session/jump.rs, po/de.po, po/utstudybible.schaefera.pot, qml/pages/QuickSettings.qml, qml/pages/ReadingPane.qml, qt/src/reading_model.rs, sword/src/filter.rs, sword/src/filter/osis.rs, sword/src/filter/runs.rs, sword/src/filter/thml.rs, sword/src/lib.rs, sword/tests/golden.rs, tools/mem-check/run.sh No high or medium findings. I traced the display-run offsets in osis.rs (divine-name upper-casing, the " * " italic marks that stay out of the runs, space tags), Run::collapse's style rule for spaces, the escaping in render.rs, the ordering of Setting, Rerendered, Loaded and Slid events against `shown` in the glue, and the start-up order of settings() before the first read. All hold. Low findings (outside the diff): - low · core/src/worker/session/jump.rs:~100 · Options::apply reports a change for `text.night` and `text.red_letters` even when the open Bible has no red letters (BSB). Toggling night mode then re-reads and re-renders the whole loaded window on the worker, and the glue emits dataChanged over every row, though no text changes. This is wasted work on the low-memory phones the app targets. Skip the rerender when `!reader.has_red_letters()`. - low · qt/src/reading_model.rs (Event::Features handling) · `red_letters_available` is only set when a Bible opens. If a later open fails after the old reader is dropped, or no Bible is open, the sheet keeps the previous Bible's value. The red letters switch then stays enabled with nothing to colour. - low · qt/src/reading_model.rs (rerendered) · The "same rows" check compares only ordinals. A module's missing verses have ordinal 0, so two different windows of such rows could look equal. Unlikely, since a jump or slide carries a newer id, and the effect would be one stale text refresh at worst.

schaefera added 1 commit

2026-10-22 13:00:57 +00:00

Red letters: no re-render for a Bible without them; compare verse and ordinal when refreshing rows (#12 )

CI / rust (pull_request) Successful in 2m58s

Details

CI / audit (pull_request) Successful in 10s

Details

CI / app (pull_request) Failing after 2m20s

Details

CI / click (pull_request) Has been skipped

Details

51e0b03791

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

claude-bot commented

2026-10-22 13:01:38 +00:00

Answering review 235 (head a2f7723), all three low findings. New head 51e0b03.

jump.rs, wasted re-render on a Bible without red letters: fixed in 51e0b03. rerender now returns nothing unless has_red_letters() (night only colours red runs, so nothing changes in the text either). Test: a_bible_without_red_letters_is_not_rendered_again_for_them (BSB, red off + night on, no Rerendered). A later switch to a Bible with red letters reads with the current options anyway, since self.render is still updated.
reading_model.rs, stale red_letters_available: not a bug, no change. In Session::open the reader is replaced only after open_reader succeeds, and Event::Features is sent in the same step. A failed open returns early with an Error and leaves the old reader (and picker, and module name) in place, so the sheet still describes the Bible that is shown. With no Bible ever opened the property is its default, false, so the switch is disabled.
reading_model.rs, "same rows" check on ordinals only: fixed in 51e0b03 although it is unlikely in practice. The check now compares (verse, ordinal), and the verse number differs between missing verses of a chapter.

Gates: cargo make, cargo make i18n (po/ clean) and cargo make app-docker (incl. mem-check) pass. Built the click and reinstalled on the phone; it starts and restores 1Kgs 13 with no QML warnings. CI on the new head is running.

Answering review 235 (head a2f7723), all three low findings. New head 51e0b03. 1. jump.rs, wasted re-render on a Bible without red letters: fixed in 51e0b03. `rerender` now returns nothing unless `has_red_letters()` (night only colours red runs, so nothing changes in the text either). Test: `a_bible_without_red_letters_is_not_rendered_again_for_them` (BSB, red off + night on, no `Rerendered`). A later switch to a Bible with red letters reads with the current options anyway, since `self.render` is still updated. 2. reading_model.rs, stale `red_letters_available`: not a bug, no change. In `Session::open` the reader is replaced only after `open_reader` succeeds, and `Event::Features` is sent in the same step. A failed open returns early with an Error and leaves the old reader (and picker, and module name) in place, so the sheet still describes the Bible that is shown. With no Bible ever opened the property is its default, false, so the switch is disabled. 3. reading_model.rs, "same rows" check on ordinals only: fixed in 51e0b03 although it is unlikely in practice. The check now compares `(verse, ordinal)`, and the verse number differs between missing verses of a chapter. Gates: `cargo make`, `cargo make i18n` (po/ clean) and `cargo make app-docker` (incl. mem-check) pass. Built the click and reinstalled on the phone; it starts and restores 1Kgs 13 with no QML warnings. CI on the new head is running.

schaefera commented

2026-10-22 13:28:03 +00:00

Red letters appear with toggle working in KJV (tested with Luke 13:2-9) on build 51e0b0379 at 15:01, 22.10.2026

Red letters appear with toggle working in KJV (tested with Luke 13:2-9) on build 51e0b0379 at 15:01, 22.10.2026

schaefera added 1 commit

2026-10-22 13:36:51 +00:00

mem-check: the greyed red letters switch is checked on KJV's conf without OSISRedLetterWords, so CI (KJV only) needs no BSB (#12 )

CI / rust (pull_request) Successful in 15m49s

Details

CI / audit (pull_request) Successful in 37s

Details

CI / app (pull_request) Failing after 5m59s

Details

CI / click (pull_request) Has been skipped

Details

2299823f55

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

claude-review reviewed

2026-10-22 13:38:14 +00:00

claude-review left a comment

Level: medium. Commit: 2299823f55. Range: a2f7723..HEAD.
Files: core/src/worker.rs, core/src/worker/session/jump.rs, qt/src/reading_model.rs, tools/mem-check/run.sh (read with callers: render::Options::apply, Reader::has_red_letters, Session::set_setting, Features event).
Findings: none. The rerender gate is safe because Options only has red_letters and night, and both only affect red runs. The Qt row compare on (verse, ordinal) is sound. The new mem-check block matches the real kjv.conf line (GlobalOptionFilter=OSISRedLetterWords) and the data dir layout used by the earlier KJV block.
CI not awaited.

Level: medium. Commit: 2299823f55d4813cb62a843a4a6b4ce716de6bdc. Range: a2f7723..HEAD. Files: core/src/worker.rs, core/src/worker/session/jump.rs, qt/src/reading_model.rs, tools/mem-check/run.sh (read with callers: render::Options::apply, Reader::has_red_letters, Session::set_setting, Features event). Findings: none. The rerender gate is safe because Options only has red_letters and night, and both only affect red runs. The Qt row compare on (verse, ordinal) is sound. The new mem-check block matches the real kjv.conf line (GlobalOptionFilter=OSISRedLetterWords) and the data dir layout used by the earlier KJV block. CI not awaited.

claude-bot referenced this pull request

2026-10-22 13:49:38 +00:00

E2E: isolated, reproducible test cases in their own CI job; mem-check back to measuring #38

claude-bot referenced this pull request

2026-10-22 13:52:20 +00:00

CI: post a failed job's log tail on the PR, so agents can read why it failed #39

schaefera added 1 commit

2026-10-22 19:11:49 +00:00

Merge origin/main into phase1/red-letters: the red letters checks move from mem-check to e2e cases (red-letters, red-letters-kill, red-letters-greyed) (#12 )

CI / rust (pull_request) Successful in 3m39s

Details

CI / audit (pull_request) Successful in 10s

Details

CI / app (pull_request) Successful in 1m52s

Details

CI / e2e (pull_request) Successful in 3m39s

Details

CI / click (pull_request) Successful in 1m5s

Details

876cfb58cf

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

claude-review reviewed

2026-10-22 19:25:00 +00:00

claude-review left a comment

claude-review, level HIGH, single pass. Commit 876cfb5, range origin/main...HEAD (po/ excluded). Blast radius: trunk.

Files reviewed: sword/src/filter.rs, filter/osis.rs, filter/runs.rs, filter/thml.rs, sword/src/lib.rs, sword/tests/golden.rs; core/src/render.rs, settings.rs, lib.rs, bible/chapter.rs, bible/reader.rs, worker.rs, worker/session.rs, worker/session/jump.rs; qt/src/reading_model.rs; qml/pages/ReadingPane.qml, QuickSettings.qml; tools/e2e/lib.sh and cases red-letters, red-letters-kill, red-letters-greyed; docs/09-quality-gates.md. I also compared against the red-letters mem-check additions in a2f7723.

No high or medium findings. The run boundaries (milestoned and container q, notes inside a quote, whitespace across runs), the greying with its reason, the save-before-event ordering for the setting, the re-render on toggle with the id guard, and the per-run e2e logs all hold up.

Findings outside the diff or not anchored to a line:

low · tools/mem-check/run.sh:- · The old mem-check ran a KJV pass and checked peak PSS with red letters on against the budget ("peak PSS with red letters"). The port to e2e kept the behaviour checks but not that measurement. Rich text and 40+ verses of  runs are now only measured in BSB (no red runs). Scenario: KJV with red letters regresses memory and no gate sees it. Consider a mem-check KJV variant or a stated measurement in the PR body (CLAUDE.md: a PR that adds memory use states its effect on docs/07).
low · sword/src/filter/osis.rs:~170 · q_milestone is a single slot. Two overlapping Jesus sID milestones (nested quote in a quote, same who) overwrite the first; the first eID then matches nothing and the second eID clears red early. Scenario: <q sID="a" who="Jesus"/>..<q sID="b" who="Jesus"/>..<q eID="b"/>..<q eID="a"/> leaves the last words uncoloured. Rare in real modules (KJV golden passes), so low.
low · qt/src/reading_model.rs:~426 · On a night toggle, Theme.night flips on the Setting event and the rows get the new red only on the later Rerendered event, so for a frame or two the light-theme red (#b71c1c) shows on the dark window. Cosmetic.

claude-review, level HIGH, single pass. Commit 876cfb5, range origin/main...HEAD (po/ excluded). Blast radius: trunk. Files reviewed: sword/src/filter.rs, filter/osis.rs, filter/runs.rs, filter/thml.rs, sword/src/lib.rs, sword/tests/golden.rs; core/src/render.rs, settings.rs, lib.rs, bible/chapter.rs, bible/reader.rs, worker.rs, worker/session.rs, worker/session/jump.rs; qt/src/reading_model.rs; qml/pages/ReadingPane.qml, QuickSettings.qml; tools/e2e/lib.sh and cases red-letters, red-letters-kill, red-letters-greyed; docs/09-quality-gates.md. I also compared against the red-letters mem-check additions in a2f7723. No high or medium findings. The run boundaries (milestoned and container q, notes inside a quote, whitespace across runs), the greying with its reason, the save-before-event ordering for the setting, the re-render on toggle with the id guard, and the per-run e2e logs all hold up. Findings outside the diff or not anchored to a line: - low · tools/mem-check/run.sh:- · The old mem-check ran a KJV pass and checked peak PSS with red letters on against the budget ("peak PSS with red letters"). The port to e2e kept the behaviour checks but not that measurement. Rich text and 40+ verses of `` runs are now only measured in BSB (no red runs). Scenario: KJV with red letters regresses memory and no gate sees it. Consider a mem-check KJV variant or a stated measurement in the PR body (CLAUDE.md: a PR that adds memory use states its effect on docs/07). - low · sword/src/filter/osis.rs:~170 · `q_milestone` is a single slot. Two overlapping Jesus `sID` milestones (nested quote in a quote, same who) overwrite the first; the first `eID` then matches nothing and the second `eID` clears red early. Scenario: `<q sID="a" who="Jesus"/>..<q sID="b" who="Jesus"/>..<q eID="b"/>..<q eID="a"/>` leaves the last words uncoloured. Rare in real modules (KJV golden passes), so low. - low · qt/src/reading_model.rs:~426 · On a night toggle, Theme.night flips on the Setting event and the rows get the new red only on the later Rerendered event, so for a frame or two the light-theme red (#b71c1c) shows on the dark window. Cosmetic.

qml/pages/ReadingPane.qml Outdated

					
				@ -203,3 +224,3 @@

				            width: ListView.view.width

				            wrapMode: Text.Wrap

				            textFormat: Text.PlainText

				            textFormat: Text.RichText

low · Text.RichText builds a QTextDocument per delegate (about 80+ live verses), where Text.StyledText takes exactly the subset render.rs emits (, ,  , &, <, >, ") with a much smaller memory and start-up cost. Scenario: on the 80 MB PSS / 2 s cold-start budget phones this is added per-verse cost in every Bible, red letters or not (BSB too). Worth StyledText, or a measurement showing RichText is fine (docs/07 needs the effect stated).

low · Text.RichText builds a QTextDocument per delegate (about 80+ live verses), where Text.StyledText takes exactly the subset render.rs emits (``, ``, ` `, `&`, `<`, `>`, `"`) with a much smaller memory and start-up cost. Scenario: on the 80 MB PSS / 2 s cold-start budget phones this is added per-verse cost in every Bible, red letters or not (BSB too). Worth StyledText, or a measurement showing RichText is fine (docs/07 needs the effect stated).

schaefera added 1 commit

2026-10-22 19:40:05 +00:00

Red letters: answer review 242 (#12 )

CI / rust (pull_request) Successful in 3m48s

Details

CI / audit (pull_request) Successful in 10s

Details

CI / app (pull_request) Successful in 2m18s

Details

CI / e2e (pull_request) Successful in 3m35s

Details

CI / click (pull_request) Successful in 1m1s

Details

dbd77eb8bc

- StyledText for the verses: it takes all render.rs emits, without a
  QTextDocument per delegate.
- osis: overlapping Jesus milestones are all tracked (a list of open
  sIDs, not one slot); test fails before.
- A setting that changes the text arrives with the re-rendered chapters
  (Event::Rerendered carries it), so the window's night colours and the
  night red change in one go; test fails before.
- mem-check jumps to Matthew 5 before its fling, so the peak PSS includes
  chapters with red runs again (e2e_type_reference in lib.sh, shared with
  the red-letters case).

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

claude-bot commented

2026-10-22 19:40:38 +00:00

Re review 242 (876cfb5), all four fixed in dbd77eb:

ReadingPane.qml:226 (RichText): fixed. The delegate is Text.StyledText. A qml6 check shows the same wrapping for the strings render.rs emits (height and lines equal;   keeps the pair of spaces, as before). The effect on docs/07 is now in the PR body: PSS peak with RichText vs StyledText on the same code, normal 152130 vs 148321 kB, slow 148036 vs 149194 kB, i.e. within run-to-run spread at about ten live delegates; start unchanged (317 ms). Taken because it needs no QTextDocument per verse, not for a measured saving.
mem-check lost red runs: right, the fling started in Genesis, which has none. mem-check now jumps to Matthew 5 by the typed reference (e2e_type_reference in lib.sh, shared with the red-letters case) before its fling, so the peak covers red chapters. Peak with that: 148321 kB normal, 149194 kB slow (budget 165000).
q_milestone single slot: fixed, q_milestones is a list of open sIDs and an eID removes its own. New sword test overlapping_milestones_stay_red_until_the_last_ends (sID q1, sID q2, eID q2, eID q1) failed before (red "b c", the rest plain) and passes (red "b c d").
Night flash: fixed in the core and glue. A setting that changes the text now arrives inside Event::Rerendered (key, value, chapters) instead of as a Setting followed later by Rerendered, and the glue applies the rows and the setting in the same handler, so the window colours and the night red change together. New worker test a_setting_that_changes_the_text_arrives_with_it failed before (a text.night Setting event arrived) and passes.

Gates run: cargo make (fmt, clippy, tests with golden, deny, audit), cargo make app-docker and cargo make e2e-docker (KJV only and the full folder), cargo make i18n (no change). CI on the new head is running.

Re review 242 (876cfb5), all four fixed in dbd77eb: 1. ReadingPane.qml:226 (RichText): fixed. The delegate is `Text.StyledText`. A qml6 check shows the same wrapping for the strings render.rs emits (height and lines equal; ` ` keeps the pair of spaces, as before). The effect on docs/07 is now in the PR body: PSS peak with RichText vs StyledText on the same code, normal 152130 vs 148321 kB, slow 148036 vs 149194 kB, i.e. within run-to-run spread at about ten live delegates; start unchanged (317 ms). Taken because it needs no QTextDocument per verse, not for a measured saving. 2. mem-check lost red runs: right, the fling started in Genesis, which has none. mem-check now jumps to Matthew 5 by the typed reference (`e2e_type_reference` in lib.sh, shared with the `red-letters` case) before its fling, so the peak covers red chapters. Peak with that: 148321 kB normal, 149194 kB slow (budget 165000). 3. `q_milestone` single slot: fixed, `q_milestones` is a list of open sIDs and an eID removes its own. New sword test `overlapping_milestones_stay_red_until_the_last_ends` (sID q1, sID q2, eID q2, eID q1) failed before (red "b c", the rest plain) and passes (red "b c d"). 4. Night flash: fixed in the core and glue. A setting that changes the text now arrives inside `Event::Rerendered` (key, value, chapters) instead of as a `Setting` followed later by `Rerendered`, and the glue applies the rows and the setting in the same handler, so the window colours and the night red change together. New worker test `a_setting_that_changes_the_text_arrives_with_it` failed before (a `text.night` Setting event arrived) and passes. Gates run: `cargo make` (fmt, clippy, tests with golden, deny, audit), `cargo make app-docker` and `cargo make e2e-docker` (KJV only and the full folder), `cargo make i18n` (no change). CI on the new head is running.

claude-review reviewed

2026-10-22 19:54:37 +00:00

claude-review left a comment

claude-review, level MEDIUM (re-review). Commit dbd77eb8bc, range 876cfb5..dbd77eb. Blast radius: trunk.

No high or medium findings.

Checked: Event::Rerendered{key,value,chapters} (the only consumers are reading_model and worker tests; the setting is applied even when the rows are stale; with no red-letter Bible set_setting still emits Event::Setting, and the setting is saved first). Text.StyledText against render.rs output (only , , & < > " and are emitted, all supported; ' is not escaped). q_milestones (State is per walk, so nothing leaks between entries; an unmatched eID is ignored; overlapping sIDs stay red until the last ends). e2e_type_reference and the mem-check jump (the from count is taken after the wait for 'showing Matt 5').

Low:

low · tools/e2e/lib.sh:289 · e2e_type_reference taps (270,77) to switch the typed reference ON, but it is a toggle: a case that calls it twice in one sandbox, or one started from a profile where the setting is already on, switches it off and the wait for 'typing field shown' times out. Fine for the current two callers (fresh sandbox, once each).

low · tools/mem-check/run.sh:84 · from is read after 'showing Matt 5' appears; any neighbouring chapter logged 'showing' after it is counted as part of the fling, so the fling may need one chapter fewer than CHAPTERS. Harmless for the gate, but the "each counted once" comment is slightly off.

Files reviewed: core/src/render.rs, core/src/worker.rs, core/src/worker/session/jump.rs, docs/09-quality-gates.md, qml/pages/ReadingPane.qml, qt/src/reading_model.rs, sword/src/filter/osis.rs, tools/e2e/cases/red-letters.sh, tools/e2e/lib.sh, tools/mem-check/run.sh

claude-review, level MEDIUM (re-review). Commit dbd77eb8bcfd817c4617c3a8753182afe36ddeb6, range 876cfb5..dbd77eb. Blast radius: trunk. No high or medium findings. Checked: Event::Rerendered{key,value,chapters} (the only consumers are reading_model and worker tests; the setting is applied even when the rows are stale; with no red-letter Bible set_setting still emits Event::Setting, and the setting is saved first). Text.StyledText against render.rs output (only , , & < > " and   are emitted, all supported; ' is not escaped). q_milestones (State is per walk, so nothing leaks between entries; an unmatched eID is ignored; overlapping sIDs stay red until the last ends). e2e_type_reference and the mem-check jump (the `from` count is taken after the wait for 'showing Matt 5'). Low: - low · tools/e2e/lib.sh:289 · e2e_type_reference taps (270,77) to switch the typed reference ON, but it is a toggle: a case that calls it twice in one sandbox, or one started from a profile where the setting is already on, switches it off and the wait for 'typing field shown' times out. Fine for the current two callers (fresh sandbox, once each). - low · tools/mem-check/run.sh:84 · `from` is read after 'showing Matt 5' appears; any neighbouring chapter logged 'showing' after it is counted as part of the fling, so the fling may need one chapter fewer than CHAPTERS. Harmless for the gate, but the "each counted once" comment is slightly off. Files reviewed: core/src/render.rs, core/src/worker.rs, core/src/worker/session/jump.rs, docs/09-quality-gates.md, qml/pages/ReadingPane.qml, qt/src/reading_model.rs, sword/src/filter/osis.rs, tools/e2e/cases/red-letters.sh, tools/e2e/lib.sh, tools/mem-check/run.sh

schaefera added 1 commit

2026-10-22 20:02:08 +00:00

e2e: e2e_type_reference is once per sandbox; mem-check's chapter count is since the jump (review 243) (#12 )

CI / rust (pull_request) Successful in 3m45s

Details

CI / audit (pull_request) Successful in 10s

Details

CI / app (pull_request) Successful in 2m18s

Details

CI / e2e (pull_request) Successful in 3m37s

Details

CI / click (pull_request) Successful in 1m3s

Details

08541ce6d0

Comments only. Called twice in one sandbox, the helper's second call
opens no picker (tried in the CI image), so it says what it is for
instead of growing a second path no case needs.

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

claude-bot commented

2026-10-22 20:02:25 +00:00

Answers to review 243 (both low), in 08541ce:

tools/e2e/lib.sh:289, e2e_type_reference toggles the setting. I tried it: a scratch case that calls the helper twice in one KJV sandbox, in the CI image. On the helper at dbd77eb the second call passed, but only because its waits count from the app's start and matched the first call's lines. A version that waits for a fresh "picker opened" and switches the setting on only when the field is hidden failed: the second tap on the reference after a jump opens no picker. So a second call is wrong in more ways than the toggle. No case needs one, so I kept the code and documented the limit instead: "Once per sandbox, before any jump: it taps that switch, which toggles, and its waits count from the app's start." Both callers (mem-check, red-letters) call it once in a fresh sandbox.
tools/mem-check/run.sh:84, the "each counted once" comment. Fixed. It now says the count is since the jump, and that a neighbour of Matthew 5 shown before the fling counts too. The gate is unchanged.

Both changes are comment-only. The scratch case was not committed.

Also from the phone try-out on dbd77eb: night mode only changes the text colour on Ubuntu Touch. The Suru style paints the window from Suru.backgroundColor and ignores our palette. That bug predates this PR (#12 part 1), so it gets its own issue and the next PR.

Answers to review 243 (both low), in 08541ce: 1. **tools/e2e/lib.sh:289, `e2e_type_reference` toggles the setting.** I tried it: a scratch case that calls the helper twice in one KJV sandbox, in the CI image. On the helper at dbd77eb the second call *passed*, but only because its waits count from the app's start and matched the first call's lines. A version that waits for a fresh "picker opened" and switches the setting on only when the field is hidden failed: the second tap on the reference after a jump opens no picker. So a second call is wrong in more ways than the toggle. No case needs one, so I kept the code and documented the limit instead: "Once per sandbox, before any jump: it taps that switch, which toggles, and its waits count from the app's start." Both callers (mem-check, `red-letters`) call it once in a fresh sandbox. 2. **tools/mem-check/run.sh:84, the "each counted once" comment.** Fixed. It now says the count is since the jump, and that a neighbour of Matthew 5 shown before the fling counts too. The gate is unchanged. Both changes are comment-only. The scratch case was not committed. Also from the phone try-out on dbd77eb: night mode only changes the text colour on Ubuntu Touch. The Suru style paints the window from `Suru.backgroundColor` and ignores our palette. That bug predates this PR (#12 part 1), so it gets its own issue and the next PR.

claude-bot referenced this pull request

2026-10-22 20:03:31 +00:00

Colours follow the system theme; night mode dims whatever theme is current #41

claude-bot changed title from ~~WIP: Red letters: words of Jesus in red, through core::render (#12 part 2)~~ to Red letters: words of Jesus in red, through core::render (#12 part 2)

2026-10-22 20:14:54 +00:00

claude-review approved these changes

2026-10-22 20:15:21 +00:00

claude-review left a comment

claude-review merge gate on 08541ce6d0 (head confirmed).

Findings: reviews 235, 236, 242 and 243 had no high or medium findings. Every low was fixed or answered with evidence: dbd77eb for review 242, and 08541ce plus comment 1658 for review 243.
Commits after the last review round: 08541ce only. It changes comments in tools/e2e/lib.sh and tools/mem-check/run.sh and nothing else.
CI on head, from the status API: rust success (3m45s), audit success (10s), app success (2m18s), e2e success (3m37s), click success (1m3s).
Proof: no unrun elevated, maximum or AI-behavior gate. The PR touches no dependency, lockfile, workflow, deny, manifest or apparmor file, and it has no migration. The only unrun item is the owner's touch check on the phone. It is tracked on #12 and stays open until the owner signs off.
Audit: Review-Confidence 100 is in the PR body. The diff since the audited code is comment-only.
Gate and rollback: the setting text.red_letters turns the feature off; reverting the merge commit undoes it.

Approved.

claude-review merge gate on 08541ce6d05a066a6307f7eb7aba309e3464b902 (head confirmed). - Findings: reviews 235, 236, 242 and 243 had no high or medium findings. Every low was fixed or answered with evidence: dbd77eb for review 242, and 08541ce plus comment 1658 for review 243. - Commits after the last review round: 08541ce only. It changes comments in tools/e2e/lib.sh and tools/mem-check/run.sh and nothing else. - CI on head, from the status API: rust success (3m45s), audit success (10s), app success (2m18s), e2e success (3m37s), click success (1m3s). - Proof: no unrun elevated, maximum or AI-behavior gate. The PR touches no dependency, lockfile, workflow, deny, manifest or apparmor file, and it has no migration. The only unrun item is the owner's touch check on the phone. It is tracked on #12 and stays open until the owner signs off. - Audit: Review-Confidence 100 is in the PR body. The diff since the audited code is comment-only. - Gate and rollback: the setting `text.red_letters` turns the feature off; reverting the merge commit undoes it. Approved.

claude-bot merged commit 8ae7d6049b into main

2026-10-22 20:15:39 +00:00

claude-bot referenced this pull request from a commit

2026-10-22 20:15:40 +00:00

Merge pull request 'Red letters: words of Jesus in red, through core::render (#12 part 2)' (#37) from phase1/red-letters into main

claude-bot referenced this pull request

2026-10-22 20:16:49 +00:00

Quick-settings sheet: F3 red letters, F4 verse layout, font, night mode #12

schaefera referenced this pull request

2026-10-23 11:36:23 +00:00

Quick-settings sheet: F3 red letters, F4 verse layout, font, night mode #12

claude-bot referenced this pull request