Fix the owner's review rail going blank on an uncurated share - #19
Merged
Conversation
`attn review share <dir>` records a root and no curated file list. Since attn-x2zq its snapshots travel under root-relative wire paths like every other share — but `local_owner_display_path`, which turns those back into absolute paths for the owner's own window, still bailed the moment the curated list was empty. So the owner's store held `hosted.md` where the window expected `/abs/path/hosted.md`. `ownerFileIdForPath` matched nothing, the focus-following effect in App.svelte concluded the open file was unshared and called `setCurrentFile(null)`, and the review rail rendered nothing — while the comments sat correctly in the store the whole time. Reviewers' comments arrived, decrypted, and persisted, and the owner simply never saw them. attn-x2zq changed the forward direction and left the inverse behind. This mirrors it: the root is the shared directory or a shared file's own parent, membership of the curated list authorises a name when there is one, and containment in the root authorises it when there is not. Wire paths are rebuilt one plain segment at a time, so one that tries to climb out of the root resolves to nothing rather than to a file beside it. Fixing the file-share root also repairs a curated single-file share, where the old code took `record.path` — the file itself — as the root and errored out of `normalized_relative_share_path` instead of matching. Why this reached main: the native Share dialog always sends a selection, so interactive use never took the broken path, and the only coverage was the curated case. The half with no coverage is the half that shipped. The new test is mutation-checked — restoring the old bail fails it and nothing else. Verified: scripts/test-hosted-review-e2e.sh against staging goes 3 passed / 1 failed → 4 passed, and the previously failing assertion is the owner's rail rendering a browser-authored comment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015TnncCnVNhggoZ9QLQiqeW
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
angusbezzina
added a commit
that referenced
this pull request
Sep 3, 2026
…3 .4) The staging deploy broke on 2026-09-03 for two reasons that had nothing to do with product code, and the pin that unblocked it was a workaround. This removes the causes. Installs no longer touch GitHub. web/package.json depended on three icon packs as `github:` refs — eyecons, catppuccin-vsc-icons, vscode-icons. The lockfile pinned each repo's commit, but npm still cloned each one and resolved THAT project's floating package.json against the live registry on every install, so an unrelated upstream publish (eslint 10 landing beside plugins capped at 9) could walk npm 10's arborist into a null node. None of the three is usable from npm: `eyecons` there is an unrelated 0.0.1 placeholder and the other two are unpublished. So the GENERATED output is now committed — 2,776 SVGs, 4.5 MB — and the packs are fetched as pinned source tarballs only when someone refreshes them (`npm run refresh:icons`). The build reads committed files and needs no pack at all, which is why `generate:icons` leaves `build`, `build:browser` and `check`. Committing output rather than vendoring sources costs ~1 MB more and takes generation off the build path entirely. Verified byte- identical to what the git-dep build produced at the same pins: every SVG, every pack module and the manifest are unchanged, and only the README differs because it now records each pack's commit and licence. Since generation no longer guards itself, `check:icons` verifies the committed tree is internally consistent — every icon a pack module imports exists, no orphans, manifest loads every pack. It deliberately does not re-run the generator: it has to pass with nothing fetched. One Node version. ci.yml ran all four jobs on 20 while the deploy ran 22 (wrangler 4 refuses older), so CI never exercised the npm the deploy uses — which is precisely why the arborist crash was invisible until a deploy. CI moves to 22, .nvmrc declares it, and web/package.json says engines >=22. The root package.json stays >=18 on purpose: `attnmd` is the end-user CLI wrapper and its consumers should not inherit our build floor. The npm@11 pin is gone from the deploy job, now that the thing it worked around is gone. Parity now gates PRs. scripts/test-hosted-review-e2e.sh only ever ran inside the dispatch-only deploy workflow, which is how an owner-side regression (empty review rail on uncurated shares, #19) survived two merges and a version bump. It runs in ci.yml in local mode — its own relay, Vite and owner — so it needs no deployed environment. Verified it catches the thing it exists for: re-introducing the #19 bug fails that case through every retry while the rest pass. Two reliability fixes found while getting there. `kill_pid` killed the npm wrapper but not the vite child it forks, so a run leaked a server that the next run then silently tested against — it now kills the tree, and refuses to start when a port it needs is already held. And the suite retries in CI only: everything shares one box there, and three local runs failed three different assertions, none reproducible. A deterministic break still fails every attempt, as the #19 revert confirms. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015TnncCnVNhggoZ9QLQiqeW
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.
Root cause of the
Verify native-browser parity on stagingfailure that has blocked the web staging deploy. Deterministic, reproduced locally, fixed and verified.Symptom
e2e/hosted-review.spec.ts:712— a comment posted in the hosted reviewer arrives in the native owner's event log but never renders in its review rail.The spec has not changed since v0.10.0, and this step passed on Aug 21. The regression entered in the window since.
What is actually broken
attn review share <dir>records a root and no curated file list. Since attn-x2zq (8991adb) its snapshots travel under root-relative wire paths like every other share. Butlocal_owner_display_path— the inverse, which restores an absolute path for the owner's own window — still returned early whenever the curated list was empty.The chain, confirmed by instrumenting the live daemon:
snapshot.ownerDisplayPathstayshosted.md; the owner's open tab is/private/tmp/.../shared/hosted.md.ownerFileIdForPathcompares them with===and returnsnull.$effect(App.svelte:1097) concludes the open file is unshared and callssetCurrentFile(null), which also nulls the snapshot.threadsForSnapshotreturns[]and the rail renders nothing.Everything upstream is healthy — the comments decrypt, persist, and sit in the store with correct anchors. The owner just never sees them.
Fix
Mirror the forward function attn-x2zq already changed: the root is the shared directory or a shared file's own parent; membership of the curated list authorises a name when there is one, containment in the root when there is not. Wire paths are rebuilt one plain segment at a time, so one that tries to climb out of the root resolves to nothing rather than to a file beside it.
This also repairs a curated single-file share, where the old code took
record.path— the file itself — as the root and errored out ofnormalized_relative_share_pathinstead of matching.Verification
scripts/test-hosted-review-e2e.shagainst staging: 3 passed / 1 failed → 4 passed.cargo test --lib review::— 595 passed.Why it reached main
The native Share dialog always sends a selection, so interactive use never took the broken path —
8991adb's own commit message notes this. The only coverage was the curated case; the uncovered half is the half that shipped.Worth a follow-up:
ci.ymldoes not run this parity suite — only the dispatch-only deploy workflow does — so a regression in the native review rail can sit on main indefinitely without CI noticing.🤖 Generated with Claude Code
https://claude.ai/code/session_015TnncCnVNhggoZ9QLQiqeW