Fix Share-for-review so the owner gets a working invite URL#5
Merged
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>
angusbezzina
changed the title
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 for review as the owner produced a misleading "Couldn't reach the review relay — the share didn't complete." error after a ~15s spin, and never showed the invite URL — even though the backend share actually succeeded (the room was created and the snapshot uploaded). With no URL to copy, the owner couldn't invite anyone.
Root cause
The invite URL was gated behind
shareTargetIsCurrent, which scannedreviewStore.snapshotsfor the shared path. That store is populated for reviewers (they receive asnapshot_createdevent) but never for the owner's own fresh share — the daemon drops the owner's snapshot envelope andReviewUpdate::ShareReadycarried no path. So the gate was always false for the owner → the URL was blanked → the 15s mint timeout fired with an empty message, rendering the generic relay error.Fix
ReviewUpdate::ShareReadynow carriesowner_display_path(the verbatim path the owner shared), populated at bothshare()outcome sites and forwarded byemit_share_outcome.applyShareReadystamps it ontocurrentShare, and the dialog's gate matches against it via a pure, unit-testedshareTargetMatches(). Works for both GUI shares andattn review share <path>from the CLI — no fragile frontend-captured intent.ShareReadylands after the 15s timeout already flipped the dialog to "error", it now recovers and shows the invite rather than leaving a spurious error (a genuine failure never populates the URL, so this can't mask a real error).Local collab harness (
task dev:collab)Test-tooling fixes that make the owner↔reviewer flow exercisable locally:
ATTN_RELAY_URLto the booted daemons (they were falling back to the no-op stub, so Share never reached the relay).typography.md) so the two otherwise-identical windows are tellable apart — share from the "Project Status" window; the reviewer window visibly switches to it on join. Warns if the two fixtures collide.Robustness (from an xhigh self-review of the diff)
Scoped
applyError(no longer discards an in-flight intent on an unrelated room's error), removed a redundant/over-eager effect that double-cleared and re-fired, resolved the reviewer-fixture default at call time, and reset the dialog's share target on close. Theowner_display_paththreading above superseded the original frontend-intent approach and dissolved the cross-stamp / CLI-path edge cases the review surfaced.Testing
cargo testreview suite 413/0, incl. a new assertion thatShareReadyround-trips the exact shared path.shareTargetMatchescases),svelte-check0 errors, vite build OK.After merge + release
Release builds bake in the production relay (
ATTN_DEFAULT_RELAY_URL=https://relay.attn.sh, already inrelease.yml), so a released binary shares out of the box: owner clicks Share → getsnpx attnmd review join 'attn://…'→ sends it → reviewer joins → live collaboration.Deferred (not blocking)
reviewStore.snapshots, which remains empty for the owner (the daemon doesn't echo the owner's own snapshot to the frontend). Cosmetic; pre-existing.shareTargetMatchesdoes raw-string path comparison (no symlink/../case canonicalization) — low risk now that the path is single-sourced from the daemon.🤖 Generated with Claude Code