Improve types for annotations and scripture view#998
Open
FyreByrd wants to merge 7 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR replaces legacy sort constants with a typed ChangesTypeScript typing and render pipeline updates
Image upload null safety
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/data/annotation-sort.ts (1)
50-60:⚠️ Potential issue | 🟠 MajorType safety gap:
toSortedsignature permits invalidSort.Colorusage with non-Highlight arrays.The generic signature
toSorted<T extends Annotation>allows callers to passBookmarkItem[]orNoteItem[]withsortType === Sort.Color, butcompareColor(line 38) requiresHighlightItemwith apenColorproperty. Thets-expect-errorsuppresses the compile error at the call site but doesn't prevent runtime failures if a developer mistakenly callstoSorted(bookmarks, Sort.Color).In practice, only the highlights page uses
Sort.Color(bookmarks and notes pages never set this sort order), but the type system doesn't enforce the constraint.🔧 Proposed fix using function overloads
Replace the current generic signature with overloads that constrain valid combinations:
-export function toSorted<T extends Annotation>(items: T[], sortType: Sort) { +export function toSorted(items: HighlightItem[], sortType: typeof Sort.Color): HighlightItem[]; +export function toSorted<T extends Annotation>(items: T[], sortType: Exclude<Sort, typeof Sort.Color>): T[]; +export function toSorted<T extends Annotation>(items: T[], sortType: Sort): T[] { if (sortType === Sort.Reference) { return items.toSorted(compareReference); } else if (sortType === Sort.Date) { return items.toSorted(compareDate); } else if (sortType === Sort.Color) { - //@ts-expect-error I don't know how to specify that this only be for Highlights - Aidan - return items.toSorted(compareColor); + return (items as HighlightItem[]).toSorted(compareColor); } return items; }This enforces at compile time that
Sort.Colorcan only be used withHighlightItem[], while preserving type refinement for the other branches.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/data/annotation-sort.ts` around lines 50 - 60, The toSorted function has a generic signature that allows any Annotation type with any Sort type, but compareColor only works with HighlightItem because it requires the penColor property. The `@ts-expect-error` suppresses the compile error but doesn't prevent runtime failures if Sort.Color is used with BookmarkItem or NoteItem. Replace the current single generic signature with function overloads that enforce compile-time constraints: create specific overload signatures that restrict Sort.Color to only HighlightItem arrays, while allowing Sort.Reference and Sort.Date for any Annotation type. Remove the `@ts-expect-error` comment once the overloads prevent invalid combinations from compiling.
🧹 Nitpick comments (2)
src/lib/data/annotation-sort.ts (2)
14-28: 💤 Low valueOptional:
compareReferencereturns-1for equal items instead of0.When two annotations share the same
bookIndex,chapter, andverse, the comparator returns-1(line 26) instead of0. This violates the comparator contract and can cause unstable sorting behavior. A proper tie-breaker (e.g., secondary sort bydate) or returning0would make the sort stable and predictable.♻️ Proposed fix
} else if (parseInt(a.verse) > parseInt(b.verse)) { return 1; + } else if (parseInt(a.verse) < parseInt(b.verse)) { + return -1; } else { - return -1; + return 0; // Equal references }Or add a tie-breaker:
} else if (parseInt(a.verse) > parseInt(b.verse)) { return 1; + } else if (parseInt(a.verse) < parseInt(b.verse)) { + return -1; } else { - return -1; + // Tie-breaker: sort by date descending + return a.date < b.date ? 1 : a.date > b.date ? -1 : 0; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/data/annotation-sort.ts` around lines 14 - 28, The `compareReference` function violates the comparator contract by returning `-1` when two annotations have equal `bookIndex`, `chapter`, and `verse` values. The final else clause should return `0` for equal items instead of always returning `-1`. Either replace the final else block to return `0` when all primary sorting criteria are equal, or add a tie-breaker comparison (such as comparing by a date property) to ensure stable and predictable sort behavior when annotations share the same reference point.
30-36: 💤 Low valueOptional:
compareDatereturns-1for equal dates instead of0.When
a.date === b.date, the comparator returns-1(line 34) instead of0, violating the comparator contract and causing unstable sort order for annotations created at the same timestamp.♻️ Proposed fix
export function compareDate(a: Annotation, b: Annotation) { if (a.date < b.date) { return 1; + } else if (a.date > b.date) { + return -1; } else { - return -1; + return 0; // Equal dates } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/data/annotation-sort.ts` around lines 30 - 36, The compareDate function in annotation-sort.ts does not properly handle the case when a.date === b.date, as it returns -1 instead of 0. Restructure the function to explicitly check all three comparison cases: if a.date < b.date return 1, else if a.date > b.date return -1, and else (when they are equal) return 0. This ensures the comparator follows the standard contract required for stable sorting.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/components/ScriptureViewSofria.svelte`:
- Around line 2552-2557: The loop in the ScriptureViewSofria.svelte file is
meant to clear lower-level list counters by iterating through increasing index
values with i++, but the delete statement inside the loop uses matchNum instead
of the loop variable i. This causes the same level counter to be deleted
repeatedly instead of clearing each subsequent level. Replace the matchNum
reference in the delete workspace statement with i so that each iteration
properly removes the next deeper level's list counter
(workspace[`level${i}ListNum`]).
In `@src/lib/data/stores/plan.ts`:
- Around line 60-62: The currentPlanState store is only updated when result is
truthy, which means if result is falsy (indicating no persisted state), the
previous plan state remains cached in the store. This stale state (such as
"completed" status) can affect downstream plan navigation decisions. Fix this by
adding an else clause to reset currentPlanState to an appropriate default or
empty value when result is falsy, ensuring that switching to a plan with no
saved state properly clears any previously cached state.
---
Outside diff comments:
In `@src/lib/data/annotation-sort.ts`:
- Around line 50-60: The toSorted function has a generic signature that allows
any Annotation type with any Sort type, but compareColor only works with
HighlightItem because it requires the penColor property. The `@ts-expect-error`
suppresses the compile error but doesn't prevent runtime failures if Sort.Color
is used with BookmarkItem or NoteItem. Replace the current single generic
signature with function overloads that enforce compile-time constraints: create
specific overload signatures that restrict Sort.Color to only HighlightItem
arrays, while allowing Sort.Reference and Sort.Date for any Annotation type.
Remove the `@ts-expect-error` comment once the overloads prevent invalid
combinations from compiling.
---
Nitpick comments:
In `@src/lib/data/annotation-sort.ts`:
- Around line 14-28: The `compareReference` function violates the comparator
contract by returning `-1` when two annotations have equal `bookIndex`,
`chapter`, and `verse` values. The final else clause should return `0` for equal
items instead of always returning `-1`. Either replace the final else block to
return `0` when all primary sorting criteria are equal, or add a tie-breaker
comparison (such as comparing by a date property) to ensure stable and
predictable sort behavior when annotations share the same reference point.
- Around line 30-36: The compareDate function in annotation-sort.ts does not
properly handle the case when a.date === b.date, as it returns -1 instead of 0.
Restructure the function to explicitly check all three comparison cases: if
a.date < b.date return 1, else if a.date > b.date return -1, and else (when they
are equal) return 0. This ensures the comparator follows the standard contract
required for stable sorting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 571f7433-ff8b-4107-a8d0-7109ecc2254b
📒 Files selected for processing (12)
src/lib/components/ScriptureViewSofria.sveltesrc/lib/data/annotation-sort.tssrc/lib/data/stores/plan.tssrc/lib/data/stores/scripture.tssrc/lib/scripts/stringUtils.tssrc/lib/scripts/verseSelectUtil.tssrc/lib/video/index.tssrc/routes/bookmarks/+page.sveltesrc/routes/highlights/+page.sveltesrc/routes/image/upload/+page.sveltesrc/routes/notes/+page.sveltesrc/routes/text/+page.svelte
FyreByrd
force-pushed
the
refactor/improve-types
branch
from
907425d to
6341d78
Compare
FyreByrd
force-pushed
the
refactor/improve-types
branch
from
6341d78 to
09e91f3
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/components/ScriptureViewSofria.svelte (1)
1792-1876:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDuplicate code in
endParagraphcauses main paragraph handling to run twice.The switch statement at line 1793 handles
case 'main'(lines 1794-1821), then immediately after the switch, there's an identicalif (sequenceType == 'main')block (lines 1824-1850) with the exact same code. WhensequenceType === 'main', both blocks execute sequentially.This causes:
appendPhrasepotentially called twiceworkspace.paragraphDivappended to root, then the now-empty div appended again- Duplicate/empty DOM elements in the rendered output
The if-else chain after the switch (lines 1824-1876) appears to be legacy code that should have been removed when the switch was introduced.
🐛 Proposed fix: Remove duplicate switch case
switch (sequenceType) { case 'main': { - if (scriptureLogs?.paragraph) { - console.log('End main paragraph'); - } - if ( - workspace.phraseDiv != null && - workspace.phraseDiv.innerText !== '' - ) { - appendPhrase(workspace); - workspace.phraseDiv = null; - } - if (versePerLine) { - if (workspace.verseDiv !== null) { - workspace.paragraphDiv.appendChild( - workspace.verseDiv.cloneNode(true) - ); - workspace.verseDiv = document.createElement('div'); - } - } - // Build div - if (workspace.paragraphDiv.innerHTML !== '') { - workspace.root.appendChild(workspace.paragraphDiv); - } - if (workspace.videoDiv) { - workspace.root.appendChild(workspace.videoDiv); - workspace.videoDiv = null; - } break; } } if (sequenceType == 'main') {Alternatively, keep the switch case and remove the if-else block for 'main'.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/components/ScriptureViewSofria.svelte` around lines 1792 - 1876, The endParagraph block contains duplicate handling for sequenceType 'main': the switch statement's case 'main' block (lines 1794-1821) contains identical code to the subsequent if (sequenceType == 'main') block (lines 1824-1850), causing appendPhrase and workspace operations to execute twice. Remove the redundant if (sequenceType == 'main') condition and its code block, but preserve the else-if conditions for 'title', 'heading', and 'introduction' that follow it. This leaves the switch statement as the sole handler for the 'main' case while maintaining the if-else chain for the other sequence types.
🧹 Nitpick comments (2)
src/lib/components/ScriptureViewSofria.svelte (2)
1505-1510: 💤 Low valueType definition for
Element.attsmay not match actual structure.The
attsproperty is typed asRecord<string, string>, but usage patterns suggest values are arrays:
- Line 2534:
element.atts['start'][0]— array indexing- Line 584:
element.atts['number']— passed where string expectedConsider
Record<string, string | string[]>or investigating the actual Proskomma schema.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/components/ScriptureViewSofria.svelte` around lines 1505 - 1510, The `atts` property in the `Element` type definition is typed as `Record<string, string>`, but the actual code usage shows values can be arrays (accessed with index notation like `element.atts['start'][0]`). Update the type definition of the `atts` property to `Record<string, string | string[]>` to accurately reflect that the values can be either strings or arrays of strings, accommodating both usage patterns throughout the codebase.
1161-1198: 💤 Low valueStyle element appended without cleanup causes duplication on remount.
The fullscreen popup styles are appended to
document.headevery time this component mounts, but never removed inonDestroy. Repeated navigation could accumulate duplicate style blocks.Consider either:
- Moving styles to a global CSS file, or
- Tracking and removing the style element in
onDestroy🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/components/ScriptureViewSofria.svelte` around lines 1161 - 1198, The style element containing the fullscreen popup styles is appended to document.head during component initialization but is never removed when the component unmounts, causing duplicate style blocks to accumulate on remount. Store a reference to the created style element so it can be properly cleaned up in the onDestroy lifecycle hook by removing it from document.head. Alternatively, extract these styles to a global CSS file to avoid appending styles dynamically in the component at all.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/lib/components/ScriptureViewSofria.svelte`:
- Around line 1792-1876: The endParagraph block contains duplicate handling for
sequenceType 'main': the switch statement's case 'main' block (lines 1794-1821)
contains identical code to the subsequent if (sequenceType == 'main') block
(lines 1824-1850), causing appendPhrase and workspace operations to execute
twice. Remove the redundant if (sequenceType == 'main') condition and its code
block, but preserve the else-if conditions for 'title', 'heading', and
'introduction' that follow it. This leaves the switch statement as the sole
handler for the 'main' case while maintaining the if-else chain for the other
sequence types.
---
Nitpick comments:
In `@src/lib/components/ScriptureViewSofria.svelte`:
- Around line 1505-1510: The `atts` property in the `Element` type definition is
typed as `Record<string, string>`, but the actual code usage shows values can be
arrays (accessed with index notation like `element.atts['start'][0]`). Update
the type definition of the `atts` property to `Record<string, string |
string[]>` to accurately reflect that the values can be either strings or arrays
of strings, accommodating both usage patterns throughout the codebase.
- Around line 1161-1198: The style element containing the fullscreen popup
styles is appended to document.head during component initialization but is never
removed when the component unmounts, causing duplicate style blocks to
accumulate on remount. Store a reference to the created style element so it can
be properly cleaned up in the onDestroy lifecycle hook by removing it from
document.head. Alternatively, extract these styles to a global CSS file to avoid
appending styles dynamically in the component at all.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 813b0620-4179-403d-897c-8b2d073252c6
📒 Files selected for processing (2)
src/lib/components/ScriptureViewSofria.sveltesrc/routes/image/upload/+page.svelte
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routes/image/upload/+page.svelte
TheNonPirate
approved these changes
Jun 29, 2026
TheNonPirate
left a comment
TheNonPirate
left a comment
Contributor
There was a problem hiding this comment.
I don't see any problems with this.
FyreByrd
force-pushed
the
refactor/improve-types
branch
from
09e91f3 to
0c902c7
Compare
Collaborator
Author
|
Resolved merge conflicts |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/lib/components/ScriptureViewSofria.svelte (3)
1480-1484: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReturn after handling header-reference clicks.
After
headerLinkClickReference(e, target), execution continues intoonClickText(e, maxSelections), so clicking a header reference can also toggle text selection.Proposed fix
if (target.classList.contains('header-ref')) { - headerLinkClickReference(e, target); + void headerLinkClickReference(e, target); + break; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/components/ScriptureViewSofria.svelte` around lines 1480 - 1484, After handling a header reference in ScriptureViewSofria.svelte, stop further click processing so the same event does not also reach onClickText(e, maxSelections). Update the click handler around headerLinkClickReference and the subsequent onClickText call to return immediately when target.classList.contains('header-ref') is true, preserving the existing header-ref behavior while preventing unintended text selection toggles.
2550-2553: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
figureDivbefore appending it.
figureSource()can returnundefined, causingaddFigureDiv()to be skipped, butendWrapperstill appendsworkspace.figureDiv!when images are enabled. A malformed/unsupportedfigwrapper can crash rendering.Proposed fix
if (usfmType === 'fig') { - if (showImage()) { + if (showImage() && workspace.figureDiv) { workspace.paragraphDiv.appendChild( - workspace.figureDiv! + workspace.figureDiv ); } + workspace.figureDiv = null;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/components/ScriptureViewSofria.svelte` around lines 2550 - 2553, The image rendering path in ScriptureViewSofria.svelte can append a missing figure container, causing a crash when figureSource() is undefined and addFigureDiv() never ran. Update the endWrapper logic around showImage() to guard workspace.figureDiv before appending it, and only append when the figure div has actually been created. Use the existing addFigureDiv(), figureSource(), and endWrapper flow to keep the check localized and prevent malformed fig wrappers from reaching the appendChild call.
1526-1531: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAlign
Element.attswith the Sofria payload shape.The payload mixes scalar attrs (
number) with collected attrs (src,href,title,lemma,id,link,start) that are arrays.Record<string, string>hides that split and lets[0]compile against plain strings, so bad scalar/array access slips through. Use a union/dedicated type or small access helpers here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/components/ScriptureViewSofria.svelte` around lines 1526 - 1531, `Element.atts` in `ScriptureViewSofria.svelte` is typed too narrowly as `Record<string, string>`, which hides the Sofria payload’s mix of scalar fields and array-valued collected attrs. Update the `Element` type to use a dedicated shape or union that distinguishes scalar attrs like `number` from array attrs like `src`, `href`, `title`, `lemma`, `id`, `link`, and `start`, and then adjust any `Element.atts` access in `ScriptureViewSofria` to use the correct typed access/helpers instead of assuming plain strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/lib/components/ScriptureViewSofria.svelte`:
- Around line 1480-1484: After handling a header reference in
ScriptureViewSofria.svelte, stop further click processing so the same event does
not also reach onClickText(e, maxSelections). Update the click handler around
headerLinkClickReference and the subsequent onClickText call to return
immediately when target.classList.contains('header-ref') is true, preserving the
existing header-ref behavior while preventing unintended text selection toggles.
- Around line 2550-2553: The image rendering path in ScriptureViewSofria.svelte
can append a missing figure container, causing a crash when figureSource() is
undefined and addFigureDiv() never ran. Update the endWrapper logic around
showImage() to guard workspace.figureDiv before appending it, and only append
when the figure div has actually been created. Use the existing addFigureDiv(),
figureSource(), and endWrapper flow to keep the check localized and prevent
malformed fig wrappers from reaching the appendChild call.
- Around line 1526-1531: `Element.atts` in `ScriptureViewSofria.svelte` is typed
too narrowly as `Record<string, string>`, which hides the Sofria payload’s mix
of scalar fields and array-valued collected attrs. Update the `Element` type to
use a dedicated shape or union that distinguishes scalar attrs like `number`
from array attrs like `src`, `href`, `title`, `lemma`, `id`, `link`, and
`start`, and then adjust any `Element.atts` access in `ScriptureViewSofria` to
use the correct typed access/helpers instead of assuming plain strings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60c1b57e-e4b3-4029-a30b-20d5bfd75e9c
📒 Files selected for processing (3)
src/lib/components/ScriptureViewSofria.sveltesrc/lib/data/stores/scripture.tssrc/routes/text/+page.svelte
🚧 Files skipped from review as they are similar to previous changes (2)
- src/routes/text/+page.svelte
- src/lib/data/stores/scripture.ts
This is necessary for upcoming changes...
ScriptureViewSofria still directly accesses many stores. This just restores the state of affairs to what it was before. We will need to do further refactoring at a later time in order to support testing.
FyreByrd
force-pushed
the
refactor/improve-types
branch
from
905739a to
4418642
Compare
Collaborator
Author
|
Resolved merge conflicts |
chrisvire
reviewed
Jul 3, 2026
| workspace.phraseDiv = div.cloneNode(true); | ||
| workspace.phraseDiv = div.cloneNode(true) as HTMLDivElement; | ||
| } | ||
| if (workspace.encloseInSpanTag) { |
Member
There was a problem hiding this comment.
This is more than just changing types. Did you find a bug?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I recommend rebase and merge, since these commits are not super closely related.
Summary by CodeRabbit