Reconfigure ESLint to work well with prettier#957
Merged
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.
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.
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.
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughESLint configuration migrated to flat config with TypeScript/Svelte/Prettier support; package dependencies updated for Svelte 5 compatibility. Svelte store exports converted to ChangesESLint Configuration & Tooling Migration
Svelte 5 Store Reactivity API
Control Flow Formatting & Code Quality
ESLint Directive & Lint Rule Management
Type Corrections & Minor Logic Fixes
Sequence Diagram(s)No sequence diagrams required: changes are primarily configuration updates, code formatting, and type/safety fixes without introducing new multi-component interactions or control flow. Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Rationale: Large PR (100+ files, 400+ lines changed) with heterogeneous modifications spanning ESLint infrastructure, Svelte 5 API migration, widespread formatting changes, and type corrections. While most changes are straightforward formatting or declarative updates, the sheer volume and variety—combined with the critical ESLint config rewrite and store API migration—demand careful review of config correctness, store reactivity semantics, and type signature consistency across consumers. Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@convert/convertAbout.ts`:
- Around line 14-21: convertAbout currently starts an asynchronous copy via
copyFile with a callback and returns immediately; change convertAbout to be
async and return/await a Promise that resolves when copyFile completes (or
rejects on error) so callers (e.g., ConvertAbout.run) can await completion and
catch errors. Replace the callback-style copyFile(srcFile, dstFile, ...) with a
Promise wrapper (or use fs.promises.copyFile) that rejects on err and resolves
after the verbose log, update convertAbout's signature to async, and ensure any
caller (ConvertAbout.run) awaits the async convertAbout so the step doesn't
succeed before the file copy finishes.
In `@convert/convertBadges.ts`:
- Around line 32-39: The callback-based copyFile call in convertBadges.ts should
be replaced with an awaited promise so the conversion flow waits for the copy
and errors propagate through the async path; replace the copyFile(..., callback)
usage inside the conversion function (where srcFile/dstFile and verbose are in
scope) with an awaited fs.promises.copyFile(srcFile, dstFile) (or the
promisified equivalent) and wrap it in try/catch or let the promise rejection
bubble so you can log the successful copy when verbose and correctly surface
errors instead of throwing inside a callback.
In `@convert/convertBooks.ts`:
- Around line 801-803: Replace the thrown error inside the fs.readFile callback
with rejecting the surrounding Promise so the error propagates into the Promise
chain (and therefore into Promise.all(docs)); specifically, in the fs.readFile
callback shown (currently doing "if (err) { throw err; }") call the Promise
reject function instead (e.g., reject(err)) so functions like the Promise
executor that produce the docs array and the Promise.all(docs) at line 531
receive the rejection rather than causing an uncaught exception.
In `@convert/convertConfig.ts`:
- Around line 592-594: The code currently silently skips malformed XML nodes by
using checks like "if (!audioTag) { continue; }" (and similar checks at the
other referenced spots), which hides source-data defects; instead change to
fail-fast by asserting/throwing when required attributes/nodes are missing:
remove the continue guards and replace them with a clear runtime failure (e.g.,
throw new Error or call a small assert helper) that includes identifying context
(page id, node name, or filename) so conversion stops and the invalid XML is
reported; update the locations around the "audioTag" check and the analogous
audio-file filename checks (and the occurrences around lines 599-601 and
1177-1179) to use these non-null assertions/throws rather than continuing.
In `@convert/convertContents.ts`:
- Around line 74-76: Several guards in convert/convertContents.ts (e.g., checks
like "if (tag === undefined) { return false; }" around the variable "tag" and
similar guards at the listed ranges) silently return benign defaults and should
instead fail fast; replace these silent fallbacks by removing the early-return
and either use non-null assertions (e.g., refer to tag! when accessing required
properties) or throw a clear Error indicating the missing XML element/attribute
(include context such as the expected tag name) so parsing fails loudly; update
all occurrences you noted (lines ~74-76, 97-99, 134-136, 149-151, 164-166,
188-190, 208-210, 238-240, 270-272, 317-319, 331-333, 342-344) to follow this
fail-fast pattern and ensure any downstream code assumes required values are
present.
In `@convert/convertSQLite.ts`:
- Around line 18-25: The callback-based copyFile call (using
copyFile(srcFileWasm, dstFileWasm, function (err) { ... })) creates an async
error-propagation gap; replace it with the promise-based API (import from
fs/promises and await copyFile(srcFileWasm, dstFileWasm)) so the conversion step
runner can deterministically observe success/failure, and either let exceptions
propagate or catch and rethrow/log via the surrounding async function; apply the
same change to the other callback-based copyFile block referenced (the one
around lines 29-36) and preserve the verbose console.log message after the
awaited copy completes.
In `@src/lib/data/scripture.js`:
- Around line 85-87: The catch block that currently reads "catch (e) { // Ignore
errors }" must not swallow errors silently: preserve the non-throwing behavior
but emit a contextual warning and include the error details (e.message or the
error object) so missing/corrupt PKF or fetch failures are visible; update the
catch in the docset loading code (the block catching e) to call your logger
(e.g., processLogger.warn or console.warn) with a clear message like "Failed to
load docset <docsetName> or PKF" and the error, and optionally record a
metric/event for failure reporting.
In `@src/lib/lexicon/components/EntryView.svelte`:
- Around line 294-296: The HTML injection currently renders raw xmlData via
{`@html` xmlData} and createElementString preserves all attributes (including
event handlers like onclick/onerror), so harden by sanitizing the output: either
filter attributes inside createElementString to drop any attributes starting
with "on", any style="expression(...)" or javascript: URLs (href/src), or run
the reconstructed string through a sanitizer such as DOMPurify.sanitize(xmlData)
before it's passed to {`@html`}; update createElementString (and any callers) to
perform attribute filtering if you prefer defensive server-side generation, or
import DOMPurify and sanitize at the point where xmlData is assigned to ensure
no executable attributes reach the {`@html`} injection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a95f9d4f-5ba3-4d08-b991-224aeed689bb
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (73)
.eslintrc.cjsconvert/convertAbout.tsconvert/convertBadges.tsconvert/convertBooks.tsconvert/convertConfig.tsconvert/convertContents.tsconvert/convertFirebase.tsconvert/convertMedia.tsconvert/convertReverseIndex.tsconvert/convertSQLite.tsconvert/convertStyles.tsconvert/fileUtils.tsconvert/index.tsconvert/tests/sab/convertConfigSAB.test.tsconvert/tests/sab/convertContentSAB.test.tseslint.config.jsexample/index.tspackage.jsonsrc/app.d.tssrc/lib/components/AudioBar.sveltesrc/lib/components/BottomNavigationBar.sveltesrc/lib/components/ContentCarousel.sveltesrc/lib/components/ContentHeading.sveltesrc/lib/components/ContentSingle.sveltesrc/lib/components/FontList.sveltesrc/lib/components/HistoryCard.sveltesrc/lib/components/HorizontalPanes.sveltesrc/lib/components/HtmlBookView.sveltesrc/lib/components/ScriptureViewSofria.sveltesrc/lib/components/SearchForm.sveltesrc/lib/components/Sidebar.sveltesrc/lib/components/StackView.sveltesrc/lib/components/TabsMenu.sveltesrc/lib/components/TextAppearanceSelector.sveltesrc/lib/components/VerticalPanes.sveltesrc/lib/data/analytics.tssrc/lib/data/annotation-share.tssrc/lib/data/audio.tssrc/lib/data/catalogData.tssrc/lib/data/highlights.tssrc/lib/data/history.tssrc/lib/data/language.tssrc/lib/data/mrucache.tssrc/lib/data/navigation.test.tssrc/lib/data/scripture.jssrc/lib/data/stores/collection.tssrc/lib/data/stores/lexicon.svelte.tssrc/lib/data/stores/scripture.jssrc/lib/data/stores/store-types.jssrc/lib/data/stores/theme.jssrc/lib/lexicon/components/EntryView.sveltesrc/lib/navigate/index.tssrc/lib/scripts/query.jssrc/lib/scripts/render.jssrc/lib/search-worker/dab-search-worker.tssrc/lib/search-worker/data/pk-verse-provider.tssrc/lib/search-worker/data/repositories/pk-search-repository-impl.tssrc/lib/search-worker/domain/test/search-session-internal.test.tssrc/lib/search/data/test/search-config-repository-impl.test.tssrc/lib/search/domain/search-query-manager-impl.tssrc/lib/utils/worker-messenger/messenger.test.tssrc/lib/video/index.tssrc/routes/about/+page.sveltesrc/routes/contents/[id]/+page.sveltesrc/routes/dev/icons/+page.sveltesrc/routes/lexicon/+layout.sveltesrc/routes/lexicon/+layout.tssrc/routes/lexicon/+page.sveltesrc/routes/plans/[id]/+page.sveltesrc/routes/quiz/[collection]/[id]/+page.sveltesrc/routes/search/[collection]/[[savedResults]]/+page.tssrc/routes/text/+page.sveltesrc/service-worker.js
💤 Files with no reviewable changes (2)
- src/app.d.ts
- .eslintrc.cjs
FyreByrd
commented
May 13, 2026
Collaborator
Author
FyreByrd
left a comment
FyreByrd
left a comment
There was a problem hiding this comment.
I'm not seeing the seven remaining errors that are referred to. There aren't really any substantive changes between this and the original PR. This is mostly a rebase to resolve merge conflicts. It won't let me approve it myself, since I am now technically the PR author.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rebase of #869 to resolve merge conflicts
PR comment from #869:
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
Release Notes
Bug Fixes
Code Quality & Improvements