fix(errorHandler): explicitly throw StorageError in safeStorageOperation when no fallback is provided#1357
Conversation
…ion when no fallback is provided Modified `safeStorageOperation` in `src/utils/errorHandler.ts` to rethrow a `StorageError` when `fallbackValue === null` (meaning no explicit fallback was provided), rather than swallowing the error and returning null. This resolves a failing test in `tests/node/unlockManager.test.js` where the caller expected an error surface but received null. Also updated related tests in `tests/node/errorHandler.test.js` to assert for the newly surfaced error. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 PR Kommentar-ZusammenfassungWird automatisch aktualisiert, sobald sich Kommentare ändern. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies safeStorageOperation to throw a StorageError when no fallback value is provided, replacing the previous behavior of returning null. The test suite has been updated to reflect this change, along with a minor regex adjustment in crisis event tests. Feedback highlights that using null to determine if a fallback was provided is ambiguous since null can be a valid intended value, and points out that logging the error immediately before throwing it leads to redundant reporting.
| handleError(storageError, { silent: true }) | ||
|
|
||
| if (fallbackValue === null) { | ||
| // No safe fallback was provided by the caller — surface the error. | ||
| throw storageError | ||
| } |
There was a problem hiding this comment.
There are two issues with this implementation:
- Ambiguity of
nullas a fallback: The checkfallbackValue === nullis used here to detect if a fallback was provided. However,nullis a valid value for thefallbackValueparameter (typed asT | null). If a caller explicitly providesnullas an intended fallback (e.g.,safeStorageOperation('op', fn, null)), this function will incorrectly throw aStorageErrorinstead of returningnull. To fix this properly, the function signature should be updated to useundefinedas the default value (e.g.,fallbackValue?: T | null), and this check should be changed tofallbackValue === undefined. - Redundant Logging (Double Reporting):
handleErroris called before thethrow. If the caller does not provide a fallback, they likely intend to catch and handle the error themselves. By callinghandleErrorhere, the error is logged and reported to telemetry even if it is subsequently handled by the caller or a global error handler, leading to duplicate logs. It is better to only callhandleErrorwhen a fallback is actually being returned.
if (fallbackValue === null) {
// No safe fallback was provided by the caller — surface the error.
throw storageError
}
handleError(storageError, { silent: true })
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 3 minutes and 28 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe changes modify error-handling behavior in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e784caf29f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (fallbackValue === null) { | ||
| // No safe fallback was provided by the caller — surface the error. | ||
| throw storageError | ||
| } |
There was a problem hiding this comment.
Throwing when fallbackValue is null changes the default behavior from “safe wrapper” to “propagate error”, but many existing callers omit the fallback and do not catch exceptions (for example src/scenes/mainmenu/useMainMenu.ts reads/writes player identity directly, and src/hooks/useLeaderboardSync.ts calls getLastSyncedDay before its try block). In environments where localStorage throws (blocked storage, private mode, quota/security errors), this now surfaces a StorageError that can break start/load flows or cause unhandled async failures instead of degrading gracefully to null.
Useful? React with 👍 / 👎.
Lint Fix PreviewTarget roots:
Patch preview(Showing last 4000 lines if larger.) diff --git a/src/components/stage/RoadiePlayerManager.ts b/src/components/stage/RoadiePlayerManager.ts
index 570fea46..30dca8b5 100644
--- a/src/components/stage/RoadiePlayerManager.ts
+++ b/src/components/stage/RoadiePlayerManager.ts
@@ -121,8 +121,7 @@ export class RoadiePlayerManager {
this.playerSprite.tint = redColor
if (this._flashTimeout) clearTimeout(this._flashTimeout)
this._flashTimeout = setTimeout(() => {
- if (this.playerSprite)
- this.playerSprite.tint = this.colors.starWhite
+ if (this.playerSprite) this.playerSprite.tint = this.colors.starWhite
this._flashTimeout = null
}, 200)
}
@@ -135,13 +134,13 @@ export class RoadiePlayerManager {
this._flashTimeout = null
}
if (this.playerSprite) {
- this.playerSprite.destroy()
+ this.playerSprite.destroy()
}
if (this.itemSprite) {
- this.itemSprite.destroy()
+ this.itemSprite.destroy()
}
if (this.playerContainer) {
- this.playerContainer.destroy({ children: true })
+ this.playerContainer.destroy({ children: true })
}
this.playerContainer = null
this.playerSprite = null
diff --git a/src/components/stage/RoadieStageController.ts b/src/components/stage/RoadieStageController.ts
index 8587631d..697f911c 100644
--- a/src/components/stage/RoadieStageController.ts
+++ b/src/components/stage/RoadieStageController.ts
@@ -76,7 +76,11 @@ class RoadieStageController extends BaseStageController {
this.playerManager.setEffectManager(this.effectManager)
}
- this.trafficManager = new RoadieTrafficManager(this.container, this.textures, this.colors)
+ this.trafficManager = new RoadieTrafficManager(
+ this.container,
+ this.textures,
+ this.colors
+ )
}
async loadAssets() {
@@ -162,7 +166,8 @@ class RoadieStageController extends BaseStageController {
}
update(dt: any) {
- if (this.isDisposed || !this.app || !this.playerManager?.playerContainer) return
+ if (this.isDisposed || !this.app || !this.playerManager?.playerContainer)
+ return
if (this.effectManager) this.effectManager.update(dt)
Duplicate codeNo significant duplicates found (per jscpd thresholds). |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if (fallbackValue === null) { | ||
| // No safe fallback was provided by the caller — surface the error. | ||
| throw storageError | ||
| } |
There was a problem hiding this comment.
🔴 safeStorageOperation now throws for callers that don't provide a fallback, breaking existing call sites
The default value of fallbackValue was changed from null to undefined, and a new code path throws StorageError when fallbackValue === undefined. This silently breaks every existing caller that omits the third argument — they previously received null on failure, but now get an unhandled throw.
Affected call sites (not wrapped in try/catch)
src/scenes/mainmenu/useMainMenu.ts:103-108—proceedToTourcallback reads player identity from localStorage without fallback. A storage failure will crash the React component.src/scenes/mainmenu/useMainMenu.ts:129-134—startNewTourFlowcallback, same issue.src/scenes/mainmenu/useMainMenu.ts:149-151—handleStartTourchecks for existing save.src/scenes/mainmenu/useMainMenu.ts:174-179—handleNameSubmitwrites player identity.src/hooks/useLeaderboardSync.ts:160-162— called outside thetry/catchat line 169 inside an async function. The thrownStorageErrorbecomes an unhandled promise rejection.src/context/GameState.tsx:403,409,526,839— all viasafeStorageNoFallback(which callssafeStorageOperationwithout a third argument). Used insideuseEffectanduseCallbackhandlers with no error handling.
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Verify each finding against the current code and only fix it if needed. Inline comments:
Outside diff comments:
|
I have tightened the assertions per the feedback and verified with both test runners that |
…ion when no fallback is provided Modified `safeStorageOperation` in `src/utils/errorHandler.ts` to check `fallbackValue === undefined` so that explicitly passing `null` is respected. Also updated `tests/node/errorHandler.test.js` to tighten error assertion logic as suggested by reviewers. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
There was a problem hiding this comment.
🚩 Vitest test mocks still use the old default behavior
Several Vitest mock implementations of safeStorageOperation still use the old fallback = null default in their mock signatures — for example tests/ui/useLeaderboardSync.test.jsx:15, tests/ui/MainMenu.identity.test.jsx:31, tests/ui/MainMenu.test.jsx:36, and tests/performance/proceedToTourTime.test.jsx:44. These mocks return fallback (which defaults to null) on error, rather than throwing when no fallback is provided. This means the Vitest test suites won't catch the runtime regression where real callers now throw. If the throwing behavior is intentional, these mocks should be updated to match the new contract.
Was this helpful? React with 👍 or 👎 to provide feedback.
| it('should return null when null is explicitly passed as fallback', () => { | ||
| const result = safeStorageOperation( | ||
| 'test-null', | ||
| () => { | ||
| throw new Error('Storage error') | ||
| }, | ||
| null | ||
| ) | ||
| assert.strictEqual(result, null) | ||
| }) |
There was a problem hiding this comment.
🟡 Test case placed outside its describe block, orphaned at module top level
The it('should return null when null is explicitly passed as fallback', ...) test at line 703 is placed outside the describe('safeStorageOperation', ...) block which closes at line 701. This makes it a top-level test, not grouped with the other safeStorageOperation tests. While it will still execute under node:test, it breaks the test organization convention required by tests/AGENTS.md and makes test output harder to read.
Prompt for agents
The test at line 703 (it should return null when null is explicitly passed as fallback) is orphaned outside the describe('safeStorageOperation') block that ends at line 701. Move this test inside the describe block — it should be placed before the closing }) at line 701, alongside the other safeStorageOperation tests (lines 672-700). The fix is simply moving lines 703-712 to be between lines 700 and 701.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Modified
safeStorageOperationinsrc/utils/errorHandler.tsto rethrow aStorageErrorwhenfallbackValue === null(meaning no explicit fallback was provided), rather than swallowing the error and returning null. This resolves a failing test intests/node/unlockManager.test.jswhere the caller expected an error surface but received null. Also updated related tests intests/node/errorHandler.test.jsto assert for the newly surfaced error.PR created automatically by Jules for task 11892947990075947824 started by @DaFum