Conversation
📝 WalkthroughWalkthroughThis PR adds nested-content parsing and propagation through the convert layer, updates ContentsData/ContentsTaskOutput and convertContents signature, and introduces four new Svelte components (ContentCarousel, ContentGrid, ContentHeading, ContentSingle). The route load and page rendering were updated to accept and render nestedItems via the new components. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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
| function parseItemId(tag: Element | HTMLElement | undefined): number { | ||
| if (tag === undefined) { | ||
| console.warn('parseItemId got an undefined tag'); | ||
| return 0; | ||
| } | ||
| return Number(tag.attributes.getNamedItem('id')!.value); | ||
| } |
There was a problem hiding this comment.
Non-null assertion on id attribute may throw.
Line 119 uses tag.attributes.getNamedItem('id')!.value which will throw if the id attribute is missing. Consider adding a guard or fallback.
🛡️ Proposed defensive fix
function parseItemId(tag: Element | HTMLElement | undefined): number {
if (tag === undefined) {
console.warn('parseItemId got an undefined tag');
return 0;
}
- return Number(tag.attributes.getNamedItem('id')!.value);
+ const idAttr = tag.attributes.getNamedItem('id');
+ if (!idAttr) {
+ console.warn('parseItemId: missing id attribute');
+ return 0;
+ }
+ return Number(idAttr.value);
}📝 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.
| function parseItemId(tag: Element | HTMLElement | undefined): number { | |
| if (tag === undefined) { | |
| console.warn('parseItemId got an undefined tag'); | |
| return 0; | |
| } | |
| return Number(tag.attributes.getNamedItem('id')!.value); | |
| } | |
| function parseItemId(tag: Element | HTMLElement | undefined): number { | |
| if (tag === undefined) { | |
| console.warn('parseItemId got an undefined tag'); | |
| return 0; | |
| } | |
| const idAttr = tag.attributes.getNamedItem('id'); | |
| if (!idAttr) { | |
| console.warn('parseItemId: missing id attribute'); | |
| return 0; | |
| } | |
| return Number(idAttr.value); | |
| } |
🤖 Prompt for AI Agents
In `@convert/convertContents.ts` around lines 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.
| const featureTags = tag.getElementsByTagName('feature'); | ||
| for (const featureTag of featureTags) { | ||
| const value = featureTag.attributes.getNamedItem('value')!.value; | ||
| const name = featureTag.attributes.getNamedItem('name')!.value; | ||
| features[name] = parseFeatureValue(value); | ||
| } |
There was a problem hiding this comment.
Non-null assertions on feature attributes may throw on malformed XML.
Lines 285-286 use ! assertions assuming value and name attributes always exist. If the XML contains a <feature> tag missing either attribute, this will throw.
🛡️ Proposed defensive fix
function parseItemFeatures(tag: Element | HTMLElement | undefined): any {
const features: any = {};
if (tag === undefined) return features;
const featureTags = tag.getElementsByTagName('feature');
for (const featureTag of featureTags) {
- const value = featureTag.attributes.getNamedItem('value')!.value;
- const name = featureTag.attributes.getNamedItem('name')!.value;
- features[name] = parseFeatureValue(value);
+ const valueAttr = featureTag.attributes.getNamedItem('value');
+ const nameAttr = featureTag.attributes.getNamedItem('name');
+ if (valueAttr && nameAttr) {
+ features[nameAttr.value] = parseFeatureValue(valueAttr.value);
+ }
}
return features;
}🤖 Prompt for AI Agents
In `@convert/convertContents.ts` around lines 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].
| <div class="contents-grid-row contents-grid-cols-3"> | ||
| {#each item.children as child} |
There was a problem hiding this comment.
Guard against undefined item.children.
The ContentItem.children field is optional (children?: ContentItem[]). If item.children is undefined, the {#each} block will throw a runtime error. Add a fallback or conditional check.
🛡️ Proposed fix
<div class="contents-grid-row contents-grid-cols-3">
- {`#each` item.children as child}
+ {`#each` item.children ?? [] as child}📝 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.
| <div class="contents-grid-row contents-grid-cols-3"> | |
| {#each item.children as child} | |
| <div class="contents-grid-row contents-grid-cols-3"> | |
| {`#each` item.children ?? [] as child} |
🤖 Prompt for AI Agents
In `@src/lib/components/ContentGrid.svelte` around lines 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.
| function renderLastTextBox(layout: string | undefined): boolean { | ||
| if (layout === undefined) return false; | ||
| const exclude: Array<string> = ['image-left-text-right', 'image-right-text-left']; | ||
| console.log(`renderLastTextBox: ${exclude.find((x) => x === layout).length === 0}`); | ||
| return !exclude.includes(layout); | ||
| } |
There was a problem hiding this comment.
Console.log on Line 33 will throw a runtime error.
The past review flagged exclude.find((x) => x === layout).length as problematic because .find() returns undefined when no match is found, and accessing .length on undefined throws. While the return statement on Line 34 was correctly fixed to use !exclude.includes(layout), the console.log on Line 33 still contains the buggy code and will crash when layout is not in the exclude array.
🐛 Proposed fix - remove or fix the console.log
function renderLastTextBox(layout: string | undefined): boolean {
if (layout === undefined) return false;
const exclude: Array<string> = ['image-left-text-right', 'image-right-text-left'];
- console.log(`renderLastTextBox: ${exclude.find((x) => x === layout).length === 0}`);
+ // Debug log removed or use: console.log(`renderLastTextBox: ${!exclude.includes(layout)}`);
return !exclude.includes(layout);
}🤖 Prompt for AI Agents
In `@src/lib/components/ContentHeading.svelte` around lines 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.
| const menu = contents.screens.find((x) => x.id === id); | ||
| const nestedItems = contents.nestedItems; | ||
| const features = contents.features; | ||
| const title = contents.title; | ||
|
|
||
| const items = []; | ||
| for (const item of menu.items) { |
There was a problem hiding this comment.
Potential null dereference if menu is not found.
If contents.screens.find() returns undefined (no screen matches the id), accessing menu.items on Line 11 will throw a runtime error.
🛡️ Proposed fix
export async function load({ params }) {
const id = Number(params.id);
const menu = contents.screens.find((x) => x.id === id);
+ if (!menu) {
+ console.warn(`No menu found for id: ${id}`);
+ return { menu: null, items: [], features: contents.features, title: contents.title, nestedItems: contents.nestedItems };
+ }
const nestedItems = contents.nestedItems;
const features = contents.features;
const title = contents.title;📝 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 menu = contents.screens.find((x) => x.id === id); | |
| const nestedItems = contents.nestedItems; | |
| const features = contents.features; | |
| const title = contents.title; | |
| const items = []; | |
| for (const item of menu.items) { | |
| const menu = contents.screens.find((x) => x.id === id); | |
| if (!menu) { | |
| console.warn(`No menu found for id: ${id}`); | |
| return { menu: null, items: [], features: contents.features, title: contents.title, nestedItems: contents.nestedItems }; | |
| } | |
| const nestedItems = contents.nestedItems; | |
| const features = contents.features; | |
| const title = contents.title; | |
| const items = []; | |
| for (const item of menu.items) { |
🤖 Prompt for AI Agents
In `@src/routes/contents/`[id]/+page.js around lines 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.
1 participant
Summary by CodeRabbit
New Features
Improvements