Handle import errors in popup data restore#956
Handle import errors in popup data restore#956PeterDaveHello merged 1 commit intoChatGPTBox-dev:masterfrom
Conversation
Review Summary by QodoHandle import errors in popup data restore
WalkthroughsDescription• Wrap popup import flow in try/catch to handle errors gracefully • Add error handling for file read, JSON parse failures • Show user-facing error alert instead of unhandled rejections • Add FileReader error handler for file read failures Diagramflowchart LR
A["File Selection"] --> B["Read File"]
B --> C{"Parse JSON"}
C -->|Success| D["Import to Storage"]
D --> E["Reload Window"]
C -->|Error| F["Show Error Alert"]
B -->|Error| F
D -->|Error| F
File Changes1. src/popup/sections/GeneralPart.jsx
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the stability and user experience of the data import feature within the popup. By wrapping the entire import flow in robust error handling, it ensures that issues arising from file reading, JSON parsing, or storage operations are gracefully caught and communicated to the user, preventing unexpected application crashes and improving overall reliability. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Code Review by Qodo
1. Unlocalized import error
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRewrote import handler in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![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 effectively addresses unhandled promise rejections during the data import process by wrapping the file reading and parsing logic in a try...catch block. This improves error handling and provides user-facing feedback on failure. I have one suggestion to further simplify the code for better readability and maintainability.
| try { | ||
| const data = await new Promise((resolve, reject) => { | ||
| const reader = new FileReader() | ||
| reader.onload = (e) => { | ||
| try { | ||
| resolve(JSON.parse(e.target.result)) | ||
| } catch (error) { | ||
| reject(error) | ||
| } | ||
| } | ||
| reader.onerror = () => { | ||
| reject(reader.error || new Error('Failed to read import file')) | ||
| } | ||
| reader.readAsText(file) | ||
| }) | ||
| await importDataIntoStorage(Browser.storage.local, data) | ||
| window.location.reload() | ||
| } catch (error) { | ||
| console.error('[popup] Failed to import data', error) | ||
| window.alert(`${t('Error')}: ${error.message || error}`) | ||
| } |
There was a problem hiding this comment.
While the current implementation correctly handles errors, it can be simplified by using the Blob.text() method, which is available on File objects. This method returns a promise that resolves with the file's content as a string, which simplifies the code by removing the need to manually wrap FileReader in a Promise. This makes the code more concise and easier to read.
try {
const fileContent = await file.text()
const data = JSON.parse(fileContent)
await importDataIntoStorage(Browser.storage.local, data)
window.location.reload()
} catch (error) {
console.error('[popup] Failed to import data', error)
window.alert(`${t('Error')}: ${error.message || error}`)
}
There was a problem hiding this comment.
Ask❓
@coderabbitai what do you think?
Answer:
The change is a solid improvement: wrapping the entire import flow in a try/catch (plus reader.onerror + JSON parse rejection) closes the unhandled-promise-rejection hole and gives users a clear failure signal while keeping the success path unchanged (importDataIntoStorage + reload).
A few concrete notes/suggestions:
-
Simplify the file read (agree with the bot suggestion): since
fileis aFile, you can useawait file.text()and drop theFileReaderpromise wrapper entirely. That reduces code and removes edge cases aroundreader.onload/reader.onerror.try { const text = await file.text() const data = JSON.parse(text) await importDataIntoStorage(Browser.storage.local, data) window.location.reload() } catch (error) { console.error('[popup] Failed to import data', error) const message = error instanceof Error ? error.message : String(error) window.alert(`${t('Error')}: ${message}`) }
-
Error message robustness:
error.message || errorcan produce[object Object]for non-Errorthrowables. Converting viaerror instanceof Error ? error.message : String(error)(as in your full diff) is safer. -
Optional: validate the imported shape: your full diff adds a “plain object” check (reject arrays/null). That’s a good guardrail to prevent applying malformed backups. Consider keeping that (and possibly checking required keys if you have a known schema).
-
i18n of messages:
t('Error')is good, but"Invalid backup file"/"Failed to read import file"should ideally be translatable too, if this UI is localized.
Overall: 👍 good fix, and I’d recommend the file.text() simplification + the more defensive error-string handling (and keeping the plain-object validation).
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
There was a problem hiding this comment.
@PeterDaveHello, the Blob.text() suggestion from gemini is a good one — I'd support adopting it. Here's my analysis:
Why it's better:
FileReader.readAsText() is the older API, whereas Blob.text() returns a promise directly, eliminating the manual new Promise((resolve, reject) => { ... }) wrapper and the nested reader.onload/reader.onerror handlers. The result is significantly less boilerplate.
Browser compatibility: Blob.text() is well established and works across many devices and browser versions, and has been available across browsers since April 2021. For a browser extension targeting modern Chrome/Firefox/Edge, there are no compatibility concerns.
One encoding note: Blob.text() always uses UTF-8, while FileReader.readAsText() can use a different encoding depending on the blob's type and a specified encoding name. Since JSON files are virtually always UTF-8, this is a non-issue here.
Combined with the isPlainObject validation (already present in the full file), the refactored version would look like:
try {
const fileContent = await file.text()
const parsedData = JSON.parse(fileContent)
if (parsedData === null || typeof parsedData !== 'object' || Array.isArray(parsedData)) {
throw new Error('Invalid backup file')
}
await importDataIntoStorage(Browser.storage.local, data)
window.location.reload()
} catch (error) {
console.error('[popup] Failed to import data', error)
const message = error instanceof Error ? error.message : String(error ?? t('Error'))
window.alert(`${t('Error')}: ${message}`)
}This is cleaner and easier to follow than the FileReader-wrapped-in-Promise approach. I'd recommend adopting it.
![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.
PeterDaveHello
force-pushed
the
handle-import-errors
branch
from
9e1e888 to
9569bc6
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
PeterDaveHello
force-pushed
the
handle-import-errors
branch
from
9569bc6 to
210926b
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| input.click() | ||
| }) |
There was a problem hiding this comment.
(Refers to lines 563-569)
📝 Info: Pre-existing: file picker cancel leaves a hanging promise
The file selection on lines 563-569 creates a Promise that only resolves via input.onchange. In some browsers, if the user cancels the file dialog, onchange never fires, leaving the Promise permanently pending. This is a pre-existing issue not introduced by this PR, and the if (!file) return guard on line 570 only helps if onchange fires with an empty file list. A common fix is to listen for a focus or change event on the window/document after input.click() to detect cancellation, or to use input.addEventListener('cancel', ...).
Was this helpful? React with 👍 or 👎 to provide feedback.
PeterDaveHello
force-pushed
the
handle-import-errors
branch
from
210926b to
f766535
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
Pull request overview
This PR hardens the popup “Import All Data” restore flow so failures during file reading, JSON parsing, or storage writes are handled gracefully instead of producing unhandled promise rejections.
Changes:
- Wrap the import/restore flow in a
try/catchto prevent unhandled promise rejections. - Switch from
FileReadertoawait file.text()for reading the selected JSON file. - Add basic validation to ensure the parsed JSON is a plain object before importing, and display a user-facing error on failure.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
PeterDaveHello
force-pushed
the
handle-import-errors
branch
from
f766535 to
13d2562
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
PeterDaveHello
force-pushed
the
handle-import-errors
branch
from
13d2562 to
b1f2894
Compare
There was a problem hiding this comment.
Pull request overview
This PR hardens the popup “Import All Data” restore flow to prevent unhandled promise rejections and provide user feedback when a restore fails.
Changes:
- Wrap the import/restore path in a
try/catchto handle file read, JSON parse, and storage errors. - Switch to
File.text()+JSON.parseand validate the backup is a plain object before importing. - On success, keep existing behavior by importing into
Browser.storage.localand reloading the page; on failure, log and alert.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } catch (error) { | ||
| console.error('[popup] Failed to import data', error) | ||
| const rawMessage = error instanceof Error ? error.message : String(error ?? '') | ||
| window.alert(rawMessage ? `${t('Error')}: ${rawMessage}` : t('Error')) |
There was a problem hiding this comment.
@PeterDaveHello — Copilot's concern is valid and worth addressing. The PR description says "Shows a simple user-facing error," but a raw SyntaxError from JSON.parse (e.g., "Unexpected token < in JSON at position 0") is quite technical and can confuse non-developer users.
Since the full technical detail is already preserved in console.error, I'd recommend mapping SyntaxError to the same friendly "Invalid backup file" string you already use elsewhere in the try block:
} catch (error) {
console.error('[popup] Failed to import data', error)
const displayMessage =
error instanceof SyntaxError
? t('Invalid backup file')
: error instanceof Error
? error.message
: String(error ?? '')
window.alert(displayMessage ? `${t('Error')}: ${displayMessage}` : t('Error'))
}This keeps the UX clean for parse failures while still surfacing meaningful messages for other error types (e.g., storage quota errors from importDataIntoStorage). If 'Invalid backup file' isn't already an i18n key, this would also be a good time to add it.
PeterDaveHello
force-pushed
the
handle-import-errors
branch
from
b1f2894 to
70002fa
Compare
There was a problem hiding this comment.
Pull request overview
This PR improves the popup “Import All Data” restore flow by preventing unhandled promise rejections and providing a user-visible failure path when file reading/parsing/storage import fails.
Changes:
- Wraps the import flow in
try/catchto prevent unhandled promise rejections during restore. - Uses
File.text()when available, with aFileReaderfallback for compatibility. - Validates that parsed JSON is a plain object before importing to extension storage.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/popup/sections/GeneralPart.jsx (1)
585-587:⚠️ Potential issue | 🟡 MinorLocalize the invalid-backup error text.
Line 586 still throws a hardcoded English message (
'Invalid backup file'), which bypasses i18n in user alerts.Based on learnings: new strings must first be added to `src/_locales/en/main.json` before propagating to other locales, and existing keys should not be changed.💡 Proposed fix
- throw new Error('Invalid backup file') + throw new Error(t('Invalid backup file'))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/popup/sections/GeneralPart.jsx` around lines 585 - 587, Replace the hardcoded error string thrown in GeneralPart.jsx (new Error('Invalid backup file')) with a localized message: first add a new key (e.g., "invalid_backup_file") and English text to src/_locales/en/main.json, then update the throw to use the project's i18n lookup (chrome.i18n.getMessage(...) or the project's getMessage/t helper) to fetch that key; do not rename existing locale keys—only add the new key and use it in the throw.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/popup/sections/GeneralPart.jsx`:
- Around line 585-587: Replace the hardcoded error string thrown in
GeneralPart.jsx (new Error('Invalid backup file')) with a localized message:
first add a new key (e.g., "invalid_backup_file") and English text to
src/_locales/en/main.json, then update the throw to use the project's i18n
lookup (chrome.i18n.getMessage(...) or the project's getMessage/t helper) to
fetch that key; do not rename existing locale keys—only add the new key and use
it in the throw.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 121fa3e9-e43a-4539-b3ba-e941679d5602
📒 Files selected for processing (1)
src/popup/sections/GeneralPart.jsx
Importing data in the popup assumed file reads, JSON parsing, and storage writes would always succeed, leaving failures unhandled and giving users no feedback. Catch errors across the import flow and validate that parsed backup data is a plain object before writing it to storage, so invalid files fail clearly without changing the successful restore path.
PeterDaveHello
force-pushed
the
handle-import-errors
branch
from
70002fa to
ed5c39d
Compare
Wrap the popup import flow in a try/catch so file read, JSON parse, and storage errors no longer surface as unhandled promise rejections.
Keep the successful import path unchanged and show a simple user-facing error when restoring data fails.
Summary by CodeRabbit