Skip to content

Handle import errors in popup data restore #956

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
PeterDaveHello merged 1 commit into from
Mar 16, 2026
Merged

Handle import errors in popup data restore #956

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 30 additions & 7 deletions src/popup/sections/GeneralPart.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -568,13 +568,36 @@ export function GeneralPart({ config, updateConfig, setTabIndex }) {
input.click()
})
Comment on lines 568 to 569
Copy link

@devin-ai-integration devin-ai-integration bot Mar 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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', ...).

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

if (!file) return
const data = await new Promise((resolve) => {
const reader = new FileReader()
reader.onload = (e) => resolve(JSON.parse(e.target.result))
reader.readAsText(file)
})
await importDataIntoStorage(Browser.storage.local, data)
window.location.reload()
try {
const fileContent =
typeof file.text === 'function'
? await file.text()
: await new Promise((resolve, reject) => {
const reader = new FileReader()
reader.onload = () => resolve(reader.result)
reader.onerror = () => reject(reader.error)
reader.readAsText(file)
})
const parsedData = JSON.parse(fileContent)
const isPlainObject =
parsedData !== null && typeof parsedData === 'object' && !Array.isArray(parsedData)

if (!isPlainObject) {
throw new Error('Invalid backup file')
}

await importDataIntoStorage(Browser.storage.local, parsedData)
window.location.reload()
} catch (error) {
console.error('[popup] Failed to import data', error)
const rawMessage =
error instanceof SyntaxError
? 'Invalid backup file'
: error instanceof Error
? error.message
: String(error ?? '')
window.alert(rawMessage ? `${t('Error')}: ${rawMessage}` : t('Error'))
Comment on lines +591 to +599
Copy link
Member Author

@PeterDaveHello PeterDaveHello Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai what do you think?

Copy link
Contributor

@coderabbitai coderabbitai bot Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

}
Comment on lines +571 to +600
Copy link

@gemini-code-assist gemini-code-assist bot Mar 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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}`)
            }

Copy link
Member Author

@PeterDaveHello PeterDaveHello Mar 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai what do you think?

Copy link
Contributor

@qodo-code-review qodo-code-review bot Mar 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 file is a File, you can use await file.text() and drop the FileReader promise wrapper entirely. That reduces code and removes edge cases around reader.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 || error can produce [object Object] for non-Error throwables. Converting via error 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).

Copy link
Contributor

@coderabbitai coderabbitai bot Mar 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skipped: comment is from another GitHub bot.

Copy link
Contributor

@coderabbitai coderabbitai bot Mar 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

}}
>
{t('Import All Data')}
Expand Down
Loading