Merged
Conversation
✅ Deploy Preview for livecodes ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Contributor
|
Size Change: +73 B (+0.01%) Total Size: 1.02 MB ℹ️ View Unchanged
|
Deploying livecodes with
|
| Latest commit: |
3e5f03c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a9ab4fbc.livecodes.pages.dev |
| Branch Preview URL: | https://fix-exceeding-localstorage.livecodes.pages.dev |
WalkthroughError handling was added to the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
![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
🤖 Fix all issues with AI agents
In `@src/livecodes/storage/simple-storage.ts`:
- Around line 25-30: The try/catch around window.localStorage.setItem(name,
JSON.stringify(value)) in simple-storage.ts currently swallows all errors;
narrow it to only handle storage-quota/security cases and surface others: catch
DOMException/QuotaExceededError (or check err.name/err.code) and in that branch
call notifyPub() as appropriate and emit a console.warn (or trigger a user
notification) with the error and the key name, but rethrow or propagate
non-quota errors (e.g., JSON.stringify TypeError) so bugs are not hidden;
reference the setItem call, the surrounding try/catch, and notifyPub to locate
where to implement this change.
🧹 Nitpick comments (1)
src/livecodes/storage/simple-storage.ts (1)
43-46:cleardouble-notifies subscribers on success.
setValue(null)already callsnotifyPub()internally (line 27), so the explicitnotifyPub()on line 45 triggers a second notification. This is pre-existing, but worth cleaning up — either remove line 45 or haveclearuselocalStorage.setItemdirectly.Proposed fix
const clear = () => { setValue(null); - notifyPub(); };
Comment on lines
+25
to
+30
| try { | ||
| window.localStorage.setItem(name, JSON.stringify(value)); | ||
| notifyPub(); | ||
| } catch { | ||
| // exceeded quota | ||
| } |
There was a problem hiding this comment.
Silent catch swallows all errors, risking unnoticed data loss.
Two concerns:
-
No user/developer feedback — if a write silently fails, the user may lose data (e.g. settings, unsaved state) with no indication. Consider at least a
console.warnso it's debuggable, and ideally surfacing a notification to the user. -
Overly broad catch — this catches every exception (e.g.,
SecurityErrorwhen storage is disabled,TypeErrorfromJSON.stringifyon circular refs), not justQuotaExceededError. Non-quota errors likely indicate bugs worth surfacing.
Proposed fix: narrow the catch and add minimal logging
const setValue = (value: T | null) => {
try {
window.localStorage.setItem(name, JSON.stringify(value));
notifyPub();
} catch {
- // exceeded quota
+ } catch (e) {
+ if (e instanceof DOMException && e.name === 'QuotaExceededError') {
+ console.warn(`[simple-storage] localStorage quota exceeded for "${name}"`);
+ } else {
+ throw e;
+ }
}
};🤖 Prompt for AI Agents
In `@src/livecodes/storage/simple-storage.ts` around lines 25 - 30, The try/catch
around window.localStorage.setItem(name, JSON.stringify(value)) in
simple-storage.ts currently swallows all errors; narrow it to only handle
storage-quota/security cases and surface others: catch
DOMException/QuotaExceededError (or check err.name/err.code) and in that branch
call notifyPub() as appropriate and emit a console.warn (or trigger a user
notification) with the error and the key name, but rethrow or propagate
non-quota errors (e.g., JSON.stringify TypeError) so bugs are not hidden;
reference the setItem call, the surrounding try/catch, and notifyPub to locate
where to implement this change.
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
1 participant
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 by CodeRabbit