Improve types for annotations and scripture view#998
Conversation
|
Warning Review limit reached
More reviews will be available in 48 minutes and 50 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR replaces untyped sort constants with a typed ChangesTypeScript Typing Improvements and Component Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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
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
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
1 participant
I recommend rebase and merge, since these commits are not super closely related.
Summary by CodeRabbit