Fix start-at-reference with one collection, only book#941
Fix start-at-reference with one collection, only book#941chrisvire merged 1 commit intosillsdev:mainfrom
Conversation
When there is one collection and only the book specified in the Start at Book and Chapter, then start-at-reference will have "GEN.". Handle this correctly.
📝 WalkthroughWalkthroughEnhanced the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/lib/data/navigation.ts (1)
54-63: Minor: misleading comment on line 54.The comment
// Book and chapter specified but only one collectiondescribes the enclosingif (ref.length === 2)block, but sits just before the innerisBlankguard — making it read as if both sub-cases apply. Consider replacing with comments that describe each sub-case:♻️ Proposed clarification
- // Book and chapter specified but only one collection - if (isBlank(ref[1])) { - // Only book specified + // Two-part reference with one collection: "BOOK.CHAPTER" or "BOOK." + if (isBlank(ref[1])) { + // Only book specified (e.g. "GEN.") — infer first chapter🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/data/navigation.ts` around lines 54 - 63, The existing comment "Book and chapter specified but only one collection" is misleading because it sits before the inner isBlank(ref[1]) guard; update the inline comments around the isBlank check and its else branch to clearly describe the two distinct sub-cases: one comment above the isBlank(ref[1]) branch saying "Only book specified — pick first chapter from this.config.bookCollections[0].books" and another above the else branch saying "Book and chapter specified — use provided chapter", keeping references to ref, isBlank, this.config.bookCollections, this.docSets and this.goto so reviewers can find the exact spots to change; do not alter logic, only replace/move the comments for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/data/navigation.ts`:
- Around line 57-60: The computation of firstChapter can produce undefined and
cause a TS strict-null error and runtime staleness; update the lookup to use
optional chaining before split and provide a string fallback so goto always
receives a string. Specifically, change the firstChapter calculation that uses
this.config.bookCollections[0].books.find(...)?.chaptersN.split('-')[0] to guard
chaptersN with ?.split and append a default like ?? '1' (ensuring a string),
then call this.goto(this.docSets[0], ref[0], firstChapter, '1') with that
guaranteed string; this addresses the TODO and prevents
updateChapter/versesByChapters from leaving this.chapter undefined.
- Line 1: firstChapter can be undefined and is being passed to goto(docSet,
book, chapter: string, verse); ensure you check that the book and its chapters
exist before deriving firstChapter from chaptersN: locate the code using ref,
ref.length === 2, the .find((b) => b.id === ref[0]) call, chaptersN and
firstChapter, and either (a) validate that the .find(...) returned a book and
chaptersN is defined before calling .split('-') and then call goto with a
non-undefined chapter, (b) provide a clear fallback chapter string when
chaptersN is missing, or (c) throw/return an error if the book or chapters are
missing so you never pass undefined to goto; also move the comment "Book and
chapter specified but only one collection" out of the inner if (isBlank(ref[1]))
block to sit above the outer if (ref.length === 2) condition so it describes
that branch.
---
Nitpick comments:
In `@src/lib/data/navigation.ts`:
- Around line 54-63: The existing comment "Book and chapter specified but only
one collection" is misleading because it sits before the inner isBlank(ref[1])
guard; update the inline comments around the isBlank check and its else branch
to clearly describe the two distinct sub-cases: one comment above the
isBlank(ref[1]) branch saying "Only book specified — pick first chapter from
this.config.bookCollections[0].books" and another above the else branch saying
"Book and chapter specified — use provided chapter", keeping references to ref,
isBlank, this.config.bookCollections, this.docSets and this.goto so reviewers
can find the exact spots to change; do not alter logic, only replace/move the
comments for clarity.
| @@ -1,3 +1,4 @@ | |||
| import { isBlank } from '$lib/scripts/stringUtils'; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the stringUtils file and check for isBlank export
fd "stringUtils" --type f -e ts -e jsRepository: sillsdev/appbuilder-pwa
Length of output: 120
🏁 Script executed:
# Search for isBlank export in stringUtils
rg -n "export.*isBlank" --type=tsRepository: sillsdev/appbuilder-pwa
Length of output: 235
🏁 Script executed:
# Also check the goto function signature in navigation.ts
rg -n "goto\s*\(" src/lib/data/navigation.ts -A 5 | head -30Repository: sillsdev/appbuilder-pwa
Length of output: 1199
🏁 Script executed:
# Check lines around 54-63 in navigation.ts to see the new code
sed -n '40,70p' src/lib/data/navigation.tsRepository: sillsdev/appbuilder-pwa
Length of output: 1386
Fix type safety: firstChapter (string | undefined) cannot be passed to goto() which requires chapter: string.
At line 60, firstChapter is assigned from an optional chain that can be undefined:
- If
.find((b) => b.id === ref[0])returns no match, the optional chain short-circuits andfirstChapter = undefined - If
.find()succeeds butchaptersNisundefined, calling.split('-')throws a TypeError (acknowledged by TODO at line 45) - Passing
firstChaptertogoto(docSet, book, chapter: string, verse)wherechapter: stringis required causes a TypeScript strict-mode compile error
Handle the undefined case: either validate the book exists before accessing its chapters, provide a fallback chapter value, or raise an error.
Also move the comment "Book and chapter specified but only one collection" out of the inner if (isBlank(ref[1])) block (it should be above line 51, describing the outer if (ref.length === 2) condition).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/data/navigation.ts` at line 1, firstChapter can be undefined and is
being passed to goto(docSet, book, chapter: string, verse); ensure you check
that the book and its chapters exist before deriving firstChapter from
chaptersN: locate the code using ref, ref.length === 2, the .find((b) => b.id
=== ref[0]) call, chaptersN and firstChapter, and either (a) validate that the
.find(...) returned a book and chaptersN is defined before calling .split('-')
and then call goto with a non-undefined chapter, (b) provide a clear fallback
chapter string when chaptersN is missing, or (c) throw/return an error if the
book or chapters are missing so you never pass undefined to goto; also move the
comment "Book and chapter specified but only one collection" out of the inner if
(isBlank(ref[1])) block to sit above the outer if (ref.length === 2) condition
so it describes that branch.
| const firstChapter = this.config.bookCollections[0].books | ||
| .find((b) => b.id === ref[0]) | ||
| ?.chaptersN.split('-')[0]; //TODO: what if chaptersN is undefined? | ||
| await this.goto(this.docSets[0], ref[0], firstChapter, '1'); |
There was a problem hiding this comment.
firstChapter can be undefined, causing a TypeScript error and a silent runtime failure.
Array.find returns undefined when no book matches ref[0], which propagates through ?.chaptersN.split('-')[0] to yield firstChapter: string | undefined. Passing that to goto(..., chapter: string, ...) is a TypeScript strict-null compile error, and at runtime an undefined chapter silently skips all branches of updateChapter when versesByChapters is non-empty, leaving this.chapter stale or uninitialized.
Add a fallback so the call always receives a valid string:
🛠️ Proposed fix
- const firstChapter = this.config.bookCollections[0].books
- .find((b) => b.id === ref[0])
- ?.chaptersN.split('-')[0]; //TODO: what if chaptersN is undefined?
- await this.goto(this.docSets[0], ref[0], firstChapter, '1');
+ const firstChapter = this.config.bookCollections[0].books
+ .find((b) => b.id === ref[0])
+ ?.chaptersN?.split('-')[0] ?? '1'; //TODO: what if chaptersN is undefined?
+ await this.goto(this.docSets[0], ref[0], firstChapter, '1');Note the secondary guard ?.chaptersN?.split (extra ? before .split) also prevents a TypeError when the book is found but chaptersN itself is undefined — the same latent risk the existing TODO acknowledges.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const firstChapter = this.config.bookCollections[0].books | |
| .find((b) => b.id === ref[0]) | |
| ?.chaptersN.split('-')[0]; //TODO: what if chaptersN is undefined? | |
| await this.goto(this.docSets[0], ref[0], firstChapter, '1'); | |
| const firstChapter = this.config.bookCollections[0].books | |
| .find((b) => b.id === ref[0]) | |
| ?.chaptersN?.split('-')[0] ?? '1'; //TODO: what if chaptersN is undefined? | |
| await this.goto(this.docSets[0], ref[0], firstChapter, '1'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/data/navigation.ts` around lines 57 - 60, The computation of
firstChapter can produce undefined and cause a TS strict-null error and runtime
staleness; update the lookup to use optional chaining before split and provide a
string fallback so goto always receives a string. Specifically, change the
firstChapter calculation that uses
this.config.bookCollections[0].books.find(...)?.chaptersN.split('-')[0] to guard
chaptersN with ?.split and append a default like ?? '1' (ensuring a string),
then call this.goto(this.docSets[0], ref[0], firstChapter, '1') with that
guaranteed string; this addresses the TODO and prevents
updateChapter/versesByChapters from leaving this.chapter undefined.
1 participant
When there is one collection and only the book specified in the Start at Book and Chapter, then start-at-reference will have "GEN.". Handle this correctly.
Summary by CodeRabbit