fix(usfm): emit mt1 for the main title - #323
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe USFM exporters now emit ChangesUSFM title marker
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to The title-marker change preserves fallback behavior and legacy compatibility with no unresolved merge-blocking risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
kaseywright
left a comment
There was a problem hiding this comment.
Reviewed this PR. Two related follow-ups that look like they were missed alongside the \mt -> \mt1 fix:
-
src/lib/usfm-converter.ts:85—generateUSFMText()still emits the legacy\mtmarker instead of\mt1. This is a sibling code path (used for chapter-assignment content viachapter-assignments.repository.ts:274, which callsconvertUSFMToUSJ(generateUSFMText(verseData))) that doesn't appear to have been updated to match the fix applied inusfm.service.ts. As-is, USFM output will be inconsistent depending on which code path generated it. -
src/domains/book-details/book-details.route.ts:94(also ~172-173) — The OpenAPI/Swagger description text for the book-details GET/PATCH routes still documents the exported main title as\mt. Since the actual export now produces\mt1, the generated API docs no longer match the real response body.
🤖 Generated with Claude Code
kaseywright
left a comment
There was a problem hiding this comment.
Requesting changes per the review comments above — the \mt -> \mt1 fix should also cover usfm-converter.ts and the stale Swagger docs in book-details.route.ts.
Emit mt1 in the chapter assignment converter and document the numbered export marker in both book details routes. Cover generated titles, chapter boundaries, USJ conversion and legacy title imports. Refs: #323
kaseywright
left a comment
There was a problem hiding this comment.
Re-reviewed the latest changes (head 8fd41af). The previous two issues are fixed — thanks. Found three more instances of the same pattern (marker fixed in code, docs/comments not updated to match):
-
src/domains/book-details/book-details.types.ts:54—bookTitlefield's OpenAPI description still says it "supplies\mtwhenever tocShortName is null." The sibling descriptions inroute.tswere updated in this PR to say\mt1, so this is now inconsistent with the docs shipping alongside it. -
src/domains/book-details/book-details.types.ts:63—tocShortNamefield's OpenAPI description still says it "supplies the\mtmain title." Same inconsistency. -
src/domains/usfm/usfm.types.ts:22—BookFields.tocShortNamedoc comment still says "also supplies\mt," despiteusfm.service.ts(which consumes this type) being changed in this same PR to emit\mt1. This one's a bit riskier since it's the doc comment on the type thatusfm.service.ts/usfm-converter.tsconsume — a future edit could reintroduce the legacy marker by trusting this comment as ground truth.
🤖 Generated with Claude Code
|
Updated all three remaining documentation references from \mt to \mt1 in book-details.types.ts and usfm.types.ts. I also merged the current main branch. The 30 focused tests, typecheck, and formatting check pass locally. |
kaseywright
left a comment
There was a problem hiding this comment.
Re-reviewed the latest changes (head 204e921). All three prior findings are addressed, and a re-review found no remaining correctness, cleanup, or consistency issues — no \mt references were missed outside intentional legacy-format test fixtures, and no downstream consumer depends on the literal marker string. Approving.
🤖 Generated with Claude Code
Closes #268.
Both the streaming USFM export and the chapter-assignment converter now write the main title as
\mt1. The book-details API documentation describes the same marker. The title fallback order and the rest of the output stay the same. I checked #264 and #275: both are merged, but neither includes this change.I updated the golden tests and added a USFM → USJ → USFM test with usfm-grammar 3.2.0. It checks the numbered title and preserves the parsed document. The marker matches the USFM reference. I did not run a manual Paratext round trip in this environment.
Validation on Node 24.14.0: 608 tests pass, including new converter coverage for chapter boundaries, USJ output and legacy
\mtimports. Lint, formatting, typecheck, build and docs checks pass. Lint reports only the three existing max-lines warnings in verse-audio files.Screenshot
This preview is generated from the same export fixture covered by the tests. It preserves the authored book details and table-of-contents fields while emitting the numbered main-title marker.
Summary by CodeRabbit
\mt1marker for main titles.\mttitle marker.