Handle import errors in popup data restore#956
Handle import errors in popup data restore#956PeterDaveHello wants to merge 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)
📝 WalkthroughWalkthroughThe 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". |
There was a problem hiding this comment.
Pull request overview
This PR hardens the popup “Import All Data” flow by catching failures during file read, JSON parsing, and storage writes so they don’t become unhandled promise rejections, and surfaces a user-facing error instead.
Changes:
- Wrapes the import flow in
try/catchto handle failures gracefully. - Adds FileReader error handling and propagates JSON parse failures.
- Logs import failures and shows an alert to the user.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const message = error instanceof Error ? error.message : String(error ?? t('Error')) | ||
| window.alert(`${t('Error')}: ${message}`) |
There was a problem hiding this comment.
@PeterDaveHello, Copilot's suggestion has partial merit. Here's the breakdown:
Where the current approach works well:
- The two custom error messages (
"Invalid backup file"and"Failed to read import file") are already user-friendly and will display cleanly as-is.
Where Copilot has a point:
JSON.parsethrows browser-generated errors like"Unexpected token '<', "<!DOCTYPE ..." is not valid JSON"— these are technical and confusing for end users.- Errors from
importDataIntoStorageare also unpredictable.
Recommended middle-ground — map known error categories to localized messages while keeping the raw detail in console.error (which already happens):
} catch (error) {
console.error('[popup] Failed to import data', error)
let message
if (error instanceof SyntaxError) {
message = t('Invalid JSON format in backup file')
} else if (error instanceof Error) {
message = error.message // already user-friendly for custom errors
} else {
message = t('Error')
}
window.alert(`${t('Error')}: ${message}`)
}This way:
SyntaxErrorfromJSON.parse→ localized, clean message- Custom
Errorobjects ("Invalid backup file","Failed to read import file") → shown as-is (already friendly) - Unknown/non-Error throws → generic
t('Error')fallback
Copilot's suggestion to fully replace everything with a single opaque message would be too aggressive and lose useful context. The middle-ground above addresses the legitimate UX concern without over-engineering it.
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". |
There was a problem hiding this comment.
Pull request overview
Improves the extension popup’s “Import All Data” restore flow by preventing unhandled promise rejections and surfacing failures to users.
Changes:
- Wraps file read → JSON parse → storage import in a
try/catch. - Adds basic validation that imported JSON is a plain object before writing to storage.
- Logs failures to the console and displays a user-facing error alert on import failure.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| window.location.reload() | ||
| } catch (error) { | ||
| console.error('[popup] Failed to import data', error) | ||
| const message = error instanceof Error ? error.message : String(error ?? t('Error')) |
There was a problem hiding this comment.
@PeterDaveHello, Copilot's observation is valid. new Error() and new Error('') both produce error.message === '', so the current code can produce an alert that just says "Error: " — not useful for the user. The fallback t('Error') in the ternary's false branch never fires when a bare Error object with no message is thrown.
A minimal, clean fix:
const rawMessage = error instanceof Error ? error.message : String(error ?? '')
const message = rawMessage || t('Error')Or, if you prefer it as a one-liner using Error.prototype.toString() (which gives at least "Error" instead of "" for a no-message Error):
const message =
(error instanceof Error ? error.message || error.toString() : String(error ?? '')) || t('Error')Either approach guarantees the alert always shows a non-empty, localized fallback when no descriptive message is available, while still preferring the actual message when it exists.
![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.
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
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.
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