Skip to content

Add user notification for background child transfer failures - #1272

Merged
malibio merged 2 commits into
mainfrom
worktree-issue-656-toast-notifications
May 30, 2026
Merged

Add user notification for background child transfer failures#1272
malibio merged 2 commits into
mainfrom
worktree-issue-656-toast-notifications

Conversation

@malibio

@malibio malibio commented May 30, 2026

Copy link
Copy Markdown
Collaborator

Closes #656

…rejects (#656)

Extends the existing ConflictNotificationStore with a 'child-transfer-failure' type and
calls conflictNotifications.add() in the catch block of createNode's background child
transfer, replacing the TODO(#656) comment. No new toast infrastructure introduced.

Co-Authored-By: Claude <noreply@anthropic.com>
@malibio

malibio commented May 30, 2026

Copy link
Copy Markdown
Collaborator Author

Code Review Summary

This is a clean, minimal change that correctly closes #656. The implementation reuses the existing ConflictNotificationStore infrastructure rather than introducing a parallel toast system — exactly the right call. All 3867 tests pass. The change is a net positive and is ready to merge with one improvement noted below.


Findings

Suggested Improvements

packages/desktop-app/src/lib/services/shared-node-store.svelte.tsCONFLICT_MESSAGE entry for child-transfer-failure is unreachable dead data

The CONFLICT_MESSAGE record is typed as Record<ConflictNotification['conflictType'], string>, so adding the 'child-transfer-failure' key is required to keep TypeScript exhaustiveness. However, the only call site that reads from this map is SharedNodeStore.resolveConflict(), which is fed by Conflict objects whose conflictType is independently typed in update-protocol.ts as 'concurrent-edit' | 'version-mismatch' | 'deleted-node' — a narrower union that does not include 'child-transfer-failure'. This means the new entry in CONFLICT_MESSAGE can never be reached at runtime through that path.

The message is instead passed directly at the conflictNotifications.add() call site in reactive-node-service.svelte.ts, which is the correct pattern. The CONFLICT_MESSAGE entry is therefore structural dead weight.

Two options:

  1. (Preferred — low effort) Decouple the CONFLICT_MESSAGE map from ConflictNotification['conflictType']. Type it against only the conflict types that SharedNodeStore actually produces: Record<Conflict['conflictType'], string>. This removes the requirement to add an entry here for every new notification type that originates outside SharedNodeStore.

  2. (Alternative) Extend Conflict['conflictType'] in update-protocol.ts to include 'child-transfer-failure' and route all notification messages through CONFLICT_MESSAGE. This consolidates all user-facing strings in one place but requires SharedNodeStore.resolveConflict() to handle the new type, which does not make semantic sense for a pure data-conflict resolver.

Option 1 is strongly preferred because it respects the Single Responsibility of each boundary: SharedNodeStore owns data-conflict messages; ReactiveNodeService owns async-operation-failure messages.


Nitpicks

Nit: packages/desktop-app/src/tests/services/reactive-node-service.test.ts — duplicate assertion structure

The test uses two sequential vi.waitFor blocks. The first waits for moveNode to be called, the second waits for the notification. The comment on the first block correctly explains the 500ms debounce. This is fine as written. A minor tightening: the intermediate assertion on moveNodeMock.mock.calls.length could be removed if you restructure to wait directly for the notification with the full 3000ms budget on a single vi.waitFor, since moveNode being called is an implementation detail, not a behavioral assertion. However the current two-phase approach makes the failure mode significantly easier to diagnose (you know exactly which step timed out), so keeping it is defensible.

Nit: packages/desktop-app/src/lib/components/conflict-toast.svelte line 54 — emoji in UI

The warning icon ⚠️ renders as an emoji. This is pre-existing and outside the scope of this PR, but a design-system icon would give more consistent cross-platform rendering.


Acceptance Criteria Review

Criterion Status
Event/notification system for async operation failures Reuses existing ConflictNotificationStore — satisfied
Show toast notification when child transfer rollback occurs conflictNotifications.add() called in the catch block — satisfied
Message is user-friendly "Changes couldn't be saved. Please try again." — satisfied
Retry action on notification Not implemented — see note below

Retry action: Issue #656 lists "Consider adding a retry action" as a criterion with "consider" language. The implementation omits it. Given the rollback already restores in-memory state (children are moved back to structureTree), the user recovers by simply reattempting the operation — a retry button would need to re-trigger the entire createNode flow with the original arguments, which is non-trivial. Omitting it is a reasonable scoping decision for this PR. If a retry action is desired, it warrants a separate issue.


Verdict: Approve pending the CONFLICT_MESSAGE type tightening (option 1 above), or with an explicit decision to accept the dead entry as-is given its TypeScript exhaustiveness value.

@malibio malibio left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Review status: APPROVED (self-review limitation prevents formal approval). Clean, minimal implementation that correctly reuses existing infrastructure. One improvement recommended: decouple CONFLICT_MESSAGE from the full ConflictNotification union type to eliminate the unreachable 'child-transfer-failure' entry. All 3867 tests pass. Full findings posted above.

…update-protocol

The map is only consumed by SharedNodeStore's conflict resolution path, which
is fed by Conflict objects from update-protocol.ts — not by notification-only
types like 'child-transfer-failure'. Narrowing the key type removes the dead
entry and respects single responsibility.

Co-Authored-By: Claude <noreply@anthropic.com>
@malibio
malibio merged commit c0d5332 into main May 30, 2026
@malibio
malibio deleted the worktree-issue-656-toast-notifications branch May 30, 2026 14:36
malibio added a commit that referenced this pull request Jun 22, 2026
* feat(ux): emit child-transfer-failure toast when background moveNode rejects (#656)

Extends the existing ConflictNotificationStore with a 'child-transfer-failure' type and
calls conflictNotifications.add() in the catch block of createNode's background child
transfer, replacing the TODO(#656) comment. No new toast infrastructure introduced.

Co-Authored-By: Claude <noreply@anthropic.com>

* fix(types): narrow CONFLICT_MESSAGE to Conflict['conflictType'] from update-protocol

The map is only consumed by SharedNodeStore's conflict resolution path, which
is fed by Conflict objects from update-protocol.ts — not by notification-only
types like 'child-transfer-failure'. Narrowing the key type removes the dead
entry and respects single responsibility.

Co-Authored-By: Claude <noreply@anthropic.com>

---------

Co-authored-by: Michael Libio <malibio@Michaels-Mac-mini.local>
Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add user notification for background child transfer failures

1 participant