Convert Storybooks#870
Conversation
|
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:
📝 WalkthroughWalkthroughAdds per-page illustration metadata to BookConfig, implements USFM paragraph/character/list transformations, updates rendering to show chapter illustrations for story books, and introduces Playwright E2E tests plus a test-run orchestration script; related unit tests and config adjustments included. ChangesStory illustrations and USFM processing
Sequence Diagram(s)sequenceDiagram
participant User
participant Page as src/routes/text/+page.svelte
participant Config as convert/convertConfig.ts
participant Converter as convert/convertBooks.ts
participant View as ScriptureViewSofria.svelte
User->>Page: Navigate to book page
Page->>Config: Load BookConfig (includes pageIllustrations)
Page->>Page: derive bookType, call getCurrentIllustrationFile()
Page->>View: render content (pass USFM)
View->>Converter: request USFM transforms (p-style, c-style, lists)
Converter-->>View: transformed USFM tokens
Page->>User: display illustration (if story) and rendered content
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
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 |
| for (const book of collection.books) { | ||
| let bookConverted = false; | ||
| switch (book.type) { | ||
| case 'story': |
There was a problem hiding this comment.
I know that it will fall to the default case, but I think it would be better to include case 'story': above default:
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
convert/convertBooks.ts (1)
532-533: Add explicit 'story' case above defaultAs suggested in the previous review, it would be clearer to include
case 'story':explicitly above thedefault:case for better code readability and maintenance.
🧹 Nitpick comments (5)
convert/convertConfig.ts (1)
533-545: Good implementation with minor suggestion for robustness.The logic correctly extracts image filenames and constructs appropriate paths with collection/book prefixes. The conditional prefix logic based on the presence of an
<images>tag is well thought out.Consider adding error handling for cases where required attributes might be missing:
const imageFileTag = page.getElementsByTagName('image-filename')[0]; if (imageFileTag) { + const pageNum = page.attributes.getNamedItem('num')?.value; + if (!pageNum) continue; // Skip if page number is missing pageIllustrations.push({ - num: Number(page.attributes.getNamedItem('num')?.value), + num: Number(pageNum), filename: book.getElementsByTagName('images')[0] ? tag.id + '-' + book.attributes.getNamedItem('id')!.value + '-' + imageFileTag.innerHTML : imageFileTag.innerHTML }); }src/routes/text/+page.svelte (1)
489-494: Consider consolidating swipe gesture logic.The swipe gesture on the illustration image duplicates the existing
doSwipefunction logic. This is functional but could lead to maintenance issues if the swipe behavior needs to be modified.Consider extracting the swipe configuration to a shared constant:
+const swipeConfig = { + timeframe: 300, + minSwipeDistance: 60, + touchAction: 'pan-y' +}; // Then use it in both places: use:swipe={swipeConfig}src/lib/components/ScriptureViewSofria.svelte (2)
2336-2365: Enhance list milestone handling for better maintainabilityThe list handling logic is functional but has a few areas for improvement:
- The padding calculation
2 * match[1] - 1 + 'rem'could result in excessive indentation for deeply nested lists- Consider extracting magic numbers into constants for better maintainability
Consider these improvements:
+ const LIST_INDENT_BASE = 1.5; // rem + const LIST_INDENT_MULTIPLIER = 1.5; // rem per level let match; if ((match = element.subType.match(/^usfm:zon(\d+)$/))) { workspace['level' + match[1] + 'ListNum'] = element.atts['start'][0]; } else if ((match = element.subType.match(/^usfm:zoli(\d+)$/))) { workspace.paragraphDiv.classList.add('list-item'); workspace.paragraphDiv.classList.add('list-decimal'); workspace.paragraphDiv.classList.add('list-inside'); workspace.paragraphDiv.style.counterSet = 'list-item ' + workspace['level' + match[1] + 'ListNum']; workspace['level' + match[1] + 'ListNum']++; workspace.paragraphDiv.style.paddingInlineStart = - 2 * match[1] - 1 + 'rem'; + LIST_INDENT_BASE + (match[1] - 1) * LIST_INDENT_MULTIPLIER + 'rem'; } else if ((match = element.subType.match(/^usfm:zuli(\d+)$/))) { workspace.paragraphDiv.classList.add('list-item'); workspace.paragraphDiv.classList.add('list-inside'); workspace.paragraphDiv.style.paddingInlineStart = - 2 * match[1] - 1 + 'rem'; + LIST_INDENT_BASE + (match[1] - 1) * LIST_INDENT_MULTIPLIER + 'rem'; if (match[1] === 2) { workspace.paragraphDiv.classList.add( '[list-style-type:circle]' ); } else if (match[1] >= 3) { workspace.paragraphDiv.classList.add( '[list-style-type:square]' ); } }
2402-2403: Remove debug console.log statementsDebug logging should be conditional or removed for production code.
case 'usfm:zcstyle': { const style = element.atts['id'][0]; - console.log('Element: '); - console.log(element); + if (scriptureLogs?.milestone) { + console.log('Character style milestone:', element); + } workspace.encloseInSpanTag = document.createElement('span'); workspace.encloseInSpanTag.classList.add(style); break; }convert/convertBooks.ts (1)
78-150: Well-implemented ordered list transformationThe ordered list transformation correctly handles nested lists, start numbers, and inline formatting. The recursive approach is appropriate for handling arbitrary nesting.
Consider adding JSDoc comments to document the complex regex patterns and transformation logic for future maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
config/index.d.ts(1 hunks)convert/convertBooks.ts(6 hunks)convert/convertConfig.ts(3 hunks)src/lib/components/ScriptureViewSofria.svelte(3 hunks)src/routes/text/+page.svelte(3 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: chrisvire
PR: sillsdev/appbuilder-pwa#808
File: convert/convertMarkdown.ts:216-222
Timestamp: 2025-05-07T13:16:04.481Z
Learning: The USFM attribute encoding workflow in appbuilder-pwa has two distinct steps: (1) Markdown to USFM conversion in convertMarkdown.ts should NOT encode attributes like title or href, (2) The encoding of these attributes should happen only in the convertBooks.ts process via the encodeJmpLinks function to prevent double-encoding.
Learnt from: chrisvire
PR: sillsdev/appbuilder-pwa#808
File: convert/convertMarkdown.ts:216-222
Timestamp: 2025-05-07T13:16:04.481Z
Learning: All USFM attribute encoding (href, title, etc.) should happen in the convertBooks process via the encodeJmpLinks function, not during the markdown to USFM conversion in convertMarkdown.ts.
📚 Learning: in svelte 5, the `page` store is imported from `$app/state` instead of `$app/stores`. this is part o...
Learnt from: chrisvire
PR: sillsdev/appbuilder-pwa#845
File: src/routes/text/+page.svelte:3-3
Timestamp: 2025-07-04T18:51:27.821Z
Learning: In Svelte 5, the `page` store is imported from `$app/state` instead of `$app/stores`. This is part of the migration from Svelte 4 to Svelte 5 where `$app/stores` is being deprecated in favor of `$app/state`.
Applied to files:
src/routes/text/+page.svelte
📚 Learning: in sveltekit 2.12 and svelte 5, `$app/stores` was deprecated in favor of `$app/state`. when migratin...
Learnt from: chrisvire
PR: sillsdev/appbuilder-pwa#845
File: src/routes/text/+page.svelte:3-3
Timestamp: 2025-07-04T18:51:27.821Z
Learning: In SvelteKit 2.12 and Svelte 5, `$app/stores` was deprecated in favor of `$app/state`. When migrating from Svelte 4 to Svelte 5, imports should be changed from `import { page } from '$app/stores';` to `import { page } from '$app/state';`. This change aligns with Svelte 5's reactivity model and provides more granular control over state updates.
Applied to files:
src/routes/text/+page.svelte
📚 Learning: in svelte 5, `$state` creates a reactive rune, not a store. stores are created with functions like `...
Learnt from: chrisvire
PR: sillsdev/appbuilder-pwa#845
File: src/routes/text/+page.svelte:201-209
Timestamp: 2025-07-04T18:53:08.369Z
Learning: In Svelte 5, `$state` creates a reactive rune, not a store. Stores are created with functions like `writable()`, `readable()`, and `derived()`. When using `bind:this` to bind DOM elements, use plain variables rather than reactive runes to ensure direct DOM property access works correctly.
Applied to files:
src/routes/text/+page.svelte
📚 Learning: the usfm attribute encoding workflow in appbuilder-pwa has two distinct steps: (1) markdown to usfm ...
Learnt from: chrisvire
PR: sillsdev/appbuilder-pwa#808
File: convert/convertMarkdown.ts:216-222
Timestamp: 2025-05-07T13:16:04.481Z
Learning: The USFM attribute encoding workflow in appbuilder-pwa has two distinct steps: (1) Markdown to USFM conversion in convertMarkdown.ts should NOT encode attributes like title or href, (2) The encoding of these attributes should happen only in the convertBooks.ts process via the encodeJmpLinks function to prevent double-encoding.
Applied to files:
src/lib/components/ScriptureViewSofria.svelteconvert/convertBooks.ts
📚 Learning: all usfm attribute encoding (href, title, etc.) should happen in the convertbooks process via the en...
Learnt from: chrisvire
PR: sillsdev/appbuilder-pwa#808
File: convert/convertMarkdown.ts:216-222
Timestamp: 2025-05-07T13:16:04.481Z
Learning: All USFM attribute encoding (href, title, etc.) should happen in the convertBooks process via the encodeJmpLinks function, not during the markdown to USFM conversion in convertMarkdown.ts.
Applied to files:
src/lib/components/ScriptureViewSofria.svelteconvert/convertBooks.ts
🔇 Additional comments (10)
config/index.d.ts (1)
75-79: LGTM! Well-structured type definition for page illustrations.The addition of the optional
pageIllustrationsproperty toBookConfigis well-designed. The structure withnumandfilenamefields provides a clear mapping between chapter numbers and illustration files, which aligns perfectly with the story book enhancement objectives.convert/convertConfig.ts (2)
521-521: LGTM! Proper initialization of pageIllustrations array.The initialization follows the established pattern in the codebase for collecting page-specific data.
728-729: LGTM! Proper inclusion of pageIllustrations in book object.The
pageIllustrationsproperty is correctly added to the book object, maintaining consistency with the TypeScript definition.src/routes/text/+page.svelte (2)
105-109: LGTM! Clean derivation of book type.The
bookTypederived value correctly extracts the book type from the configuration, following Svelte's reactivity patterns.
497-560: LGTM! Well-structured layout changes.The restructuring to use flexbox with a conditional illustration above the scrollable content is well-implemented. The
growclass ensures the scrollable area takes up remaining space, and existing functionality like borders, scroll position saving, and navigation buttons are preserved.convert/convertBooks.ts (5)
43-45: LGTM!The paragraph style tag transformation correctly converts
\p_styletags to milestone markers with appropriate id attributes.
52-56: LGTM!Clean orchestration function that properly delegates list transformations.
61-76: LGTM!The unordered list transformation correctly handles nested lists with dynamic level processing and appropriate milestone markers.
898-913: LGTM! Story book handling is well-implementedThe special handling for story books correctly treats the first file as metadata and subsequent files as chapter pages, with appropriate chapter marker insertion.
386-388: Filter functions properly integratedThe new transformation functions are correctly added to the USFM filter pipeline in the appropriate order.
| if (workspace.encloseInSpanTag) { | ||
| const textNode = workspace.phraseDiv.firstChild; | ||
| workspace.encloseInSpanTag.appendChild(textNode); | ||
| workspace.phraseDiv.innerHTML = workspace.encloseInSpanTag; | ||
| workspace.phraseDiv.replaceChild( | ||
| workspace.encloseInSpanTag, | ||
| workspace.phraseDiv.firstChild | ||
| ); | ||
| workspace.encloseInSpanTag = undefined; | ||
| } //Enclose the text in a previously-created span tag |
There was a problem hiding this comment.
Potential null reference issue in span wrapping logic
The code attempts to wrap text in a span tag but has several issues:
- Line 462 incorrectly assigns the span element as innerHTML instead of appending it as a child element
- The subsequent
replaceChildcall (lines 463-466) attempts to replace the first child that was just set as innerHTML - No null checks for
workspace.phraseDiv.firstChildbefore manipulation
Apply this fix to properly wrap the text node:
if (workspace.encloseInSpanTag) {
const textNode = workspace.phraseDiv.firstChild;
- workspace.encloseInSpanTag.appendChild(textNode);
- workspace.phraseDiv.innerHTML = workspace.encloseInSpanTag;
- workspace.phraseDiv.replaceChild(
- workspace.encloseInSpanTag,
- workspace.phraseDiv.firstChild
- );
+ if (textNode) {
+ workspace.encloseInSpanTag.appendChild(textNode);
+ workspace.phraseDiv.replaceChild(
+ workspace.encloseInSpanTag,
+ textNode
+ );
+ }
workspace.encloseInSpanTag = undefined;
} //Enclose the text in a previously-created span tag📝 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 (workspace.encloseInSpanTag) { | |
| const textNode = workspace.phraseDiv.firstChild; | |
| workspace.encloseInSpanTag.appendChild(textNode); | |
| workspace.phraseDiv.innerHTML = workspace.encloseInSpanTag; | |
| workspace.phraseDiv.replaceChild( | |
| workspace.encloseInSpanTag, | |
| workspace.phraseDiv.firstChild | |
| ); | |
| workspace.encloseInSpanTag = undefined; | |
| } //Enclose the text in a previously-created span tag | |
| if (workspace.encloseInSpanTag) { | |
| const textNode = workspace.phraseDiv.firstChild; | |
| if (textNode) { | |
| workspace.encloseInSpanTag.appendChild(textNode); | |
| workspace.phraseDiv.replaceChild( | |
| workspace.encloseInSpanTag, | |
| textNode | |
| ); | |
| } | |
| workspace.encloseInSpanTag = undefined; | |
| } //Enclose the text in a previously-created span tag |
🤖 Prompt for AI Agents
In src/lib/components/ScriptureViewSofria.svelte lines 459 to 468, the code
incorrectly assigns a span element to innerHTML and attempts to replace a child
node without null checks, risking runtime errors. To fix this, remove the
innerHTML assignment, ensure workspace.phraseDiv.firstChild is not null before
manipulation, and properly append or replace child nodes by directly
manipulating DOM elements rather than using innerHTML.
| <div class="flex flex-col overflow-y-auto"> | ||
| {#if bookType === 'story'} | ||
| <!-- svelte-ignore a11y_missing_attribute --> | ||
| <img | ||
| src={getCurrentIllustrationFile()} | ||
| class="w-screen object-cover" | ||
| use:swipe={{ | ||
| timeframe: 300, | ||
| minSwipeDistance: 60, | ||
| touchAction: 'pan-y' | ||
| }} | ||
| onswipe={doSwipe} | ||
| /> | ||
| {/if} |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify image rendering with undefined src.
The image element should handle cases where getCurrentIllustrationFile() returns null or undefined to prevent broken image displays.
Consider adding a conditional check:
{#if bookType === 'story'}
+ {@const illustrationFile = getCurrentIllustrationFile()}
+ {#if illustrationFile}
<!-- svelte-ignore a11y_missing_attribute -->
<img
- src={getCurrentIllustrationFile()}
+ src={illustrationFile}
class="w-screen object-cover"
use:swipe={{
timeframe: 300,
minSwipeDistance: 60,
touchAction: 'pan-y'
}}
onswipe={doSwipe}
/>
+ {/if}
{/if}🏁 Script executed:
#!/bin/bash
# Search for similar image handling patterns in the codebase
ast-grep --pattern $'<img
$$$
src={$_}
$$$
/>'Length of output: 53
🏁 Script executed:
#!/bin/bash
# Locate the definition and all usages of getCurrentIllustrationFile to verify its return values
rg -n "getCurrentIllustrationFile" -C3 .Length of output: 1096
🏁 Script executed:
#!/bin/bash
# Display the full body of getCurrentIllustrationFile to determine its return behavior
sed -n '369,400p' src/routes/text/+page.svelteLength of output: 1493
Guard image rendering when illustrationFile is undefined
To avoid broken images and potential runtime errors, assign the result of getCurrentIllustrationFile() to a local variable and only render the <img> when it’s truthy.
• File: src/routes/text/+page.svelte (around line 483)
• Steps:
- Extract
illustrationFile:{@const illustrationFile = getCurrentIllustrationFile()} - Wrap the
<img>in{#if illustrationFile}…{/if}
Suggested patch:
{#if bookType === 'story'}
+ {@const illustrationFile = getCurrentIllustrationFile()}
+ {#if illustrationFile}
<!-- svelte-ignore a11y_missing_attribute -->
<img
- src={getCurrentIllustrationFile()}
+ src={illustrationFile}
class="w-screen object-cover"
use:swipe={{
timeframe: 300,
minSwipeDistance: 60,
touchAction: 'pan-y'
}}
onswipe={doSwipe}
/>
+ {/if}
{/if}You may also harden getCurrentIllustrationFile() against missing data:
function getCurrentIllustrationFile() {
- let illustrations = config?.bookCollections
+ const illustrations = config?.bookCollections
.find(x => x.id === $refs.collection)
.books.find(x => x.id === $refs.book)
?.pageIllustrations;
+ if (!illustrations) return;
for (let i = 0; i < illustrations.length; i++) {
if (illustrations[i].num === Number($refs.chapter)) {
return base + '/illustrations/' + illustrations[i].filename;
}
}
}📝 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="flex flex-col overflow-y-auto"> | |
| {#if bookType === 'story'} | |
| <!-- svelte-ignore a11y_missing_attribute --> | |
| <img | |
| src={getCurrentIllustrationFile()} | |
| class="w-screen object-cover" | |
| use:swipe={{ | |
| timeframe: 300, | |
| minSwipeDistance: 60, | |
| touchAction: 'pan-y' | |
| }} | |
| onswipe={doSwipe} | |
| /> | |
| {/if} | |
| <div class="flex flex-col overflow-y-auto"> | |
| {#if bookType === 'story'} | |
| {@const illustrationFile = getCurrentIllustrationFile()} | |
| {#if illustrationFile} | |
| <!-- svelte-ignore a11y_missing_attribute --> | |
| <img | |
| src={illustrationFile} | |
| class="w-screen object-cover" | |
| use:swipe={{ | |
| timeframe: 300, | |
| minSwipeDistance: 60, | |
| touchAction: 'pan-y' | |
| }} | |
| onswipe={doSwipe} | |
| /> | |
| {/if} | |
| {/if} | |
| </div> |
🤖 Prompt for AI Agents
In src/routes/text/+page.svelte around lines 483 to 496, the image rendering
uses getCurrentIllustrationFile() directly, which can cause broken images if it
returns undefined. To fix this, assign the result of
getCurrentIllustrationFile() to a local variable named illustrationFile using
{@const illustrationFile = getCurrentIllustrationFile()} before the conditional
block, then wrap the <img> element inside an {#if illustrationFile}...{/if}
block so the image only renders when illustrationFile is truthy.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
convert/tests/sab/convertConfigSAB.test.ts (1)
112-116: Consider also asserting the expected emptiness for story booksYou now skip the assertions for
section,testament, andabbreviationwhenbkk.type === 'story'.
For extra safety (and clearer documentation of intent) add a shortelsebranch that explicitly checks these fields areundefined/empty for story books.
This prevents silent drift if the parser later starts populating them.package.json (1)
23-26: Document the combined test runner
"test": "ts-node convert/runTests.ts"now does both e2e and unit tests.
Consider a short comment in the script section (or README) so contributors realise that Playwright runs first and can use--grepetc. through the wrapper.convert/tests/sab/storybook.test.ts (1)
4-6: Tokenisation hides whitespace-sensitive regressions
tokensOfsplits on\\s+, so ordering is preserved but all whitespace is discarded.
If the transformer later introduces unintended line-breaks/indentation you will not detect it.
Consider comparing the raw strings after normalising only leading indentation, or useexpect(result.trim()).toBe(expected.trim()).playwright/storybookTest.e2e.ts (1)
32-36: PrefertoHaveCSSover manualevaluateInstead of
expect( container.evaluate((el) => el.style.getPropertyValue('padding-inline-start')) ).resolves.toBe('1rem');simply use
await expect(container.first()).toHaveCSS('padding-inline-start', '1rem');This keeps assertions consistent and avoids mixing locator evaluation with test-runner expectations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsontest_data/projects/sab/web_gospels.zipis excluded by!**/*.zip
📒 Files selected for processing (7)
.gitignore(1 hunks)convert/runTests.ts(1 hunks)convert/tests/sab/convertConfigSAB.test.ts(1 hunks)convert/tests/sab/storybook.test.ts(1 hunks)package.json(1 hunks)playwright.config.ts(1 hunks)playwright/storybookTest.e2e.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- playwright.config.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: chrisvire
PR: sillsdev/appbuilder-pwa#808
File: convert/convertMarkdown.ts:216-222
Timestamp: 2025-05-07T13:16:04.481Z
Learning: The USFM attribute encoding workflow in appbuilder-pwa has two distinct steps: (1) Markdown to USFM conversion in convertMarkdown.ts should NOT encode attributes like title or href, (2) The encoding of these attributes should happen only in the convertBooks.ts process via the encodeJmpLinks function to prevent double-encoding.
Learnt from: chrisvire
PR: sillsdev/appbuilder-pwa#808
File: convert/convertMarkdown.ts:216-222
Timestamp: 2025-05-07T13:16:04.481Z
Learning: All USFM attribute encoding (href, title, etc.) should happen in the convertBooks process via the encodeJmpLinks function, not during the markdown to USFM conversion in convertMarkdown.ts.
📚 Learning: 2025-05-07T13:16:04.481Z
Learnt from: chrisvire
PR: sillsdev/appbuilder-pwa#808
File: convert/convertMarkdown.ts:216-222
Timestamp: 2025-05-07T13:16:04.481Z
Learning: The USFM attribute encoding workflow in appbuilder-pwa has two distinct steps: (1) Markdown to USFM conversion in convertMarkdown.ts should NOT encode attributes like title or href, (2) The encoding of these attributes should happen only in the convertBooks.ts process via the encodeJmpLinks function to prevent double-encoding.
Applied to files:
convert/tests/sab/convertConfigSAB.test.ts
🧬 Code Graph Analysis (1)
convert/tests/sab/storybook.test.ts (1)
convert/convertBooks.ts (1)
transformLists(52-57)
| let args = ''; | ||
| for (let i = 2; i < process.argv.length; i++) { | ||
| args += process.argv[i]; | ||
| } | ||
| console.log('playwright test ' + args); | ||
| spawn('playwright test ' + args, { | ||
| shell: true, | ||
| stdio: 'inherit' | ||
| }).on('close', (code) => { | ||
| console.log('vitest ' + args); | ||
| spawn('vitest ' + args, { | ||
| shell: true, | ||
| stdio: 'inherit' | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix CLI-arg handling and propagate failures
Current runner concatenates args without spaces (--run--reporter) and always runs Vitest even if Playwright fails, masking CI errors.
-let args = '';
-for (let i = 2; i < process.argv.length; i++) {
- args += process.argv[i];
-}
+const args = process.argv.slice(2).join(' ');-spawn('playwright test ' + args, { … }).on('close', (code) => {
- console.log('vitest ' + args);
- spawn('vitest ' + args, { … });
-});
+const pw = spawn(`playwright test ${args}`, { shell: true, stdio: 'inherit' });
+pw.on('close', (code) => {
+ if (code !== 0) process.exit(code);
+ console.log(`vitest ${args}`);
+ const vt = spawn(`vitest ${args}`, { shell: true, stdio: 'inherit' });
+ vt.on('close', (vc) => process.exit(vc));
+});This preserves spacing, halts on the first failing suite, and forwards the exit code to the parent process.
📝 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.
| let args = ''; | |
| for (let i = 2; i < process.argv.length; i++) { | |
| args += process.argv[i]; | |
| } | |
| console.log('playwright test ' + args); | |
| spawn('playwright test ' + args, { | |
| shell: true, | |
| stdio: 'inherit' | |
| }).on('close', (code) => { | |
| console.log('vitest ' + args); | |
| spawn('vitest ' + args, { | |
| shell: true, | |
| stdio: 'inherit' | |
| }); | |
| }); | |
| // replace the old args loop with a single join | |
| const args = process.argv.slice(2).join(' '); | |
| console.log('playwright test ' + args); | |
| const pw = spawn(`playwright test ${args}`, { | |
| shell: true, | |
| stdio: 'inherit' | |
| }); | |
| pw.on('close', (code) => { | |
| if (code !== 0) process.exit(code); | |
| console.log(`vitest ${args}`); | |
| const vt = spawn(`vitest ${args}`, { | |
| shell: true, | |
| stdio: 'inherit' | |
| }); | |
| vt.on('close', (vc) => process.exit(vc)); | |
| }); |
🤖 Prompt for AI Agents
In convert/runTests.ts around lines 3 to 17, the code concatenates CLI arguments
without spaces, causing merged arguments, and it runs Vitest regardless of
Playwright's success, hiding failures. Fix this by joining process.argv elements
with spaces to preserve argument separation, modify the Playwright spawn to
check its exit code, and only run Vitest if Playwright succeeds. Also, ensure
the script exits with the same code as the first failing process to properly
propagate errors.
TheNonPirate
force-pushed
the
feature/convertStorybook
branch
from
7763c09 to
b84d2fe
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
convert/runTests.ts (1)
8-13:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStill not propagating failing test-suite exit codes.
Line 8 ignores Playwright's exit status, and the Vitest child is never awaited, so this wrapper can still exit successfully after a failed suite. This was already raised earlier and is still unresolved.
🤖 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 `@convert/runTests.ts` around lines 8 - 13, The current 'close' handler ignores the Playwright exit code and never waits for the spawned Vitest process; change the handler so it captures the ChildProcess returned by spawn (e.g., const vitest = spawn('vitest ' + args, {...})), attach a 'close' (or 'exit') listener to that child, and when the vitest child closes call process.exit() with the proper non-zero code (prefer vitest's exit code if non-zero, otherwise fall back to the Playwright 'code' from the outer 'close' handler) so failing test-suite exit codes are propagated and the wrapper exits with the correct status.
🤖 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 `@convert/convertConfig.ts`:
- Around line 592-603: The code silently creates pageIllustrations entries with
NaN for num; instead, validate the page 'num' attribute on page
(page.attributes.getNamedItem('num')) before pushing: parse it to an integer,
check !isNaN and that it is a positive integer (or meets expected constraints),
and if invalid or missing throw an Error with a clear message including
identifying context (e.g., page.tagName/id or surrounding book id) so conversion
fails fast; update the block that constructs pageIllustrations (the code
referencing page, imageFileTag, book and page.attributes.getNamedItem('num')) to
perform this validation and throw rather than pushing entries with NaN.
In `@convert/runTests.ts`:
- Around line 3-7: The current code builds a single shell string using args and
calls spawn with shell:true which breaks quoted/space-containing args; change
the spawn call to forward arguments as an array by using process.argv.slice(2)
directly (e.g. spawn('playwright', ['test', ...process.argv.slice(2)], { shell:
false, stdio: 'inherit' })) so argument boundaries created by
process.argv.slice(2) are preserved; update any references to the earlier args
variable accordingly and remove the shell:true usage.
In `@src/routes/text/`+page.svelte:
- Around line 399-410: In getCurrentIllustrationFile(), the code assumes
illustrationURLs['/src/gen-assets/illustrations/'+illustrations[i].filename]
exists and immediately reads .default, which throws if the key is missing; guard
the glob lookup by first obtaining the entry (e.g., const entry =
illustrationURLs['/src/gen-assets/illustrations/'+illustrations[i].filename])
and only return entry.default when entry is defined, otherwise return a safe
fallback (undefined or a placeholder) or continue searching; update
getCurrentIllustrationFile to perform this existence check before accessing
.default so missing keys don't cause runtime errors.
---
Duplicate comments:
In `@convert/runTests.ts`:
- Around line 8-13: The current 'close' handler ignores the Playwright exit code
and never waits for the spawned Vitest process; change the handler so it
captures the ChildProcess returned by spawn (e.g., const vitest = spawn('vitest
' + args, {...})), attach a 'close' (or 'exit') listener to that child, and when
the vitest child closes call process.exit() with the proper non-zero code
(prefer vitest's exit code if non-zero, otherwise fall back to the Playwright
'code' from the outer 'close' handler) so failing test-suite exit codes are
propagated and the wrapper exits with the correct status.
🪄 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: 5f1d92fa-f4bc-4684-859b-a03e73faa06f
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsontest_data/projects/sab/web_gospels.zipis excluded by!**/*.zip
📒 Files selected for processing (12)
.gitignoreconfig/index.d.tsconvert/convertBooks.tsconvert/convertConfig.tsconvert/runTests.tsconvert/tests/sab/convertConfigSAB.test.tsconvert/tests/sab/storybook.test.tspackage.jsonplaywright.config.tsplaywright/storybookTest.e2e.tssrc/lib/components/ScriptureViewSofria.sveltesrc/routes/text/+page.svelte
🚧 Files skipped from review as they are similar to previous changes (8)
- playwright.config.ts
- convert/tests/sab/convertConfigSAB.test.ts
- config/index.d.ts
- package.json
- playwright/storybookTest.e2e.ts
- convert/tests/sab/storybook.test.ts
- src/lib/components/ScriptureViewSofria.svelte
- convert/convertBooks.ts
| const imageFileTag = page.getElementsByTagName('image-filename')[0]; | ||
| if (imageFileTag) { | ||
| pageIllustrations.push({ | ||
| num: Number(page.attributes.getNamedItem('num')?.value), | ||
| filename: book.getElementsByTagName('images')[0] | ||
| ? tag.id + | ||
| '-' + | ||
| book.attributes.getNamedItem('id')!.value + | ||
| '-' + | ||
| imageFileTag.innerHTML | ||
| : imageFileTag.innerHTML | ||
| }); |
There was a problem hiding this comment.
Fail fast when page num is missing or invalid.
At Line 595, Number(page.attributes.getNamedItem('num')?.value) can silently yield NaN, which creates unusable pageIllustrations entries instead of surfacing bad XML immediately.
Suggested fix
const imageFileTag = page.getElementsByTagName('image-filename')[0];
if (imageFileTag) {
+ const pageNum = parseInt(page.attributes.getNamedItem('num')!.value, 10);
+ if (Number.isNaN(pageNum)) {
+ throw new Error(`Invalid page num for book ${book.id}`);
+ }
pageIllustrations.push({
- num: Number(page.attributes.getNamedItem('num')?.value),
+ num: pageNum,
filename: book.getElementsByTagName('images')[0]
? tag.id +
'-' +Based on learnings: For conversion modules under convert/, adopt fail-fast behavior and surface invalid/missing XML attributes rather than silently recovering.
🤖 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 `@convert/convertConfig.ts` around lines 592 - 603, The code silently creates
pageIllustrations entries with NaN for num; instead, validate the page 'num'
attribute on page (page.attributes.getNamedItem('num')) before pushing: parse it
to an integer, check !isNaN and that it is a positive integer (or meets expected
constraints), and if invalid or missing throw an Error with a clear message
including identifying context (e.g., page.tagName/id or surrounding book id) so
conversion fails fast; update the block that constructs pageIllustrations (the
code referencing page, imageFileTag, book and
page.attributes.getNamedItem('num')) to perform this validation and throw rather
than pushing entries with NaN.
| const args = process.argv.slice(2).join(' '); | ||
| console.log('playwright test ' + args); | ||
| spawn('playwright test ' + args, { | ||
| shell: true, | ||
| stdio: 'inherit' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's verify the file exists and check its content
cat -n convert/runTests.tsRepository: sillsdev/appbuilder-pwa
Length of output: 519
🏁 Script executed:
# Let's check the Node.js spawn documentation and common patterns
# First, let's see if there are any other spawn calls in the repository that use best practices
rg "spawn\(" --type ts -A 3 | head -50Repository: sillsdev/appbuilder-pwa
Length of output: 414
🏁 Script executed:
# Let's verify Node.js spawn API behavior with a quick test
# Check if there's any documentation or test files that show the correct pattern
fd -e md -e txt -e js -e ts | xargs grep -l "spawn.*args" | head -5Repository: sillsdev/appbuilder-pwa
Length of output: 87
🏁 Script executed:
# Let me verify the exit code propagation issue mentioned in the internal scratchpad
# Check if the code properly handles exit codes from both spawn calls
grep -A 10 "spawn('vitest" convert/runTests.tsRepository: sillsdev/appbuilder-pwa
Length of output: 155
Forward argv as an argument array instead of a shell string.
process.argv.slice(2) preserves argument boundaries correctly. Joining it into a single string and passing to spawn('playwright test ' + args, { shell: true, ... }) causes the shell to re-parse the string, breaking arguments with spaces or quotes. For example, -t "story book" will be split into separate tokens and fail to match reliably. Use spawn('playwright', ['test', ...process.argv.slice(2)], { shell: false }) instead.
🤖 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 `@convert/runTests.ts` around lines 3 - 7, The current code builds a single
shell string using args and calls spawn with shell:true which breaks
quoted/space-containing args; change the spawn call to forward arguments as an
array by using process.argv.slice(2) directly (e.g. spawn('playwright', ['test',
...process.argv.slice(2)], { shell: false, stdio: 'inherit' })) so argument
boundaries created by process.argv.slice(2) are preserved; update any references
to the earlier args variable accordingly and remove the shell:true usage.
| function getCurrentIllustrationFile() { | ||
| let illustrations = config?.bookCollections | ||
| .find((x) => x.id === $refs.collection) | ||
| .books.find((x) => x.id === $refs.book)?.pageIllustrations; | ||
| for (let i = 0; i < illustrations.length; i++) { | ||
| if (illustrations[i].num === Number($refs.chapter)) { | ||
| return illustrationURLs[ | ||
| '/src/gen-assets/illustrations/' + illustrations[i].filename | ||
| ].default; //This works for both npm run dev and npm run build/preview, but it does have the hardcoded '/src/gen-assets/illustrations/'. Is there a better way to do this? | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Guard glob lookup before accessing .default.
At Lines 405-407, a missing key in illustrationURLs will throw (Cannot read properties of undefined (reading 'default')).
Suggested fix
function getCurrentIllustrationFile() {
- let illustrations = config?.bookCollections
- .find((x) => x.id === $refs.collection)
- .books.find((x) => x.id === $refs.book)?.pageIllustrations;
+ const illustrations =
+ config?.bookCollections
+ .find((x) => x.id === $refs.collection)
+ ?.books.find((x) => x.id === $refs.book)?.pageIllustrations ?? [];
for (let i = 0; i < illustrations.length; i++) {
if (illustrations[i].num === Number($refs.chapter)) {
- return illustrationURLs[
- '/src/gen-assets/illustrations/' + illustrations[i].filename
- ].default; //This works for both npm run dev and npm run build/preview, but it does have the hardcoded '/src/gen-assets/illustrations/'. Is there a better way to do this?
+ const key = '/src/gen-assets/illustrations/' + illustrations[i].filename;
+ return illustrationURLs[key]?.default;
}
}
}📝 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 getCurrentIllustrationFile() { | |
| let illustrations = config?.bookCollections | |
| .find((x) => x.id === $refs.collection) | |
| .books.find((x) => x.id === $refs.book)?.pageIllustrations; | |
| for (let i = 0; i < illustrations.length; i++) { | |
| if (illustrations[i].num === Number($refs.chapter)) { | |
| return illustrationURLs[ | |
| '/src/gen-assets/illustrations/' + illustrations[i].filename | |
| ].default; //This works for both npm run dev and npm run build/preview, but it does have the hardcoded '/src/gen-assets/illustrations/'. Is there a better way to do this? | |
| } | |
| } | |
| } | |
| function getCurrentIllustrationFile() { | |
| const illustrations = | |
| config?.bookCollections | |
| .find((x) => x.id === $refs.collection) | |
| ?.books.find((x) => x.id === $refs.book)?.pageIllustrations ?? []; | |
| for (let i = 0; i < illustrations.length; i++) { | |
| if (illustrations[i].num === Number($refs.chapter)) { | |
| const key = '/src/gen-assets/illustrations/' + illustrations[i].filename; | |
| return illustrationURLs[key]?.default; | |
| } | |
| } | |
| } |
🤖 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/routes/text/`+page.svelte around lines 399 - 410, In
getCurrentIllustrationFile(), the code assumes
illustrationURLs['/src/gen-assets/illustrations/'+illustrations[i].filename]
exists and immediately reads .default, which throws if the key is missing; guard
the glob lookup by first obtaining the entry (e.g., const entry =
illustrationURLs['/src/gen-assets/illustrations/'+illustrations[i].filename])
and only return entry.default when entry is defined, otherwise return a safe
fallback (undefined or a placeholder) or continue searching; update
getCurrentIllustrationFile to perform this existence check before accessing
.default so missing keys don't cause runtime errors.
FyreByrd
left a comment
FyreByrd
left a comment
There was a problem hiding this comment.
Looks like a good start. Unfortunately, the pictures in the provided storybook can get quite large, which really messes with the text visibility.
On this largest one, I can't even scroll on the area where the text is supposed to be.
|
According to #596, it is not supposed to display the text when in landscape mode; it is only supposed to display the picture as a full screen image. |
Noted. I'll need to revisit this with Chris or David. I don't think this is the behavior we would want on laptop. |
|
Per conversation with @davidmoore1
It is also worth noting that what you have right now is not actually a proper implementation of "landscape mode" |
|
What would a proper implementation of landscape mode be? |
See the second image in #596 (comment) I think what's missing here is that in landscape mode the picture is supposed to be deliberately fullscreen. There is still some active discussion between me, Chris, and David about how we want to handle landscape vs. portrait mode for the PWA, so this can wait for a bit until we've reached an agreement in our discussion. |
3 participants
Per #596:
This PR is intended to modify storybook files into a form that the Proskomma USFM parser can parse. I took some code from #645.
I've gotten the basics of storybooks with text and images implemented.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores