Feature/integrate eslint #869
Conversation
Add a proper eslint configuration using extensions for svelte, typescript, and prettier. This should ensure eslint does not mess with prettier's settings while still enabling high-quality linting. Remove the outdated .eslintrc.cjs in favor of eslint.config.js.
Mainly converting let to const, and var to let.
This change corrects video/index.ts to comply with `no-case-declarations`. `getEmbeddedVideoUrl` was using `let` and `const` declarations inside cases without an appropriate block scope on each case. As a result of this sloppiness, reused variable names were falling through suspiciously. This change adds a block scope to each case, and makes any local `pattern` and `match` variables const.
Satisfy eslint by breaking up the expect on bkk.chapters in convert/tests/sab/convertConfigSAB.test.ts to a separate 0-check and undefined-check. Jest also notes that using toBeDefined() is better than a raw undefined check. https://jestjs.io/docs/expect#tobedefined
Tell eslint to ignore the require import rule for tailwind's configuration. Require seems to be a standard import mechanism for tailwind: https://v2.tailwindcss.com/docs/configuration#plugins
Function wasn't being called, lol.
Function wasn't being called, lol.
`dab-search-worker.ts`'s `searchDictionary()` did not assign to `results` after it was set. Declare `results` as `const`.
Instead of calling hasOwnProperty off of an object directly, use `Object.property` to make the call. This avoids potential subtle problems, explained in this link: https://eslint.org/docs/latest/rules/no-prototype-builtins#rule-details
Doesn't like empty blocks, as they may be confusing.
Replace empty object types (`{}`) with `object`.
This prevents random values from being inserted into the interface.
Could be a BREAKING CHANGE!
Previously used `var`.
Other OS's had blocks, not sure why MacOS missed out.
BottomNavigationBar's `handleclick()` used a `let` binding inside a case. Refactor to add a block around the case.
Sidebar was using a bash-style `and` shortcut instead of a proper `if`. Fix it.
Move to a newer version of eslint-plugin-svelte in order to take advantage of better rules and a better config syntax. Disable `svelte/no-dupe-style-properties` as it is very noisy.
Don't warn about missing keys in each-blocks.
Remove brackets surrounding literal values.
This rule triggers a warning on statements that eslint does not use, but the svelte LSP does use.
Several `goto` statements are used without reference to `{base}` because
they use a parameter instead of a literal url.
Silence `eslint` warnings on those in particular.
A link element attempted to set the font-family style attribute, but misspelled it as "font-famly". Correct spelling.
WalkthroughThis update applies a series of code quality improvements and dependency updates across the project. It refactors variable declarations for immutability, clarifies test assertions, enhances ESLint configuration, updates type annotations, and corrects minor typos and style bindings. Several dependencies are upgraded, and ESLint configuration files are restructured. Additionally, many single-line conditional statements are expanded into block statements with braces for clarity and consistency. Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant ESLint
participant Svelte
participant TypeScript
Developer->>ESLint: Runs lint with new config
ESLint->>TypeScript: Uses parser and rules for TS files
ESLint->>Svelte: Uses Svelte plugin for .svelte files
ESLint->>ESLint: Applies Prettier and custom rule overrides
ESLint-->>Developer: Reports lint results
Estimated code review effort2 (~15 minutes) Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
🧠 Learnings (1)src/routes/about/+page.svelte (6)Learnt from: judah-sotomayor Learnt from: chrisvire Learnt from: judah-sotomayor Learnt from: chrisvire Learnt from: judah-sotomayor Learnt from: judah-sotomayor ✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🧠 Learnings (1)src/routes/about/+page.svelte (6)Learnt from: judah-sotomayor Learnt from: chrisvire Learnt from: judah-sotomayor Learnt from: chrisvire Learnt from: judah-sotomayor Learnt from: judah-sotomayor 🔇 Additional comments (1)
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/lib/data/stores/theme.js (1)
57-65: UseObject.hasOwnfor terser & cleaner checksGreat move switching away from a direct
hasOwnPropertycall.
Since Node 16 / modern browsers you can shorten the pattern toObject.hasOwn(obj, key), which is more readable and avoids touching the prototype chain entirely.- if (Object.prototype.hasOwnProperty.call(newProperties, 'background-color')) { + if (Object.hasOwn(newProperties, 'background-color')) { ... - if (Object.prototype.hasOwnProperty.call(newProperties, 'color')) { + if (Object.hasOwn(newProperties, 'color')) {No behavioural change, just syntactic sugar.
src/routes/text/+page.svelte (1)
483-485: Consider using theme-driven color instead of hard-coding"gray"Hard-coding the chevron color to the literal string
"gray"makes it impossible for themes (e.g. dark mode, branded color palettes) to override the arrow colour. The rest of the component already pulls colours from$themeColorsfor run-time flexibility.-<ChevronIcon size={36} color="gray" deg={$direction === 'ltr' ? 180 : 0} /> +<ChevronIcon + size={36} + color={$themeColors['IconChevron'] ?? 'gray'} + deg={$direction === 'ltr' ? 180 : 0} />If a design-system token is not yet available, consider adding one so the navigation arrows follow the active theme automatically.
Also applies to: 517-518
src/lib/components/FontList.svelte (1)
26-36: Duplicatefont-familyinline style – keep only one declaration
<li>already setsstyle:font-family={font}; repeating it on the nested<a>is unnecessary and slightly increases bundle size.- <a - on:click={() => handleClick(font)} - style:background-color={font === selectedFont - ? $themeColors['ButtonSelectedColor'] - : ''} - style:color={$monoIconColor} - style:font-family={font} - role="button" - > + <a + on:click={() => handleClick(font)} + style:background-color={font === selectedFont + ? $themeColors['ButtonSelectedColor'] + : ''} + style:color={$monoIconColor} + role="button" + >Trimming duplicate styles keeps the DOM cleaner and avoids accidental drift if one instance changes.
src/lib/components/TextSelectionToolbar.svelte (1)
188-191: Hard-coded"white"may clash with dark themesThe erase-highlight button uses a literal white background which becomes invisible on light themes and visually jarring on dark ones. Prefer re-using an existing theme colour or a semantic token.
-style:background-color="white" +style:background-color={$themeColors['HighlighterPenErase'] ?? '#ffffff'}Adding a dedicated erase colour to the theme palette keeps the UI consistent across themes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (34)
.eslintrc.cjs(0 hunks)convert/convertBooks.ts(2 hunks)convert/convertConfig.ts(3 hunks)convert/convertReverseIndex.ts(1 hunks)convert/tests/sab/convertConfigSAB.test.ts(1 hunks)eslint.config.js(2 hunks)example/index.ts(1 hunks)package.json(3 hunks)src/app.d.ts(0 hunks)src/lib/components/BottomNavigationBar.svelte(1 hunks)src/lib/components/FontList.svelte(1 hunks)src/lib/components/HistoryCard.svelte(1 hunks)src/lib/components/Sidebar.svelte(2 hunks)src/lib/components/TextSelectionToolbar.svelte(1 hunks)src/lib/data/analytics.ts(1 hunks)src/lib/data/annotation-share.ts(1 hunks)src/lib/data/audio.ts(3 hunks)src/lib/data/catalogData.ts(1 hunks)src/lib/data/highlights.ts(2 hunks)src/lib/data/history.ts(2 hunks)src/lib/data/language.ts(3 hunks)src/lib/data/mrucache.ts(1 hunks)src/lib/data/navigation.test.ts(3 hunks)src/lib/data/scripture.js(1 hunks)src/lib/data/stores/store-types.js(1 hunks)src/lib/data/stores/theme.js(1 hunks)src/lib/navigate/index.ts(2 hunks)src/lib/search-worker/dab-search-worker.ts(1 hunks)src/lib/search/data/test/search-config-repository-impl.test.ts(1 hunks)src/lib/utils/worker-messenger/messenger.test.ts(1 hunks)src/lib/video/index.ts(4 hunks)src/routes/contents/[id]/+page.svelte(1 hunks)src/routes/lexicon/+page.ts(2 hunks)src/routes/text/+page.svelte(2 hunks)
💤 Files with no reviewable changes (2)
- src/app.d.ts
- .eslintrc.cjs
🧰 Additional context used
🧠 Learnings (11)
src/lib/components/FontList.svelte (2)
Learnt from: judah-sotomayor
PR: sillsdev/appbuilder-pwa#820
File: src/lib/components/Modal.svelte:28-34
Timestamp: 2025-06-05T17:25:45.457Z
Learning: In the sillsdev/appbuilder-pwa repository, the Modal.svelte component has an existing bug where `{{ id }.showModal()}` syntax was used instead of calling the exported `showModal` function directly. This bug predates Svelte 5 migration efforts.
Learnt from: judah-sotomayor
PR: sillsdev/appbuilder-pwa#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.
src/lib/components/HistoryCard.svelte (3)
Learnt from: judah-sotomayor
PR: sillsdev/appbuilder-pwa#840
File: src/lib/components/NoteDialog.svelte:68-70
Timestamp: 2025-06-16T18:48:21.767Z
Learning: In Svelte 5, event handler syntax changed from Svelte 4. Use `onclick={handler}` instead of `on:click={handler}`. Event modifiers like `|preventDefault` are no longer supported and must be handled inside the handler function. Only one handler per event is allowed per element.
Learnt from: judah-sotomayor
PR: sillsdev/appbuilder-pwa#840
File: src/lib/components/NoteDialog.svelte:68-70
Timestamp: 2025-06-16T18:48:21.767Z
Learning: In Svelte 5, the event handler syntax has changed from Svelte 4. The `onclick` attribute is valid in Svelte 5, unlike Svelte 4 where `on:click` was required.
Learnt from: judah-sotomayor
PR: sillsdev/appbuilder-pwa#820
File: src/lib/components/Modal.svelte:28-34
Timestamp: 2025-06-05T17:25:45.457Z
Learning: In the sillsdev/appbuilder-pwa repository, the Modal.svelte component has an existing bug where `{{ id }.showModal()}` syntax was used instead of calling the exported `showModal` function directly. This bug predates Svelte 5 migration efforts.
src/routes/contents/[id]/+page.svelte (2)
Learnt from: judah-sotomayor
PR: sillsdev/appbuilder-pwa#820
File: src/lib/components/Modal.svelte:28-34
Timestamp: 2025-06-05T17:25:45.457Z
Learning: In the sillsdev/appbuilder-pwa repository, the Modal.svelte component has an existing bug where `{{ id }.showModal()}` syntax was used instead of calling the exported `showModal` function directly. This bug predates Svelte 5 migration efforts.
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`.
src/routes/text/+page.svelte (3)
Learnt from: judah-sotomayor
PR: sillsdev/appbuilder-pwa#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.
Learnt from: judah-sotomayor
PR: sillsdev/appbuilder-pwa#820
File: src/lib/components/Modal.svelte:28-34
Timestamp: 2025-06-05T17:25:45.457Z
Learning: In the sillsdev/appbuilder-pwa repository, the Modal.svelte component has an existing bug where `{{ id }.showModal()}` syntax was used instead of calling the exported `showModal` function directly. This bug predates Svelte 5 migration efforts.
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.
src/lib/data/scripture.js (2)
Learnt from: judah-sotomayor
PR: sillsdev/appbuilder-pwa#850
File: src/routes/notes/edit/[noteid]/+page.js:0-0
Timestamp: 2025-06-23T20:54:56.765Z
Learning: In SvelteKit, the correct syntax for throwing HTTP errors in load functions is `error(statusCode, { message: 'error message' })` without the `throw` keyword. The error function from '@sveltejs/kit' should be called directly, not thrown like a regular JavaScript Error.
Learnt from: judah-sotomayor
PR: sillsdev/appbuilder-pwa#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.
src/lib/components/BottomNavigationBar.svelte (3)
Learnt from: judah-sotomayor
PR: sillsdev/appbuilder-pwa#840
File: src/lib/components/NoteDialog.svelte:68-70
Timestamp: 2025-06-16T18:48:21.767Z
Learning: In Svelte 5, event handler syntax changed from Svelte 4. Use `onclick={handler}` instead of `on:click={handler}`. Event modifiers like `|preventDefault` are no longer supported and must be handled inside the handler function. Only one handler per event is allowed per element.
Learnt from: judah-sotomayor
PR: sillsdev/appbuilder-pwa#820
File: src/lib/components/Modal.svelte:28-34
Timestamp: 2025-06-05T17:25:45.457Z
Learning: In the sillsdev/appbuilder-pwa repository, the Modal.svelte component has an existing bug where `{{ id }.showModal()}` syntax was used instead of calling the exported `showModal` function directly. This bug predates Svelte 5 migration efforts.
Learnt from: judah-sotomayor
PR: sillsdev/appbuilder-pwa#840
File: src/lib/components/NoteDialog.svelte:68-70
Timestamp: 2025-06-16T18:48:21.767Z
Learning: In Svelte 5, the event handler syntax has changed from Svelte 4. The `onclick` attribute is valid in Svelte 5, unlike Svelte 4 where `on:click` was required.
src/lib/components/Sidebar.svelte (2)
Learnt from: judah-sotomayor
PR: sillsdev/appbuilder-pwa#840
File: src/lib/components/NoteDialog.svelte:68-70
Timestamp: 2025-06-16T18:48:21.767Z
Learning: In Svelte 5, event handler syntax changed from Svelte 4. Use `onclick={handler}` instead of `on:click={handler}`. Event modifiers like `|preventDefault` are no longer supported and must be handled inside the handler function. Only one handler per event is allowed per element.
Learnt from: judah-sotomayor
PR: sillsdev/appbuilder-pwa#820
File: src/lib/components/Modal.svelte:28-34
Timestamp: 2025-06-05T17:25:45.457Z
Learning: In the sillsdev/appbuilder-pwa repository, the Modal.svelte component has an existing bug where `{{ id }.showModal()}` syntax was used instead of calling the exported `showModal` function directly. This bug predates Svelte 5 migration efforts.
src/lib/data/stores/store-types.js (2)
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.
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`.
convert/convertBooks.ts (2)
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.
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.
src/lib/components/TextSelectionToolbar.svelte (4)
Learnt from: judah-sotomayor
PR: sillsdev/appbuilder-pwa#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.
Learnt from: judah-sotomayor
PR: sillsdev/appbuilder-pwa#840
File: src/lib/components/NoteDialog.svelte:68-70
Timestamp: 2025-06-16T18:48:21.767Z
Learning: In Svelte 5, event handler syntax changed from Svelte 4. Use `onclick={handler}` instead of `on:click={handler}`. Event modifiers like `|preventDefault` are no longer supported and must be handled inside the handler function. Only one handler per event is allowed per element.
Learnt from: judah-sotomayor
PR: sillsdev/appbuilder-pwa#840
File: src/lib/components/NoteDialog.svelte:68-70
Timestamp: 2025-06-16T18:48:21.767Z
Learning: In Svelte 5, the event handler syntax has changed from Svelte 4. The `onclick` attribute is valid in Svelte 5, unlike Svelte 4 where `on:click` was required.
Learnt from: judah-sotomayor
PR: sillsdev/appbuilder-pwa#820
File: src/lib/components/Modal.svelte:28-34
Timestamp: 2025-06-05T17:25:45.457Z
Learning: In the sillsdev/appbuilder-pwa repository, the Modal.svelte component has an existing bug where `{{ id }.showModal()}` syntax was used instead of calling the exported `showModal` function directly. This bug predates Svelte 5 migration efforts.
package.json (2)
Learnt from: judah-sotomayor
PR: sillsdev/appbuilder-pwa#840
File: src/lib/components/NoteDialog.svelte:94-95
Timestamp: 2025-06-16T18:49:02.188Z
Learning: The appbuilder-pwa project uses Svelte 5, as evidenced by "svelte": "^5" in package.json and the use of Svelte 5 runes syntax like $props() and $derived(). In Svelte 5, onclick is the preferred event handler syntax over on:click.
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.
🧬 Code Graph Analysis (3)
convert/convertReverseIndex.ts (1)
convert/Task.ts (1)
FileContent(1-4)
src/lib/search-worker/dab-search-worker.ts (1)
src/lib/data/stores/lexicon.ts (1)
initializeDatabase(48-69)
src/routes/lexicon/+page.ts (1)
src/lib/data/stores/lexicon.ts (1)
initializeDatabase(48-69)
⏰ 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 (47)
src/lib/data/stores/store-types.js (1)
15-18: Safer property-existence check looks goodReplacing
stores.hasOwnProperty(key)with the prototype-safe call eliminates edge-case bugs ifstoresever shadowshasOwnProperty. Change is correct and side-effect free.src/lib/search/data/test/search-config-repository-impl.test.ts (1)
33-33: LGTM! Critical test assertion fix.This correctly fixes the test assertion by adding parentheses to properly invoke the
toBeDefined()matcher. Without parentheses, the assertion would never execute, making the test ineffective.src/lib/utils/worker-messenger/messenger.test.ts (1)
253-253: LGTM! Consistent test assertion fix.This correctly fixes the test assertion by adding parentheses to properly invoke the
toBeDefined()matcher. This change is consistent with similar fixes across the codebase and ensures the test actually validates thatio.lastPostedis defined.src/lib/data/catalogData.ts (2)
16-16: No breaking changes detected forsequencestypeAll occurrences of
sequencesare initialized with objects or empty arrays—no primitives are ever assigned. The change from{}toobjectis safe.
24-25: CatalogData’stagsandquizzesRemain Object-Only and No Primitives FoundAll usages assign plain objects (
{ … }), object maps ({ [key: string]: string }), orany→ object, and no primitive values are ever assigned totagsorquizzes. Key findings:• sab-proskomma-tools/post-query/catalog.ts assigns
tagsas{ [key: string]: string }
• src/lib/data/navigation.test.ts uses{}literals for both properties
• convert/convertBooks.ts writes throughany, which is allowed byobjectNo breaking changes detected—type tightening to
objectis safe.src/lib/data/history.ts (1)
48-48: LGTM! Good use ofconstfor immutable variables.Converting these
letdeclarations toconstis appropriate since thehistoryvariable is only assigned once and never reassigned in both functions. This change enforces immutability and follows modern JavaScript best practices.Also applies to: 67-67
src/lib/data/annotation-share.ts (1)
20-20: LGTM! Proper use ofconstfor immutable variables.Converting these
letdeclarations toconstis correct since thetextvariable is assigned once and never reassigned in both functions. This enforces immutability and aligns with modern JavaScript best practices.Also applies to: 25-28
src/lib/data/mrucache.ts (1)
40-40: LGTM! Correct use ofconstin for-of loop.Using
constfor the destructured loop variables is appropriate since they shouldn't be reassigned within the loop body. This follows modern JavaScript best practices and prevents accidental modification of loop variables.src/lib/navigate/index.ts (1)
10-11: Disabling ESLint Rule for Hash-Based Navigation is AppropriateThe
svelte/no-navigation-without-baserule is enforced as an error in eslint.config.js to guard against missing base-path navigation, but our custom hash-based routing (viagetRoute(route)which prepends the base) and full-URL navigations aren’t recognized by this rule. Inline disabling here is the simplest, correct approach—no underlying pattern change is needed.Locations to note:
- src/lib/navigate/index.ts: lines 10–11
- src/lib/navigate/index.ts: lines 24–25
src/lib/data/language.ts (1)
21-21: LGTM! Excellent use ofconstfor immutable variables.All these variable declaration changes are appropriate improvements:
similarityis assigned once fromcompareLanguages()callmatchis assigned once fromfindLanguage()callparts1andparts2are assigned once fromsplit()callsUsing
constenforces immutability and follows modern JavaScript best practices without affecting functionality.Also applies to: 40-40, 69-70
src/lib/data/scripture.js (1)
73-75: LGTM - Improved clarity with explicit intent commentAdding the comment clarifies that the empty catch block is intentional, which is a good practice for maintainability.
convert/convertBooks.ts (2)
152-152: LGTM - Appropriate const usageChanging to
constis correct sinceimageCaptionis never reassigned after initial assignment.
239-239: LGTM - Standard for-of loop patternUsing
constin for-of loops is the recommended pattern since each iteration gets a new const binding.example/index.ts (1)
25-29: LGTM - Improved consistency with explicit block scopingAdding curly braces makes the
darwincase consistent with other cases in the switch statement and provides explicit block scoping.src/lib/data/highlights.ts (2)
70-70: LGTM - Modern loop variable declarationChanging from
vartoletprovides proper block scoping for the loop variable, following modern JavaScript best practices.
125-125: LGTM - Consistent block scopingUsing
letinstead ofvarfor the loop variable provides block scoping and aligns with modern JavaScript conventions.src/lib/components/Sidebar.svelte (2)
50-52: LGTM - Improved readability with explicit if statementConverting the ternary expression to an explicit if statement makes the escape key handling more readable and clear.
261-268: LGTM - Appropriate ESLint rule suppressionThe ESLint disable comments are correctly applied for external menu item links that intentionally don't use the base path, which is a valid exception to the
svelte/no-navigation-without-baserule.src/lib/components/HistoryCard.svelte (1)
30-30: LGTM! Appropriate ESLint suppression for navigation.This targeted suppression aligns with the PR's ESLint integration objectives and follows the pattern mentioned in the AI summary for similar navigation components.
src/lib/data/navigation.test.ts (3)
249-249: Good immutability improvement.Changing to
constis appropriate sinceconfig2is not reassigned after initialization.
274-274: Good immutability improvement.Changing to
constis appropriate sinceconfig2is not reassigned after initialization.
459-459: Good immutability improvement.Changing to
constis appropriate sinceconfig2is not reassigned after initialization.convert/convertConfig.ts (3)
896-902: Good immutability improvement.Changing to
constis appropriate sincetranslationMappingsis only modified via property assignments, never reassigned.
990-990: Good immutability improvement.Changing to
constis appropriate sincefirebaseis only modified via property assignments, never reassigned.
1164-1164: Good immutability improvement.Changing to
constis appropriate sincedefaultLayoutis not reassigned after initialization.convert/convertReverseIndex.ts (1)
149-149: Good immutability improvement.Changing to
constis appropriate sincefilesis only mutated viapush()operations, never reassigned.src/routes/lexicon/+page.ts (3)
54-54: Good immutability improvement.Changing to
constis appropriate sincedbis not reassigned after the async initialization.
55-57: Excellent improvements to both immutability and readability.The change to
constis appropriate sinceresultsis not reassigned, and the multi-line SQL query formatting significantly improves readability.
78-80: Good immutability and style improvement.Changing to
constand combining declaration with assignment is more concise and appropriate sincefirstTwoCharsis not reassigned.convert/tests/sab/convertConfigSAB.test.ts (1)
108-109: Excellent improvement to test assertion clarity!The original assertion
expect(bkk.chapters).not.toBe(0 || undefined)had a logical flaw since0 || undefinedevaluates toundefined. The new approach with separate assertions correctly validates both conditions: thatchaptersis not 0 and that it's defined.src/lib/data/analytics.ts (1)
90-92: Good TypeScript best practice improvement!Changing from boxed
Stringtype to primitivestringtype aligns with TypeScript best practices. Primitive types are preferred over their boxed counterparts in TypeScript type annotations.src/lib/data/audio.ts (5)
75-75: Good improvement usingconstfor immutable DOM elements!Replacing
varwithconstfor DOM source elements that are not reassigned improves code safety and follows modern JavaScript practices.
82-82: Consistent improvement withconstdeclaration!Good use of
constfor the CAF source element that's not reassigned.
86-86: Properconstusage for MP3 source element!Correctly using
constfor the MP3 source element that's created once and not reassigned.
95-95: Appropriateconstdeclaration for MP3 source!Good use of
constfor the MP3 source element in the 3GP case.
479-479: Excellent use ofconstfor regex test result!The
containsAlphavariable is assigned once from a regex test and never reassigned, makingconstthe appropriate choice here.src/lib/search-worker/dab-search-worker.ts (2)
10-10: Good use ofconstfor database reference!The
dbvariable is assigned once frominitializeDatabaseand never reassigned, makingconstthe appropriate choice.
13-16: Excellent improvements to both variable declaration and formatting!Using
constfor theresultsvariable is appropriate since it's assigned once and not reassigned. The improved SQL query formatting with proper line breaks also enhances readability.src/lib/video/index.ts (5)
72-95: Excellent addition of block scoping for YouTube case!Adding braces around the case block prevents potential variable name conflicts between cases and improves code clarity. This is a good practice when declaring variables within switch cases.
97-113: Good block scoping andconstusage for Vimeo case!The block scoping braces prevent variable conflicts, and using
constforpatternandmatchvariables is appropriate since they're not reassigned within the case.
114-122: Proper block scoping and immutable variables for Daily case!Consistent application of block scoping and
constdeclarations for variables that aren't reassigned.
124-133: Clean block scoping for Jesus case!Good addition of block scoping braces for consistency with other cases.
135-138: Consistent block scoping for HLS case!Maintaining consistency with block scoping across all switch cases, even for simple ones.
package.json (1)
36-44: LGTM! ESLint and related packages upgraded successfully.The upgrades to ESLint v9.30.1, typescript-eslint v8.36.0, and eslint-plugin-svelte v3.10.1 align well with the PR objectives for modernizing the ESLint configuration.
Also applies to: 56-56
eslint.config.js (1)
1-92: Excellent ESLint configuration with proper Prettier integration!The new flat config format is well-structured with:
- Proper integration of TypeScript, Svelte, and Prettier configurations
- Appropriate rule overrides aligned with the PR objectives
- Good use of
svelteConfigfor enhanced Svelte rule compatibilitysrc/routes/contents/[id]/+page.svelte (1)
59-65: Good fix for proper case block scoping.Adding braces to the case statement creates proper block scope and prevents potential variable declaration issues.
src/lib/components/BottomNavigationBar.svelte (1)
71-75: Consistent case block scoping improvement.The addition of braces properly scopes the
gotoLinkvariable declaration within the case block.
Add the `curly` rule to enforce use of braces in if-statements. Then run `eslint . --fix` to correct all violations, and `prettier --write .` to reformat.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/lib/search-worker/data/repositories/pk-search-repository-impl.ts (1)
62-62: Fix the typo in the method name.The method name
phraseIsEmtpyhas a typo - it should bephraseIsEmpty(missing 'p' in 'Empty').Apply this diff to fix the typo:
- private phraseIsEmtpy(phrase: string) { + private phraseIsEmpty(phrase: string) {And update the method calls:
- if (this.phraseIsEmtpy(phrase)) { + if (this.phraseIsEmpty(phrase)) {- if (this.phraseIsEmtpy(phrase)) { + if (this.phraseIsEmpty(phrase)) {Also applies to: 83-83, 112-112
src/lib/search-worker/domain/test/search-session-internal.test.ts (1)
126-130: LGTM! Good stylistic improvement with a minor formatting fix needed.The addition of explicit braces around the else block improves code consistency and readability, aligning with the broader ESLint configuration changes in this PR.
However, there's a minor formatting inconsistency in the error message on line 128.
Apply this diff to fix the formatting inconsistency:
- `expected new query responses, but got types '${response1.type}' and ${response2.type}` + `expected new query responses, but got types '${response1.type}' and '${response2.type}'`convert/convertContents.ts (1)
196-196: Consider using optional chaining for cleaner conditional logicThe current
book && book.typecheck can be simplified using optional chaining.- if (book && book.type) { + if (book?.type) {convert/convertConfig.ts (1)
34-36: Simplify the useless ternary operator.The ternary operator
value === 'true' ? true : falseis redundant since the comparison already returns a boolean.-} else if (['true', 'false'].includes(value)) { - value = value === 'true' ? true : false; -} +} else if (['true', 'false'].includes(value)) { + value = value === 'true'; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
convert/convertAbout.ts(1 hunks)convert/convertBadges.ts(1 hunks)convert/convertBooks.ts(11 hunks)convert/convertConfig.ts(37 hunks)convert/convertContents.ts(1 hunks)convert/convertFirebase.ts(1 hunks)convert/convertManifest.ts(1 hunks)convert/convertMedia.ts(3 hunks)convert/convertReverseIndex.ts(3 hunks)convert/convertSQLite.ts(1 hunks)convert/convertStyles.ts(3 hunks)convert/index.ts(1 hunks)eslint.config.js(2 hunks)src/lib/components/AudioBar.svelte(1 hunks)src/lib/components/HorizontalPanes.svelte(3 hunks)src/lib/components/LexiconEntryView.svelte(2 hunks)src/lib/components/ScriptureViewSofria.svelte(1 hunks)src/lib/components/SearchForm.svelte(1 hunks)src/lib/components/TabsMenu.svelte(1 hunks)src/lib/components/VerticalPanes.svelte(3 hunks)src/lib/data/audio.ts(8 hunks)src/lib/data/stores/collection.ts(1 hunks)src/lib/data/stores/scripture.js(1 hunks)src/lib/scripts/query.js(1 hunks)src/lib/scripts/render.js(1 hunks)src/lib/search-worker/data/pk-verse-provider.ts(2 hunks)src/lib/search-worker/data/repositories/pk-search-repository-impl.ts(2 hunks)src/lib/search-worker/domain/test/search-session-internal.test.ts(1 hunks)src/lib/search/domain/search-query-manager-impl.ts(2 hunks)src/routes/lexicon/+page.svelte(1 hunks)src/routes/plans/[id]/+page.svelte(1 hunks)src/routes/quiz/[collection]/[id]/+page.svelte(1 hunks)src/routes/search/[collection]/[[savedResults]]/+page.ts(1 hunks)src/routes/text/+page.svelte(3 hunks)src/service-worker.js(2 hunks)
✅ Files skipped from review due to trivial changes (18)
- src/routes/quiz/[collection]/[id]/+page.svelte
- src/lib/components/SearchForm.svelte
- convert/convertManifest.ts
- src/routes/search/[collection]/[[savedResults]]/+page.ts
- convert/convertFirebase.ts
- src/lib/data/stores/scripture.js
- convert/convertAbout.ts
- src/lib/scripts/render.js
- src/lib/search/domain/search-query-manager-impl.ts
- src/lib/data/stores/collection.ts
- src/lib/scripts/query.js
- src/lib/search-worker/data/pk-verse-provider.ts
- convert/convertBadges.ts
- src/service-worker.js
- src/lib/components/HorizontalPanes.svelte
- convert/convertStyles.ts
- convert/convertBooks.ts
- src/lib/components/TabsMenu.svelte
🚧 Files skipped from review as they are similar to previous changes (4)
- src/routes/text/+page.svelte
- convert/convertReverseIndex.ts
- src/lib/data/audio.ts
- eslint.config.js
🧰 Additional context used
🧠 Learnings (4)
src/routes/plans/[id]/+page.svelte (2)
Learnt from: judah-sotomayor
PR: sillsdev/appbuilder-pwa#840
File: src/lib/components/NoteDialog.svelte:68-70
Timestamp: 2025-06-16T18:48:21.767Z
Learning: In Svelte 5, event handler syntax changed from Svelte 4. Use `onclick={handler}` instead of `on:click={handler}`. Event modifiers like `|preventDefault` are no longer supported and must be handled inside the handler function. Only one handler per event is allowed per element.
Learnt from: judah-sotomayor
PR: sillsdev/appbuilder-pwa#820
File: src/lib/components/Modal.svelte:28-34
Timestamp: 2025-06-05T17:25:45.457Z
Learning: In the sillsdev/appbuilder-pwa repository, the Modal.svelte component has an existing bug where `{{ id }.showModal()}` syntax was used instead of calling the exported `showModal` function directly. This bug predates Svelte 5 migration efforts.
src/lib/components/VerticalPanes.svelte (4)
Learnt from: judah-sotomayor
PR: sillsdev/appbuilder-pwa#840
File: src/lib/components/NoteDialog.svelte:68-70
Timestamp: 2025-06-16T18:48:21.767Z
Learning: In Svelte 5, event handler syntax changed from Svelte 4. Use `onclick={handler}` instead of `on:click={handler}`. Event modifiers like `|preventDefault` are no longer supported and must be handled inside the handler function. Only one handler per event is allowed per element.
Learnt from: judah-sotomayor
PR: sillsdev/appbuilder-pwa#840
File: src/lib/components/NoteDialog.svelte:68-70
Timestamp: 2025-06-16T18:48:21.767Z
Learning: In Svelte 5, the event handler syntax has changed from Svelte 4. The `onclick` attribute is valid in Svelte 5, unlike Svelte 4 where `on:click` was required.
Learnt from: judah-sotomayor
PR: sillsdev/appbuilder-pwa#820
File: src/lib/components/Modal.svelte:28-34
Timestamp: 2025-06-05T17:25:45.457Z
Learning: In the sillsdev/appbuilder-pwa repository, the Modal.svelte component has an existing bug where `{{ id }.showModal()}` syntax was used instead of calling the exported `showModal` function directly. This bug predates Svelte 5 migration efforts.
Learnt from: judah-sotomayor
PR: sillsdev/appbuilder-pwa#840
File: src/lib/components/NoteDialog.svelte:94-95
Timestamp: 2025-06-16T18:49:02.188Z
Learning: The appbuilder-pwa project uses Svelte 5, as evidenced by "svelte": "^5" in package.json and the use of Svelte 5 runes syntax like $props() and $derived(). In Svelte 5, onclick is the preferred event handler syntax over on:click.
convert/convertConfig.ts (2)
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.
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.
convert/convertContents.ts (2)
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.
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.
🪛 Biome (1.9.4)
convert/convertConfig.ts
[error] 35-35: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
convert/convertContents.ts
[error] 196-196: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ 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 (14)
src/lib/search-worker/data/repositories/pk-search-repository-impl.ts (2)
62-64: LGTM on the formatting change.The addition of explicit braces around the early return statement improves code consistency and aligns with the ESLint configuration updates mentioned in the PR objectives.
83-85: LGTM on the formatting change.The addition of explicit braces around the early return statement improves code consistency and aligns with the ESLint configuration updates mentioned in the PR objectives.
src/lib/components/AudioBar.svelte (1)
43-45: LGTM! Consistent brace style improvement.The expansion of the single-line conditional to use explicit braces aligns with ESLint's curly braces rule and improves code readability and consistency across the codebase.
src/routes/plans/[id]/+page.svelte (1)
172-174: LGTM! Consistent with codebase style improvements.The expansion of the single-line conditional to use explicit braces maintains consistency with the broader style improvements across the codebase and enhances readability.
src/routes/lexicon/+page.svelte (1)
191-193: LGTM! Proper brace style enforcement.The expansion of the single-line conditional to use explicit braces aligns with ESLint rules and improves code consistency throughout the codebase.
convert/convertMedia.ts (1)
23-28: LGTM! Consistent brace style across all conditionals.The expansion of single-line conditional statements to use explicit braces throughout the file maintains consistency with ESLint rules and improves code readability. The logic for verbose logging and error handling remains unchanged.
Also applies to: 31-33, 47-54, 120-122
convert/convertSQLite.ts (1)
18-23: LGTM! Consistent brace style in callback functions.The expansion of single-line conditional statements to use explicit braces in the copyFile callbacks maintains consistency with ESLint rules and improves code readability. The error handling and logging logic remains unchanged.
Also applies to: 29-34
convert/convertContents.ts (1)
192-194: LGTM: Improved code consistency with explicit bracesThe addition of braces around single-line conditionals enhances readability and aligns with consistent coding standards throughout the codebase.
Also applies to: 198-200
convert/index.ts (1)
128-130: LGTM: Improved code consistency with explicit bracesThe addition of braces around single-line conditionals enhances readability and maintains consistent formatting throughout the file watching logic.
Also applies to: 132-134
src/lib/components/LexiconEntryView.svelte (1)
37-39: LGTM: Improved code consistency with explicit bracesThe addition of braces around single-line conditionals enhances readability and maintains consistent formatting throughout the component logic.
Also applies to: 200-202
src/lib/components/ScriptureViewSofria.svelte (1)
1364-1366: LGTM! Good stylistic improvement for consistency.This change converts a single-line conditional early return to use explicit braces, which improves code readability and consistency with the broader codebase formatting updates mentioned in the PR objectives.
src/lib/components/VerticalPanes.svelte (1)
21-23: LGTM: Consistent bracing improves code clarity.The addition of explicit braces to all conditional statements enhances code readability and consistency. The logic for pointer event handling remains unchanged while following modern JavaScript style conventions.
Also applies to: 32-34, 35-37, 49-51
convert/convertConfig.ts (2)
970-976: LGTM: Proper use of const for immutable variables.The changes from
lettoconstfortranslationMappings,firebase, anddefaultLayoutare appropriate since these variables are not reassigned after initialization. This improves code clarity and prevents accidental mutations.Also applies to: 1073-1073, 1261-1261
228-230: LGTM: Consistent bracing improves code maintainability.The systematic addition of braces to all conditional statements throughout the file enhances code consistency and readability. This aligns with modern JavaScript/TypeScript style guides and ESLint best practices, making the codebase more maintainable.
Also applies to: 234-236, 405-407, 430-432, 447-449, 461-467, 468-470, 473-475, 488-490, 493-495, 503-505, 508-510, 530-532, 542-544, 546-548, 550-552, 561-563, 567-569, 580-582, 587-589, 602-604, 644-646, 769-771, 777-779, 781-783, 785-787, 795-797, 812-814, 819-821, 823-825, 850-855, 858-864, 890-894, 983-985, 992-994, 997-1001, 1011-1013, 1065-1067, 1094-1096, 1120-1122, 1146-1148, 1158-1160, 1168-1170, 1213-1215, 1253-1255, 1268-1270, 1298-1300, 1318-1320, 1338-1340, 1352-1354, 1355-1357, 1405-1407, 1433-1435, 1478-1480
1 participant
Reconfigure ESLint to work well with prettier
Per #864.
This pull-request includes several related changes:
These rules include svelte and typescript extensions,
as well as an integration to disable rules that conflict with prettier.
All configuration is now consolidated in
eslint.config.js, eliminating.eslintrc.cjs.I've turned off
no-dupe-style-properties,require-each-key, andno-unused-svelte-ignorebecause they are spurious for our codebase.I've also disabled the typescript
no-require-importsfor the tailwind configuration file, which usesrequirestyle imports.While I haven't fixed some of the more in-depth issues, every trivial issue should be corrected in this series of commits.
This includes a number of empty objects, missing blocks around case statements, and a hundred or so conversions from
varandlettoconst.As best as I can tell no behavior changes were introduced here.
I am a bit unsure about 8a67ec3, which changes the types on the CatalogData interface to
objectfrom the empty-object type. This could cause an issue if empty-object behavior was intended, but that seems unlikely.As it stands, the linting tests will fail because of seven unresolved errors.
Each of these is more in-depth than I want to do in a short commit, and will receive a separate PR after some discussion about the best way to resolve them.
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Style
constandletappropriately across many files for improved immutability and clarity.Refactor
Chores
Tests
Documentation