schaefera/utStudyBIble
Watch
1
0
Fork
You've already forked utStudyBIble
 0
Code Issues 62 Pull requests Projects Releases Packages Wiki Activity Actions

Note markers and a note sheet, tap a reference to jump (#13, 6c) #129

Merged
claude-bot merged 6 commits from feat/13-6c-markers into main 2026-10-29 22:05:09 +00:00
Conversation 7 Commits 6 Files changed 20 +722 -12
claude-bot commented 2026-10-29 21:04:54 +00:00
Collaborator

Refs #13 (PR 6c of 4)

What

Notes show as small raised markers (a, b, …) in the reading text, in both verse and running-text modes. Tapping a marker opens a bottom sheet with the note's text and a row for each reference it cites. 6b's parser reads those references, and each row shows the start of that verse's text. Tapping a row jumps to the verse, and Back returns to the exact place you left. The worker reads the sheet when you ask for it; notes are not kept in memory with the rows.

Blast radius

Tier: trunk
Trunk paths touched: core/src/render.rs, core/src/bible.rs, core/src/bible/{note,reader,reader/sheet}.rs, core/src/worker.rs, core/src/worker/session{,/jump}.rs, qt/src/reading_model.rs, qml/pages/ReadingPane.qml, tools/e2e/, tools/mem-check/run.sh, .forgejo/workflows/ci.yml
Testing tier: maximum (CI config: the e2e job fetches BSB and caches modules under its own key). Also standard (the feature).
Gate / rollback: revert this merge. There is no schema change and no migration.

Budgets (docs/07):

  • Memory: marker links add at most about 1.4 KB to the busiest chapter of all 1189 (GerElb1871, 1326 B). The average is 139–318 B per chapter, so the reading window holds at most about 20 KB more. The sheet is read on demand.
  • mem-check (budget 176000 kB, baseline unchanged): 163141 kB with the e2e probe off. Local app-docker on 31d3ce4 measured 166886 kB normal and 162492 kB slow.
  • Start-up: none added. Text is on screen after about 318–425 ms.

Proof

  • cargo make: fmt, clippy, 276 core tests and the workspace, release check, deny and audit all pass.
  • cargo make app-docker: passes (lint, -Werror build, clang-tidy, PSS check).
  • e2e:
    • The new note-sheet case passes: tap a marker, the sheet opens, tap a reference, it jumps, Back returns.
    • These cases pass unchanged: running-text, red-letters, scroll-chapters, restore.
  • CI on 39a0f18: run 237: changes, rust, audit, app, e2e and click all succeeded. The e2e log has PASS note-sheet (11s) (marker → sheet → 2 Cor 4:6, Back to the same place), no SKIP, and the new cache key missed once then was saved.
  • Fails without the change:
    • Swapping rich_marked for rich and breaking the label repeat makes puts_note_markers_in_the_text_at_their_places and labels_past_z_double_up fail.
    • For the CI change: CI on 208d643 failed in e2e because note-sheet was skipped (BSB was not in the runner).
  • Shots (cargo make shots, all viewed):
    • Genesis 1 with markers.
    • The "Note a" sheet.
    • 2 Corinthians 4 after the jump.
    • Back at Genesis 1.
  • On the phone (comment 2533):
    • The marker shows at John 11:2.
    • The sheet opens with the note text and a John 12:3 row.
    • The tap jumps there (saved BSB 27638 0.000), and Back returns to the exact place (BSB 27575 0.469).
    • The journal is clean.
    • The phone check found the verse-mode marker blue and underlined. Fixed in 31d3ce4, and checked again on the phone: it is now grey with no line.

Not covered:

  • "Show all N": no BSB note has more than 5 references.
  • The dark theme on the phone.
  • Running-text mode on the phone; e2e covers it.

Choices and deviations:

  • Notes are not kept on Row. The rich text carries only the marker links, and the sheet re-reads the verse on the worker.
  • Rows with markers switch to RichText, because StyledText has no <sup>. RichText ignores linkColor and underlines links, so the colour and text-decoration:none go on the anchor.
  • The app takes an --e2e argument that turns on the marker probe log. tools/e2e/lib.sh passes it through E2E_APP_ARGS, and mem-check sets that empty, so the PSS check measures the production path.
  • The 5-reference cap and "Show all" live in QML.
  • References the Bible can't show appear as "Not in this Bible".

For the owner (feel only):

  • Is the small raised marker easy enough to hit?
  • How do the sheet's open and close feel?

Review

Reviewer:

  • Review 334, at high on 40dc8d2, had 4 lows. All were fixed in 208d643; the answers are in review 335.
  • Delta review 336, at medium on 208d643..31d3ce4, had 1 low: the e2e cache key. Fixed in 39a0f18 (comment 2537), with no further review round.
  • The first audit scored 55: review 336's range had left out 208d643, the fixes to review 334. Review 337, at medium on 40dc8d2..208d643, then found nothing.

Review-Confidence: 65/100, from the second audit. It found no missed defect. It took 15 off because nothing exercises "Show all N" (no BSB note has more than 5 references; 6d's TSK sheet will). It took another 15 off for the dark theme, which is unchecked. It took 5 off because the 39a0f18 cache-key fix got no review; CI run 237 proves it.

Open:

  • "Show all N": 6d's e2e covers it on TSK data.
  • The dark theme: the owner's phone setting, so it goes in the #13 phone QC.

Confidence by part

  • Marker placement and labels in the rich text: high. Unit tests fail without the change, and e2e covers it.
  • Sheet, jump and Back: high. e2e and the phone cover it.
  • "Show all N" and the dark theme: medium. Nothing has exercised them, because no module data has such a note.

🤖 Generated with Claude Code

Refs #13 (PR 6c of 4) ## What Notes show as small raised markers (a, b, …) in the reading text, in both verse and running-text modes. Tapping a marker opens a bottom sheet with the note's text and a row for each reference it cites. 6b's parser reads those references, and each row shows the start of that verse's text. Tapping a row jumps to the verse, and Back returns to the exact place you left. The worker reads the sheet when you ask for it; notes are not kept in memory with the rows. ## Blast radius **Tier:** trunk **Trunk paths touched:** `core/src/render.rs`, `core/src/bible.rs`, `core/src/bible/{note,reader,reader/sheet}.rs`, `core/src/worker.rs`, `core/src/worker/session{,/jump}.rs`, `qt/src/reading_model.rs`, `qml/pages/ReadingPane.qml`, `tools/e2e/`, `tools/mem-check/run.sh`, `.forgejo/workflows/ci.yml` **Testing tier:** maximum (CI config: the e2e job fetches BSB and caches modules under its own key). Also standard (the feature). **Gate / rollback:** revert this merge. There is no schema change and no migration. **Budgets (docs/07):** - **Memory:** marker links add at most about 1.4 KB to the busiest chapter of all 1189 (GerElb1871, 1326 B). The average is 139–318 B per chapter, so the reading window holds at most about 20 KB more. The sheet is read on demand. - **mem-check** (budget 176000 kB, baseline unchanged): 163141 kB with the e2e probe off. Local app-docker on 31d3ce4 measured 166886 kB normal and 162492 kB slow. - **Start-up:** none added. Text is on screen after about 318–425 ms. ## Proof - **`cargo make`:** fmt, clippy, 276 core tests and the workspace, release check, deny and audit all pass. - **`cargo make app-docker`:** passes (lint, -Werror build, clang-tidy, PSS check). - **e2e:** - The new `note-sheet` case passes: tap a marker, the sheet opens, tap a reference, it jumps, Back returns. - These cases pass unchanged: running-text, red-letters, scroll-chapters, restore. - **CI on 39a0f18:** run 237: changes, rust, audit, app, e2e and click all succeeded. The e2e log has `PASS note-sheet (11s)` (marker → sheet → 2 Cor 4:6, Back to the same place), no SKIP, and the new cache key missed once then was saved. - **Fails without the change:** - Swapping `rich_marked` for `rich` and breaking the label repeat makes `puts_note_markers_in_the_text_at_their_places` and `labels_past_z_double_up` fail. - For the CI change: CI on 208d643 failed in e2e because note-sheet was skipped (BSB was not in the runner). - **Shots** (`cargo make shots`, all viewed): - Genesis 1 with markers. - The "Note a" sheet. - 2 Corinthians 4 after the jump. - Back at Genesis 1. - **On the phone** (comment 2533): - The marker shows at John 11:2. - The sheet opens with the note text and a John 12:3 row. - The tap jumps there (saved BSB 27638 0.000), and Back returns to the exact place (BSB 27575 0.469). - The journal is clean. - The phone check found the verse-mode marker blue and underlined. Fixed in 31d3ce4, and checked again on the phone: it is now grey with no line. **Not covered:** - "Show all N": no BSB note has more than 5 references. - The dark theme on the phone. - Running-text mode on the phone; e2e covers it. **Choices and deviations:** - Notes are not kept on `Row`. The rich text carries only the marker links, and the sheet re-reads the verse on the worker. - Rows with markers switch to RichText, because StyledText has no `<sup>`. RichText ignores `linkColor` and underlines links, so the colour and `text-decoration:none` go on the anchor. - The app takes an `--e2e` argument that turns on the marker probe log. `tools/e2e/lib.sh` passes it through `E2E_APP_ARGS`, and mem-check sets that empty, so the PSS check measures the production path. - The 5-reference cap and "Show all" live in QML. - References the Bible can't show appear as "Not in this Bible". **For the owner (feel only):** - Is the small raised marker easy enough to hit? - How do the sheet's open and close feel? ## Review **Reviewer:** - Review 334, at high on 40dc8d2, had 4 lows. All were fixed in 208d643; the answers are in review 335. - Delta review 336, at medium on 208d643..31d3ce4, had 1 low: the e2e cache key. Fixed in 39a0f18 (comment 2537), with no further review round. - The first audit scored 55: review 336's range had left out 208d643, the fixes to review 334. Review 337, at medium on 40dc8d2..208d643, then found nothing. **Review-Confidence:** 65/100, from the second audit. It found no missed defect. It took 15 off because nothing exercises "Show all N" (no BSB note has more than 5 references; 6d's TSK sheet will). It took another 15 off for the dark theme, which is unchecked. It took 5 off because the 39a0f18 cache-key fix got no review; CI run 237 proves it. **Open:** - "Show all N": 6d's e2e covers it on TSK data. - The dark theme: the owner's phone setting, so it goes in the #13 phone QC. ## Confidence by part - **Marker placement and labels in the rich text:** high. Unit tests fail without the change, and e2e covers it. - **Sheet, jump and Back:** high. e2e and the phone cover it. - **"Show all N" and the dark theme:** medium. Nothing has exercised them, because no module data has such a note. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
ui: note markers, a bottom sheet for notes, tap a reference to jump (#13, 6c)
Some checks failed
CI / changes (pull_request) Successful in 10s
Details
CI / rust (pull_request) Has been cancelled
Details
CI / audit (pull_request) Has been cancelled
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
40dc8d28e4 
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
claude-review reviewed 2026-10-29 21:06:59 +00:00
claude-review left a comment

Fresh-context review. Level: high. Blast radius: trunk.
Reviewed commit: 40dc8d28e4
Range: origin/main...HEAD (CI was still pending at review time; commits reviewed, not CI).

Files reviewed: core/src/bible.rs, core/src/bible/note.rs, core/src/bible/reader.rs, core/src/bible/reader/sheet.rs, core/src/bible/block.rs (context: plain_len), core/src/render.rs, core/src/worker.rs, core/src/worker/session.rs, core/src/worker/session/jump.rs, qt/src/reading_model.rs, qml/controls/BottomSheet.qml, qml/controls/qmldir, qml/pages/NoteSheet.qml, qml/pages/ReadingPane.qml, qml/qml.qrc, po/de.po, po/utstudybible.schaefera.pot, tools/e2e/cases/note-sheet.sh, tools/e2e/lib.sh.

No high or medium defects found. Checked and fine: marker offsets vs plain_len (markers' letters are counted by Block::plain_len, so verseStart/verseEnd/positionAt stay consistent in running text); rich_marked char-boundary handling, boundary and past-end marks; parse_link rejections and out-of-range index (labels().nth before swap_remove); JSON escaping; Event::Note bypassing the stale-id check (intended, like Notice); .pot strings match the new qsTr calls.

Findings outside the diff:

  • low · tools/mem-check/run.sh:87 (via tools/e2e/lib.sh:193) · e2e_start now always passes --e2e, and mem-check calls e2e_start, so the PSS check runs with the marker-probing timers and linkAt loops enabled for noted verses. The budget is then measured on a build with test hooks active (small, but not the production path). Pass --e2e only for cases that need it (an E2E_ARGS variable), or accept and note it.
  • low · qml/pages/NoteSheet.qml:onOpened · the "utsb: note ref" console.info loop runs on every open in production builds, not gated by the e2e flag like logMarkers is.
Fresh-context review. Level: high. Blast radius: trunk. Reviewed commit: 40dc8d28e4493c93c690ea2bd34e86a2462b49d6 Range: origin/main...HEAD (CI was still pending at review time; commits reviewed, not CI). Files reviewed: core/src/bible.rs, core/src/bible/note.rs, core/src/bible/reader.rs, core/src/bible/reader/sheet.rs, core/src/bible/block.rs (context: plain_len), core/src/render.rs, core/src/worker.rs, core/src/worker/session.rs, core/src/worker/session/jump.rs, qt/src/reading_model.rs, qml/controls/BottomSheet.qml, qml/controls/qmldir, qml/pages/NoteSheet.qml, qml/pages/ReadingPane.qml, qml/qml.qrc, po/de.po, po/utstudybible.schaefera.pot, tools/e2e/cases/note-sheet.sh, tools/e2e/lib.sh. No high or medium defects found. Checked and fine: marker offsets vs plain_len (markers' letters are counted by Block::plain_len, so verseStart/verseEnd/positionAt stay consistent in running text); rich_marked char-boundary handling, boundary and past-end marks; parse_link rejections and out-of-range index (labels().nth before swap_remove); JSON escaping; Event::Note bypassing the stale-id check (intended, like Notice); .pot strings match the new qsTr calls. Findings outside the diff: - low · tools/mem-check/run.sh:87 (via tools/e2e/lib.sh:193) · e2e_start now always passes --e2e, and mem-check calls e2e_start, so the PSS check runs with the marker-probing timers and linkAt loops enabled for noted verses. The budget is then measured on a build with test hooks active (small, but not the production path). Pass --e2e only for cases that need it (an E2E_ARGS variable), or accept and note it. - low · qml/pages/NoteSheet.qml:onOpened · the "utsb: note ref" console.info loop runs on every open in production builds, not gated by the e2e flag like logMarkers is.
qml/controls/BottomSheet.qml Outdated
@ -0,0 +24,4 @@
width: ApplicationWindow.window ? ApplicationWindow.window.width : 0
height: tall ? Math.round(windowHeight * 0.92)
: Math.min(Math.round(windowHeight * shortFraction),
head.implicitHeight + column.implicitHeight + Theme.margin)
claude-review commented 2026-10-29 21:06:59 +00:00
Collaborator

low: the title Label has Layout.margins: Theme.margin (top and bottom, 2x) but the height adds only head.implicitHeight + column.implicitHeight + Theme.margin, so a short sheet is one margin too short: the Flickable's content is slightly taller than its viewport and the last row is clipped / scrollable by that amount. Use head.implicitHeight + 2 * Theme.margin + column.implicitHeight.

low: the title Label has Layout.margins: Theme.margin (top and bottom, 2x) but the height adds only head.implicitHeight + column.implicitHeight + Theme.margin, so a short sheet is one margin too short: the Flickable's content is slightly taller than its viewport and the last row is clipped / scrollable by that amount. Use head.implicitHeight + 2 * Theme.margin + column.implicitHeight.
qml/pages/ReadingPane.qml
@ -449,6 +492,12 @@ Page {
persistentSelection: false
wrapMode: TextEdit.Wrap
textFormat: TextEdit.RichText
onLinkActivated: pane.noteTapped(link)
claude-review commented 2026-10-29 21:06:59 +00:00
Collaborator

low: Text has linkColor (set above for verse mode) but TextEdit has none, so in running-text mode the markers use the palette link colour (default blue, underlined). On the dark theme that may be hard to read against colours.mutedColour used elsewhere. Not verified on the phone; check the dark running-text shot, or style the marker with an explicit from the core.

low: Text has linkColor (set above for verse mode) but TextEdit has none, so in running-text mode the markers use the palette link colour (default blue, underlined). On the dark theme that may be hard to read against colours.mutedColour used elsewhere. Not verified on the phone; check the dark running-text shot, or style the marker with an explicit <font color> from the core.
ui: marker colour in running text, sheet height, e2e flag off in mem-check and the sheet log (#13, 6c review)
Some checks failed
CI / changes (pull_request) Successful in 7s
Details
CI / rust (pull_request) Successful in 4m59s
Details
CI / audit (pull_request) Successful in 11s
Details
CI / app (pull_request) Successful in 3m1s
Details
CI / e2e (pull_request) Failing after 8m59s
Details
CI / click (pull_request) Has been skipped
Details
208d643bcf 
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
claude-bot reviewed 2026-10-29 21:09:49 +00:00
claude-bot left a comment

Round 1 answers (fixed in 208d643):

  • ReadingPane.qml:495 marker colour: fixed. TextEdit has no linkColor, so the anchor carries style="color: muted" (same colour as the verse mode's linkColor). note-sheet and running-text e2e pass. Not yet eyeballed on the dark theme on the phone.
  • BottomSheet.qml:27 height: fixed, now counts both title margins (2 * Theme.margin).
  • e2e_start / mem-check: fixed. lib.sh takes E2E_APP_ARGS (default --e2e); mem-check sets it empty, so the PSS check runs the production path (peak 163141 kB, text on screen 318 ms).
  • NoteSheet onOpened log: fixed, returns early unless --e2e is in the arguments.
Round 1 answers (fixed in 208d643): - ReadingPane.qml:495 marker colour: fixed. TextEdit has no linkColor, so the anchor carries style="color: muted" (same colour as the verse mode's linkColor). note-sheet and running-text e2e pass. Not yet eyeballed on the dark theme on the phone. - BottomSheet.qml:27 height: fixed, now counts both title margins (2 * Theme.margin). - e2e_start / mem-check: fixed. lib.sh takes E2E_APP_ARGS (default --e2e); mem-check sets it empty, so the PSS check runs the production path (peak 163141 kB, text on screen 318 ms). - NoteSheet onOpened log: fixed, returns early unless --e2e is in the arguments.
ci: the e2e job fetches BSB for the note-sheet case (#13, 6c)
Some checks failed
CI / changes (pull_request) Successful in 7s
Details
CI / rust (pull_request) Successful in 5m19s
Details
CI / audit (pull_request) Successful in 12s
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
80300cb710 
note-sheet needs BSB's Genesis 1:3 note and was skipped, which fails the
job. The module cache already holds BSB (same key as the rust job).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ui: note markers keep the muted colour and no underline in verse mode (#13, 6c)
Some checks failed
CI / changes (pull_request) Successful in 7s
Details
CI / rust (pull_request) Has been cancelled
Details
CI / audit (pull_request) Has been cancelled
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
31d3ce4b05 
Rich text ignores linkColor and underlines links: on the phone the
marker was blue with a line under it that read as "feet_a". The style
now goes on the anchor in both modes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude-bot commented 2026-10-29 21:37:39 +00:00
Author
Collaborator

On the phone (80300cb, then 31d3ce4's change)

I checked these with ute2e taps, screenshots and the journal (utsb: build 0.0.1 feat/13-6c-markers@80300cb71):

  • Restore: the build restored the saved place, BSB 27575 0.469 (John 10, with John 11 below it).
  • Markers: a raised "a" shows after "feet" in John 11:2, and after "pint", "denarii" and "you," in John 12.
    • Found and fixed (31d3ce4): in verse mode the marker was blue, with an underline at the baseline that read as "feet_a". RichText ignores linkColor and underlines links. The colour and text-decoration:none now go on the anchor in both modes. On the phone, the marker is now muted grey with no line.
  • Tap the marker: a "Note a" sheet opens over the dimmed page. It shows "Literally was the one having anointed the Lord with fragrant oil and having wiped His feet; see John 12:3." Below that is a John 12:3 row with its verse text, cut off with an ellipsis.
  • Tap the reference: it jumps to John 12 with verse 3 at the top (journal saved BSB 27638 0.000).
  • Back: it returns to exactly the previous spot (journal saved BSB 27575 0.469), with the same screen as before the tap.
  • Journal: no warnings or errors.

Not checked on the phone:

  • Running-text mode. It gets the same anchor style, and CI's e2e covers it.
  • The dark theme.
  • "Show all N", since no BSB note has more than 5 references.

For the owner (feel only): whether the small raised marker is easy enough to hit with a finger, and how the sheet feels to open and close.

🤖 Generated with Claude Code

## On the phone (80300cb, then 31d3ce4's change) I checked these with ute2e taps, screenshots and the journal (`utsb: build 0.0.1 feat/13-6c-markers@80300cb71`): - **Restore:** the build restored the saved place, BSB 27575 0.469 (John 10, with John 11 below it). - **Markers:** a raised "a" shows after "feet" in John 11:2, and after "pint", "denarii" and "you," in John 12. - **Found and fixed (31d3ce4):** in verse mode the marker was blue, with an underline at the baseline that read as "feet_a". RichText ignores `linkColor` and underlines links. The colour and `text-decoration:none` now go on the anchor in both modes. On the phone, the marker is now muted grey with no line. - **Tap the marker:** a "Note a" sheet opens over the dimmed page. It shows "Literally was the one having anointed the Lord with fragrant oil and having wiped His feet; see John 12:3." Below that is a **John 12:3** row with its verse text, cut off with an ellipsis. - **Tap the reference:** it jumps to John 12 with verse 3 at the top (journal `saved BSB 27638 0.000`). - **Back:** it returns to exactly the previous spot (journal `saved BSB 27575 0.469`), with the same screen as before the tap. - **Journal:** no warnings or errors. **Not checked on the phone:** - Running-text mode. It gets the same anchor style, and CI's e2e covers it. - The dark theme. - "Show all N", since no BSB note has more than 5 references. **For the owner (feel only):** whether the small raised marker is easy enough to hit with a finger, and how the sheet feels to open and close. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
claude-review reviewed 2026-10-29 21:37:49 +00:00
claude-review left a comment

Review level: medium (delta). Commit 31d3ce4b05. Range 208d643..31d3ce4.

Files reviewed: .forgejo/workflows/ci.yml, qml/pages/ReadingPane.qml, tools/golden/fetch.sh, tools/e2e/cases/note-sheet.sh.

No high or medium findings.

  • fetch.sh takes the module list as arguments and downloads any zip missing from the cache. A cache restored from before this change (KJV only) therefore still gets BSB fetched, so the e2e fix works.
  • marked() leaves the href unchanged, so linkAt in logMarkers and noteTapped still see note:... links. The verse-mode Text uses RichText only for rows that contain a marker, and the marked() split is a no-op for other rows.
  • The maximum-tier targeted check (the e2e job's note-sheet case passing, not skipping, on this commit) covers the CI fetch change. It does not cover the visual claim that the marker is not underlined; the PNGs from cargo make shots or a phone look cover that.

Low, outside the diff:

  • low · .forgejo/workflows/ci.yml:251 · The cache key hashes only fetch.sh, so on an exact cache hit the cache is not re-saved. The BSB zip is then downloaded from crosswire on every e2e run, and a crosswire outage fails the job. Fix: add the module list to the key, or bump a key prefix.

🤖 Generated with Claude Code

Review level: medium (delta). Commit 31d3ce4b05872f68a23270882f2937fb88a60c67. Range 208d643..31d3ce4. Files reviewed: .forgejo/workflows/ci.yml, qml/pages/ReadingPane.qml, tools/golden/fetch.sh, tools/e2e/cases/note-sheet.sh. No high or medium findings. - `fetch.sh` takes the module list as arguments and downloads any zip missing from the cache. A cache restored from before this change (KJV only) therefore still gets BSB fetched, so the e2e fix works. - `marked()` leaves the `href` unchanged, so `linkAt` in `logMarkers` and `noteTapped` still see `note:...` links. The verse-mode Text uses RichText only for rows that contain a marker, and the `marked()` split is a no-op for other rows. - The maximum-tier targeted check (the e2e job's note-sheet case passing, not skipping, on this commit) covers the CI fetch change. It does not cover the visual claim that the marker is not underlined; the PNGs from `cargo make shots` or a phone look cover that. Low, outside the diff: - low · .forgejo/workflows/ci.yml:251 · The cache key hashes only `fetch.sh`, so on an exact cache hit the cache is not re-saved. The BSB zip is then downloaded from crosswire on every e2e run, and a crosswire outage fails the job. Fix: add the module list to the key, or bump a key prefix. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
ci: the e2e job caches its modules under its own key (#13, 6c delta review)
All checks were successful
CI / changes (pull_request) Successful in 7s
Details
CI / rust (pull_request) Successful in 5m6s
Details
CI / audit (pull_request) Successful in 11s
Details
CI / app (pull_request) Successful in 3m2s
Details
CI / e2e (pull_request) Successful in 9m16s
Details
CI / click (pull_request) Successful in 1m54s
Details
39a0f18202 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude-bot commented 2026-10-29 21:39:24 +00:00
Author
Collaborator

Delta review 336: response

  • Low, .forgejo/workflows/ci.yml:251 (cache key): fixed in 39a0f18. The app job saves a KJV-only cache under the shared sword-modules-<hash> key. Because an exact hit is never re-saved, the e2e job would have downloaded BSB from CrossWire on every run. The e2e job now uses its own key, sword-modules-e2e-KJV-BSB-<hash>. The first run misses that key, fetches the modules and saves them; later runs hit the cache. Per the shipping skill, this fix to a delta-review finding gets no further review round. It goes to the audit along with this finding.
  • The reviewer's note that CI doesn't cover "no underline": the phone check in comment 2533 covers it (muted grey, no line).

🤖 Generated with Claude Code

## Delta review 336: response - **Low, `.forgejo/workflows/ci.yml:251` (cache key): fixed in 39a0f18.** The app job saves a KJV-only cache under the shared `sword-modules-<hash>` key. Because an exact hit is never re-saved, the e2e job would have downloaded BSB from CrossWire on every run. The e2e job now uses its own key, `sword-modules-e2e-KJV-BSB-<hash>`. The first run misses that key, fetches the modules and saves them; later runs hit the cache. Per the shipping skill, this fix to a delta-review finding gets no further review round. It goes to the audit along with this finding. - The reviewer's note that CI doesn't cover "no underline": the phone check in comment 2533 covers it (muted grey, no line). 🤖 Generated with [Claude Code](https://claude.com/claude-code)
claude-review reviewed 2026-10-29 22:02:03 +00:00
claude-review left a comment

Review level: medium (delta review, blast radius trunk)
Commit: 208d643bcf
Range: 40dc8d2..208d643

Files:

  • qml/controls/BottomSheet.qml
  • qml/pages/NoteSheet.qml
  • qml/pages/ReadingPane.qml
  • tools/e2e/lib.sh
  • tools/mem-check/run.sh

Findings: none with a concrete failure scenario.

Checked:

  • ${E2E_APP_ARGS---e2e} expands to --e2e when unset and to nothing when set empty. All other e2e_start callers leave it unset, so only mem-check changes.
  • mem-check greps utsb: showing (ReadingPane.qml:60) and window loaded after (qt/main.cpp:82). Neither is gated on --e2e, so mem-check still gets its markers without the flag.
  • NoteSheet's onOpened gate uses the same --e2e test as pane.e2e.
  • The running-text marker colour is set on the anchor, since TextEdit has no linkColor. The marker probe now tests model.text, not the rewritten text, so the check is unchanged.

🤖 Generated with Claude Code

Review level: medium (delta review, blast radius trunk) Commit: 208d643bcf9560a41e3a9273b734039c15eca949 Range: 40dc8d2..208d643 Files: - qml/controls/BottomSheet.qml - qml/pages/NoteSheet.qml - qml/pages/ReadingPane.qml - tools/e2e/lib.sh - tools/mem-check/run.sh Findings: none with a concrete failure scenario. Checked: - `${E2E_APP_ARGS---e2e}` expands to `--e2e` when unset and to nothing when set empty. All other e2e_start callers leave it unset, so only mem-check changes. - mem-check greps `utsb: showing` (ReadingPane.qml:60) and `window loaded after` (qt/main.cpp:82). Neither is gated on --e2e, so mem-check still gets its markers without the flag. - NoteSheet's onOpened gate uses the same `--e2e` test as `pane.e2e`. - The running-text marker colour is set on the anchor, since TextEdit has no linkColor. The marker probe now tests `model.text`, not the rewritten text, so the check is unchanged. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
claude-bot changed title from WIP: reader: note markers, a bottom sheet for notes, tap a reference to jump (#13, 6c) to Note markers and a note sheet, tap a reference to jump (#13, 6c) 2026-10-29 22:04:13 +00:00
claude-review approved these changes 2026-10-29 22:04:52 +00:00
claude-review left a comment

Merge gate at medium on head 39a0f18.

Checked:

  • Findings: reviews 334, 336 and 337 had no high or medium findings. The lows were fixed (208d643, 39a0f18).
  • Last commit: I read git show 39a0f18. It only gives the e2e job's module cache its own key (sword-modules-e2e-KJV-BSB-...), so the KJV-only cache the app job saves can't shadow it. The cache path and the fetch step are unchanged.
  • CI on head: the combined status is success. changes, rust, audit, app, e2e and click all passed. The e2e log shows PASS note-sheet (11s), with no SKIP and no FAIL. This is the maximum tier's targeted check.
  • PR body Proof: it lists no unrun elevated, maximum or AI-behavior gate. The two uncovered items (the dark theme, and "Show all N") are listed as Open with a plan for each. Review-Confidence is 65 from the second audit, which found no missed defect.

🤖 Generated with Claude Code

Merge gate at medium on head 39a0f18. Checked: - Findings: reviews 334, 336 and 337 had no high or medium findings. The lows were fixed (208d643, 39a0f18). - Last commit: I read `git show 39a0f18`. It only gives the e2e job's module cache its own key (`sword-modules-e2e-KJV-BSB-...`), so the KJV-only cache the app job saves can't shadow it. The cache path and the fetch step are unchanged. - CI on head: the combined status is success. changes, rust, audit, app, e2e and click all passed. The e2e log shows `PASS note-sheet (11s)`, with no SKIP and no FAIL. This is the maximum tier's targeted check. - PR body Proof: it lists no unrun elevated, maximum or AI-behavior gate. The two uncovered items (the dark theme, and "Show all N") are listed as Open with a plan for each. Review-Confidence is 65 from the second audit, which found no missed defect. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Sign in to join this conversation.
Reviewers
No reviewers
claude-review
Labels
Clear labels   
priority
Could
 MoSCoW: nice to have, done when time allows

  
priority
Must
 MoSCoW: the release isn't viable without it

  
priority
Should
 MoSCoW: important, but the release works without it

  
priority
Won't
 MoSCoW: not this release (kept for later)

  
type
Bug

No labels
priority
Could
priority
Must
priority
Should
priority
Won't
type
Bug
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
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!129
No description provided.