Fix owner Share dialog timing out instead of showing the invite URL#5
Draft
angusbezzina wants to merge 5 commits into
Draft
Fix owner Share dialog timing out instead of showing the invite URL#5angusbezzina wants to merge 5 commits into
angusbezzina wants to merge 5 commits into
Conversation
Clicking Share on a markdown file succeeded on the backend (relay round-trip + ShareReady with a real invite URL) but the dialog timed out after 15s with "Couldn't reach the review relay" and never showed the URL. Root cause: commit e1bf35d gated the invite URL on `shareTargetIsCurrent`, which scanned `reviewStore.snapshots` for the shared path. That store is populated for reviewers (who receive a snapshot_created event) but never for the owner's own fresh share -- the daemon drops snapshot envelopes in forward_transport_event and ShareReady carries no path. So the gate was always false for the owner, blanking the URL until the 15s mint timeout. Fix (frontend-only, narrow blast radius): thread the path the frontend already knows onto the share record. ShareDialog.onStart records the in-flight path via reviewStore.noteShareIntent() when reviewShare fires; applyShareReady stamps it as currentShare.ownerDisplayPath (persisted on the room.share record so reselect/rehydration keeps it); shareTargetIsCurrent now matches against that path via a new pure shareTargetMatches() in room-ui.ts. A failed/timed-out mint releases the intent (clearShareIntent / onAbort + applyError) so a stale path can't bleed into a later share. Preserves single-file, folder, child-of-folder, and unshared-file semantics. No daemon change. Tests: 6 new shareTargetMatches cases in room-ui.test.ts (tsx defineCase harness; the project has no vitest). Deferred follow-up: emit the owner's own snapshot to the frontend at share time so reviewStore.snapshots is populated for the owner -- the architecturally complete daemon fix that would also repair the file-tree "shared" marker and owner roomDisplayName (both already broken for the owner before this gate, out of scope here). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
`scripts/dev-collab.sh` set ATTN_RELAY_URL but never exported it, and `start_dual` (scripts/lib/dual-instance.sh) launched the owner/reviewer daemons with only ATTN_HOME prefixed. So the daemons started with no relay configured and fell back to the scaffold stub -- Share never reached the relay and the dialog timed out, making the collab flow impossible to test locally even though the relay was healthy. Forward ATTN_RELAY_URL (defaulting to empty, which the daemon treats as "unset") on both daemon launches in start_dual, and export it from dev-collab.sh. Empty stays safe for callers that don't want a relay; when set, the daemons now attach the real relay. Verified end to end: `task dev:collab` owner now logs `review bootstrap attached (relay=http://localhost:8787)`, Share publishes a snapshot and the dialog shows the invite, and the reviewer joins over ws://localhost:8787. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
Validated end-to-end locally ✅Added a second commit fixing the local collab harness: E2E evidence (driven via the debug-build automation flags):
So both layers are confirmed: the dialog surfaces the invite URL for the owner, and the full owner→reviewer round-trip works locally. |
…ellable apart The owner and reviewer windows both opened the same fixture and were pixel-identical, so it was easy to click Share on the wrong one. Since the join always routes to the reviewer daemon, sharing from the reviewer window loops the invite back to itself (joins its own room, "No peers") while the owner window sits idle — looking like collab silently failed. Open the reviewer on a different fixture (default typography.md) via a new ATTN_DUAL_REVIEWER_FIXTURE knob (defaults to the owner's fixture for backward compat). Now the owner window shows basic.md / "Project Status" and the reviewer shows typography.md until it joins, at which point it switches to the owner's doc — making both "which window do I share from" and "did the join work" visually obvious. Terminal hints updated to say SHARE FROM the owner window. Verified: owner h1 "Project Status" vs reviewer h1 "Heading Level 1"; after owner Share + reviewer join, reviewer h1 -> "Project Status" and both review bars show the other peer (owner "…Live R", reviewer "Live O"). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
From the xhigh review of this branch: #1 (ShareReady after the 15s timeout): a slow relay could answer after the mint timeout already flipped the dialog to 'error' and the onAbort effect had cleared the share-intent, so a *successful* share showed a spurious error and "Try again" minted a duplicate room. Fixed two ways: the ready transition now also recovers from the 'error' state when a valid inviteUrl arrives, and the over-eager onAbort clear is removed (a genuine daemon failure never populates inviteUrl, so recovery can't mask a real error). #5/#8 (onAbort effect): the `$effect(() => { if (phase==='error') onAbort?.() })` lacked the `if (!open) return` guard its siblings have and re-fired on every parent re-render (fresh closure dep); it also double-cleared the intent alongside applyError. Removed the effect, the onAbort prop, and the now-dead clearShareIntent() store method. #3 (applyError scope): applyError cleared pendingShareTargetPath for ANY review error, so an unrelated error on another live owner room during the ~15s mint window discarded a valid in-flight share intent. Now it clears only on a non-room-scoped error (room_id: None — what a failed share emits). #6 (dual-instance reviewer fixture): ATTN_DUAL_REVIEWER_FIXTURE was defaulted at source time, so a caller that set ATTN_DUAL_FIXTURE after sourcing got a reviewer fixture pinned to the old default. Resolved at start_dual call time via a local instead. Verified: web tests 31/0, svelte-check 0 errors, vite build OK, bash -n OK, and a share happy-path smoke against the relay still reaches the invite URL. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ent seam The durable fix for review finding #9 (and the #2/#4/#7 family that stemmed from it). The owner's shared path is now carried on the ShareReady payload straight from the daemon, instead of being reconstructed on the frontend via a captured-intent slot. Rust: ShareOutcome and ReviewUpdate::ShareReady gain owner_display_path (= path.to_string_lossy(), the verbatim path the frontend passed to reviewShare). Populated at both share() outcome sites (fresh mint + re-share) and forwarded by emit_share_outcome. New assertion in share_emits_invite_with_expected_shape locks the round-trip. Frontend: applyShareReady stamps currentShare.ownerDisplayPath from payload.ownerDisplayPath. Removed pendingShareTargetPath, noteShareIntent, the ShareDialog onStart wiring, and applyError's intent-clear — the whole seam is gone. This dissolves the earlier findings it caused: - #2 un-keyed cross-stamp (no global slot anymore), - #4 / #7 CLI `attn review share` yielding ownerDisplayPath='' (the path now comes from the daemon for CLI and GUI shares alike), - the noteShareIntent-timing footgun. Also resolves two smaller findings: - #11: App resets shareTargetPath when the share dialog closes so a stale path can't feed shareTargetIsCurrent on the next open. - sweep: dev:collab warns when the owner and reviewer fixtures collide (windows would be indistinguishable). This change is genuinely testable where the seam was not: the path logic now lives in Rust (cargo test), not the un-unit-testable runes store. Verified: cargo review tests 413/0 (incl. the new path assertion), web tests 31/0, svelte-check 0 errors, vite build OK, bash -n OK, and a share happy-path smoke against the relay shows the invite (gate matches the daemon-supplied path). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Clicking Share on a markdown file as the owner succeeded on the backend — the relay round-trip completed and the daemon emitted
ReviewUpdate::ShareReadywith a real invite URL — but the dialog timed out after 15s with "Couldn't reach the review relay — the share didn't complete." and never showed the URL. The error was misleading: the relay was reachable; the URL existed; the frontend just refused to display it.Root cause
Commit
e1bf35dgated the invite URL behindshareTargetIsCurrent, which scansreviewStore.snapshotsfor an entry matching the shared path. That store is populated for reviewers (they receive asnapshot_createdevent) but never for the owner's own fresh share:forward_transport_eventdrops snapshot envelopes (manager.rs— "today they just persist via the InboundPipeline"), andReviewUpdate::ShareReadycarries no path.So
shareTargetIsCurrentwas alwaysfalsefor the owner →existingInviteUrl/ownerSigningKey/existingRoomIdblanked → the 15sMINT_TIMEOUT_MSfired → generic error.Fix (frontend-only, narrow blast radius)
Thread the path the frontend already knows onto the share record, instead of depending on the never-populated
snapshotsstore:ShareDialog.onStartrecords the in-flight path viareviewStore.noteShareIntent()the instantreviewSharefires.applyShareReadystamps it ascurrentShare.ownerDisplayPath, persisted on theroom.sharerecord so reselect/rehydration keeps it.shareTargetIsCurrentnow matches against that path via a new pure, unit-testedshareTargetMatches()inroom-ui.ts(t === p || t.startsWith(p + '/'), trailing-slash tolerant).onAbort→clearShareIntent(), plusapplyError) so a stale path can't bleed into a later (e.g. CLI-initiated) share. The timeout path emits no daemon error, so the dialog signals release directly.Preserves single-file, folder, child-of-folder, and unshared-file semantics. No daemon change.
Tests & verification
shareTargetMatchescases inroom-ui.test.ts(the project'stsxdefineCaseharness — there is no vitest): exact file, folder, nested child, trailing-slash, prefix-collision guard, null/empty safety.npm test(31 files, 0 failures),npm run check(svelte-check, 0 errors),npm run buildall green.Deferred follow-up (not required for this fix)
Emit the owner's own snapshot to the frontend at share time so
reviewStore.snapshotsis populated for the owner — the architecturally complete daemon-side fix. It would additionally repair two second-order symptoms that are already broken for the owner today (pre-existing, out of scope here): the file-tree "shared" marker (App.sveltesharedPaths) and the owner-sideroomDisplayName. Concretely: threadowner_display_paththroughReviewUpdate::ShareReady, or fan anEventImportedper published doc via the existingreviewEvent→applyEventpath (dedup byeventId/snapshotIdabsorbs the WS replay).🤖 Generated with Claude Code