Conversation
Started taking code from PR sillsdev#645. Discovered that storybook files seem to be modified before the code in convertBooks runs.
WalkthroughThis update introduces support for chapter-specific illustrations in story-type books, enhances USFM parsing and rendering for various styled and list elements, and adds Playwright-based end-to-end testing. It modifies configuration parsing, Svelte components, test scripts, and introduces new transformation logic for USFM tags, while updating related tests and scripts to accommodate these features. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SveltePage as +page.svelte
participant ScriptureView as ScriptureViewSofria.svelte
participant Config as convertConfig.ts
participant USFM as convertBooks.ts
User->>SveltePage: Navigates to story book page
SveltePage->>Config: Loads BookConfig (with pageIllustrations)
SveltePage->>SveltePage: Determines bookType, gets current illustration
SveltePage->>ScriptureView: Renders content with illustration (if story)
ScriptureView->>USFM: Parses USFM, applies new milestone and list transformations
USFM-->>ScriptureView: Returns transformed USFM tokens
ScriptureView->>ScriptureView: Renders styled lists, paragraphs, and inline spans
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| 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.
| 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 base + '/illustrations/' + illustrations[i].filename; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Handle undefined illustrations array.
The function will throw a runtime error if pageIllustrations is undefined or null, which is likely for books that don't have illustrations.
function getCurrentIllustrationFile() {
let illustrations = config?.bookCollections
.find((x) => x.id === $refs.collection)
.books.find((x) => x.id === $refs.book)?.pageIllustrations;
+ if (!illustrations || illustrations.length === 0) {
+ return null;
+ }
for (let i = 0; i < illustrations.length; i++) {
if (illustrations[i].num === Number($refs.chapter)) {
return base + '/illustrations/' + illustrations[i].filename;
}
}
+ return null; // No illustration found for this chapter
}📝 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 base + '/illustrations/' + illustrations[i].filename; | |
| } | |
| } | |
| } | |
| function getCurrentIllustrationFile() { | |
| let illustrations = config?.bookCollections | |
| .find((x) => x.id === $refs.collection) | |
| .books.find((x) => x.id === $refs.book)?.pageIllustrations; | |
| if (!illustrations || illustrations.length === 0) { | |
| return null; | |
| } | |
| for (let i = 0; i < illustrations.length; i++) { | |
| if (illustrations[i].num === Number($refs.chapter)) { | |
| return base + '/illustrations/' + illustrations[i].filename; | |
| } | |
| } | |
| return null; // No illustration found for this chapter | |
| } |
🤖 Prompt for AI Agents
In src/routes/text/+page.svelte around lines 369 to 378, the function
getCurrentIllustrationFile assumes pageIllustrations is always defined, which
can cause runtime errors if it is undefined or null. To fix this, add a check to
ensure pageIllustrations exists before iterating over it. If it is undefined or
null, handle that case gracefully by returning undefined or an appropriate
fallback value.
| <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)
convert/runTests.ts
Outdated
| 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.
2 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
.gitignoreto exclude test result files.