Implement hashed file paths #903
Conversation
WalkthroughMigrates generated assets from Changes
Sequence Diagram(s)sequenceDiagram
participant Converter as Converter (build)
participant FS as File System
participant Bundler as Vite Bundler
participant Runtime as Browser
rect rgb(248,248,240)
Note over Converter,FS: Conversion phase creates hashed assets
Converter->>FS: read original asset -> compute MD5 -> createHashedFile(...)
FS-->>Converter: hashed asset written under src/gen-assets/
end
rect rgb(240,248,255)
Note over Bundler,Runtime: Bundling exposes asset URLs via import.meta.glob
Bundler->>FS: embed mapping (path -> URL) from src/gen-assets
Runtime->>Bundler: page request
Bundler-->>Runtime: page with resolved hashed asset URLs (via glob maps)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Areas needing extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
FyreByrd
force-pushed
the
feature/hash-buster
branch
2 times, most recently
from
de5e608 to
ddfe3f2
Compare
|
Rebased on main |
This is necessary to use the `base` option in glob imports for #903 I did have to rebuild the package-lock.json in order to resolve some peer dependency issues.
This is necessary to use the `base` option in glob imports for #903 I did have to rebuild the package-lock.json in order to resolve some peer dependency issues.
Change convertMedia.ts to place icons in `src/generatedAssets`. Replace uses of deprecated `base` with calls to `resolve()` in sidebar. Add aliases for static, images, and generatedAssets. Add gitignore line for new icons location.
FyreByrd
force-pushed
the
feature/hash-buster
branch
from
ddfe3f2 to
73143b2
Compare
FyreByrd
force-pushed
the
feature/hash-buster
branch
from
7440f73 to
d9a8dc7
Compare
We really need to finish moving towards Svelte 5 or at least TS
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 22
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/components/ScriptureViewSofria.svelte (1)
1186-1205: Add error handling for missing illustrations.When an illustration file is not found in the glob imports (line 1187),
imageSourcesilently becomes an empty string. This causes several issues:
- Setting
img.srcto an empty string (line 1193) triggers a network request to the current page URL- The fullscreen zoom feature (line 1196) will malfunction
- No warning is logged, making debugging difficult
Apply this diff to add error handling:
function createIllustrationBlock(source: string, caption: string | null) { - const imageSource = illustrations['./' + source]?.default ?? ''; + const imageSource = illustrations['./' + source]?.default; + + if (!imageSource) { + console.warn(`Illustration not found: ${source}`); + // Return early or use a placeholder + const divFigure = document.createElement('div'); + divFigure.classList.add('image-block'); + divFigure.style.display = 'none'; + return { block: divFigure, source: '' }; + } + const divFigure = document.createElement('div'); divFigure.classList.add('image-block');Alternatively, if missing illustrations should still display (relying on
checkImageExiststo hide them), at least add logging:- const imageSource = illustrations['./' + source]?.default ?? ''; + const imageSource = illustrations['./' + source]?.default ?? ''; + if (!imageSource) { + console.warn(`Illustration file not found in glob imports: ${source}. Available keys:`, Object.keys(illustrations).slice(0, 5)); + }src/routes/share/+page.svelte (1)
105-110: Remove debug console.log.The console.log statement appears to be a debugging artifact that should be removed before merging to production.
Apply this diff:
- {(console.log( - badgeLanguages.includes($language) - ? `Using ${appStoreBadge}` - : `Fallback to ${fallbackAppStoreBadge}` - ), - '')}
🧹 Nitpick comments (17)
src/routes/lexicon/+page.svelte (1)
2-2: Remove unused import.The
baseimport is no longer used after switching to glob imports for asset loading.Apply this diff:
- import { base } from '$app/paths';convert/convertSQLite.ts (1)
9-11: Update the comment to reflect the new destination path.The comment references "static folder" but the files are now written to
src/generatedAssets/.Apply this diff:
/** - * Copies sqlite-wasm.wasm and data.sqlite to the static folder. + * Copies sqlite-wasm.wasm and data.sqlite to the generated assets folder. */convert/convertAbout.ts (1)
8-11: Update the comment to reflect the new destination path.The comment references "static folder" but the file is now written to
src/generatedAssets/.Apply this diff:
/** - * Copies about.partial.html to static folder + * Copies about.partial.html to generated assets folder */src/lib/components/CollectionList.svelte (2)
9-12: Consider adding error handling for missing assets.The glob pattern eagerly loads all illustrations, but there's no runtime validation that the required assets exist in the map.
If an illustration file is missing or the key doesn't match, line 52 will throw a runtime error. Consider adding a fallback or validation:
const illustrations = import.meta.glob('./*', { eager: true, base: '/src/generatedAssets/illustrations' }) as Record<string, { default: Object }>; // Helper function function getIllustration(imageName: string): string | undefined { const key = './' + imageName; return illustrations[key]?.default as string | undefined; }Then use it with optional chaining:
{#if d.image} <img src={getIllustration(d.image) ?? '/fallback.png'} /> {/if}
50-53: Add defensive checks for missing illustration keys.Direct property access without validation could cause runtime errors if the expected illustration doesn't exist in the preloaded map.
Apply this pattern:
-<img - class="layout-image" - src={illustrations['./' + d.image].default as string} -/> +<img + class="layout-image" + src={illustrations['./' + d.image]?.default as string ?? ''} + alt={d.name} +/>The
?.optional chaining prevents errors if the key doesn't exist.src/routes/lexicon/+page.ts (1)
49-57: Add defensive check for missing reversal index URLs.Direct property access on line 51 could throw a runtime error if the expected reversal index doesn't exist in the preloaded map.
Apply this pattern:
for (const [key, ws] of Object.entries(dictionaryConfig.writingSystems)) { if (!ws.type.includes('main')) { - const response = await fetch(reversalIndexUrls[`./${key}/index.json`].default); + const url = reversalIndexUrls[`./${key}/index.json`]?.default; + if (!url) { + console.warn(`Reversal index URL not found for language: ${key}`); + continue; + } + const response = await fetch(url); if (response.ok) { reversalIndexes[key] = (await response.json()) as ReversalIndex; } else { console.warn(`Failed to load reversal index for language: ${key}`); } } }src/routes/text/+page.svelte (1)
494-494: Consider adding a fallback for missing border assets.While optional chaining is present (
?.default), if the border asset doesn't exist, the CSS variable will be set tourl(undefined), which is invalid CSS.Consider adding a fallback:
-style="--borderImageSource=url({borders['./border.png']?.default});" +style="--borderImageSource=url({borders['./border.png']?.default ?? 'none'});"Or check if the border exists before rendering:
{#if borders['./border.png']?.default} style="--borderImageSource=url({borders['./border.png'].default});" {/if}convert/convertMedia.ts (1)
110-115: Ensuresrc/generatedAssetsbase directory exists before copyingIf
ConvertMediaever runs before any other task that createssrc/generatedAssets,cpSynctopath.join('src/generatedAssets', p)can fail because the parent directory is missing. SincemkdirSyncis already imported, you can defensively create the base dir once at the start ofconvertMedia.async convertMedia( dataDir: string, configData: ConfigTaskOutput, verbose: number, modifiedDirectories: string[] ) { + // Ensure base output directory exists before rimraf/copy operations + mkdirSync(path.join('src', 'generatedAssets'), { recursive: true }); @@ await Promise.all( modifiedDirectories.map((p) => - rimraf(path.join('src/generatedAssets', p)).then(() => { - if (verbose) console.log(`removed ${path.join('src/generatedAssets', p)}`); + rimraf(path.join('src', 'generatedAssets', p)).then(() => { + if (verbose) { + console.log(`removed ${path.join('src', 'generatedAssets', p)}`); + } return p; }) ) ); @@ for (const p of modifiedDirectories) { cloneDirectory( path.join(dataDir, p), - path.join('src/generatedAssets', p), + path.join('src', 'generatedAssets', p), verbose, !required.includes(p) ); @@ - const tabsPath = path.join('src', 'generatedAssets', 'icons', 'tabs'); + const tabsPath = path.join('src', 'generatedAssets', 'icons', 'tabs');(The last hunk just keeps joins consistent.)
Also applies to: 118-125, 141-153
.github/workflows/main.yml (1)
44-49: Make project_data cache key sensitive to changes undertest_data/projects
hashFiles('test_data/projects', 'convert/*.ts')hashes the directory entry, not its contents, so cache invalidation won’t occur when individual project zip files change. Using a recursive glob will better reflect the actual inputs:- key: ${{ runner.os }}-projects-${{ hashFiles('test_data/projects', 'convert/*.ts') }} + key: ${{ runner.os }}-projects-${{ hashFiles('test_data/projects/**', 'convert/*.ts') }} @@ - key: ${{ runner.os }}-projects-${{ hashFiles('test_data/projects', 'convert/*.ts') }} + key: ${{ runner.os }}-projects-${{ hashFiles('test_data/projects/**', 'convert/*.ts') }} @@ - key: ${{ runner.os }}-projects-${{ hashFiles('test_data/projects', 'convert/*.ts') }} + key: ${{ runner.os }}-projects-${{ hashFiles('test_data/projects/**', 'convert/*.ts') }} @@ - key: ${{ runner.os }}-projects-${{ hashFiles('test_data/projects', 'convert/*.ts') }} + key: ${{ runner.os }}-projects-${{ hashFiles('test_data/projects/**', 'convert/*.ts') }}Also applies to: 116-122, 139-144, 179-184
convert/convertManifest.ts (1)
17-17: Minor optimization: Create icons directory only if icons exist.The
static/iconsdirectory is created whenever a manifest exists, even if no icons are present. While harmless, you could optimize by creating it only when the first icon is encountered.Consider moving this line inside the icon processing block (after line 38) with a flag to ensure it's created only once:
- mkdirSync(path.join('static', 'icons'), { recursive: true }); const fileContents = readFileSync(srcFile).toString(); const lines = fileContents.split('\n'); + let iconsCreated = false; contents = lines .map((line) => { ... if (line.includes('"src":') && line.includes('./icons')) { + if (!iconsCreated) { + mkdirSync(path.join('static', 'icons'), { recursive: true }); + iconsCreated = true; + } ... } })convert/fileUtils.ts (1)
5-8: Consider binary file handling and add error handling.Two concerns with
getHashedName:
Binary files: Converting the file contents to a string using
String(readFileSync(fullPath))could corrupt binary data. If this function is used for images or other binary assets, the hash might be computed on corrupted data.Error handling:
readFileSynccan throw if the file doesn't exist or isn't readable.If binary files are expected, read as Buffer directly:
export function getHashedName(dataDir: string, src: string) { const fullPath = join(dataDir, src); - return getHashedNameFromContents(String(readFileSync(fullPath)), src); + try { + const contents = readFileSync(fullPath); + return getHashedNameFromContents(contents.toString('utf8'), src); + } catch (error) { + throw new Error(`Failed to read ${fullPath}: ${error.message}`); + } }Alternatively, if binary files are in scope, update
getHashedNameFromContentsto acceptBuffer | stringand handle both cases.src/lib/components/BookTabs.svelte (1)
51-51: Add fallback for missing icons.The code directly accesses
tabIcons[\./${getImageName(...)}`].default` without checking if the icon exists in the imported map. If an icon is missing, this will cause a runtime error.Add a helper function with error handling:
+ function getIconSrc(tabType) { + const imageName = getImageName(tabType); + const icon = tabIcons[`./${imageName}`]; + if (!icon) { + console.error(`Icon not found: ${imageName}`); + return ''; // or a fallback icon path + } + return icon.default; + } <picture class:invert={$theme === 'Dark'}> <img - src={tabIcons[`./${getImageName(bookTabs.mainType)}`].default} + src={getIconSrc(bookTabs.mainType)}Apply the same pattern on line 67.
Also applies to: 67-67
src/lib/components/BottomNavigationBar.svelte (1)
136-136: Add fallback for missing icons.Similar to
BookTabs.svelte, accessingmenuIcons[...]without validation could cause runtime errors if an icon is missing from the generated assets.Add error handling:
+ function getIconSrc(item) { + const iconKey = `./${item.images[0].file}`; + const icon = menuIcons[iconKey]; + if (!icon) { + console.error(`Menu icon not found: ${item.images[0].file}`); + return ''; // or a fallback icon + } + return icon.default; + } <img - src={menuIcons[`./${item.images[0].file}`].default} + src={getIconSrc(item)} alt=""convert/convertStyles.ts (1)
79-102: Consider failing build on missing fonts.Currently, if a font is not found, a warning is logged but the build continues with a potentially invalid path reference. Consider whether this should be a hard error to catch missing font files during build.
If you want to fail on missing fonts, apply this pattern:
} else { console.warn( `${srcFile}: Could not locate ${path.join(dataDir, fontPath)}` ); + throw new Error(`Missing font file: ${fontPath}`); }src/routes/quiz/[collection]/[id]/+page.js (1)
37-37: Add optional chaining for missing quiz files.While the try-catch handles fetch errors, it won't provide a clear error message if the quiz file is not in the glob map. Consider adding optional chaining.
Apply this diff:
- const response = await fetch(quizzes[`./${collection}/quizzes/${id}.json`].default); + const quizUrl = quizzes[`./${collection}/quizzes/${id}.json`]?.default; + if (!quizUrl) { + throw new Error(`Quiz not found: ${collection}/quizzes/${id}.json`); + } + const response = await fetch(quizUrl);convert/convertContents.ts (2)
156-169: Consider using explicit string prefix removal.The regex
/contents\//without thegflag only replaces the first occurrence. IfcreateHashedFileconsistently returns paths starting withcontents/, consider using more explicit string methods:- ).replace(/contents\//, ''); + ).replace(/^contents\//, '');The
^anchor makes the intent clearer and ensures only the leading prefix is removed.
51-53: Consider consolidating the interface declarations.The
ContentsTaskOutputinterface is declared twice (lines 51-53 and 286-288). While TypeScript's declaration merging makes this work, consolidating into a single declaration would be clearer:-export interface ContentsTaskOutput extends TaskOutput { - taskName: 'ConvertContents'; -} - const data: ContentsData = {}; export interface ContentsTaskOutput extends TaskOutput { taskName: 'ConvertContents'; + data: ContentsData; }Then remove the duplicate declaration at lines 286-288.
Also applies to: 286-288
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (48)
.github/workflows/lint.yml(0 hunks).github/workflows/main.yml(1 hunks).github/workflows/test.yml(0 hunks).gitignore(1 hunks).prettierignore(1 hunks)config/index.d.ts(1 hunks)convert/convertAbout.ts(1 hunks)convert/convertBadges.ts(1 hunks)convert/convertBooks.ts(12 hunks)convert/convertConfig.ts(5 hunks)convert/convertContents.ts(3 hunks)convert/convertManifest.ts(4 hunks)convert/convertMedia.ts(4 hunks)convert/convertPlans.ts(1 hunks)convert/convertReverseIndex.ts(1 hunks)convert/convertSQLite.ts(2 hunks)convert/convertStyles.ts(3 hunks)convert/fileUtils.ts(1 hunks)convert/index.ts(1 hunks)data-sandbox/index.ts(1 hunks)example/index.ts(2 hunks)package.json(1 hunks)src/app.html(0 hunks)src/lib/components/BookTabs.svelte(3 hunks)src/lib/components/BottomNavigationBar.svelte(2 hunks)src/lib/components/CollectionList.svelte(2 hunks)src/lib/components/FontList.svelte(1 hunks)src/lib/components/HtmlBookView.svelte(1 hunks)src/lib/components/ScriptureViewSofria.svelte(5 hunks)src/lib/components/Sidebar.svelte(6 hunks)src/lib/components/TextAppearanceSelector.svelte(1 hunks)src/lib/data/audio.ts(3 hunks)src/lib/data/catalogData.ts(2 hunks)src/lib/data/plansData.ts(2 hunks)src/lib/data/scripture.js(2 hunks)src/lib/data/stores/lexicon.ts(1 hunks)src/lib/scripts/milestoneLinks.ts(2 hunks)src/lib/video/index.ts(3 hunks)src/routes/+layout.svelte(3 hunks)src/routes/about/+page.js(1 hunks)src/routes/lexicon/+page.svelte(2 hunks)src/routes/lexicon/+page.ts(2 hunks)src/routes/quiz/[collection]/[id]/+page.js(2 hunks)src/routes/quiz/[collection]/[id]/+page.svelte(10 hunks)src/routes/share/+page.js(1 hunks)src/routes/share/+page.svelte(2 hunks)src/routes/text/+page.svelte(3 hunks)svelte.config.js(1 hunks)
💤 Files with no reviewable changes (3)
- .github/workflows/lint.yml
- .github/workflows/test.yml
- src/app.html
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-06-16T18:47:15.122Z
Learnt from: judah-sotomayor
Repo: sillsdev/appbuilder-pwa PR: 840
File: src/lib/components/TextAppearanceSelector.svelte:165-197
Timestamp: 2025-06-16T18:47:15.122Z
Learning: In src/lib/components/TextAppearanceSelector.svelte, the user judah-sotomayor considers theme selection buttons not relevant for screen-reader users and prefers not to add accessibility labels to theme buttons.
Applied to files:
src/lib/components/TextAppearanceSelector.svelte
📚 Learning: 2025-07-04T18:51:27.821Z
Learnt from: chrisvire
Repo: sillsdev/appbuilder-pwa PR: 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/quiz/[collection]/[id]/+page.sveltesrc/routes/share/+page.svelte
📚 Learning: 2025-07-04T18:51:27.821Z
Learnt from: chrisvire
Repo: sillsdev/appbuilder-pwa PR: 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/quiz/[collection]/[id]/+page.sveltesrc/routes/share/+page.svelte
📚 Learning: 2025-05-07T13:16:04.481Z
Learnt from: chrisvire
Repo: sillsdev/appbuilder-pwa PR: 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:
convert/convertBooks.ts
📚 Learning: 2025-05-07T13:16:04.481Z
Learnt from: chrisvire
Repo: sillsdev/appbuilder-pwa PR: 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/convertBooks.ts
🧬 Code graph analysis (11)
src/lib/data/scripture.js (1)
src/lib/data/stores/scripture.js (2)
docSet(110-110)docSet(110-110)
src/routes/lexicon/+page.ts (1)
src/routes/quiz/[collection]/[id]/+page.js (1)
response(37-37)
src/lib/data/stores/lexicon.ts (1)
src/routes/quiz/[collection]/[id]/+page.js (1)
response(37-37)
convert/convertManifest.ts (1)
convert/fileUtils.ts (2)
createHashedFile(21-35)createHashedFileFromContents(37-54)
convert/convertStyles.ts (1)
convert/fileUtils.ts (1)
createHashedFile(21-35)
convert/convertConfig.ts (1)
convert/fileUtils.ts (1)
getHashedName(5-8)
src/lib/data/plansData.ts (3)
src/routes/plans/+page.js (1)
plans(11-11)src/routes/plans/[id]/+page.js (1)
planConfig(10-10)src/routes/plans/[id]/settings/+page.js (1)
planConfig(8-8)
convert/index.ts (1)
convert/convertManifest.ts (1)
ConvertManifest(69-82)
src/lib/data/audio.ts (1)
src/lib/scripts/stringUtils.ts (1)
pathJoin(200-214)
convert/convertBooks.ts (2)
convert/fileUtils.ts (2)
createHashedFile(21-35)getHashedNameFromContents(10-19)src/routes/quiz/[collection]/[id]/+page.js (3)
collection(13-13)quizzes(4-8)id(12-12)
convert/convertContents.ts (1)
convert/fileUtils.ts (1)
createHashedFile(21-35)
🪛 actionlint (1.7.8)
.github/workflows/main.yml
14-14: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
129-129: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
169-169: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (51)
.prettierignore (1)
16-16: Add/project_datato Prettier ignore list — good change.The addition makes sense given the PR's focus on generated assets and hash-busting. The entry logically groups with other generated/data directories (like
/data,/build,example_data,test_data).Minor observation: There's a slight inconsistency in path notation—some entries use a leading slash (e.g.,
/build,/data), while others don't (e.g.,static,example_data,test_data). If standardizing to all root-level paths with leading slashes is a codebase convention, consider applying it uniformly here as well (optional cleanup).src/lib/components/TextAppearanceSelector.svelte (1)
161-161: Based on the script output, I can now verify the store initialization:
currentFontStore Analysis:
- It's a derived store (line 170 in
scripture.js) that returnsconfig.fonts[0].familywhen not initialized- Otherwise returns
$currentFonts[$refs.collection], which is initialized with fallback logic (lines 151-160)- Each collection gets either a valid matching font or defaults to
config.fonts[0].familyThe optional chaining change in
TextAppearanceSelector.svelte:161is valid defensive coding. While the store initialization appears solid, an edge case exists: if$currentFontdoesn't match any font inconfig.fonts(due to data corruption or config changes),find()returnsundefined, causing a runtime error without the?.operator.The fix correctly prevents this by safely returning
undefinedif no matching font is found, allowing the button to render without crashing.
convert/convertReverseIndex.ts (1)
34-34: Path migration verified—all reversal asset references already updated to new location.The change from
static/reversaltosrc/generatedAssets/reversalis correct. Verification confirms:
- No hardcoded references to the old
static/reversalpath remain in the codebase- The consumer in
src/routes/lexicon/+page.ts(line 13) already configured withbase: '/src/generatedAssets/reversal'- Asset loading mechanism via
import.meta.glob()correctly targets the new pathsrc/lib/components/FontList.svelte (1)
38-38: Need to search for where the configuration is loaded to understand if bookFonts/colFonts can actually contain fonts not in config.fonts.Based on the script output from the first execution and the web search results, I have enough information to make a determination. Let me finalize the analysis:
From the earlier script output:
fontChoicesis a derived store that returns eitherbookFonts,colFonts, orallFontsallFontsis safely derived fromconfig.fonts.map(), so it cannot have mismatchesbookFontsandcolFontscome from configuration metadata and could potentially contain font names not inconfig.fonts- The optional chaining
?.on line 38 prevents a crash iffind()returns undefined- If no font is found, the UI renders as empty string (undefined in template)
The change is a valid defensive improvement. The optional chaining is correct ES2020+ syntax supported in Svelte. The potential issue (font in fontChoices but not in config.fonts) could occur if bookCollections/books metadata has stale or incorrect references, but this is handled safely.
Good defensive improvement; confirm if fallback is needed.
The optional chaining prevents a potential crash if a font from
$fontChoicesis not found inconfig.fonts. This could occur if book/collection metadata contains font family names that don't exist inconfig.fonts.If this scenario can happen (e.g., due to stale metadata), consider adding a fallback to display the font family name:
{config.fonts.find((x) => x.family === font)?.name ?? font}This provides better UX than rendering an empty string.
convert/index.ts (1)
41-50: Reordering is safe—no inter-task dependencies exist.Verification confirms that
ConvertManifestandConvertMediaare independent:
ConvertManifestreads only from the sourcemanifest.jsonfile indataDir, not from generated media outputs- Neither task reads outputs from the other (checked via
outputs.get())- Both write to separate files in
src/generatedAssetswith no blocking dependencies- The reordering has no functional impact
src/lib/data/audio.ts (2)
409-409: LGTM: Good defensive programming.Adding optional chaining here prevents potential runtime errors if
bookCollectionsis undefined or null.
19-27: Now let me examine the actual code and the repository context:Incorrect review comment. The directories
src/generatedAssets/audioandsrc/generatedAssets/timingsdon't exist in the repository. Glob patterns are treated like import specifiers and must be either relative (start with ./) or absolute (start with /, resolved relative to project root). Vite processes glob imports at build time through its macro expansion.Since these directories cannot be verified to exist in the source code, the shell script verification approach is fundamentally flawed. However, examining the actual code and understanding how it's used is necessary before drawing conclusions about whether this represents a real issue or expected behavior.
Likely an incorrect or invalid review comment.
src/routes/lexicon/+page.svelte (1)
24-28: Consider whether eager loading of all reversal JSONs aligns with your bundle size targets.The glob configuration is syntactically correct and follows Vite's documented behavior where base accepts an absolute path anchored at the project root, prepending this string to generated import specifiers.
However, I cannot verify the actual bundle size impact because
src/generatedAssets/is build-time generated and not available in the repository. The pattern of eager-loading (eager: true) is used consistently across 20+ locations in your codebase for similar assets (styles, badges, videos, images, collections, etc.), suggesting this is an intentional architectural choice to trade upfront bundle size for guaranteed runtime availability without network requests.Confirm this eager-loading approach is appropriate given your expected number of reversal JSON files and acceptable initial load time for your users.
src/lib/components/ScriptureViewSofria.svelte (3)
1171-1174: LGTM!The destructuring correctly adapts to the new return signature of
createIllustrationBlock. Ignoring thesourcevalue is appropriate here since the image source is already applied within the function, andaddIllustrationsonly needs the block element for placement.
1287-1291: LGTM!The destructuring correctly extracts both
blockandsource, and properly usesimageSourcefor the existence check. This demonstrates the intended usage of the refactoredcreateIllustrationBlockreturn value.
62-65: ****The original concern about the
baseoption not being supported is incorrect. Vite'simport.meta.globaccepts abaseoption as documented in the Vite Features guide, so the code at lines 62-65 is valid and will work as intended.The
src/generatedAssets/illustrations/directory not appearing in the repository is expected—it's in.gitignoreand is generated at build time.The code correctly uses the
baseoption to resolve the glob pattern to the intended directory, and the fallback to an empty string on line 1187 is handled bycheckImageExists, which hides the element if the image cannot be fetched.Likely an incorrect or invalid review comment.
convert/convertPlans.ts (1)
25-25: LGTM - Path migration is consistent.The destination path change from
static/planstosrc/generatedAssets/plansaligns with the PR's cache-busting strategy.convert/convertSQLite.ts (1)
14-23: LGTM - Consistent path migration.The destination paths have been correctly updated to use
src/generatedAssets/.convert/convertAbout.ts (1)
13-13: LGTM - Consistent path migration.The destination path has been correctly updated to use
src/generatedAssets/.example/index.ts (1)
73-77: LGTM - Good defensive coding.Ensuring a clean temp directory by deleting and recreating it prevents potential issues with stale files from previous runs.
convert/convertBadges.ts (1)
11-11: LGTM - Consistent path migration.The destination path change to
src/generatedAssets/badgesaligns with the PR's asset migration strategy.src/routes/lexicon/+page.ts (1)
11-15: LGTM - Consistent asset preloading pattern.The use of
import.meta.globwith the?urlquery parameter for reversal indexes aligns with the PR's asset-loading strategy.src/routes/text/+page.svelte (1)
64-64: LGTM - Consistent asset preloading.The border assets are eagerly preloaded using the same
import.meta.globpattern used throughout the PR.src/lib/data/scripture.js (1)
8-12: PKF asset glob wiring looks consistent with generatedAssets usageUsing
import.meta.glob('./*.pkf', { eager: true, base: '/src/generatedAssets/collections', query: '?url' })keeps docset resolution aligned with the new generatedAssets layout and avoids hard‑coded static paths..gitignore (1)
20-27: Ignoring generated manifests andsrc/generatedAssetsis appropriateTreating
static/manifest*.jsonand the entiresrc/generatedAssetstree as generated artifacts keeps the repo clean and matches the new hashed asset pipeline..github/workflows/main.yml (1)
9-122: Overall workflow structure and caching strategy look solidSplitting setup, lint, and test into separate jobs with shared caches for
node_modulesand pre‑convertedproject_datashould give good CI performance while keeping the expensive Java/AppBuilders step isolated.src/lib/components/Sidebar.svelte (1)
6-8: Base URL handling and header assets integration look goodSwitching to
resolve('/')for the home link and importing the drawer header images via$imageskeeps routing/base handling and static assets consistent with the rest of the app.Also applies to: 128-132
convert/convertManifest.ts (2)
1-3: LGTM!The imports are appropriate for the hashing functionality being added.
61-67: LGTM!The hashed manifest generation and URL tracking approach is clean and aligns well with the cache-busting objectives.
convert/fileUtils.ts (1)
10-19: LGTM! MD5 is appropriate for cache busting.The use of MD5 with base64url encoding is suitable for generating cache-busting hashes. This is not a security concern since the hashes are used only to detect content changes, not for cryptographic purposes.
config/index.d.ts (1)
63-63: LGTM!The addition of the optional
hashedFileNamefield is well-documented and appropriately typed. The comment clarifies its current scope.src/lib/components/HtmlBookView.svelte (1)
37-39: LGTM!The conditional URL construction correctly uses
hashedFileNamewhen available and falls back tofilefor non-hashed books.src/lib/components/BookTabs.svelte (1)
18-21: LGTM! Using import.meta.glob for eager preloading.The use of
import.meta.globwitheager: trueis appropriate for preloading all tab icons. This ensures icons are bundled and available synchronously.src/lib/components/BottomNavigationBar.svelte (1)
11-14: LGTM! Consistent pattern with other components.The use of
import.meta.globfor preloading menu icons follows the same pattern asBookTabs.svelte.src/routes/about/+page.js (1)
5-6: ✓ Verified:$assetsalias is correctly configuredThe verification confirms that the
$assetsalias is properly set up insvelte.config.jsat line 25, pointing to'./src/generatedAssets/'. This ensures the dynamic importimport('$assets/about.partial.html?raw')will resolve correctly. The code change from HTTP fetch to dynamic import is sound.data-sandbox/index.ts (1)
22-22: Collection assets are correctly generated to the new path—no action required.Verification confirms that
convert/convertBooks.tsproperly generates all collection assets tosrc/generatedAssets/collections:
- Quiz files written to
src/generatedAssets/collections/{collectionId}/quizzes/(lines 435–445)- Index file written to
src/generatedAssets/collections/index.json(line 535)- Catalog entries written to
src/generatedAssets/collections/catalog/(lines 514–517)The path change in
data-sandbox/index.tsline 22 aligns with where files are actually generated.Note: Lines 419–421 in
convert/convertBooks.tscreate an unusedstatic/collectionsdirectory (stale code from the previous path), but this does not affect functionality since all actual file writes target the correct new location.src/routes/share/+page.js (1)
4-21: LGTM! Good defensive coding.The dynamic asset resolution via
import.meta.globwith optional chaining and conditional fetch provides robust error handling. The pattern correctly targets the generated badges directory.src/routes/share/+page.svelte (1)
10-13: LGTM! Type-safe asset loading.The eager glob import with explicit type assertion ensures compile-time safety. The pattern correctly targets SVG badges from the generated assets directory.
src/lib/video/index.ts (1)
4-12: LGTM! Consistent glob pattern for video assets.The dual glob imports correctly separate thumbnails and videos into distinct maps with appropriate type assertions.
src/routes/+layout.svelte (2)
3-5: LGTM! Clean direct imports for static assets.The direct imports leverage the $assets alias correctly and provide type safety for build-time known assets.
35-38: LGTM! Styles glob configured correctly.The eager glob import for styles aligns with the asset generation strategy across the codebase.
svelte.config.js (1)
21-26: LGTM! Path aliases support new asset structure.The aliases correctly map to the generated assets directories and enable cleaner imports throughout the codebase.
convert/convertConfig.ts (1)
506-506: LGTM! Signature change enables hash-based filenames.The addition of
dataDirparameter is necessary for computing hashed filenames and aligns with the PR objectives.convert/convertStyles.ts (1)
21-23: LGTM! Styles migrated to generated assets.The destination path correctly targets the new generatedAssets structure. Fonts remaining in
static/fontsappears intentional for direct URL access in CSS.src/routes/quiz/[collection]/[id]/+page.js (1)
4-8: LGTM! Recursive glob pattern appropriate for nested structure.The
**pattern correctly matches the nested collection/quizzes directory structure. The?urlquery is consistent with other JSON asset loading patterns.convert/convertContents.ts (1)
81-88: LGTM!The introduction of
hasContentsDirflag and conditional directory creation is a clean approach to ensure the destination directory is ready before asset processing.package.json (1)
12-13: LGTM!Adding
svelte-kit syncbeforesvelte-checkensures generated types are up-to-date, preventing type check failures due to stale type definitions. This aligns with SvelteKit best practices.src/routes/quiz/[collection]/[id]/+page.svelte (4)
4-4: LGTM!The import of
pagefrom$app/stateis correct for Svelte 5, replacing the deprecated$app/storespattern. Based on learnings.
84-88: LGTM!The backward compatibility approach using
staticAssetsflag based on version comparison is well-designed, ensuring smooth migration between asset resolution strategies.
146-147: LGTM!Good use of optional chaining and nullish coalescing to handle missing images gracefully. This defensive pattern should be applied consistently to all glob map accesses (see comment on lines 222-227).
21-34: Bundle size impact of eager loading cannot be verified in this environment.The directories containing assets (
src/generatedAssets/*) are generated at build time and excluded from version control (in.gitignore). Unable to count actual assets or measure bundle impact in the sandbox.However, code inspection reveals important context:
The eager loading appears necessary because:
- Assets are accessed synchronously during quiz execution (audio playback, image display)
- Functions like
playSound(),getImageSource(), andgetAnswerAudio()expect immediate access to asset objects- Lazy loading would require deferred imports, incompatible with these synchronous call patterns
The broad glob patterns (
./*) do load all files in each directory, unlike the related+page.jswhich usesquery: '?url'for URLs instead of modules. This design distinction suggests intentional module pre-loading for asset access.To validate this approach: measure bundle size with/without eager loading, confirm whether all eagerly-loaded assets are actually used in typical quiz scenarios, and review if runtime performance is acceptable.
convert/convertBooks.ts (5)
204-227: LGTM!The addition of
contextparameter toupdateImgTagsenables proper hashed file generation while maintaining clean separation of concerns. The function correctly handles missing images by filtering them out.
414-423: LGTM!The conditional directory creation for HTML books ensures the necessary directory structure exists before file operations. The use of
static/for illustrations and HTML collection files is appropriate since these need to be statically served.
596-608: Verify hash computation timing for HTML books.The hash is computed from the original content before filtration (
before = getHashedNameFromContents(content, book.file)), but the filtered content is written to the file. This means:
- The filename hash is stable based on the source file
- If only the filters change (not the source), the filename remains the same
- Cached versions won't be busted when filters change
Is this the intended behavior? If filter changes should also bust the cache, consider computing the hash after filtration:
function convertHtmlBook(context: ConvertBookContext, book: BookConfig, files: any[]) { const srcFile = path.join(context.dataDir, 'books', context.bcId, book.file); let content = fs.readFileSync(srcFile, 'utf-8'); - const before = getHashedNameFromContents(content, book.file); content = applyFilters(content, htmlFilterFunctions, context.bcId, book.id, context); + const hashedName = getHashedNameFromContents(content, book.file); - // file name already in config from before filtration - // don't want to modify config to account for filtration files.push({ - path: path.join('static', 'collections', context.bcId, before), + path: path.join('static', 'collections', context.bcId, hashedName), content }); }Alternatively, if the current approach is intentional, clarify in the comments why hashing should be pre-filtration.
367-367: LGTM!The migration of asset paths from
static/tosrc/generatedAssets/is consistent throughout the conversion pipeline, aligning with the PR's objective to implement hash-based cache busting for generated assets.Also applies to: 438-438, 493-493, 500-500, 509-509, 528-528, 535-535
336-343: LGTM!The introduction of
ConvertBookContextis a clean design that bundles related contextual data, making the filter pipeline more maintainable and extensible.
Throw error from convertManifest if icon missing Throw error if audio/timing missing Throw error if catalog not found Throw error if missing plan data Throw error for missing docset Throw error for missing wasm/sqlite Log error for missing reversals
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
convert/convertBooks.ts (1)
191-277: UseConvertBookContext.dataDirinisImageMissingto avoid hard-coded paths.
removeMissingFiguresandupdateImgTagsnow both hinge onisImageMissing(imageSource), butisImageMissingstill uses a hard-coded'data/illustrations'root:return !fs.existsSync(path.join('data', 'illustrations', imageSource));If
convertBooksis ever run with adataDirother than'data', this will incorrectly treat existing images as missing (and strip<fig>/<img>), or vice versa. It also defeats the point of threadingConvertBookContextthrough the filters.Consider changing
isImageMissingto accept the context and use itsdataDir, and pass that through from the filters. For example:-function isImageMissing(imageSource: string): boolean { - return !fs.existsSync(path.join('data', 'illustrations', imageSource)); -} +function isImageMissing(imageSource: string, context: ConvertBookContext): boolean { + return !fs.existsSync(path.join(context.dataDir, 'illustrations', imageSource)); +}And update the call sites:
- if (isImageMissing(imageSource)) { + if (isImageMissing(imageSource, _ctx)) { ... - if (isImageMissing(fileName)) { + if (isImageMissing(fileName, context)) {This keeps the logic correct for non-default data directories and aligns with the new context plumbing.
Also applies to: 234-257
src/lib/data/catalogData.ts (1)
37-61: Clarify behavior when catalog fetch/parse fails (result may beundefined)If the fetch fails,
response.okis false, orresponse.json()throws, the.catchwill log the error butresultwill remainundefined, even though the function is typed to returnPromise<CatalogData>.Consider making the failure mode explicit, for example:
export async function loadCatalog(docSet: string): Promise<CatalogData> { let result: CatalogData; @@ - await fetchFn(entry) + await fetchFn(entry) .then((response) => { if (!response.ok) { throw new Error( `Could not get catalog for ${docSet} (Response ${response.status}: ${response.statusText})` ); } return response.json(); }) .then((data) => { result = data; }) - .catch((error) => { - console.error(`Could not get catalog for ${docSet}:`, error); - }); - return result; + .catch((error) => { + console.error(`Could not get catalog for ${docSet}:`, error); + throw error; + }); + return result!;or alternatively adjust the return type to allow
undefinedand handle that at call sites. Either way, callers shouldn’t unexpectedly receiveundefinedwhereCatalogDatais promised.
♻️ Duplicate comments (4)
src/lib/video/index.ts (2)
213-216: Handle missing thumbnail entries more defensively (optional)If
video.thumbnailever doesn’t correspond to a key inthumbnails, the CSS will end up withurl('undefined'). That won’t throw, but it does produce a broken background image. A small guard keeps this resilient:- videoContainerDiv.style.setProperty( - 'background-image', - `url('${thumbnails['./' + video.thumbnail]}')` - ); + const thumbnailUrl = thumbnails[`./${video.thumbnail}`] ?? ''; + if (thumbnailUrl) { + videoContainerDiv.style.setProperty( + 'background-image', + `url('${thumbnailUrl}')` + ); + }This still assumes your asset pipeline is normally correct, but degrades more gracefully if it’s not.
223-227: Guard against missing video asset map entries when wiringonclick(optional)If
videos[\./${video.filename}`]is missing, the current code will generateplayVideoFile('ID', 'undefined')`, which won’t crash but will silently fail to play anything. You can cheaply guard and fall back to the online URL:- if (sourceType === 'assets') { - videoLink.setAttribute( - 'onclick', - `playVideoFile('${id}', '${videos[`./${video.filename}`]}'); return false;` - ); - } else { - videoLink.setAttribute( - 'onclick', - "playOnlineVideo('" + id + "', '" + videoUrl + "'); return false;" - ); - } + if (sourceType === 'assets') { + const videoSrc = videos[`./${video.filename}`]; + if (videoSrc) { + videoLink.setAttribute( + 'onclick', + `playVideoFile('${id}', '${videoSrc}'); return false;` + ); + } else { + // Fallback to online URL if local asset is missing + videoLink.setAttribute( + 'onclick', + `playOnlineVideo('${id}', '${videoUrl}'); return false;` + ); + } + } else { + videoLink.setAttribute( + 'onclick', + `playOnlineVideo('${id}', '${videoUrl}'); return false;` + ); + }That way, an occasional mismatch between config and generated assets doesn’t break the UI interaction.
src/routes/quiz/[collection]/[id]/+page.svelte (1)
21-37: Defensively handle missing audio assets in glob maps (and avoid.replaceonundefined)The new glob-based audio loading is good, but a missing entry in
quizAssetsorclipscan cause failures, especially here:function getQuizAssetAudio(file) { return staticAssets ? 'assets/' + file : quizAssets[`./${file}`].replace(/^\//, ''); }If
quizAssets[\./${file}`]is undefined (bad filename, asset not generated, etc.), this will throw a runtime error due to.replaceonundefined`. Past reviews on this file have already flagged similar issues for glob maps.You can harden this and also improve fallback behavior for
clips:const staticAssets = compareVersions(config.programVersion, '12.0') < 0; function getQuizAssetAudio(file) { - return staticAssets ? 'assets/' + file : quizAssets[`./${file}`].replace(/^\//, ''); + if (staticAssets) { + return `assets/${file}`; + } + + const key = `./${file}`; + const url = quizAssets[key]; + if (!url) { + console.warn(`Quiz audio asset not found for key ${key}, falling back to /assets/${file}`); + return `assets/${file}`; + } + + return url.replace(/^\//, ''); } function getAnswerAudio(quiz, correct) { let sound; if (correct) { - sound = quiz.rightAnswerAudio - ? clips[`./${getRandomAudio(quiz.rightAnswerAudio)}`] - : getQuizAssetAudio('quiz-right-answer.mp3'); + sound = quiz.rightAnswerAudio + ? clips[`./${getRandomAudio(quiz.rightAnswerAudio)}`] ?? + getQuizAssetAudio('quiz-right-answer.mp3') + : getQuizAssetAudio('quiz-right-answer.mp3'); } else { - sound = quiz.wrongAnswerAudio - ? clips[`./${getRandomAudio(quiz.wrongAnswerAudio)}`] - : getQuizAssetAudio('quiz-wrong-answer.mp3'); + sound = quiz.wrongAnswerAudio + ? clips[`./${getRandomAudio(quiz.wrongAnswerAudio)}`] ?? + getQuizAssetAudio('quiz-wrong-answer.mp3') + : getQuizAssetAudio('quiz-wrong-answer.mp3'); } return sound; }You might also want similar null-safe access in the explanation/question/answer audio paths (lines 245, 255, 288, 306) to avoid silent failures when a clip entry is missing.
Also applies to: 87-91, 221-231, 235-246, 255-256, 288-289, 306-307
convert/fileUtils.ts (1)
5-18: Add error handling around synchronous file writes to avoid brittle buildsThe hashing helpers look good, but
createHashedFileandcreateHashedFileFromContentsstill callcopyFileSync/writeFileSyncwithout guarding against IO errors (permissions, disk full, missing parent directories, etc.). A failure here will hard-crash the conversion step with a less-informative stack trace.You can wrap the writes similarly to earlier review guidance:
export function createHashedFile(dataDir: string, src: string, verbose: number, destPrefix = '') { const fullPath = join(dataDir, src); const hashedPath = join(destPrefix, getHashedName(dataDir, src)); const dest = join('static', hashedPath); - if (hashedPath && !existsSync(dest)) { - copyFileSync(fullPath, dest); - if (verbose) console.log(`converted ${src} to ${dest}`); - } else if (verbose) { + if (hashedPath && !existsSync(dest)) { + try { + copyFileSync(fullPath, dest); + if (verbose) console.log(`converted ${src} to ${dest}`); + } catch (error: any) { + console.error(`Failed to copy ${fullPath} to ${dest}:`, error); + throw error; + } + } else if (verbose) { console.log(`skipping ${dest}`); } return hashedPath; } export function createHashedFileFromContents( contents: string, src: string, verbose: number, destPrefix = '' ) { const hashedPath = join(destPrefix, getHashedNameFromContents(contents, src)); const dest = join('static', hashedPath); - if (!existsSync(dest)) { - writeFileSync(dest, contents); - if (verbose) console.log(`converted ${src} to ${dest}`); - } else if (verbose) { + if (!existsSync(dest)) { + try { + writeFileSync(dest, contents); + if (verbose) console.log(`converted ${src} to ${dest}`); + } catch (error: any) { + console.error(`Failed to write hashed contents for ${src} to ${dest}:`, error); + throw error; + } + } else if (verbose) { console.log(`skipping ${dest}`); } return hashedPath; }This keeps the build deterministic and surfaces IO problems clearly.
Also applies to: 31-45, 47-64
🧹 Nitpick comments (5)
src/lib/video/index.ts (1)
4-15: Glob-based asset maps look consistent with the hashed-assets approachUsing
import.meta.glob('./*', { import: 'default', eager: true, base: '/src/gen-assets/…' })to buildthumbnails/videosmaps is a solid way to resolve hashed assets at build time. TheRecord<string, string>typing matches the expected URL strings from default asset imports, so this looks good as-is. If the number of generated assets grows large, you might later consider narrowing the glob pattern or droppingeager: trueto keep bundles smaller, but that’s an optional perf tweak, not a blocker.convert/convertBadges.ts (1)
11-21: Make badge output directory creation recursive for robustness
mkdirSync(dstBadgeDir)will throw ifsrc/gen-assetsdoesn’t exist yet andConvertBadgeshappens to run before other generators create the parent. Usingrecursive: truematches the pattern used elsewhere (e.g., tab icon setup) and makes this safer.- if (!existsSync(dstBadgeDir)) { - mkdirSync(dstBadgeDir); - } + if (!existsSync(dstBadgeDir)) { + mkdirSync(dstBadgeDir, { recursive: true }); + }convert/convertManifest.ts (1)
65-71: Confirm howmanifestUrl.jsonis consumed and consider old-manifest cleanup.
createHashedFileFromContentscorrectly writes a content-hashedstatic/manifest.<hash>.jsonandmanifestUrl.jsonrecords only the hashed filename, not the/static/prefix. Please confirm the runtime code that readsmanifestUrl.jsonis prepending the correct static base (andBUILD_BASE_PATHif applicable), and consider whether you want a lightweight cleanup step to prune oldmanifest.*.jsonfiles to keepstatic/from accumulating stale manifests over time.convert/convertBooks.ts (1)
641-652: Consider hashing HTML after filters so filenames truly track final content.In
convertHtmlBookyou currently:let content = fs.readFileSync(srcFile, 'utf-8'); const before = getHashedNameFromContents(content, book.file); content = applyFilters(content, htmlFilterFunctions, context.bcId, book.id, context); files.push({ path: path.join('static', 'collections', context.bcId, before), content });This means the filename hash is based on the pre-filter content, while the bytes written may change as filters evolve (e.g., updated
<img>rewriting). That weakens cache-busting, since a code change that alters the HTML could leave the filename unchanged.If feasible, consider hashing the final
contentinstead:- const before = getHashedNameFromContents(content, book.file); - content = applyFilters(content, htmlFilterFunctions, context.bcId, book.id, context); + content = applyFilters(content, htmlFilterFunctions, context.bcId, book.id, context); + const before = getHashedNameFromContents(content, book.file);If you intentionally want the hash to depend only on the original source file (to keep paths stable across filter tweaks), it’d be worth adding a brief comment to document that tradeoff.
convert/convertStyles.ts (1)
21-24: Avoid usingpath.joinfor URL segments in CSS font pathsThe new font handling correctly hashes and relocates fonts, but using
path.jointo buildfontPath(and then writing it back into CSS) can introduce backslashes on Windows (fonts\MyFont.woff2), which won’t match/fonts/MyFont.woff2in CSS and can break the replacement.You can keep filesystem joins for disk paths but use POSIX-style joining (or manual concatenation) for URL segments:
- const fontName = line.split('/fonts/')[1].replace(/("|')\).*$/, ''); - const fontPath = path.join('fonts', fontName); + const fontName = line.split('/fonts/')[1].replace(/("|')\).*$/, ''); + const fontPathFs = path.join('fonts', fontName); + const fontPathUrl = `fonts/${fontName}`; let finalPath = fontPath; - if (existsSync(path.join(dataDir, fontPath))) { - finalPath = createHashedFile(dataDir, fontPath, verbose); + if (existsSync(path.join(dataDir, fontPathFs))) { + finalPath = createHashedFile(dataDir, fontPathFs, verbose); } else if (existsSync(path.join(dataDir, 'cloud', fontName))) { finalPath = createHashedFile( path.join(dataDir, 'cloud'), fontName, verbose, 'fonts' ); } else { console.warn( - `${srcFile}: Could not locate ${path.join(dataDir, fontPath)}` + `${srcFile}: Could not locate ${path.join(dataDir, fontPathFs)}` ); } - line = line.replace('/' + fontPath, '/' + finalPath); + line = line.replace('/' + fontPathUrl, '/' + finalPath);This keeps CSS URLs consistent across platforms while still using
path.joinfor filesystem operations.Also applies to: 79-101
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
.github/workflows/main.yml(1 hunks).gitignore(1 hunks)convert/convertAbout.ts(1 hunks)convert/convertBadges.ts(1 hunks)convert/convertBooks.ts(18 hunks)convert/convertManifest.ts(4 hunks)convert/convertMarkdown.ts(1 hunks)convert/convertMedia.ts(4 hunks)convert/convertPlans.ts(1 hunks)convert/convertReverseIndex.ts(1 hunks)convert/convertSQLite.ts(2 hunks)convert/convertStyles.ts(3 hunks)convert/fileUtils.ts(1 hunks)data-sandbox/index.ts(1 hunks)package.json(1 hunks)src/lib/components/BookTabs.svelte(3 hunks)src/lib/components/BottomNavigationBar.svelte(2 hunks)src/lib/components/CollectionList.svelte(2 hunks)src/lib/components/HtmlBookView.svelte(1 hunks)src/lib/components/ScriptureViewSofria.svelte(5 hunks)src/lib/components/Sidebar.svelte(6 hunks)src/lib/data/audio.ts(3 hunks)src/lib/data/catalogData.ts(2 hunks)src/lib/data/plansData.ts(2 hunks)src/lib/data/scripture.js(2 hunks)src/lib/data/stores/lexicon.ts(1 hunks)src/lib/scripts/milestoneLinks.ts(2 hunks)src/lib/video/index.ts(3 hunks)src/routes/+layout.svelte(3 hunks)src/routes/lexicon/+page.svelte(2 hunks)src/routes/lexicon/+page.ts(2 hunks)src/routes/quiz/[collection]/[id]/+page.js(2 hunks)src/routes/quiz/[collection]/[id]/+page.svelte(10 hunks)src/routes/share/+page.js(1 hunks)src/routes/share/+page.svelte(2 hunks)src/routes/text/+page.svelte(3 hunks)svelte.config.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (16)
- src/routes/lexicon/+page.svelte
- convert/convertSQLite.ts
- src/routes/+layout.svelte
- .gitignore
- convert/convertPlans.ts
- src/routes/share/+page.svelte
- src/lib/components/ScriptureViewSofria.svelte
- package.json
- src/lib/components/HtmlBookView.svelte
- src/lib/scripts/milestoneLinks.ts
- convert/convertAbout.ts
- src/lib/data/plansData.ts
- src/routes/lexicon/+page.ts
- src/lib/components/BottomNavigationBar.svelte
- src/routes/share/+page.js
- src/lib/components/CollectionList.svelte
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-05-07T13:16:04.481Z
Learnt from: chrisvire
Repo: sillsdev/appbuilder-pwa PR: 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:
convert/convertMarkdown.tsconvert/convertBooks.ts
📚 Learning: 2025-06-23T20:54:56.765Z
Learnt from: judah-sotomayor
Repo: sillsdev/appbuilder-pwa PR: 850
File: src/routes/notes/edit/[noteid]/+page.js:0-0
Timestamp: 2025-06-23T20:54:56.765Z
Learning: In SvelteKit projects, verify the specific error handling pattern used in the codebase, as there may be variations from the standard `throw error(status, message)` syntax shown in official documentation.
Applied to files:
convert/fileUtils.ts
📚 Learning: 2025-05-07T13:16:04.481Z
Learnt from: chrisvire
Repo: sillsdev/appbuilder-pwa PR: 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/convertBooks.ts
📚 Learning: 2025-07-04T18:51:27.821Z
Learnt from: chrisvire
Repo: sillsdev/appbuilder-pwa PR: 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/quiz/[collection]/[id]/+page.svelte
📚 Learning: 2025-07-04T18:51:27.821Z
Learnt from: chrisvire
Repo: sillsdev/appbuilder-pwa PR: 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/quiz/[collection]/[id]/+page.svelte
🧬 Code graph analysis (7)
convert/convertStyles.ts (1)
convert/fileUtils.ts (1)
createHashedFile(31-45)
src/lib/data/stores/lexicon.ts (1)
src/routes/quiz/[collection]/[id]/+page.js (1)
response(38-38)
src/lib/data/audio.ts (1)
src/lib/scripts/stringUtils.ts (1)
pathJoin(200-214)
src/lib/data/scripture.js (1)
src/lib/data/stores/scripture.js (2)
docSet(110-110)docSet(110-110)
convert/convertBooks.ts (2)
convert/fileUtils.ts (2)
createHashedFile(31-45)getHashedNameFromContents(20-29)src/routes/quiz/[collection]/[id]/+page.js (4)
collection(14-14)quizzes(4-9)book(17-17)id(13-13)
src/lib/data/catalogData.ts (2)
src/lib/data/scripture.js (3)
key(33-33)docSet(20-20)entry(34-34)src/lib/data/stores/scripture.js (3)
docSet(110-110)docSet(110-110)entry(48-48)
convert/convertManifest.ts (1)
convert/fileUtils.ts (2)
createHashedFile(31-45)createHashedFileFromContents(47-64)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint
- GitHub Check: test
🔇 Additional comments (24)
.github/workflows/main.yml (2)
1-216: Good: Previous checkout@v2 → v4 concern has been resolved.All three checkout actions (lines 14, 129, 169) now use
actions/checkout@v4, addressing the deprecation concern from the earlier review.
64-74: Docker environment unavailable for container inspection.The sandbox environment does not have Docker available, preventing verification of the actual container contents and image size. However, code inspection confirms the concern is valid: line 71 copies the entire root filesystem (
/) into$HOME/app-builders/, yet line 74 shows onlysab.shis actually used, and line 73 operates only on*.shfiles.To resolve this, manually verify:
- Whether
ghcr.io/sillsdev/app-builders:latestcontains significant non-tool files that could be excluded- Whether the
.shscripts depend on files outside their own directory (e.g., libraries, config in parent directories)- Whether extracting only specific files (e.g.,
sab.sh,dab.sh) would break runtime dependenciessrc/lib/data/stores/lexicon.ts (3)
48-59: Confirmimport.meta.globkey format withbaseoptionBecause you’re passing a
baseoption intoimport.meta.glob, the exact shape of the returned keys can differ by Vite version/config (e.g.,./data.sqlitevs/src/gen-assets/data.sqlite). Since later lookups use./sql-wasm.wasmand./data.sqliteas keys, please double‑check that the keys produced at build time actually match these literals; otherwise initialization will always hit the “asset not found” error even when the files exist. A quick runtimeconsole.log(Object.keys(sqliteUrl), Object.keys(wasmUrl))in dev or a small assertion test would make this explicit.
66-72: Explicit wasm asset check closes earlier vague TypeError riskThe
wasmKey/wasmEntrypattern with a clear error message if the entry is missing makes the failure mode much more descriptive and avoids the previous.defaultaccess TypeError. This is a solid improvement to initialization robustness.
79-84: SQLite asset resolution is now validated before fetchThe analogous
dbKey/dbEntryguard for the SQLite file mirrors the wasm handling and ensures missing or misconfigured assets fail fast with a clear error instead of an obscure runtime failure. This keeps the lexicon DB initialization behavior predictable.src/routes/text/+page.svelte (3)
498-498: LGTM: CSS variable approach aligns with hashed asset strategy.The dynamic CSS variable injection correctly implements the hashed asset pattern described in the PR objectives. The approach is consistent with similar implementations in other components.
Note: Ensure the border asset verification from the previous comment passes to confirm
borders['./border.png']resolves correctly.
587-587: LGTM: Correct CSS variable usage.The update to use
var(--borderImageSource)correctly references the CSS custom property set on line 498, completing the dynamic border asset implementation.
64-68: The border asset is build-time generated and follows the established pattern; however, verify input data contains the required border file and consider error handling.The
src/gen-assets/borders/directory is created during the build process byconvert/convertMedia.ts, which copies from input data. This is the intended design and follows the same pattern used for images, videos, audio, and other assets throughout the codebase.However, the concern about missing error handling remains valid: if the input data lacks
borders/border.png, thenborders['./border.png']will beundefined, resulting in invalid CSS (url(undefined)). While the code follows codebase conventions, you should:
- Verify your input data contains
borders/border.png- Consider adding a fallback:
const borderUrl = borders['./border.png'] || '';src/lib/components/BookTabs.svelte (1)
18-22: Glob‑based tab icon loading looks correctUsing
import.meta.glob('./*', { import: 'default', eager: true, base: '/src/gen-assets/icons/tabs' })and indexing withtabIcons[\./${getImageName(...)}`]` aligns with the new generated‑assets layout and should resolve the correct hashed icon URLs at build time. No further issues from this change.Also applies to: 52-52, 68-68
svelte.config.js (1)
22-25: New$staticand$assetsaliases align with asset layoutThe aliases point to the expected folders and match how assets are being imported elsewhere (e.g.,
$assets/images/...). This should make the new gen‑assets pipeline easier to use.convert/convertMarkdown.ts (1)
10-17: Signature change to include context parameter is safeAdding
_ctx: unknowntoconvertMarkdownsToMilestonesextends the API for context passing without changing the conversion behavior, and the underscore naming avoids unused‑parameter noise. The link/USFM logic remains untouched, which is correct given that USFM attribute encoding is handled elsewhere.convert/convertMedia.ts (1)
63-69: Media copy/removal retargeting tosrc/gen-assetsis consistentSwitching the rimraf targets, clone destinations, and legacy tab‑icon path to
src/gen-assets/...keeps this task aligned with the rest of the asset pipeline while preserving existing behavior (including recursive directory copies and logging). The added TODOs on rarely‑used directories are also fine as internal notes.Also applies to: 111-113, 121-123, 141-153
src/lib/data/scripture.js (1)
8-13: PKF asset globbing and guarded lookup are solidPreloading PKF files via
import.meta.glob('./*.pkf', { import: 'default', eager: true, base: '/src/gen-assets/collections', query: '?url' })and havinggetDocSetUrlresolve./${docSet}.pkfwith an explicit error on missing entries makes the docSet URL resolution both build‑time friendly and safer than the prior unchecked access. This integrates well with the hashed gen‑assets setup.Also applies to: 32-38
src/lib/components/Sidebar.svelte (1)
6-8: New menu icon loading andimageSrcSetimplementation look correctUsing
menuIcons = import.meta.glob('./*', { import: 'default', eager: true, base: '/src/gen-assets/icons/menu-items' })and then buildingsrcsetentries as${url} ${multiplier}x(with a separate density suffix and a filtered list) fixes the earlier glob‑key issue and should yield valid responsive iconsrcsets. The<img>fallback sourcing frommenuIcons[\./${item.images[0].file}`] ?? ''` is also consistent.Also applies to: 43-47, 81-96, 272-276
convert/convertManifest.ts (1)
14-21: Icon hashing logic is now robust and fail-fast.The switch to a regex-based
"src"match plus an explicitexistsSynccheck andErroron missing icon fixes the earlier fragile parsing and silent failure mode. This should make manifest processing much more predictable and surface missing-icon issues early. No further changes needed here.Also applies to: 30-49
src/lib/data/audio.ts (1)
19-29: Glob-based asset loading and existence checks look correct; just verify key alignment.Using
import.meta.glob('./*', { import: 'default', eager: true, base: '/src/gen-assets/audio' })and then resolvingaudioKey = \./${audio.filename}`/timingKey = `./${audio.timingFile}`is a good fit, and the explicit checks with clearErrormessages address the earlier risk of undefined.defaultaccess. Please double-check at build time that the glob actually emits keys of the form./` so these lookups never miss due to a path mismatch (e.g., stray subdirectories or leading slashes).Also applies to: 440-457
convert/convertBooks.ts (4)
118-158: JMP encoding is correctly centralized inconvertBooks.Having
encodeJmpLinkshere in the books conversion pipeline (instead of inconvertMarkdown.ts) keeps all USFM attribute encoding in one place and avoids double-encoding ofhref/titleattributes. This matches the earlier workflow guidance about where attribute encoding should happen.Based on learnings
234-257: Hashed illustration handling and supporting directories look consistent.
updateImgTagsnow filters out missing images and rewrites surviving<img>tags to usecreateHashedFilewithcontext.dataDirandillustrations/${fileName}, emitting<img src="${base}/${imagePath}">. Combined with thestatic/illustrationsandstatic/collections/<collection.id>mkdirs when HTML books are present, this wiring is coherent with the rest of the static asset strategy. OnceisImageMissingis updated to honorcontext.dataDir, this pipeline should be solid.Also applies to: 409-420, 459-468
332-364: Filter function signature/context wiring is consistent; just confirmconvertMarkdownsToMilestones.Defining
FilterFunctionas(text, bcId, bookId, context)and updating bothusfmFilterFunctionsandhtmlFilterFunctionsto passcontextthroughapplyFiltersgives a clear, uniform hook for context-dependent transforms (like hashed assets). All the local filters in this file now match that shape; please just confirm thatconvertMarkdownsToMilestonesinconvertMarkdown.tsalso acceptscontext: ConvertBookContextas its fourth parameter so everything stays consistent at the type level.Also applies to: 784-799
409-420: Collection assets now consistently live undersrc/gen-assets/collections.The updated paths for quizzes (
src/gen-assets/collections/<bcId>/quizzes/*.json), frozen pkf archives (src/gen-assets/collections/<docSet>.pkf), catalog entries, andcollections/index.jsonall line up with the new generated-assets convention. This matches the import patterns elsewhere (e.g., quiz routes usingbase: '/src/gen-assets/collections'), so the relocation fromstatic/collectionslooks coherent.Also applies to: 533-555, 571-581
data-sandbox/index.ts (1)
22-27: Sandbox now reads pkf data fromsrc/gen-assets/collections, matching converters.Pointing
staticDiratsrc/gen-assets/collectionskeeps the sandbox loader in sync with whereConvertBooksnow writes.pkfandindex.json. This preserves the existing behavior with the new asset layout.convert/convertReverseIndex.ts (1)
33-36: Reversal assets relocated undersrc/gen-assets/reversalin line with other generated assets.Updating
outputDirtosrc/gen-assets/reversal/<language>keeps reversal index files consistent with the new generated-assets structure used elsewhere (collections, badges, etc.). The rest of the logic is unchanged and should continue to work as before.src/lib/data/catalogData.ts (1)
1-6: Nice defensive check on catalog glob lookupThe new
key/entryguard before fetching the catalog JSON resolves the earlier risk of accessing a missing glob entry and gives a clear error if the asset is absent. This aligns well with the new asset-loading approach.Also applies to: 39-45
src/routes/quiz/[collection]/[id]/+page.svelte (1)
4-5: ****The original review comment is incorrect. In SvelteKit 2.x / Svelte 5,
pageimported from$app/stateis a read-only reactive object (not a Svelte store), so the$prefix is not needed. Access params directly aspage.paramsrather than$page.params.The code in this PR correctly uses
page.params.collectionthroughout, which is the proper pattern. The review's assertion that this pattern "will likely not work" is incorrect—ignore this comment and proceed with the current implementation.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.gitignore(1 hunks)convert/convertStyles.ts(3 hunks)package.json(1 hunks)src/routes/quiz/[collection]/[id]/+page.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- src/routes/quiz/[collection]/[id]/+page.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: lint
🔇 Additional comments (2)
.gitignore (2)
18-18: Good pattern choice for hashed manifest files.The wildcard pattern
static/manifest*.jsoncorrectly supports hashed versions of the manifest file (e.g.,manifest-abc123.json) as part of the cache-busting strategy, while being more maintainable than hardcoding individual filenames.
25-25: Appropriate addition for generated assets directory.Adding
src/gen-assetsto gitignore aligns with the PR's migration of generated assets and is correct since generated content should not be committed.Verify that
src/gen-assetscontains only generated files and no legitimate source code.
Fixes #865
This pull request implement hash-based access for asset files that are designed to be updated.
This includes most images, language resources, and other assets that do not need direct access.
In most cases vite was able to handle this, with the files included via glob import.
There were a few that we manually had to hash instead, notably icons and images included in static files (i.e. manifest.json, HTML books).
Summary by CodeRabbit
Build & Performance
CI / Workflows
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.