Skip to content

Ensures message is sent w/ sync-done & error events#1358

Open
DougReeder wants to merge 2 commits intomasterfrom
event-msg-falsy
Open

Ensures message is sent w/ sync-done & error events#1358
DougReeder wants to merge 2 commits intomasterfrom
event-msg-falsy

Conversation

@DougReeder
Copy link
Contributor

sync-done should always pass a message object with a boolean completed property.

error should always pass an Error object as the message.

This PR complements remotestorage/remotestorage-widget#147

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR standardizes emitted event payloads to match the documented/public API expectations: sync-done should always provide a { completed: boolean } result object, and error should always provide an Error instance.

Changes:

  • Ensure Dropbox sync failure path emits sync-done with { completed: false }.
  • Ensure IndexedDB database-level errors emit an Error object instead of a non-error/empty payload.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
src/indexeddb.ts Updates IndexedDB db.onerror handler to emit an Error instance for error events.
src/dropbox.ts Updates Dropbox sync hook to emit sync-done with { completed: false } on fetchDelta failure.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/indexeddb.ts Outdated
Comment on lines +402 to +403
(db as any).onerror = evt => {
remoteStorage._emit('error', new Error(`database error: ${evt?.type}`));
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

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

The emitted Error message here is likely not very actionable: for IndexedDB onerror events, evt.type will usually just be "error". Consider extracting the underlying IndexedDB error (e.g., from evt.target.error / evt.target.error.message when available) so consumers get a meaningful error message while still receiving an Error instance.

Suggested change
(db as any).onerror = evt => {
remoteStorage._emit('error', new Error(`database error: ${evt?.type}`));
(db as any).onerror = (evt: any) => {
const idbError = evt && evt.target && (evt.target as any).error;
const detail =
(idbError && (idbError.message || String(idbError))) ||
(evt && evt.type) ||
'unknown';
remoteStorage._emit('error', new Error(`database error: ${detail}`));

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good idea, but extracting the error from the event should be in a helper function.

Again, I think we need an IDB test harness for Mocha before we can write a test for this.

Copy link
Member

Choose a reason for hiding this comment

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

OK, sure, but the code suggestion would still improve the error message, no?

Could you create a new issue for the test harness you envision?

@silverbucket
Copy link
Member

@DougReeder I activated a copilot review for you to address or ignore as you see fit! I will try to review soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants