Snapshot authorship, click targeting, and shares without a file list

scripts/test-e2e.sh

@@ -77,6 +77,35 @@ assert_truthy() {
     fi
 }
 
+# Poll `--eval` until it returns something truthy, or give up after ~4s.
+# Defined up here rather than beside its first use so every suite can reach it.
+poll_eval() {
+    local js="$1"
+    local tries=0
+    local out=""
+    while [ "$tries" -lt 40 ]; do
+        out=$("$ATTN" --eval "$js" 2>/dev/null || echo "")
+        case "$out" in
+            ''|null|'""'|false|0) ;;
+            *) echo "$out"; return 0 ;;
+        esac
+        sleep 0.1
+        tries=$((tries + 1))
+    done
+    echo "$out"
+}
+
+# Wait for the document heading to actually BECOME `$1`.
+#
+# `--wait-for h1` cannot do this: an h1 from the previously-open document
+# already satisfies it, so it returns instantly and whatever fixed sleep
+# follows is racing the navigation. Losing that race made "Navigate to
+# basic.md" read the previous file's heading (attn-537h).
+wait_for_heading() {
+    local expected="$1"
+    poll_eval "(document.querySelector('h1')?.textContent || '').includes('$expected') ? 'yes' : null" >/dev/null
+}
+
 screenshot() {
     local name="$1"
     local path
@@ -233,10 +262,8 @@ echo "--- Navigate Between Files ---"
 # Click basic.md in the sidebar
 "$ATTN" --click 'text=basic.md'
 
-# Wait for navigation to complete — h1 should contain "Project Status"
-"$ATTN" --wait-for 'h1' --timeout 5000 >/dev/null 2>&1
-# Give rendering a moment to settle after navigation
-sleep 0.3
+# Wait for the heading to become basic.md's, not merely for AN h1 to exist.
+wait_for_heading "Project Status"
 result=$("$ATTN" --query 'h1' | jq -r '.elements[0].text' 2>/dev/null || echo "")
 assert_contains "Navigate to basic.md" "$result" "Project Status"
 screenshot "05-navigate-basic"
@@ -255,15 +282,31 @@ sleep 0.3
 # and the row text contains the filename — `text=` matches text content.
 "$ATTN" --click 'text=child.md'
 
-# Wait for navigation to complete
-"$ATTN" --wait-for 'h1' --timeout 5000 >/dev/null 2>&1
-sleep 0.3
+wait_for_heading "Nested Document"
 result=$("$ATTN" --query 'h1' | jq -r '.elements[0].text' 2>/dev/null || echo "")
 assert_contains "Navigate to nested child.md" "$result" "Nested Document"
 
-# Verify breadcrumb shows nested path
-result=$("$ATTN" --query '[class*="breadcrumb"], nav[aria-label]' | jq -r '.elements[0].text // ""' 2>/dev/null || echo "")
-assert_contains "Breadcrumb shows nested path" "$result" "child.md"
+# The nested file is the one the app considers open.
+#
+# This replaces an assertion on a breadcrumb. The native app has no breadcrumb
+# and has not had one for some time: `web/src/lib/PathBreadcrumb.svelte` is
+# imported by nothing, and a live window reports zero matches for
+# `[class*=breadcrumb]`, `nav[aria-label]` and `[data-slot*=breadcrumb]`. The
+# old selector could only ever find nothing, so the case was asserting the
+# absence of a control rather than any behaviour (attn-537h).
+#
+# What it was reaching for — "the app is showing the nested file" — is real and
+# observable: the sidebar marks the open row `data-active="true"` and carries
+# its full path in `data-path`.
+# `--eval` hands back a JSON-encoded string, which escapes the separators —
+# the path arrives as `...\/nested\/child.md`, so a bare `nested/child.md`
+# never appears in it. Strip the escaping rather than assert on the escaped
+# spelling, so the message still shows a readable path when this fails.
+result=$(poll_eval "(() => {
+    const row = document.querySelector('[data-path][data-active=\"true\"]');
+    return row ? row.getAttribute('data-path') : null;
+})()" | tr -d '\\')
+assert_contains "Nested file is the active sidebar row" "$result" "nested/child.md"
 screenshot "06-nested-file"
 
 # ===================================================================
@@ -276,22 +319,6 @@ echo "=== Test Suite 3: Relative Images (images.md) ==="
 # Poll a synchronous eval until it stops returning `null`/empty. `--eval` hands
 # back whatever the expression evaluates to, JSON-encoded, and does NOT await a
 # Promise — so waiting has to happen out here, not in the page.
-poll_eval() {
-    local js="$1"
-    local tries=0
-    local out=""
-    while [ "$tries" -lt 40 ]; do
-        out=$("$ATTN" --eval "$js" 2>/dev/null || echo "")
-        case "$out" in
-            ''|null|'""'|false|0) ;;
-            *) echo "$out"; return 0 ;;
-        esac
-        sleep 0.1
-        tries=$((tries + 1))
-    done
-    echo "$out"
-}
-
 start_daemon "$FIXTURES/images.md"
 
 # Single-file mode, so directory ordering in tests/fixtures/ is irrelevant here.

src/daemon.rs

@@ -1361,6 +1361,35 @@ function __resolve(sel) {
     }
     return Array.from(document.querySelectorAll(sel));
 }
+
+// Which of several `text=` matches should a CLICK land on.
+//
+// A text match is satisfied by every element whose trimmed text equals the
+// target, which for one sidebar row is the <li>, the <button> inside it, and
+// the <span> inside that. Taking the first in document order takes the <li> —
+// an element with no click handler — so the click reported success and nothing
+// happened. That is how "Navigate to basic.md" failed while its sibling case
+// passed vacuously: the document had not moved for either, and the sibling
+// happened to already be open (attn-537h).
+//
+// Prefer the innermost INTERACTIVE match, since that is the thing a person
+// would have clicked. Fall back to the innermost match of any kind, whose
+// click still bubbles to whatever handler is above it.
+function __best(els) {
+    if (els.length < 2) return els[0];
+    var interactive = els.filter(function (el) {
+        return el.matches('button, a, [role=button], input, select, textarea, label, [data-sidebar="menu-button"]');
+    });
+    var pool = interactive.length > 0 ? interactive : els;
+    // Innermost = the one containing no other candidate.
+    for (var i = 0; i < pool.length; i++) {
+        var containsAnother = pool.some(function (other) {
+            return other !== pool[i] && pool[i].contains(other);
+        });
+        if (!containsAnother) return pool[i];
+    }
+    return pool[0];
+}
 "#;
 
     match action {
@@ -1371,7 +1400,7 @@ function __resolve(sel) {
 {resolve_fn}
 var els = __resolve({sel_json});
 if (els.length === 0) return JSON.stringify({{status:'not_found',selector:{sel_json}}});
-els[0].click();
+__best(els).click();
 return JSON.stringify({{status:'ok'}});
 }})()"#,
             )

src/review/assets.rs

@@ -35,18 +35,19 @@ const IMAGE_EXTENSIONS: &[(&str, &str)] = &[
     ("jpeg", "image/jpeg"),
     ("gif", "image/gif"),
     ("webp", "image/webp"),
+    ("avif", "image/avif"),
     ("bmp", "image/bmp"),
     ("ico", "image/x-icon"),
     ("svg", "image/svg+xml"),
 ];
 
 /// Largest single asset that may be published. Generous for a screenshot or a
 /// diagram, small enough that one pathological file cannot dominate a share.
-pub const MAX_ASSET_BYTES: u64 = 8 * 1024 * 1024;
+pub const MAX_ASSET_BYTES: u64 = 3 * 1024 * 1024;
 
 /// Ceiling on everything one document contributes. A snapshot is base64url'd
 /// into an encrypted envelope, so the wire cost is ~4/3 of this.
-pub const MAX_TOTAL_BYTES: u64 = 32 * 1024 * 1024;
+pub const MAX_TOTAL_BYTES: u64 = 16 * 1024 * 1024;
 
 /// Ceiling on how many assets one document contributes, so a generated file
 /// with a thousand thumbnails cannot stall a share.

src/review/bootstrap.rs

@@ -4703,27 +4703,46 @@ fn selected_share_wire_path(
     let Some(record) = all.get(room_id.as_str()) else {
         return Ok(None);
     };
-    if record.selected_paths.is_empty() {
-        return Ok(None);
-    }
-    let canonical_root = std::path::Path::new(&record.path)
-        .canonicalize()
-        .map_err(|error| {
-            BootstrapError::Store(format!(
-                "canonicalize selected share root {}: {error}",
-                record.path
-            ))
-        })?;
+    // A share with no curated file list still has a root, and every file it
+    // publishes still needs a portable name (attn-x2zq).
+    //
+    // Returning None here meant `attn review share <path>` — and the legacy
+    // share(path) wrapper, which passes an empty selection — produced a share
+    // whose images could not travel at all: publish_asset_snapshot fails
+    // without a wire path, so the bytes were scanned, approved, and then
+    // dropped with only a per-image log line. It was invisible in testing
+    // because the native Share dialog always sends a selection.
+    //
+    // The root is the shared directory, or a shared file's own parent. Files
+    // outside it are still refused, by normalized_relative_share_path below.
+    let record_root = std::path::Path::new(&record.path);
+    let root_for_wire = if record.is_dir {
+        record_root.to_path_buf()
+    } else {
+        record_root
+            .parent()
+            .map(std::path::Path::to_path_buf)
+            .unwrap_or_else(|| record_root.to_path_buf())
+    };
+    let canonical_root = root_for_wire.canonicalize().map_err(|error| {
+        BootstrapError::Store(format!(
+            "canonicalize selected share root {}: {error}",
+            root_for_wire.display()
+        ))
+    })?;
     let canonical_path = path.canonicalize().map_err(|error| {
         BootstrapError::Store(format!(
             "canonicalize shared file {}: {error}",
             path.display()
         ))
     })?;
-    if !record
-        .selected_paths
-        .iter()
-        .any(|selected| std::path::Path::new(selected) == canonical_path)
+    // With a curated list, membership of it is what authorises a wire path.
+    // Without one, containment in the share root is.
+    if !record.selected_paths.is_empty()
+        && !record
+            .selected_paths
+            .iter()
+            .any(|selected| std::path::Path::new(selected) == canonical_path)
     {
         return Ok(None);
     }
@@ -5727,6 +5746,64 @@ mod tests {
         }
     }
 
+    #[test]
+    fn a_share_without_a_curated_list_still_mints_wire_paths() {
+        // attn-x2zq. Returning None here meant `attn review share <path>` and
+        // the legacy share(path) wrapper published a share whose images could
+        // not travel: publish_asset_snapshot needs a portable name, so the
+        // bytes were scanned, approved, and then dropped. Invisible in testing
+        // because the native Share dialog always sends a selection.
+        let root = TempDir::new().expect("tmp");
+        let store_root = root.path().join("store");
+        std::fs::create_dir_all(&store_root).expect("store dir");
+        let docs = root.path().join("docs");
+        std::fs::create_dir_all(docs.join("nested")).expect("docs dir");
+        std::fs::write(docs.join("notes.md"), b"# hi").expect("doc");
+        std::fs::write(docs.join("chart.svg"), b"<svg/>").expect("asset");
+        std::fs::write(docs.join("nested/diagram.png"), b"\x89PNG").expect("nested asset");
+
+        let room: RoomId =
+            serde_json::from_value(serde_json::Value::String("room-no-selection".into())).unwrap();
+
+        // A FOLDER share: the root is the folder itself.
+        record_local_share(&store_root, &room, &docs, true).expect("record dir share");
+        assert_eq!(
+            selected_share_wire_path(&store_root, &room, &docs.join("chart.svg"))
+                .expect("wire path")
+                .as_deref(),
+            Some("chart.svg"),
+            "an asset beside the document gets a portable name"
+        );
+        assert_eq!(
+            selected_share_wire_path(&store_root, &room, &docs.join("nested/diagram.png"))
+                .expect("wire path")
+                .as_deref(),
+            Some("nested/diagram.png"),
+            "and keeps its subdirectory"
+        );
+
+        // A SINGLE-FILE share: the root is that file's own parent, so its
+        // siblings are still nameable.
+        let file_room: RoomId =
+            serde_json::from_value(serde_json::Value::String("room-single-file".into())).unwrap();
+        record_local_share(&store_root, &file_room, &docs.join("notes.md"), false)
+            .expect("record file share");
+        assert_eq!(
+            selected_share_wire_path(&store_root, &file_room, &docs.join("chart.svg"))
+                .expect("wire path")
+                .as_deref(),
+            Some("chart.svg"),
+            "a single-file share roots at the file's directory"
+        );
+
+        // Containment still holds: nothing above the root is nameable.
+        std::fs::write(root.path().join("outside.svg"), b"<svg/>").expect("outside");
+        assert!(
+            selected_share_wire_path(&store_root, &room, &root.path().join("outside.svg")).is_err(),
+            "a file outside the share root has no wire path"
+        );
+    }
+
     #[test]
     fn v3_fragment_without_an_owner_key_still_parses() {
         // Backward skew: an invite minted before attn-lb7p has no owner key.