diff --git a/scripts/dev-collab.sh b/scripts/dev-collab.sh index 5873f60..04035b1 100755 --- a/scripts/dev-collab.sh +++ b/scripts/dev-collab.sh @@ -36,6 +36,10 @@ cd "$PROJECT_DIR" : "${ATTN_RELAY_URL:=http://localhost:8787}" : "${FIXTURE_PATH:=tests/fixtures/basic.md}" +# Reviewer opens a visually distinct fixture so the two identical windows can be +# told apart: share from the owner window (this fixture); the reviewer window +# switches to it on join. +: "${REVIEWER_FIXTURE_PATH:=tests/fixtures/typography.md}" : "${ATTN_BIN:=$PROJECT_DIR/target/debug/attn}" # Pin the per-instance ATTN_HOMEs the task issue asks for, then forward @@ -43,7 +47,11 @@ cd "$PROJECT_DIR" export ATTN_DUAL_OWNER="/tmp/attn-collab-owner" export ATTN_DUAL_REVIEWER="/tmp/attn-collab-reviewer" export ATTN_DUAL_FIXTURE="$PROJECT_DIR/$FIXTURE_PATH" +export ATTN_DUAL_REVIEWER_FIXTURE="$PROJECT_DIR/$REVIEWER_FIXTURE_PATH" export ATTN_BIN +# Export the relay URL so the daemons booted by start_dual inherit it; without +# this they fall back to the scaffold stub and Share never reaches the relay. +export ATTN_RELAY_URL # Relay process state. RELAY_PID="" @@ -141,7 +149,8 @@ join_reviewer() { return 0 fi - log "Click [Share] in the owner window, copy the invite URL, then paste it here." + log "Click [Share] in the OWNER window — the one showing '$FIXTURE_PATH'." + log "(The reviewer window shows '$REVIEWER_FIXTURE_PATH' until it joins.) Copy the invite, then paste it here." log "(Empty line cancels and leaves the daemons running.)" printf 'Paste invite > ' IFS= read -r invite || true @@ -189,11 +198,18 @@ trap cleanup EXIT INT TERM start_relay +# Warn if the two windows would be indistinguishable (e.g. FIXTURE_PATH was +# overridden to the reviewer's default) — the whole point is to tell them apart +# for the Share step. +if [ "$REVIEWER_FIXTURE_PATH" = "$FIXTURE_PATH" ]; then + log "WARNING: owner and reviewer fixtures are identical ($FIXTURE_PATH) — the two windows will look the same. Set REVIEWER_FIXTURE_PATH to a different file to tell them apart." +fi + log "Booting owner + reviewer daemons (ATTN_HOME isolation)" start_dual wait_for_dual 'h1' 20000 -log "Owner window opened (fixture: $FIXTURE_PATH, ATTN_HOME=$ATTN_DUAL_OWNER)" -log "Reviewer window opened (ATTN_HOME=$ATTN_DUAL_REVIEWER) — idle until join" +log "Owner window opened — shows '$FIXTURE_PATH'; SHARE FROM THIS WINDOW. ATTN_HOME=$ATTN_DUAL_OWNER" +log "Reviewer window opened — shows '$REVIEWER_FIXTURE_PATH' until it joins, then switches to the owner's doc. ATTN_HOME=$ATTN_DUAL_REVIEWER" join_reviewer diff --git a/scripts/lib/dual-instance.sh b/scripts/lib/dual-instance.sh index be780ad..47837dd 100644 --- a/scripts/lib/dual-instance.sh +++ b/scripts/lib/dual-instance.sh @@ -44,6 +44,10 @@ __attn_dual_lib_dir() { : "${ATTN_DUAL_REPO:=$(__attn_dual_lib_dir)}" : "${ATTN_BIN:=$ATTN_DUAL_REPO/target/debug/attn}" : "${ATTN_DUAL_FIXTURE:=$ATTN_DUAL_REPO/tests/fixtures/review/scenario-comment-survives-edit.md}" +# The reviewer can open a DIFFERENT fixture than the owner so the two windows +# are visually distinguishable until the reviewer joins (then it switches to the +# owner's shared doc). Resolved at start_dual time (NOT here) so a caller that +# sets ATTN_DUAL_FIXTURE *after* sourcing this lib still gets it as the default. # Stashed PIDs. Empty until start_dual sets them; cleared by stop_dual. ATTN_DUAL_OWNER_PID="" @@ -116,20 +120,31 @@ attn_reviewer() { start_dual() { __attn_dual_require_bin || return 1 + # Resolve the reviewer fixture at call time so a caller that sets + # ATTN_DUAL_FIXTURE after sourcing this lib still gets it as the default. + local reviewer_fixture="${ATTN_DUAL_REVIEWER_FIXTURE:-$ATTN_DUAL_FIXTURE}" + if [ ! -f "$ATTN_DUAL_FIXTURE" ]; then echo "dual-instance: fixture missing at $ATTN_DUAL_FIXTURE" >&2 return 1 fi + if [ ! -f "$reviewer_fixture" ]; then + echo "dual-instance: reviewer fixture missing at $reviewer_fixture" >&2 + return 1 + fi # Wipe any leftover state from a previous (possibly crashed) run. rm -rf "$ATTN_DUAL_OWNER" "$ATTN_DUAL_REVIEWER" mkdir -p "$ATTN_DUAL_OWNER" "$ATTN_DUAL_REVIEWER" - ATTN_HOME="$ATTN_DUAL_OWNER" "$ATTN_BIN" --no-fork "$ATTN_DUAL_FIXTURE" \ + # Forward ATTN_RELAY_URL (if set) so the daemons attach the real relay + # instead of the scaffold stub — Share/Join/collab flows need it. Empty is + # treated as "unset" by the daemon, so this is safe when no relay is wanted. + ATTN_HOME="$ATTN_DUAL_OWNER" ATTN_RELAY_URL="${ATTN_RELAY_URL:-}" "$ATTN_BIN" --no-fork "$ATTN_DUAL_FIXTURE" \ >"$ATTN_DUAL_OWNER/daemon.stdout.log" 2>"$ATTN_DUAL_OWNER/daemon.stderr.log" & ATTN_DUAL_OWNER_PID=$! - ATTN_HOME="$ATTN_DUAL_REVIEWER" "$ATTN_BIN" --no-fork "$ATTN_DUAL_FIXTURE" \ + ATTN_HOME="$ATTN_DUAL_REVIEWER" ATTN_RELAY_URL="${ATTN_RELAY_URL:-}" "$ATTN_BIN" --no-fork "$reviewer_fixture" \ >"$ATTN_DUAL_REVIEWER/daemon.stdout.log" 2>"$ATTN_DUAL_REVIEWER/daemon.stderr.log" & ATTN_DUAL_REVIEWER_PID=$! } diff --git a/src/review/bootstrap.rs b/src/review/bootstrap.rs index 3400b0b..de26023 100644 --- a/src/review/bootstrap.rs +++ b/src/review/bootstrap.rs @@ -659,6 +659,11 @@ impl BootstrapConfig { pub struct ShareOutcome { pub room_id: RoomId, pub invite: String, + /// Absolute path (file or folder) the owner shared, exactly as the frontend + /// passed it to `reviewShare` (`path.to_string_lossy()`). The Share dialog + /// matches this against the active target to recognise its own room (see + /// `shareTargetMatches`), so it must be the verbatim path string. + pub owner_display_path: String, /// `true` if the room was newly created, `false` if `Share` was a no-op /// because the bindings file already had an entry pointing at a live room. pub newly_created: bool, @@ -953,6 +958,7 @@ impl Bootstrapper { Ok(ShareOutcome { room_id, invite, + owner_display_path: path.to_string_lossy().to_string(), newly_created: true, owner_signing_key: identity.public_signing_key.clone(), mode: room_mode_wire(policy.mode), @@ -999,6 +1005,7 @@ impl Bootstrapper { return Ok(Some(ShareOutcome { room_id, invite, + owner_display_path: path.to_string_lossy().to_string(), newly_created: false, owner_signing_key: identity.public_signing_key.clone(), mode: room_mode_wire(policy_mode), @@ -2612,11 +2619,16 @@ mod tests { .await; let (_doc_tmp, path) = temp_markdown_file("x.md", "# X\n"); + let expected_display_path = path.to_string_lossy().to_string(); let outcome = boot .share(path, RoomMode::Async, None) .await .expect("share"); assert!(outcome.invite.starts_with("attn://review/")); + assert_eq!( + outcome.owner_display_path, expected_display_path, + "ShareOutcome must carry the exact path the owner shared so the dialog recognises its own room", + ); assert!(outcome.invite.contains("#key=")); // The invite must round-trip through `parse_invite`. let parsed = parse_invite(&outcome.invite).expect("parse"); diff --git a/src/review/manager.rs b/src/review/manager.rs index 6e4e23f..45afe5f 100644 --- a/src/review/manager.rs +++ b/src/review/manager.rs @@ -155,6 +155,10 @@ pub enum ReviewUpdate { ShareReady { room_id: RoomId, invite_url: String, + /// Absolute path the owner shared, so the dialog can recognise its own + /// room without relying on a frontend-captured intent. Serialised as + /// `ownerDisplayPath`. + owner_display_path: String, owner_signing_key: String, mode: String, expires_at: u64, @@ -1346,6 +1350,7 @@ impl ReviewManager { (self.update_tx)(ReviewUpdate::ShareReady { room_id: outcome.room_id, invite_url: outcome.invite, + owner_display_path: outcome.owner_display_path, owner_signing_key: outcome.owner_signing_key, mode: outcome.mode, expires_at: outcome.expires_at, diff --git a/web/src/App.svelte b/web/src/App.svelte index 3b18138..567ea68 100644 --- a/web/src/App.svelte +++ b/web/src/App.svelte @@ -96,6 +96,7 @@ isReviewerView, collabRoleFor, collabSeedReady, + shareTargetMatches, } from './lib/review/room-ui'; import { requestReviewDecorationsRebuild, @@ -275,16 +276,16 @@ // invite; when it's a NEW target (e.g. sharing a folder while a single file is // shared), the dialog mints a fresh room so the owner "switches over" to it. let shareTargetIsCurrent = $derived.by(() => { - const roomId = reviewStore.currentShare?.roomId; - if (!roomId) return false; - const target = (shareTargetPath ?? activePath ?? '').replace(/\/+$/, ''); - if (target.length === 0) return false; - return reviewStore.snapshots.some((s) => { - if (s.roomId !== roomId) return false; - const p = s.ownerDisplayPath; - if (!p) return false; - return p === target || p.startsWith(`${target}/`); - }); + const share = reviewStore.currentShare; + if (!share?.roomId) return false; + const target = shareTargetPath ?? activePath ?? ''; + return shareTargetMatches(share.ownerDisplayPath, target); + }); + // Reset the share target when the dialog closes so a stale path can't feed + // shareTargetIsCurrent on the next open; openShareDialogForPath always sets it + // fresh before reopening, so clearing here is safe. + $effect(() => { + if (!shareDialogOpen) shareTargetPath = null; }); const loadedMtimeByPath = new Map(); // Reactive map of disk mtimes for HTML files, keyed by path. Bumping an @@ -1966,6 +1967,7 @@ reviewStore.applyShareReady({ roomId: payload.roomId, inviteUrl: payload.inviteUrl, + ownerDisplayPath: payload.ownerDisplayPath, ownerSigningKey: payload.ownerSigningKey, mode: payload.mode, expiresAt: payload.expiresAt, diff --git a/web/src/lib/ShareDialog.svelte b/web/src/lib/ShareDialog.svelte index f019390..7c8bdc0 100644 --- a/web/src/lib/ShareDialog.svelte +++ b/web/src/lib/ShareDialog.svelte @@ -141,9 +141,13 @@ void autoMint(); }); - // Transition minting → ready when the daemon's ShareReady IPC lands. + // Transition → ready when the daemon's ShareReady IPC lands. Recovers from a + // timed-out mint too: a slow relay can answer AFTER the 15s timeout already + // flipped phase to 'error', and the invite is still valid — so surface it + // rather than leaving a spurious error on screen. A genuine daemon failure + // never populates inviteUrl, so this can't paper over a real error. $effect(() => { - if (phase === 'minting' && inviteUrl.length > 0) { + if ((phase === 'minting' || phase === 'error') && inviteUrl.length > 0) { if (mintTimeout) { clearTimeout(mintTimeout); mintTimeout = null; diff --git a/web/src/lib/review/room-ui.test.ts b/web/src/lib/review/room-ui.test.ts index d12fdcd..6966259 100644 --- a/web/src/lib/review/room-ui.test.ts +++ b/web/src/lib/review/room-ui.test.ts @@ -9,6 +9,7 @@ import { collabSeedReady, isReviewerView, roomDisplayName, + shareTargetMatches, shouldActivateRoomStatus, shouldAutoSelectOnlyRoom, shouldForgetRoomStatus, @@ -228,6 +229,66 @@ defineCase('roomDisplayName: null when no snapshots for the room yet', () => { assert(roomDisplayName(snaps, 'r1' as RoomId) === null, 'no snapshots → null so the caller falls back to the short id'); }); +// --- shareTargetMatches (owner-share path gate; replaces snapshot scan) ----- + +defineCase('shareTargetMatches: single shared file matches itself', () => { + assert( + shareTargetMatches('/Users/me/plan.md', '/Users/me/plan.md') === true, + 'the exact shared file must be recognized as current', + ); +}); + +defineCase('shareTargetMatches: shared folder matches the folder itself', () => { + assert( + shareTargetMatches('/Users/me/planning', '/Users/me/planning') === true, + 'the shared folder path must match itself', + ); +}); + +defineCase('shareTargetMatches: child file under a shared folder matches', () => { + assert( + shareTargetMatches('/Users/me/planning', '/Users/me/planning/a.md') === true, + 'a markdown file beneath the shared folder must be recognized as current', + ); + assert( + shareTargetMatches('/Users/me/planning', '/Users/me/planning/sub/c.md') === true, + 'a nested child of the shared folder must also match', + ); +}); + +defineCase('shareTargetMatches: trailing slashes are tolerated on both sides', () => { + assert( + shareTargetMatches('/Users/me/planning/', '/Users/me/planning/a.md') === true, + 'a folder share stored with a trailing slash must still match its children', + ); + assert( + shareTargetMatches('/Users/me/plan.md', '/Users/me/plan.md/') === true, + 'a stray trailing slash on the target must not break an exact match', + ); +}); + +defineCase('shareTargetMatches: a different unshared file does NOT match', () => { + assert( + shareTargetMatches('/Users/me/plan.md', '/Users/me/other.md') === false, + 'an unrelated file must not be treated as the current share', + ); + assert( + shareTargetMatches('/Users/me/planning', '/Users/me/planningX/a.md') === false, + 'a sibling whose name merely shares a prefix must not match (segment boundary required)', + ); + assert( + shareTargetMatches('/Users/me/planning', '/Users/me/planning-notes.md') === false, + 'a non-boundary prefix collision must not match', + ); +}); + +defineCase('shareTargetMatches: empty / null inputs never match', () => { + assert(shareTargetMatches(null, '/Users/me/plan.md') === false, 'no active share → no match'); + assert(shareTargetMatches('/Users/me/plan.md', null) === false, 'no target selected → no match'); + assert(shareTargetMatches('', '') === false, 'both empty → no match'); + assert(shareTargetMatches(undefined, undefined) === false, 'undefined inputs are safe → no match'); +}); + let failed = 0; for (const run of cases) { const result = run(); diff --git a/web/src/lib/review/room-ui.ts b/web/src/lib/review/room-ui.ts index e78a324..33b5c69 100644 --- a/web/src/lib/review/room-ui.ts +++ b/web/src/lib/review/room-ui.ts @@ -122,3 +122,27 @@ export function roomDisplayName( const prefix = dir ? `${baseName(dir)}/ ` : ''; return `${prefix}(${paths.length} files)`; } + +/** + * Does `target` fall under the path a room was shared for? + * + * The owner shares either a single file or a whole folder. A folder share is + * recognized for the folder itself AND for any markdown file beneath it, so + * opening the Share dialog on a child of a shared folder re-shows the existing + * invite instead of minting a fresh room. A different, unshared file (including + * a sibling whose name merely shares a prefix) does NOT match — the prefix test + * is path-segment aware (`P + '/'`). + * + * Both sides are trailing-slash-normalized so a folder share recorded as + * '/p/dir' or '/p/dir/' both match the child '/p/dir/child.md'. Empty/nullish + * inputs never match (no active share / no target selected). + */ +export function shareTargetMatches( + sharePath: string | null | undefined, + target: string | null | undefined, +): boolean { + const p = (sharePath ?? '').replace(/\/+$/, ''); + const t = (target ?? '').replace(/\/+$/, ''); + if (p.length === 0 || t.length === 0) return false; + return t === p || t.startsWith(`${p}/`); +} diff --git a/web/src/lib/review/store.svelte.ts b/web/src/lib/review/store.svelte.ts index 1fd280c..7a3f361 100644 --- a/web/src/lib/review/store.svelte.ts +++ b/web/src/lib/review/store.svelte.ts @@ -81,6 +81,7 @@ export interface ReviewRoomSummary { roomId: RoomId; inviteUrl: string; ownerSigningKey: string; + ownerDisplayPath: string; mode: 'live' | 'async' | 'hybrid'; expiresAt: number; }; @@ -163,6 +164,7 @@ export class ReviewStore { roomId: RoomId; inviteUrl: string; ownerSigningKey: string; + ownerDisplayPath: string; mode: 'live' | 'async' | 'hybrid'; expiresAt: number; } | null>(null); @@ -514,6 +516,7 @@ export class ReviewStore { applyShareReady(payload: { roomId: RoomId; inviteUrl: string; + ownerDisplayPath: string; ownerSigningKey: string; mode: 'live' | 'async' | 'hybrid'; expiresAt: number; @@ -522,10 +525,14 @@ export class ReviewStore { const nextDismissed = new Set(this.dismissedRoomIds); nextDismissed.delete(payload.roomId); this.dismissedRoomIds = nextDismissed; + // `ownerDisplayPath` is the path the daemon actually shared, carried on the + // ShareReady payload — so the dialog's shareTargetMatches gate works for any + // share (GUI or `attn review share`) without a frontend-captured intent. const share = { roomId: payload.roomId, inviteUrl: payload.inviteUrl, ownerSigningKey: payload.ownerSigningKey, + ownerDisplayPath: payload.ownerDisplayPath, mode: payload.mode, expiresAt: payload.expiresAt, }; diff --git a/web/src/lib/types.ts b/web/src/lib/types.ts index 8588d7a..452a320 100644 --- a/web/src/lib/types.ts +++ b/web/src/lib/types.ts @@ -875,6 +875,9 @@ export interface ReviewShareReady { kind: 'share_ready'; roomId: RoomId; inviteUrl: string; + /** Absolute path the owner shared (file or folder); the dialog matches it + * against the active target to recognise its own room. */ + ownerDisplayPath: string; /** Base64url Ed25519 public signing key; fingerprint is sha256(this). */ ownerSigningKey: string; mode: 'live' | 'async' | 'hybrid';