chore(deps): upgrade eslint and @eslint/js to v10 (major)#215
Merged
Conversation
- Bump eslint ^9.9.1 → ^10.0.0 (resolves 10.2.1)
- Bump @eslint/js ^9.9.1 → ^10.0.0 (resolves 10.0.1)
- Upgrade typescript-eslint 8.46.0 → 8.59.0 (required for ESLint v10.0.1+ compatibility)
ESLint v10 removed the FlatESLint export that @typescript-eslint/utils@8.46.0
depended on; upgrading to typescript-eslint@8.59.0 (which declares eslint
^10.0.0 as a valid peer) fixes the runtime crash.
ESLint v10 updated eslint:recommended with two new rules that fired on
existing code — fixed all violations:
- preserve-caught-error: added { cause: e } to 4 rethrown errors in
generateAuthUrl.ts, generatePortalUrl.ts, navigateToKinde.ts,
callAccountApi.ts
- no-useless-assignment: removed dead initial assignments in
generateAuthUrl.ts (codeChallenge = "") and callAccountApi.ts (items = [])
All 616 tests pass. Lint is clean.
Closes/replaces: #198
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughError-handling changes: multiple utilities now rethrow errors attaching the original exception as the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
- callAccountApi: test fetch network error path (catch block with { cause })
- navigateToKinde: test popup auth failure when addEventListener throws
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
lib/utils/navigateToKinde.ts (1)
157-161: Minor: error concatenation is now redundant withcause.Since the original error is attached via
{ cause: error }, interpolating it into the message ("Popup authentication failed: " + error) duplicates the information and produces output likePopup authentication failed: Error: Listener registration failed. Consider dropping the concatenation to keep the message clean:♻️ Proposed tweak
- throw new Error("Popup authentication failed: " + error, { cause: error }); + throw new Error("Popup authentication failed", { cause: error });Note: the existing test asserts
toContain("Popup authentication failed"), so it would still pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/utils/navigateToKinde.ts` around lines 157 - 161, In the catch block around waitForMessage in navigateToKinde.ts, remove the concatenation of the original error into the message and instead create the Error with a clean message and attach the original as the cause (e.g., throw new Error("Popup authentication failed", { cause: error })); update the throw in that catch around waitForMessage so the message is concise while preserving the original error via the cause property.lib/utils/token/accountApi/callAccountApi.ts (1)
33-37: Minor: interpolated${error}duplicates info now carried bycause.With
cause: errorattached, stringifyingerrorinto the message yields messages likeFailed to fetch from …: Error: Network failure. The test atcallAccountApi.test.ts:96-99only assertstoContain("Failed to fetch from …"), so trimming the interpolation would be safe and a bit cleaner:♻️ Optional cleanup
- throw new Error(`Failed to fetch from ${domain.value}/${route}: ${error}`, { - cause: error, - }); + throw new Error(`Failed to fetch from ${domain.value}/${route}`, { + cause: error, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/utils/token/accountApi/callAccountApi.ts` around lines 33 - 37, In the catch block inside callAccountApi (in callAccountApi.ts) remove the interpolated `${error}` from the thrown Error message and keep the existing cause: error so the Error message reads only "Failed to fetch from ${domain.value}/${route}" while preserving the original error via the cause option (i.e., update the throw in the catch of callAccountApi to omit the stringified error but still pass cause: error).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/utils/navigateToKinde.ts`:
- Around line 157-161: In the catch block around waitForMessage in
navigateToKinde.ts, remove the concatenation of the original error into the
message and instead create the Error with a clean message and attach the
original as the cause (e.g., throw new Error("Popup authentication failed", {
cause: error })); update the throw in that catch around waitForMessage so the
message is concise while preserving the original error via the cause property.
In `@lib/utils/token/accountApi/callAccountApi.ts`:
- Around line 33-37: In the catch block inside callAccountApi (in
callAccountApi.ts) remove the interpolated `${error}` from the thrown Error
message and keep the existing cause: error so the Error message reads only
"Failed to fetch from ${domain.value}/${route}" while preserving the original
error via the cause option (i.e., update the throw in the catch of
callAccountApi to omit the stringified error but still pass cause: error).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 03a29b38-c22a-4479-827d-1a9ddeca922c
⛔ Files ignored due to path filters (2)
package.jsonis excluded by!**/*.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/*.yaml
📒 Files selected for processing (6)
lib/utils/generateAuthUrl.tslib/utils/generatePortalUrl.tslib/utils/navigateToKinde.test.tslib/utils/navigateToKinde.tslib/utils/token/accountApi/callAccountApi.test.tslib/utils/token/accountApi/callAccountApi.ts
The async listener waits 5ms, but scheduleNotification uses setTimeout(0) which can take several ms on loaded CI runners. Combined, the original 10ms wait was insufficient. Increased to 50ms to match the safety margin used in the adjacent async listener test (10ms internal + 20ms wait = 2x + buffer).
Per CodeRabbit review: don't interpolate error into the message when
the original error is already preserved via the cause property.
- navigateToKinde.ts: 'Popup authentication failed: ' + error -> 'Popup authentication failed'
- callAccountApi.ts: 'Failed to fetch from ...route: ${error}' -> 'Failed to fetch from ...route'
KomanRudden
reviewed
Apr 23, 2026
KomanRudden
reviewed
Apr 23, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/sessionManager/stores/memory.test.ts (1)
262-269: Restore real timers safely on failure paths.If an assertion or awaited operation between Line 262 and Line 269 throws,
vi.useRealTimers()is never reached and fake timers leak into subsequent tests in this file — which all rely on realsetTimeout(e.g.,await new Promise((resolve) => setTimeout(resolve, 0))at Lines 286, 295, 325, 345, 372, 394, 411, 415, 419). That can produce confusing cascading failures or hangs on CI.Prefer scoping the restoration in
afterEach(or atry/finally) so it always runs.♻️ Proposed fix using afterEach
beforeEach(() => { sessionManager = new MemoryStorage(); }); + afterEach(() => { + vi.useRealTimers(); + }); +And remove the inline
vi.useRealTimers()at Line 269:vi.useFakeTimers(); sessionManager.subscribe(syncListener); sessionManager.subscribe(asyncListener); await sessionManager.setSessionItem(StorageKeys.idToken, "mixedTest"); await vi.runAllTimersAsync(); - vi.useRealTimers(); expect(syncCalled).toBe(true); expect(asyncCalled).toBe(true);Note:
afterEachwill need to be added to the imports fromvitest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sessionManager/stores/memory.test.ts` around lines 262 - 269, The test enables fake timers with vi.useFakeTimers() around sessionManager.subscribe(...) and await sessionManager.setSessionItem(...), but vi.useRealTimers() is called inline and can be skipped on failures; ensure timers are always restored by moving vi.useRealTimers() into a guaranteed cleanup (either add an afterEach import from vitest that calls vi.useRealTimers(), or wrap the fake-timer section in a try/finally that calls vi.useRealTimers()); update the test that uses vi.useFakeTimers() (the block around sessionManager.setSessionItem and vi.runAllTimersAsync) to rely on the global afterEach or the try/finally so subsequent tests using real setTimeout behave correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/sessionManager/stores/memory.test.ts`:
- Around line 262-269: The test enables fake timers with vi.useFakeTimers()
around sessionManager.subscribe(...) and await
sessionManager.setSessionItem(...), but vi.useRealTimers() is called inline and
can be skipped on failures; ensure timers are always restored by moving
vi.useRealTimers() into a guaranteed cleanup (either add an afterEach import
from vitest that calls vi.useRealTimers(), or wrap the fake-timer section in a
try/finally that calls vi.useRealTimers()); update the test that uses
vi.useFakeTimers() (the block around sessionManager.setSessionItem and
vi.runAllTimersAsync) to rely on the global afterEach or the try/finally so
subsequent tests using real setTimeout behave correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 67dcfc85-e2e3-42af-90c9-9ca074d76bf6
⛔ Files ignored due to path filters (2)
package.jsonis excluded by!**/*.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/*.yaml
📒 Files selected for processing (3)
lib/sessionManager/stores/memory.test.tslib/utils/navigateToKinde.tslib/utils/token/accountApi/callAccountApi.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/utils/navigateToKinde.ts
- lib/utils/token/accountApi/callAccountApi.ts
Contributor
Author
|
@coderabbitai I have addressed your nitpick as well. |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
…lAccountApi.test.ts
victoreronmosele
approved these changes
Apr 25, 2026
1 task
KomanRudden
approved these changes
Apr 28, 2026
Koosha-Owji
approved these changes
Apr 28, 2026
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
4 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.
Summary
Upgrades ESLint and
@eslint/jsfrom v9 to v10, replacing the Renovate PR #198 which had a merge conflict and an incompatibility withtypescript-eslint@8.46.0.Resolves/replaces: #198
Dependency changes
eslint^9.9.1→ resolves9.37.0^10.0.0→ resolves10.2.1@eslint/js^9.9.1→ resolves9.37.0^10.0.0→ resolves10.0.1typescript-eslint8.46.0(locked)8.59.0(updated)Why typescript-eslint needed upgrading too
The Renovate PR (#198) locked
eslint@10.0.0- before theFlatESLintnamed export was removed ineslint@10.0.1.@typescript-eslint/utils@8.46.0importsFlatESLintfrom ESLint directly, causing a runtime crash (TypeError: Class extends value undefined) with ESLint ≥ 10.0.1.typescript-eslint@8.59.0is the fixed release that no longer depends on the removed export and explicitly declareseslint: ^10.0.0as a valid peer dependency. The project's existing constraint (^8.3.0) already allows this upgrade.ESLint v10 migration fixes
ESLint v10 updated
eslint:recommendedwith two new rules that fired on existing code. All violations are fixed in this PR:preserve-caught-error(4 fixes)Errors rethrown in catch blocks now require the original error to be passed as
cause. Added{ cause: e }to rethrown errors in:lib/utils/generateAuthUrl.tslib/utils/generatePortalUrl.tslib/utils/navigateToKinde.tslib/utils/token/accountApi/callAccountApi.tsno-useless-assignment(2 fixes)Initial assignments whose value is immediately overwritten before being read are now flagged. Removed dead assignments in:
lib/utils/generateAuthUrl.ts-let codeChallenge = ""immediately overwritten in both if/else brancheslib/utils/token/accountApi/callAccountApi.ts-let items = []immediately overwritten by the first API call resultOther ESLint v10 breaking changes - not applicable here
eslintrcsupport removedeslint.config.js)eslint-envcomments now errorsSourceCode/ rule context methods removedno-shadow-restricted-namesreportsglobalThisby default^20.19.0 || ^22.13.0 || >=24requiredTest results
pnpm lint- passes (0 errors)pnpm test --run- 616 tests pass, 20 skipped (no regressions)