Conversion and display of content items for the new data from SAB #936
Conversion and display of content items for the new data from SAB #936chrisvire merged 12 commits intosillsdev:mainfrom
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdded nested-item support and richer parsing to the contents converter (multilingual fields, link/layout metadata, children arrays); exported parsing helpers; updated route/page to surface nestedItems; added four Svelte UI components (Carousel, Grid, Heading, Single) and extensive tests for content parsing. Changes
Sequence Diagram(s)sequenceDiagram
participant Converter as Converter (convertContents)
participant Server as Route Load (load)
participant Page as Page (+page.svelte)
participant UI as UI Component (Carousel/Grid/Heading/Single)
Note over Converter,Server: Converter produces ContentsData (items, nestedItems, children)
Converter->>Server: persist ContentsData artifact
Server->>Page: load() returns { menu, items, features, title, nestedItems }
Page->>UI: select component and pass item (may include children)
UI-->>Page: emit onClick or request reference text
Page->>Server: fetch reference text (if needed)
Server-->>Page: return reference text
Page-->>UI: deliver reference text to component
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 9
🤖 Fix all issues with AI agents
In `@convert/convertContents.ts`:
- Around line 139-147: The parseItemHeading function incorrectly Boolean-casts
the heading attribute string and calls getNamedItem('heading') twice; update
parseItemHeading to fetch the attribute once (e.g., const attr =
tag.attributes.getNamedItem('heading')) and return attr ? attr.value === 'true'
: false so that values like "false" are handled correctly and the attribute
lookup is not repeated.
- Around line 245-279: In parseItemLink, the callback passed to
scriptureConfig.bookCollections?.some currently returns true when a matching
book with book.type is found but implicitly returns undefined otherwise; change
the callback in the .some call (the function taking collection => { ... }) to
explicitly return false in the branch when no match is found (i.e., after the if
(book && book.type) block) so every code path returns a boolean and satisfies
Biome/CI rules.
In `@src/lib/components/ContentCarousel.svelte`:
- Around line 38-42: The function hasIdUnderMouseItem currently uses an OR chain
that always returns true; update hasIdUnderMouseItem so after verifying item is
defined and item.hasAttribute('id') you check that the id is non-empty (use an
AND chain or a single truthy/non-empty test) so empty string IDs return false
and traversal can continue; locate the function hasIdUnderMouseItem and replace
the final return condition with a non-empty check (e.g., ensure item.id is not
undefined/null/empty or use a trimmed truthy check).
In `@src/lib/components/ContentHeading.svelte`:
- Around line 30-35: The function renderLastTextBox currently calls
exclude.find(...).length which will throw when find returns undefined; change
the logic to safely check membership (e.g., use exclude.includes(layout) or
check the find result !== undefined) and return the negation accordingly, and
ensure the function uses the correct input (the existing guard should handle
undefined but still use a safe membership test). Also replace the incorrect use
of features['layout'] with item.features['layout'] where the item’s layout is
intended (matching the other occurrences at lines near 79, 84, and 94), so
rendering uses the item's layout not the component prop.
- Around line 68-71: The template assumes item.subtitle always exists but
ContentItem.subtitle is optional, so guard accesses to avoid runtime errors;
update the conditional rendering and the interpolation in ContentHeading.svelte
to use optional chaining or explicit checks on item.subtitle (e.g., in the {`#if`
features['show-subtitles'] === true} block and the expression that reads
item.subtitle[$language] or item.subtitle.default) so the displayed value
becomes item.subtitle?.[$language] ?? item.subtitle?.default ?? '' (or
equivalent null checks) to safely fallback to an empty string when subtitle is
undefined.
In `@src/lib/components/ContentSingle.svelte`:
- Around line 41-45: The click handler is wired directly to onClick which
ignores any resolved fallback handler; update the element's onclick to call the
resolved onClickCallback (or use onClickCallback in place of onClick) so the
component honors fallback behavior — locate the JSX/Svelte element with
id={item.id} and change its onclick binding to invoke onClickCallback(event,
item) (or the appropriate resolved handler variable) instead of onClick so the
fallback/derived callback is used.
- Around line 71-74: The subtitle access can throw if item.subtitle is
undefined; update the rendering guarded by features['show-subtitles'] to safely
handle absent subtitles by using optional chaining on item.subtitle and falling
back to an empty string or other default; specifically modify the expression
that reads item.subtitle[$language] and item.subtitle.default to use
item.subtitle?.[$language] and item.subtitle?.default (or equivalent nullish
checks) so ContentSingle.svelte and the features['show-subtitles'] branch won't
cause a runtime error when subtitle is missing.
- Around line 31-36: renderLastTextBox currently calls exclude.find(...).length
which throws when find returns undefined; change the logic to safely test
membership (e.g., use exclude.includes(layout) or check the find result !==
undefined) and return the negated membership result; also fix the inconsistent
call site that passes features['layout'] — replace it with
item.features['layout'] to match the other layout checks (ensure all callers use
item.features['layout']).
In `@src/routes/contents/`[id]/+page.js:
- Around line 6-21: The console.warn message is inverted: inside the loop that
iterates menu.items and sets found = contents.items.find(...), when found is
falsy you should log that the item was missing rather than "Found items"; update
the warning in the block that uses variables menu, item, found, and
contents.items (the for loop near items.push(found)) to a clear message such as
"Missing item for menu item: ${item.id}" or similar, including the item.id (and
optional context like menu id) so the log accurately reflects the missing-item
condition.
🧹 Nitpick comments (3)
convert/convertContents.ts (3)
149-170: Gate or remove the debugconsole.loginparseItemTitle.This will spam logs during conversions.
🧹 Suggested cleanup
- console.log( - `title is nested: ${isTagInnerNestedItem(titleTag)} ${titleTag.innerHTML} ${titleTag.parentElement?.parentElement?.tagName}` - );
329-330: Remove the empty block left inconvertContents.🧹 Suggested cleanup
- { - }
364-502: Gate the child-item debug log behindverbose.The
console.logfor each child will be noisy in normal runs.🧹 Suggested cleanup
- console.log(`Child Tag: ${itemChild.tagName}`); + if (verbose >= 3) console.log(`Child Tag: ${itemChild.tagName}`);
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
convert/convertContents.ts (1)
104-111:⚠️ Potential issue | 🟡 Minor
decodeFromXmlonly replaces the first occurrence of each entity.The
replace()method without thegflag only replaces the first match. If XML content contains multiple instances of the same entity (e.g.,&&), subsequent occurrences won't be decoded.🐛 Proposed fix using replaceAll or regex with g flag
function decodeFromXml(input: string): string { return input - .replace('"', '"') - .replace(''', "'") - .replace('<', '<') - .replace('>', '>') - .replace('&', '&'); + .replaceAll('"', '"') + .replaceAll(''', "'") + .replaceAll('<', '<') + .replaceAll('>', '>') + .replaceAll('&', '&'); }
🤖 Fix all issues with AI agents
In `@convert/convertContents.ts`:
- Around line 114-120: The parseItemId function currently uses a non-null
assertion on tag.attributes.getNamedItem('id') which will throw if the element
has no id attribute; update parseItemId to safely read the id (e.g., use
tag.getAttribute('id') or check getNamedItem('id') !== null), handle the null
case by logging a warning and returning a safe numeric fallback (e.g., 0) or
NaN, then convert the string to Number only when present; ensure you reference
the parseItemId function and the tag variable when making the guard.
- Around line 283-288: The loop over featureTags assumes each <feature> has name
and value attributes and uses non-null assertions which can throw on malformed
XML; update the loop in convertContents.ts (the for...of over featureTags / the
featureTag handling) to safely retrieve attributes via
featureTag.attributes.getNamedItem('name') and getNamedItem('value'), check that
both are non-null before using them, and skip (or log a warning) when either is
missing instead of dereferencing; if present, use the attribute .value and
continue to call parseFeatureValue(value) and assign features[name].
In `@src/lib/components/ContentGrid.svelte`:
- Around line 47-48: The template iterates over item.children but
ContentItem.children is optional, which will throw if undefined; update the
{`#each` item.children as child} usage in ContentGrid.svelte to guard by using a
conditional/fallback such as iterating over item.children ?? [] or wrapping the
block with an {`#if` item.children} check so the loop only runs when children is
present, referencing the ContentItem type and the item.children property and
keeping the existing child variable and block contents unchanged.
In `@src/lib/components/ContentHeading.svelte`:
- Around line 30-35: The console.log in renderLastTextBox uses
exclude.find(...).length which can throw when find returns undefined; remove or
replace that log with a safe check using exclude.includes(layout) or
Boolean(exclude.find(x => x === layout)) to avoid accessing .length on
undefined, and ensure the function still returns !exclude.includes(layout) as
implemented; update the console.log (or delete it) inside renderLastTextBox to
use the safe expression.
In `@src/routes/contents/`[id]/+page.js:
- Around line 5-11: The code assumes menu exists after const menu =
contents.screens.find((x) => x.id === id); which can be undefined and cause a
crash when reading menu.items; add a guard immediately after that line to handle
the missing screen (e.g., if (!menu) return an appropriate error/404 or throw a
descriptive Error mentioning id) so subsequent code that iterates over
menu.items or uses menu does not run; update any callers/returns in this load
function to match the chosen error/response behavior.
🧹 Nitpick comments (4)
src/routes/contents/[id]/+page.js (1)
12-13: Minor: Combine declaration and assignment.The
let found;declaration followed by immediate assignment can be combined for clarity.✏️ Suggested simplification
- let found; - found = contents.items.find((x) => x.id === item.id); + const found = contents.items.find((x) => x.id === item.id);src/lib/components/ContentGrid.svelte (1)
50-53: Consider converting numericidto string for HTML attribute.The
child.idis a number (perContentItemtype), but HTMLidattributes should be strings. While browsers typically coerce numbers to strings, being explicit prevents potential issues.✏️ Suggested fix
<div class="contents-grid-item-block contents-grid-item-block-base contents-link-ref" - id={child.id} + id={`grid-item-${child.id}`} onclick={(event) => onClickCallback(event, child)} >convert/convertContents.ts (2)
327-328: Remove empty block.Lines 327-328 contain an empty block
{ }that appears to be leftover code with no effect.🧹 Proposed fix
if (hasContentsDir) { createOutputDir(destDir); } else { deleteOutputDir(destDir); } - { - } const contentsFile = path.join(dataDir, 'contents.xml');
156-158: Debugconsole.logstatements should be gated by verbose level.Lines 156-158 and 437 contain unconditional
console.logstatements that will output during every conversion. Other debug logs in this file are properly gated withif (verbose >= 3). Consider applying the same pattern for consistency.✏️ Proposed fix
For line 156-158:
- console.log( - `title is nested: ${isTagInnerNestedItem(titleTag)} ${titleTag.innerHTML} ${titleTag.parentElement?.parentElement?.tagName}` - ); + if (verbose >= 3) console.log( + `title is nested: ${isTagInnerNestedItem(titleTag)} ${titleTag.innerHTML} ${titleTag.parentElement?.parentElement?.tagName}` + );For line 437:
- console.log(`Child Tag: ${itemChild.tagName}`); + if (verbose >= 3) console.log(`Child Tag: ${itemChild.tagName}`);Note: The
verboseparameter would need to be passed toparseItemTitlefor this to work.Also applies to: 437-437
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/lib/components/ContentHeading.svelte`:
- Around line 15-29: The template never uses loadReferenceTextCallback (resolved
from loadReferenceText), so remove the unused prop/variable and its fallback:
delete loadReferenceTextCallback, the loadReferenceText ??
loadReferenceTextFallback resolution on Line 17, and the
loadReferenceTextFallback function; also remove any prop or exported variable
named loadReferenceText from the component API; alternatively, if this callback
is intended to be invoked, add calls to loadReferenceTextCallback in the
template/logic where reference text should be loaded (e.g., on mount or on a
relevant click), referencing the existing function name
loadReferenceTextCallback and its fallback loadReferenceTextFallback so the
behavior is wired correctly.
- Around line 78-96: The branch that handles non-horizontal image layouts calls
{`@render` img(item)} but never renders {`@render` text(item)}, and it also
erroneously calls renderLastTextBox(features['layout']) without the item prefix;
update that conditional to use renderLastTextBox(item.features['layout']) and
add {`@render` text(item)} immediately after {`@render` img(item)} so headings with
an image and non-horizontal layouts (e.g., "image-above-text-below") render both
the image and the text via img(item) and text(item).
In `@src/lib/components/ContentSingle.svelte`:
- Around line 15-29: The resolved callback loadReferenceTextCallback and its
fallback loadReferenceTextFallback in ContentSingle.svelte are unused dead code;
remove the unused binding by deleting the loadReferenceText resolution (the
const loadReferenceTextCallback = loadReferenceText ??
loadReferenceTextFallback) and the loadReferenceTextFallback function, or if the
template should use it, wire loadReferenceTextCallback into the appropriate
template/event where reference text is needed; ensure you also update any
exports/props that supply loadReferenceText so there are no orphaned props.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/routes/contents/`[id]/+page.svelte:
- Around line 52-61: The checkImageSize function can return undefined and leaves
a debug console.warn; update checkImageSize (in +page.svelte) to remove the
console.warn and always return a valid style string (e.g., return '' or 'width:
auto' when no image-width and layout conditions match) so interpolations never
produce "undefined" in style attributes; ensure the returned strings include
proper CSS termination (semicolon) if other style fragments are concatenated.
🧹 Nitpick comments (3)
src/routes/contents/[id]/+page.svelte (3)
63-66: The'undefined'string check hints at an upstream data issue.Checking for the string literal
'undefined'is a reasonable defensive measure, but it typically indicates that somewhere upstream,undefinedis being coerced to a string (e.g., via template interpolation orString()). Consider tracking down and fixing the root cause so this guard isn't needed.
107-109:getContentsMenuis an unused stub with only aconsole.log.This function is never called in the template. If it's planned for future use, consider removing it until needed to avoid dead code. If it's work-in-progress, a
// TODOcomment would clarify intent.
3-3: Consider migrating$app/storesto$app/state.This import predates this PR, but since the project uses Svelte 5,
$app/storesis deprecated in favor of$app/state. This could be addressed in a follow-up. Based on learnings, in SvelteKit 2.12+ and Svelte 5, imports should be changed fromimport { page } from '$app/stores'toimport { page } from '$app/state'.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@convert/convertContents.ts`:
- Around line 173-185: parseItemSubtitle, parseItemLink, and parseItemFeatures
are currently using tag.getElementsByTagName(...) and thus grab descendant tags
from nested child items; update these functions to mirror parseItemTitle's
behavior by filtering results with isTagInnerNestedItem(tag, element,
upperLayer) when upperLayer is true (or alternatively iterate only over direct
child nodes) so only immediate child tags are parsed for the parent item;
reference the functions parseItemSubtitle, parseItemLink, parseItemFeatures,
parseItemTitle and the helper isTagInnerNestedItem and replace unfiltered
getElementsByTagName usage with the same filtered iteration pattern
parseItemTitle uses.
🧹 Nitpick comments (3)
src/routes/contents/[id]/+page.svelte (1)
266-307: No fallback for unrecognizeditemTypevalues in the nested rendering path.If an item's
itemTypeis something other than'grid','carousel','heading', or'single', it will be silently dropped. Consider adding an{:else}branch with a warning or a default rendering to help surface unexpected data during development.Proposed addition
{:else if item.itemType === 'single'} <ContentSingle {item} {imageFolder} {onClick} {checkImageSize} {contentsFontSize} features={$page.data.features} /> + {:else} + <!-- Unknown item type: silently skip or log --> {/if}convert/convertContents.ts (2)
538-540: DuplicateContentsTaskOutputinterface — consider consolidating.This interface is declared at Line 58 (with
taskName) and again here (withdata). TypeScript will merge them, so it works, but having a single declaration is clearer for maintainability.Proposed consolidation
Merge both declarations into one (e.g., at line 58):
export interface ContentsTaskOutput extends TaskOutput { taskName: 'ConvertContents'; + data: ContentsData; }Then remove the second declaration at lines 538–540.
122-137: Unused parameterscontentItemContainerandprevItemType.Neither parameter is referenced inside the function body. Similarly, the variables
prevItemType(Line 371) andcurrentContentContainer(Line 372) in the main loop are tracked but never consumed. If these are scaffolding for future logic, consider adding a// TODOcomment; otherwise, remove them to reduce noise.Proposed cleanup
-function parseItemType( - tag: Element | HTMLElement | undefined, - contentItemContainer: boolean, - prevItemType: string | undefined = undefined -): string | undefined { +function parseItemType(tag: Element | HTMLElement | undefined): string | undefined { if (tag === undefined) return undefined; let itemType: string | undefined = undefined; if (tag.hasAttribute('type')) { itemType = String(tag.attributes.getNamedItem('type')!.value); } else { itemType = 'single'; // the only item type that is not specified in contents.xml } return itemType; }And at the call site (Line 389):
-const itemType = parseItemType(itemTag, contentItemContainer); +const itemType = parseItemType(itemTag);Remove
prevItemType(Line 371),currentContentContainer(Line 372), and the update at Line 494 if no longer needed.
eomerdws
changed the title
eomerdws
changed the title
chrisvire
left a comment
chrisvire
left a comment
There was a problem hiding this comment.
Great work!
If you want to add tests, you can do that in this PR or open another.
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 (2)
convert/convertContents.ts (2)
104-111:⚠️ Potential issue | 🔴 Critical
decodeFromXmlonly replaces the first occurrence of each XML entity.
String.prototype.replace()with a string pattern (not a regex with/g) replaces only the first match. If input contains multiple instances of the same entity (e.g.,&foo&bar), subsequent occurrences are left encoded.🐛 Proposed fix — use `replaceAll`
function decodeFromXml(input: string): string { return input - .replace('"', '"') - .replace(''', "'") - .replace('<', '<') - .replace('>', '>') - .replace('&', '&'); + .replaceAll('"', '"') + .replaceAll(''', "'") + .replaceAll('<', '<') + .replaceAll('>', '>') + .replaceAll('&', '&'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convert/convertContents.ts` around lines 104 - 111, The decodeFromXml function only replaces the first occurrence of each XML entity because it uses String.prototype.replace with string patterns; update decodeFromXml to perform global replacements (e.g., use replaceAll or replace with global regexes like /"/g) for each entity (", ', <, >, &) and ensure the replacement order avoids double-decoding (replace & last or handle & carefully) so all instances in the input are properly decoded.
357-365:⚠️ Potential issue | 🔴 CriticalAdd null check for
featuresTagto prevent runtime error.Line 360 calls
featuresTag.getElementsByTagName('feature')without checking iffeaturesTagis undefined. If the XML structure doesn't include a top-level<features>element directly under<contents>, this will crash with "Cannot read property 'getElementsByTagName' of undefined". Add a guard like:const featuresTag = document.getElementsByTagName('features')[0]; if (featuresTag) { const featureTags = featuresTag.getElementsByTagName('feature'); // ... rest of the loop }Additionally,
getElementsByTagName('feature')is recursive and will incorrectly capture any<feature>tags nested inside<contents-item>children if those items contain<features>blocks. UsefeatureTags.childrenor a more specific selector to only capture direct children.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convert/convertContents.ts` around lines 357 - 365, Guard against a missing <features> element and restrict to direct child <feature> nodes: check that featuresTag (from document.getElementsByTagName('features')[0]) is non-null before calling methods on it, then iterate only over its direct child feature elements (e.g., use featuresTag.children or a scoped selector like featuresTag.querySelectorAll(':scope > feature')) to fill data.features using parseFeatureValue; update the block that assigns data.features in convertContents.ts (symbols: featuresTag, featureTags, parseFeatureValue, data.features) accordingly.
🧹 Nitpick comments (7)
convert/convertContents.ts (3)
56-60: DuplicateContentsTaskOutputinterface declarations rely on declaration merging.The interface is declared at line 58 (with
taskName) and again at line 542 (withdata). TypeScript merges them, but this is unusual within a single file and easy to miss. Consolidating into one declaration improves clarity.♻️ Consolidate into a single declaration
export interface ContentsTaskOutput extends TaskOutput { taskName: 'ConvertContents'; + data: ContentsData; }Then remove the second declaration at line 542–544.
Also applies to: 542-544
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convert/convertContents.ts` around lines 56 - 60, There are two separate declarations of the ContentsTaskOutput interface in this file (one with taskName and another with data), relying on TypeScript declaration merging; consolidate them into a single interface declaration that includes both the taskName: 'ConvertContents' and the data: ContentsData properties (extending TaskOutput as before), and remove the duplicate declaration so only one ContentsTaskOutput exists (update any references if needed).
375-376: Unused variables:currentContentContaineris declared but never read;prevItemTypeis only used in a verbose log.
currentContentContainer(line 376) is assigned nowhere after declaration.prevItemType(line 375) is updated at line 498 but only read at line 395 inside a verbose log. If these are placeholders for future logic, consider adding a TODO or removing them to reduce confusion.Also applies to: 498-498
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convert/convertContents.ts` around lines 375 - 376, Remove or document the two unused locals to eliminate confusion: drop the unused currentContentContainer variable altogether from convertContents.ts (it's never assigned/used), and either remove prevItemType or restrict it to the verbose logging path (or add a clear TODO comment) in the function where prevItemType is set and compared—ensure you update the assignment site in the code that currently updates prevItemType so it no longer assigns to a dead variable if you remove it; if you keep prevItemType for future use, add a TODO comment explaining its planned purpose and limit assignments to when verbose logging actually reads it.
122-137:contentItemContainerparameter is accepted but never used.
parseItemTypetakescontentItemContaineras a parameter but doesn't reference it in the function body. The caller at line 393 computes it and passes it, but the function ignores it. Either remove the parameter or use it.♻️ Proposed simplification
export function parseItemType( tag: Element | HTMLElement | undefined, - contentItemContainer: boolean, prevItemType: string | undefined = undefined ): string | undefined {And update the call site at line 393:
- const itemType = parseItemType(itemTag, contentItemContainer); + const itemType = parseItemType(itemTag);And the test calls accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convert/convertContents.ts` around lines 122 - 137, The function parseItemType accepts contentItemContainer but never uses it, so remove the unused parameter from parseItemType's signature and body, update all call sites that pass contentItemContainer to call parseItemType with the remaining parameters (e.g., replace parseItemType(tag, contentItemContainer, prevItemType) with parseItemType(tag, prevItemType)), and adjust any tests to match the new signature; alternatively, if the parameter is actually required, modify parseItemType to use contentItemContainer in its logic (refer to the parseItemType function) instead of leaving it unused.convert/tests/sab/convertContentSAB.test.ts (4)
41-48: EmptybeforeAll/afterAllhooks andcleanupfunction are dead code.These are stubs with only TODO comments. They add noise without behavior. Consider removing them until they're needed, or tracking the TODOs in an issue.
Also applies to: 112-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convert/tests/sab/convertContentSAB.test.ts` around lines 41 - 48, The test file contains empty lifecycle hooks and a no-op cleanup function (beforeAll, afterAll, and cleanup) that are dead code; either remove these unused stubs from convert/tests/sab/convertContentSAB.test.ts or implement them if setup/teardown is required: delete the beforeAll and afterAll hooks and the cleanup function if not used, or replace the TODOs with concrete checks (e.g., verify required files exist in beforeAll and perform test data deletion in afterAll) and ensure any calls reference the implemented cleanup; update or remove the TODO comments accordingly so the test file has no no-op scaffolding.
260-265: Mocked tests unnecessarily depend oncontentsFileExistsguard.Tests at lines 260, 293 (and the mocked
isTagInnerNestedItem/tagHasInnerNestedItemstests starting at 608, 621, 652, 694, 725) callsetup()and early-return whencontents.xmlis absent, even though they use inline mocked XML and never touchtestSetupdata. This means these tests silently skip in environments where the file is missing, despite being self-contained.Similarly, "handles undefined input" tests (lines 148, 177, 214, 325, 359, 411, 496, 511, 543, 570) don't need the file at all — they just pass
undefinedto parsing functions.Consider removing the file-existence guard from self-contained tests so they always execute.
♻️ Example fix for the single-type mocked test
test('convertContents: Get Item Type send a MOCKED test of a single ...', () => { - const testSetup: TestSetup = setup(undefined, true); - if (!testSetup.contentsFileExists) { - console.warn('Contents file does not exist, so test cannot be run'); - return; - } - const xml = ` <contents> ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convert/tests/sab/convertContentSAB.test.ts` around lines 260 - 265, Remove the unnecessary contentsFileExists guard and its early-return from self-contained mocked tests that use inline XML or pass undefined; instead either (a) avoid calling setup() when its only purpose is checking the file, or (b) call setup(undefined, true) but do not conditional-return on testSetup.contentsFileExists. Specifically update the tests referencing setup() and contentsFileExists in the convertContents test and the mocked isTagInnerNestedItem / tagHasInnerNestedItems tests so they always run; ensure tests still create their inline XML fixtures and call convertContents, isTagInnerNestedItem, or tagHasInnerNestedItems directly (or pass undefined) without depending on file presence.
1-6: Unused import and type-only imports used as value imports.
Scriptfromvm(line 3) is imported but never used.LangContainerandLinkMeta(lines 9–10) are types and should useimport typeto avoid emitting runtime references.♻️ Proposed fix
import { existsSync, readFileSync } from 'fs'; import path from 'path'; -import { Script } from 'vm'; import type { ScriptureConfig } from '$config'; import jsdom from 'jsdom'; import { afterAll, beforeAll, expect, test } from 'vitest'; import { isTagInnerNestedItem, - LangContainer, - LinkMeta, parseItemAudio, parseItemFeatures, parseItemId, parseItemImage, parseItemLayoutCollection, parseItemLayoutMode, parseItemLink, parseItemSubtitle, parseItemTitle, parseItemType, tagHasInnerNestedItems } from '../../convertContents'; +import type { LangContainer, LinkMeta } from '../../convertContents';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convert/tests/sab/convertContentSAB.test.ts` around lines 1 - 6, Remove the unused runtime import Script from 'vm' (it's never referenced) and change the value imports for the type-only symbols LangContainer and LinkMeta to type-only imports (use "import type" for LangContainer and LinkMeta) so they don't emit runtime references; locate the imports that currently bring in Script, LangContainer, and LinkMeta and update them accordingly.
170-174: Prefer idiomatic Vitest matchers over boolean expressions insideexpect.Throughout the file, patterns like
expect(id === 0).toBeFalsy()andexpect(Object.keys(x).length === 0).toBeTruthy()are used. These produce unhelpful failure messages (e.g., "expected true, received false"). Using direct matchers yields clearer diagnostics.♻️ Examples
- expect(id === 0).toBeFalsy(); + expect(id).not.toBe(0); - expect(id === 0).toBeTruthy(); + expect(id).toBe(0); - expect(Object.keys(titleUpperFalse).length === 0).toBeTruthy(); + expect(Object.keys(titleUpperFalse)).toHaveLength(0); - expect(testResult === tagHasInnerNestedItems(item)).toSatisfy((r) => r); + expect(tagHasInnerNestedItems(item)).toBe(testResult);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convert/tests/sab/convertContentSAB.test.ts` around lines 170 - 174, Replace boolean-expression assertions with idiomatic Vitest matchers: for the loop using parseItemId and id, change expect(id === 0).toBeFalsy() to expect(id).not.toBe(0) and keep expect(id).toSatisfy((r) => typeof r === 'number') or prefer expect(typeof id).toBe('number'); likewise replace patterns like expect(Object.keys(x).length === 0).toBeTruthy() with expect(Object.keys(x)).toHaveLength(0) or directly expect(x).toEqual({}) depending on intent. Update assertions wherever similar boolean-expression checks appear to use direct matchers for clearer failure messages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@convert/tests/sab/convertContentSAB.test.ts`:
- Around line 488-492: The current test in convertContentSAB.test.ts calls
expect(link.linkTarget.match(/^[0-9]*$/i)) without a matcher, making it a no-op;
change it to assert properly—either use
expect(link.linkTarget).toMatch(/^[0-9]*$/) or assert the match result is not
null with expect(link.linkTarget.match(/^[0-9]*$/i)).not.toBeNull() (within the
same if block where link.linkType === 'screen' and link.linkTarget is defined).
- Around line 526-541: Rename the misspelled test descriptions that use
"assgined" to "assigned" in the test suite—specifically update the test titled
"convertContents: Layout Mode if assgined is not empty" (and the other
occurrence with the same typo) to read "convertContents: Layout Mode if assigned
is not empty" so the test names are spelled correctly; search for the string
"assgined" in convertContentSAB.test.ts and replace with "assigned" (no code
logic changes required).
---
Outside diff comments:
In `@convert/convertContents.ts`:
- Around line 104-111: The decodeFromXml function only replaces the first
occurrence of each XML entity because it uses String.prototype.replace with
string patterns; update decodeFromXml to perform global replacements (e.g., use
replaceAll or replace with global regexes like /"/g) for each entity
(", ', <, >, &) and ensure the replacement order avoids
double-decoding (replace & last or handle & carefully) so all instances
in the input are properly decoded.
- Around line 357-365: Guard against a missing <features> element and restrict
to direct child <feature> nodes: check that featuresTag (from
document.getElementsByTagName('features')[0]) is non-null before calling methods
on it, then iterate only over its direct child feature elements (e.g., use
featuresTag.children or a scoped selector like
featuresTag.querySelectorAll(':scope > feature')) to fill data.features using
parseFeatureValue; update the block that assigns data.features in
convertContents.ts (symbols: featuresTag, featureTags, parseFeatureValue,
data.features) accordingly.
---
Nitpick comments:
In `@convert/convertContents.ts`:
- Around line 56-60: There are two separate declarations of the
ContentsTaskOutput interface in this file (one with taskName and another with
data), relying on TypeScript declaration merging; consolidate them into a single
interface declaration that includes both the taskName: 'ConvertContents' and the
data: ContentsData properties (extending TaskOutput as before), and remove the
duplicate declaration so only one ContentsTaskOutput exists (update any
references if needed).
- Around line 375-376: Remove or document the two unused locals to eliminate
confusion: drop the unused currentContentContainer variable altogether from
convertContents.ts (it's never assigned/used), and either remove prevItemType or
restrict it to the verbose logging path (or add a clear TODO comment) in the
function where prevItemType is set and compared—ensure you update the assignment
site in the code that currently updates prevItemType so it no longer assigns to
a dead variable if you remove it; if you keep prevItemType for future use, add a
TODO comment explaining its planned purpose and limit assignments to when
verbose logging actually reads it.
- Around line 122-137: The function parseItemType accepts contentItemContainer
but never uses it, so remove the unused parameter from parseItemType's signature
and body, update all call sites that pass contentItemContainer to call
parseItemType with the remaining parameters (e.g., replace parseItemType(tag,
contentItemContainer, prevItemType) with parseItemType(tag, prevItemType)), and
adjust any tests to match the new signature; alternatively, if the parameter is
actually required, modify parseItemType to use contentItemContainer in its logic
(refer to the parseItemType function) instead of leaving it unused.
In `@convert/tests/sab/convertContentSAB.test.ts`:
- Around line 41-48: The test file contains empty lifecycle hooks and a no-op
cleanup function (beforeAll, afterAll, and cleanup) that are dead code; either
remove these unused stubs from convert/tests/sab/convertContentSAB.test.ts or
implement them if setup/teardown is required: delete the beforeAll and afterAll
hooks and the cleanup function if not used, or replace the TODOs with concrete
checks (e.g., verify required files exist in beforeAll and perform test data
deletion in afterAll) and ensure any calls reference the implemented cleanup;
update or remove the TODO comments accordingly so the test file has no no-op
scaffolding.
- Around line 260-265: Remove the unnecessary contentsFileExists guard and its
early-return from self-contained mocked tests that use inline XML or pass
undefined; instead either (a) avoid calling setup() when its only purpose is
checking the file, or (b) call setup(undefined, true) but do not
conditional-return on testSetup.contentsFileExists. Specifically update the
tests referencing setup() and contentsFileExists in the convertContents test and
the mocked isTagInnerNestedItem / tagHasInnerNestedItems tests so they always
run; ensure tests still create their inline XML fixtures and call
convertContents, isTagInnerNestedItem, or tagHasInnerNestedItems directly (or
pass undefined) without depending on file presence.
- Around line 1-6: Remove the unused runtime import Script from 'vm' (it's never
referenced) and change the value imports for the type-only symbols LangContainer
and LinkMeta to type-only imports (use "import type" for LangContainer and
LinkMeta) so they don't emit runtime references; locate the imports that
currently bring in Script, LangContainer, and LinkMeta and update them
accordingly.
- Around line 170-174: Replace boolean-expression assertions with idiomatic
Vitest matchers: for the loop using parseItemId and id, change expect(id ===
0).toBeFalsy() to expect(id).not.toBe(0) and keep expect(id).toSatisfy((r) =>
typeof r === 'number') or prefer expect(typeof id).toBe('number'); likewise
replace patterns like expect(Object.keys(x).length === 0).toBeTruthy() with
expect(Object.keys(x)).toHaveLength(0) or directly expect(x).toEqual({})
depending on intent. Update assertions wherever similar boolean-expression
checks appear to use direct matchers for clearer failure messages.
| if (link.linkType === 'screen' && link.linkTarget !== undefined) { | ||
| console.log(link.linkTarget); | ||
| // NOTE: LinkTargets for screens are id numbers of the screen | ||
| expect(link.linkTarget.match(/^[0-9]*$/i)); | ||
| } |
There was a problem hiding this comment.
Missing assertion matcher — this expect is a no-op.
Line 491 calls expect(...) on the regex match result but never chains a matcher (e.g., .not.toBeNull()). The test silently passes regardless of the match outcome.
🐛 Proposed fix
if (link.linkType === 'screen' && link.linkTarget !== undefined) {
console.log(link.linkTarget);
// NOTE: LinkTargets for screens are id numbers of the screen
- expect(link.linkTarget.match(/^[0-9]*$/i));
+ expect(link.linkTarget.match(/^[0-9]*$/i)).not.toBeNull();
}📝 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.
| if (link.linkType === 'screen' && link.linkTarget !== undefined) { | |
| console.log(link.linkTarget); | |
| // NOTE: LinkTargets for screens are id numbers of the screen | |
| expect(link.linkTarget.match(/^[0-9]*$/i)); | |
| } | |
| if (link.linkType === 'screen' && link.linkTarget !== undefined) { | |
| console.log(link.linkTarget); | |
| // NOTE: LinkTargets for screens are id numbers of the screen | |
| expect(link.linkTarget.match(/^[0-9]*$/i)).not.toBeNull(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@convert/tests/sab/convertContentSAB.test.ts` around lines 488 - 492, The
current test in convertContentSAB.test.ts calls
expect(link.linkTarget.match(/^[0-9]*$/i)) without a matcher, making it a no-op;
change it to assert properly—either use
expect(link.linkTarget).toMatch(/^[0-9]*$/) or assert the match result is not
null with expect(link.linkTarget.match(/^[0-9]*$/i)).not.toBeNull() (within the
same if block where link.linkType === 'screen' and link.linkTarget is defined).
| test('convertContents: Layout Mode if assgined is not empty', () => { | ||
| const testSetup: TestSetup = setup(undefined, true, true); | ||
| if (!testSetup.contentsFileExists) { | ||
| console.warn('Contents file does not exist, so test cannot be run'); | ||
| return; | ||
| } | ||
|
|
||
| for (const item of testSetup.itemTags ? testSetup.itemTags : []) { | ||
| const layoutMode = parseItemLayoutMode(item); | ||
| if (Object.keys(item).length > 0) { | ||
| expect(layoutMode).not.toSatisfy((r) => r === undefined || r === ''); | ||
| } else { | ||
| expect(layoutMode).not.toSatisfy((r) => r === ''); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Typos in test names: "assgined" → "assigned".
Lines 526 and 554 have the misspelling "assgined" in test descriptions.
✏️ Proposed fix
-test('convertContents: Layout Mode if assgined is not empty', () => {
+test('convertContents: Layout Mode if assigned is not empty', () => {-test('convertContents: Layout Collection if assgined is not empty', () => {
+test('convertContents: Layout Collection if assigned is not empty', () => {📝 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.
| test('convertContents: Layout Mode if assgined is not empty', () => { | |
| const testSetup: TestSetup = setup(undefined, true, true); | |
| if (!testSetup.contentsFileExists) { | |
| console.warn('Contents file does not exist, so test cannot be run'); | |
| return; | |
| } | |
| for (const item of testSetup.itemTags ? testSetup.itemTags : []) { | |
| const layoutMode = parseItemLayoutMode(item); | |
| if (Object.keys(item).length > 0) { | |
| expect(layoutMode).not.toSatisfy((r) => r === undefined || r === ''); | |
| } else { | |
| expect(layoutMode).not.toSatisfy((r) => r === ''); | |
| } | |
| } | |
| }); | |
| test('convertContents: Layout Mode if assigned is not empty', () => { | |
| const testSetup: TestSetup = setup(undefined, true, true); | |
| if (!testSetup.contentsFileExists) { | |
| console.warn('Contents file does not exist, so test cannot be run'); | |
| return; | |
| } | |
| for (const item of testSetup.itemTags ? testSetup.itemTags : []) { | |
| const layoutMode = parseItemLayoutMode(item); | |
| if (Object.keys(item).length > 0) { | |
| expect(layoutMode).not.toSatisfy((r) => r === undefined || r === ''); | |
| } else { | |
| expect(layoutMode).not.toSatisfy((r) => r === ''); | |
| } | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@convert/tests/sab/convertContentSAB.test.ts` around lines 526 - 541, Rename
the misspelled test descriptions that use "assgined" to "assigned" in the test
suite—specifically update the test titled "convertContents: Layout Mode if
assgined is not empty" (and the other occurrence with the same typo) to read
"convertContents: Layout Mode if assigned is not empty" so the test names are
spelled correctly; search for the string "assgined" in convertContentSAB.test.ts
and replace with "assigned" (no code logic changes required).
2 participants
Summary by CodeRabbit
New Features
Improvements
Tests