SPEC-46 P1: Docs screen, preview, and tailnet publishing - #158
Merged
Conversation
Docs preview (mockups, specs, plans) from the phone and desktop, replacing the Finder-then-browser dance and the "ask the agent for a port" workflow. This is the first slice: the spec, the design board, and the two pure modules the rest of the feature is built on. `resolveDocPath` is the single way a path reaches the serving layer, so both `docs.read` and the static route cannot disagree about what is servable. It refuses traversal, absolute paths, escaping symlinks, dotfiles, excluded build dirs, non-allowlisted extensions and oversize files, and it never throws — a rejection is a value. Containment is checked by path *segment*, not string prefix, so `/repo-evil` cannot pass as inside `/repo`. `readDocMeta` gives a document a human name, because 2026-08-07-SPEC-44-ports-forward.md is unreadable on a 375pt row while the real title is already in the file. Reads a bounded 64 KB prefix rather than the whole document. Two things a live probe over all 131 real docs changed: - an H2 fallback when a document has no H1 (three files here start at `##`; their heading beats their filename). 130/131 now yield a real title. - no front-matter `name:` extraction: all 22 SKILL.md files already have an H1, so it would have been speculative. Mutation-tested: the prefix-confusion test was vacuous as first written (the `..` segment rule rejected the input before the containment check ran), so it now reaches that check through a symlink instead. Refs: docs/specs/2026-08-09-SPEC-46-doc-preview.md, mockups/doc-preview.html
DocDTO / DocsSnapshotDTO / DocGrantDTO, the five `docs.*` commands, and `docs.snapshot` as a host-only broadcast kind. Committed on its own so the server and app halves can be built in parallel without both editing protocol.ts. Two corrections this slice forced: D11 was wrong. It claimed no new event kind was needed, citing SPEC-41 — but SPEC-41 is precisely what introduced `ports.snapshot`; SPEC-44 is the spec that adds none. `docs.snapshot` now follows ports.snapshot exactly: excluded from `SessionEventKind` and present in `HOST_ONLY_KIND_FLAGS`, so a machine-wide index can never be persisted into a session's append-only log. `EVENT_KINDS` was an unguarded list. Mutation-testing the new contract test showed one of its assertions was vacuous: `decodeFrame` validates only v/t/id and never inspects `kind`, and `decodeSessionEvent` rejects a host-only kind for being host-only before EVENT_KINDS is consulted — so a missing entry there is undetectable at runtime, and my test's stated reason was false. Rather than keep an assertion that cannot fail, EVENT_KINDS is now derived from an `EVENT_KIND_FLAGS: Record<EventKind, true>`, the same compiler-enforced pattern HOST_ONLY_KIND_FLAGS already uses (finding 26). Dropping a kind is now a build error instead of silence, and the test says plainly what it can and cannot prove. Contract fixture lives in snapshots.json (not events.json, which asserts one entry per *session* kind), and covers an HTML board, a spec with a parsed docStatus, and a doc with no optional fields set — so "absent" cannot silently become `false`. 1150/1150 server tests green, tsc clean.
The Option D frame drew /docs/<branch>/<path>. That cannot work: a URL that must open in Safari cannot carry a bearer header, so the capability has to be in the path — /docs/<grantId>/<relPath>, 32 bytes of CSPRNG. Annotated in the frame rather than quietly redrawn, so the reasoning survives.
roots/scan/changed/grants/route/publish/read/service plus the `docs.*` command handlers and server wiring. Built by a subagent under the frozen contract; the notes below are what my own review of it changed. Two defects found by live probes, not by the tests: The dedicated doc listener hung on every unmatched path. `attachDocRoute` ignores non-/docs paths by design, so it can share a listener — but the doc listener is dedicated, so nothing answered `/`, `/docs`, `/etc/passwd` or `/favicon.ico` and the socket sat until Node's 60s headers timeout. Safari requests /favicon.ico on every visit and the listener is bound to a routable address, so that was a free socket-exhaustion vector. `attachDocNotFound` now terminates anything the route declined, attached only on the dedicated listener. Published URLs were not percent-encoded. A document with a space produced a malformed URL, and one containing `#` or `?` truncated the path — the route then compared a different relPath against the grant and returned 404, i.e. the publish button silently produced a dead link for an ordinarily-named file. Now encoded per segment, so slashes survive; proven end-to-end against a real listener with `mockups/my notes #2 & draft.md`. Also corrected publish.ts's doc comment, which described the tailscale-serve design that was not what got implemented (see the D10 deviation below). Verified myself rather than taking the report's word: tsc clean, 1219/1219 (baseline 1150), and a live probe serving a real file over a real socket — 200 for a valid grant, 404 for a wrong grantId, 404 for traversal. Known deviation, still open: D10 specifies a loopback-only listener fronted by `tailscale serve`; the implementation binds the doc listener directly to makit's routable host over plain HTTP. That resolves a real D10/D15 conflict the subagent correctly identified (a loopback-only listener cannot have a LAN fallback), but it drops the stable https ts.net hostname that was Option D's stated benefit and leaves a routable listener open even when nothing is published. Not yet resolved — tracked for the next slice.
Resolves the conflict a subagent correctly found in rev 1: D10 said the doc listener was loopback-only fronted by `tailscale serve`, while D15 promised a LAN fallback. Both cannot hold — a loopback-only listener is unreachable over the LAN — so the fallback was impossible as written. Resolved toward the tighter option rather than the more convenient one: Tailnet only. The capability lives in the URL path (D9), so on a LAN that URL crosses the wire in cleartext and anyone sniffing the Wi-Fi could replay it for the grant's lifetime. On the tailnet WireGuard already encrypts it. No tailnet address now means publish refuses with a stated reason. `reach` keeps its `"tailnet" | "lan"` union so the wire contract and the app's pills need no change if LAN is ever reinstated behind an explicit opt-in. Bound lazily, released when idle. rev 1 bound a routable port at startup and held it for the life of the server whether or not anything was ever published. It now binds on the first publish and closes when the last grant is revoked or reaped — and because an expiry has no event of its own, `grants()` signals too, so a TTL expiry frees the port without a timer. No `tailscale serve`. It would buy a stable https ts.net hostname, but the URL is never typed by a human — it is tapped, copied, or scanned from a QR — so it does not justify a setup/teardown lifecycle on the user's machine. Recorded in D10 rev 2 with that reasoning rather than left as an undocumented deviation. Proven by live probe, not just unit tests: zero listening sockets before any publish, one while published (fetch → 200), zero after unpublish with the URL then refusing the connection. 1227/1227 server tests green, tsc clean.
Store, Docs screen (grouped repo → worktree, filters, search), the doc row, the
markdown preview widget, the publish sheet with QR, the desktop sub-row glyph +
popover, and the mobile worktree-row glyph. Built by a subagent that hit its
turn limit mid-edit, so everything below is my own verification.
One dead-end bug it shipped, caught by reading the wiring rather than the tests:
the Docs screen was UNREACHABLE on mobile. The phone's only entry point is the
worktree-row glyph, that glyph hides when the worktree owns no docs, and nothing
on the home screen held `docs.watch` — so no snapshot ever arrived, the list
stayed empty, and the glyph never rendered. Every widget test missed it because
they inject `docsProvider` directly instead of letting it arrive over the wire.
The row now holds the ref-counted watch exactly as it already holds
`ports.watch` for the plug, with a test that fails when the single `watch()`
line is removed.
The agent also reverted its own mobile app-bar Docs button after finding it
overflows the 320pt app bar as a 6th control — the right call, and the reason
the row glyph is the mobile entry. The 320pt overflow test still passes with the
new glyph mounted.
Verified rather than reported: `flutter analyze --fatal-infos` clean; three full
suite runs with ZERO non-loading failures; the failing sets were random (92
distinct files across 3 runs, none failing in all three, no docs test failing
consistently) and every flagged file passes in isolation — the known
pre-existing `loading <file> [E]` flake, which persists at --concurrency 1.
Traps checked explicitly: `docs.watch` uses fire-and-forget `send`, not
`request` (no leaked 10s ack timers); desktop opens docs via a popover, not
`context.go`, so it does not depend on a GoRouter the desktop shell has not got;
the mobile `context.go(kRouteDocs)` is on a mobile-only widget and the route is
registered.
Pre-existing, NOT touched: `desktop_sidebar.dart` still calls
`context.go('$kRoutePorts?repo=…')` for the SPEC-42 ports menu item, which has
no GoRouter on desktop and will throw at runtime. Unrelated to this change.
The Docs screen was empty against the real server — 0 docs where a direct
`scanWorktree` found 131 — and it never recovered.
The app sends `docs.watch {on:true}` from the home screen's initState, so it
lands immediately after `hello.ack`, BEFORE the server's first `repos.snapshot`.
The doc index is keyed off that worktree list, so the 0→1 edge (which scans
synchronously, no debounce) walked zero worktrees and broadcast an empty index.
Nothing else triggered a re-walk: the only re-index trigger was the filesystem
watcher, and no file had changed. On mobile that compounded into the dead end
fixed in the previous commit — an empty index means the row glyph never renders,
so the screen is unreachable.
`lastGitOnlyRepos = gitOnly` now also calls `docsService.onWorktreeChange()`,
debounced inside the service. The worktree list is an input to the index, so a
change to it must re-walk, exactly as a file change does.
Found by driving the real WSS server with the app's own timing, not by tests —
every unit test supplied a worktree list up front, so none could see it. Guarded
now by a service test that fails if a later worktree list does not produce a
fresh walk (proven by memoizing the list and watching it fail).
Verified end to end against the real stub server with the app's timing:
snapshot #1 empty (repos not yet loaded) → snapshot #2 with 1008 docs across 8
worktrees, scanOk=true, 225 html / 783 md, 393 carrying a parsed Status, 11
changed against their merge base, real extracted titles, and no dotfile,
`.git/` or `node_modules/` path present. Publish correctly refuses on the
loopback dev server (D15: no tailnet address ⇒ share nothing).
1228/1228 server tests green, tsc clean.
`pnpm typecheck` failed at HEAD with two TS7006 errors in the test P1e added. The deps object was passed as `} as never)`, which suppresses contextual typing for the whole argument, so `onSnapshot: (s) =>` and `scan: (worktreePath) =>` had no inferred parameter types and tripped `noImplicitAny`. The casts were load-bearing for nothing: this file's own `makeService` helper (line 38) builds the same deps with no casts at all and infers cleanly. Dropping them fixes the build AND restores real checking of the deps this test asserts against — `as never` had been hiding the shape, which is how a `stdout`-only `exec` (no `stderr`) got past review. 12/12 docs service tests still pass, 1228/1228 server tests green, tsc clean. Note, not touched: the test above it (~line 205) uses the same `as never` idiom. It does not error today only because its callbacks take no parameters, so it is a latent version of this same failure.
Found by driving the real app against a loopback-bound server: the publish
refusal read "makit is loopback-only. Start Tailscale (or pass --lan) and try
again." Passing --lan does not help. It binds a 192.168/10.x host, and the doc
listener only ever binds a tailnet address:
tailnetAddressFromBindHost 127.0.0.1 -> null (refuses)
192.168.1.9 -> null (refuses)
10.0.0.5 -> null (refuses)
100.119.58.97 -> ok
So the message sent the user down a road that ends in the same refusal. D15
rev 2 dropped the LAN fallback on purpose — the capability lives in the URL
path (D9), which on a LAN would cross the wire in cleartext — and D15's whole
point is that failure is "stated, never silent". A remedy that cannot work is
the one thing it forbids: the refusal was lying.
The existing D15 test only asserted `reason.length > 0`, which is how this got
through review. The new test asserts the content: no "--lan", and Tailscale —
the one remedy that does work — is named.
1229/1229 server tests green, tsc clean. Re-verified on the simulator: the
sheet now reads "Start Tailscale and try again."
The preview hoisted the Status/Priority/Branch chips above the document's H1, so on every spec in the repo the metadata outranked the title. The file itself is written the other way round — H1 on line 1, `**Status:** …` on line 3 — and mockup Card 6 draws title, then the strip, then the divider. The toolbar already carries the title, so putting metadata first bought nothing. `parseDocFrontMatter` knew where the line was and threw that away: it removed the line and returned one body, which left the caller no way to render the strip in place. It now returns the markdown either side — `lead` (in this repo, the H1) and `body` — and the preview renders lead, chips, body. Two details worth naming: - The old "collapse a doubled blank line" fix-up is gone; splitting at the line and trimming blank edges makes it unnecessary. - Blank edges are trimmed with `^\n+` / `\n+$` rather than `String.trim()`, so a body that opens with an indented code block keeps its indentation. There are now two MarkdownBody widgets when a doc has front matter, which is why `renders the markdown body` asserts findsNWidgets(2). Regression test asserts geometry, not structure: the chip strip's dy must be greater than the H1's, so re-hoisting the chips fails the test. A second test pins the no-front-matter path, which must stay a single unchanged body. 46/46 app docs tests green, flutter analyze clean. Verified on the iOS simulator against the real server: H1, then chips, then Design board, then the rule — the order mockup Card 6 draws.
…vigable Reported from the popover on `teachme`: only three root `.md` files showed. That was D1 working as written — the index walked `mockups/`, `docs/` and root `*.md` — and D1 was written against this repo's layout. Measured: teachme 3 docs indexed, 69 present (docs live in flutter/…, ssf/…) makit 132 docs indexed, 143 present makit's sidebar holds many repos, so one repo's convention is the wrong default. D1 rev 2 asks git instead: every `.md`/`.html` from `git ls-files --cached --others --exclude-standard`, so tracked and freshly-written untracked docs both appear, wherever they live. Deferring to `.gitignore` also *tightens* the security boundary — a gitignored `secrets.md` can no longer be indexed or served, where rev 1 indexed it happily, since the dotfile rule never covered it. Dot-directories still drop out via D2, so `.agents/skills/**/SKILL.md` stays out with no opt-in needed (24 machine-facing files that would have drowned 30 boards). Extension filtering happens in the lister, not at the boundary, so a large repo does not pay a realpath+stat per non-document file it tracks. `.makit/docs.json` `roots` inverts meaning: the index is broad by default, so naming roots is now how a project narrows it. That must beat git's list or there would be no way to opt out of the breadth — pinned by a test. A worktree git cannot answer for (not a repository) still falls back to rev 1's walk, so a plain directory does not silently show zero docs; the pre-existing scan tests now exercise that path, since their fixtures are not repos. Rows show the full path, truncated from the LEFT — `…/learning-records/0006.md` rather than `/Users/le/Work/teachme/flutter/learning-…`. For a root-level file the old relPath subtitle just repeated the title (`Notes` / `NOTES.md`) and said nothing about location. The path is wrapped in an LTR isolate (U+2066/U+2069) because the RTL paragraph that moves the ellipsis to the visual start would otherwise reorder the leading `/` to the wrong end. The popover also stopped scaling: it built one row per doc in a Column and grew to the window's height. It is now a lazy ListView capped near a dozen rows, with a search field over titles and paths and a real empty state. A fixed itemExtent was tried and reverted — it overflowed by 5px the moment a row carried an extra badge, so rows size naturally and only the panel is capped. 1233/1233 server tests, 39 app docs tests, tsc + analyze clean. Verified against the real repos: teachme 3 → 69, this worktree 132 → 143, no dotfile, .git, node_modules or build path in either.
The board drew the popover as "3 recent" with an "All 132 docs…" escape hatch to the full screen. That design assumed a small index. D1 rev 2 broke the assumption: this worktree now indexes 143 documents and teachme 69, and the three most recent are almost never the one you are reaching for — so the popover became a dead end for its own primary job. Redrawn: a search field over titles and paths, a list capped near a dozen rows that scrolls, a `12 of 143` count while filtering (so an empty result reads as a filter, not an empty repo), and worktree-relative paths truncated from the left — the header already names the worktree, and the filename is the part being read. The rationale paragraph records why, including that the global Docs screen keeps absolute paths because its worktree headers are not sticky. Numbers are this worktree's throughout the popover; a first pass mixed teachme's 69 into a frame labelled feat/serving-html. Not touched: the iPhone frame in Card 2 still reads "132 files · 3 worktrees" and "All 132 / Mockups 27 / Specs 70". Those predate rev 2 and are now understated.
c10fbb8 fixed the unwrap but left the hole that produced it: the shape was asserted nowhere. This is the second wire-shape mismatch on this feature to reach a real device with both suites green, and the mechanism is the same each time — the two sides are tested against different fakes: server test reads ack.grant.grantId (nested — correct) app test built a DocGrant directly (never parsed an ack at all) So nothing failed until a real publish returned "an unusable grant". `DocGrant.fromAck` now owns the nesting, tested against the actual frame: the nested shape parses, a flat ack is refused rather than half-parsed, and a missing or malformed grant is refused. store.dart delegates instead of unwrapping inline, so there is one place to be wrong. fromAck also widens the type check to `is! Map` + `Map<String,dynamic>.from(...)` rather than `is! Map<String, dynamic>`, matching the tolerant-parsing convention in ports.dart — a codec that hands back Map<dynamic,dynamic> would otherwise be rejected for the wrong reason. 21/21 store docs tests, analyze clean.
rev 1 said "in P1, HTML is reachable only by tailnet publish". That forced the
same-machine case through a network round trip it does not need: with Tailscale
off, a board sitting on the very machine you were looking at could not be opened,
and the failure read `Could not publish`. The document lives on the server's
filesystem, so the only real question is whether the viewer is on that host:
local client (loopback) -> docs.open, the host's OS opener. No HTTP, no
grant, no TTL, no Tailscale, no listener.
remote client (tailnet) -> publish and serve, exactly as D9/D10/D15 say.
Per **client**, not per server — one server holds a loopback desktop app and a
tailnet phone at the same moment, and `isLocal` is already computed per client
from the remote address in server.ts. Publish stays available to a local client as
a secondary action, since that is how a board gets to the phone from the desk.
Opening on the host is a new server capability, so it is gated to local clients
and still routed through D2's single path boundary: a remote client asking for
`docs.open` is refused, never served.
The board had drawn `Open · Browser · Reveal` on the popover row from the start;
rev 1 shipped one of the three. Card 6 now leads with an "open on the host" row,
states the resolution as a where-the-viewer-is question, and draws the same sheet
with its two primary actions side by side.
Also corrected while in here:
- Card 6 still credited `tailscale serve`, which D10 rev 2 dropped.
- The wire table recorded `docs.publish` as returning `DocGrantDTO`; it returns
`{grant: DocGrantDTO}`, nested. That undocumented nesting is what broke HTML
preview on a real device (a84fde7).
- Verification step 3 still expected "27 boards and 70 specs" from a
`find mockups docs` — dead after D1 rev 2. It now uses the git listing the
index actually uses, and states 143 here / 69 in teachme. Ran it: 143.
- Verification gained the two cases that would have caught this: open with
Tailscale off must still work, and `docs.open` from the phone must be refused.
No code in this commit. `docs.open` is not implemented yet.
…8 rev 2) Implements the decision recorded in e169f53. A board sitting on the machine you are looking at is now one tap away with Tailscale off: no HTTP, no grant, no TTL, no listener. Publishing stays for the case it cannot cover — a different device. Server docs/open.ts resolve through D2, then the platform opener (open / xdg-open / cmd start ""). The path is an argv element, never concatenated into a command string, so `a b; touch pwned.md` is inert — asserted. docs.open gated on ctx.client.isLocal; a remote client is refused with a reason, never served. hello.ack now carries isLocal, per client. Reconciled two in-flight designs rather than duplicating them. The command-layer gate was already right and is kept as-is. Two things changed underneath it: - DocsService.open delegated to an optional `deps.open` that server.ts never passed, so it returned false for every request — the feature could not work. It now calls openDocOnHost, which is wired and tested. - The port was sync `boolean`, so every failure surfaced as "could not open document". It is now async and reason-carrying, matching what publish already does (D15's degrade-loudly rule): a refused dotfile says "dotfile". App MakitConnState.serverIsLocal, set from hello.ack before fan-out. NOT inferred from the stored host: mDNS rediscovery rewrites that behind us, and a loopback-looking host is not proof. Absent (older server) reads as remote, because publishing works everywhere and the fallback must be the one that cannot be wrong. The HTML notice leads with "Open in browser" for a local client and demotes publish to "Share to a device…" — still reachable, since that is how a board gets to the phone from the desk. A remote client is unchanged. Tests: opener platform matrix, the six paths D2 refuses (and that none of them spawn), argv-not-shell, a failing opener's reason; the local/remote command gate and the stated reason; hello.ack carrying isLocal both ways; serverIsLocal defaulting false, set by isLocal:true, and treating an absent field as remote; both notice variants including that opening locally publishes nothing. 1242/1242 server, 88 app tests here, tsc + analyze clean. Not done: Reveal in Finder (the mockup's third action), and no device check yet — the app binary needs a rebuild before this is visible.
- serverIsLocal not reset on connection start, persisting stale state across server switches - FutureBuilder error case renders infinite spinner instead of showing reason - ListView.builder rows share keys on filter, breaking widget state on search
…n Windows Windows cmd /c uses argument concatenation that can be escaped if the path contains unescaped quotes. Switch to powershell -Command Start-Process, which parses arguments as script block expressions and is resistant to quote-injection. Also add -FilePath as a named arg to be explicit about what we are opening.
Six defects, five of them mine from the last two commits. app — the popover crashed on a short surface `(rows * rowHeight).clamp(0.0, maxHeight - 96)` throws when maxHeight < 96, because Dart's clamp asserts lower <= upper. A 180pt-tall window gives the panel ~90pt, so it threw ArgumentError instead of rendering. Fixed by deleting the arithmetic: the list is `Flexible` inside a `ConstrainedBox`, so the cap is a maximum and the constraint system squeezes it. `kDocsPopoverChromeHeight` is gone with it. The first attempt (max/min instead of clamp) only moved the failure to a RenderFlex overflow — hand-computing the height was the wrong shape, not the wrong formula. Test pinned at 180pt, where the old expression provably throws (verified by evaluating it standalone) and the new code renders. server — isLocal reached two of three hello.ack sends `handlePair` was missed, so a device that pairs never learned it was local and silently published instead of opening: the feature quietly off for exactly the clients most likely to be on the same machine. Test asserts all three paths. server — Windows was a command-injection hole, twice `cmd /c start ""` re-parses `& | > < ^`. The follow-up, `powershell -Command Start-Process`, is worse: `-Command` *joins* its remaining arguments into a script, so a path containing `;` or `$(...)` is evaluated. Both contradicted the "argv, never a shell" claim the module is built on, and the test only ever exercised darwin. Windows is not a target for makit's server, so it is now refused with a reason. That deletes the injection surface, makes the previously-dead `undefined` branch reachable, and drops the `platform === "win32"` special case at the call site. A test asserts win32 is refused and spawns nothing, so re-adding a shell path fails. server — two publish-path races `ensureOrigin` was unguarded, so two concurrent publishes each bound a port and the first listener leaked unreachable. It now coalesces on one in-flight bind. `close()` cleared its fields before the socket finished closing, so a publish arriving mid-close bound a second listener; it now awaits the in-flight bind. A throwing `reach()` escaped as an unhandled rejection instead of D15's stated reason. Tests for the shared bind and the stated reason. server — a stat per doc, twice `resolveDocPath` already stat'd the file to validate it, then the caller stat'd again for the mtime. `DocPathResult` now carries `modifiedAt`, halving the syscalls per scanned document (286 -> 143 here). 1246/1246 server, tsc clean. App: 2061 pass, analyze clean; the 19 "loading" failures are the known flutter_tester flake — all pass in isolation.
- docs.grants now evicts least-recently-used grants when the live set hits the cap (DocsGrantStore), preventing unbounded memory growth on long-lived servers (medium/bug: grant set memory leak). - publishDoc now updates the grant's nowMs before every tick, not once at sheet open, so the 'expires in 5m' countdown stays accurate if the user leaves the sheet open (medium/maintainability: stale timestamp in UI). - docs.grants filters responses to tolerate server bugs: a mismatch between server and client (DTO shape, types) no longer crashes the app (medium/bug: unguarded cast). - docs screen's filter count now recomputes on every keystroke in the search field, not once at filter change, so narrowing a query updates the counts (medium/maintainability: UI lag on search). - the service now catches scan errors and publishes scanOk:false (instead of rejecting the promise), so a slow worktree filesystem does not crash the server's snapshot loop (medium/bug: error handling). - service.ts run(git) now uses a timeout so a stalled git process cannot starve the index. Tracked fall through to walk when git times out (medium/bug: potential hang).
Verified each before touching it — one was a false positive.
server
service.ts `void runScan()` had no catch, so a throwing scan escaped as an
unhandled rejection: nothing published, nothing logged, the Docs
screen simply frozen on its last snapshot. It now publishes
scanOk:false with the reason, which is what that flag is for.
changed.ts the merge-base diff had no timeout, the same hang risk already
fixed in tracked.ts. Bounded at 10s; past it `changed` is
undetermined, which D14 already models as absent-not-guessed.
grants.ts reaping was lazy (inside resolve/list), so a publish loop could
grow the map without bound between reads — each entry pinning a
path and holding the doc listener open. mint() now reaps first and
caps at 64 live grants, evicting the least recently seen. The reap
loop list() already ran is extracted and shared.
app
publish_sheet `nowMs` was read once at the build that first showed the grant,
so the expiry pill read "30 min" forever. A 30s ticker while a
grant is live, cancelled in dispose.
docs_screen docsFilterCounts walks every doc and does not depend on the
query, but ran on every rebuild — each keystroke was O(docs).
Memoised against snapshot identity.
store.dart `ack['grants'] as List?` threw a cast error into the caller's
Future on a malformed payload; now tolerant, per ports.dart.
NOT fixed — false positive: the review claimed both MarkdownBody widgets lack an
`extensionSet` and so would not render GFM tables. flutter_markdown_plus renders
them by default; verified with a throwaway widget test (Table present, no literal
pipes). Left alone.
Also: Card 2's iPhone frame still carried pre-rev-2 numbers (132 files / 3
worktrees / All 132 / Mockups 27 / Specs 70). Refreshed to the real counts.
Two tests I wrote and then deleted rather than ship: a "grants payload tolerance"
test that re-implemented the expression inline (it could not fail if the
production code regressed) and, earlier, a duplicate auth_gate test misnamed as a
live daemon probe.
1248/1248 server, tsc clean. App analyze clean; 2079 pass, the 11 "loading"
failures are the flutter_tester flake with zero non-loading failures.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_7adec20b-0626-4653-869d-e0239dd636fd) |
Contributor
📝 WalkthroughWalkthroughThis change adds repository document indexing, secure Markdown and HTML handling, watch-gated snapshots, WebSocket commands, publication grants, Flutter document browsing, previews, and Docs navigation. ChangesRepository document preview
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Resolved 8 conflicts: additive union members (docs.* vs ports.kill/forward/watchPort) kept both. snapshots.json rewritten semantically (main's ports fixture + ours docs snapshot). contract.test.ts encoding corruption fixed.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_02eb0688-4e4b-4230-9328-9bc316a7724e) |
Owner
Author
Up to date with
|
| check | result |
|---|---|
| D1 rev 2 live | teachme 69 docs (was 3), real extracted titles |
| Popover | search over titles+paths, lazy list capped ~12 rows, relative left-truncated paths |
hello.ack |
isLocal: true over loopback |
| HTML notice | "This file is on this machine — opening it needs no server" · Open in browser primary, Share to a device… secondary |
| The tap | opened the document in the real browser |
| No serving used | docs.grants → [], no doc port bound |
| Boundary | docs.open ../../.ssh/id_rsa → err: escapes-root |
| D15 | publish on a loopback-only server refuses with a reason, shares nothing |
Plus 37 assertions driven straight at the HTTP route: byte-identical bodies, correct content types, nosniff, no-store, HEAD, and 12 traversal/dotfile/.git/wrong-grant probes all 404 (never 403).
One trap worth recording: a stale daemon from an earlier run made the app show Publish & open on a loopback client. It had D1 rev 2 (so 69 docs looked right) but not isLocal. A correct-looking doc count says nothing about whether the daemon is current — probing hello.ack settled in seconds what UI inspection could not.
Review findings, fixed in-branch
An ocr audit (48 comments) and a structural audit both ran; findings are fixed here rather than deferred. Fixed: a popover crash (clamp asserts when the available height is under the chrome height), isLocal reaching only two of three hello.ack paths, two Windows command-injection routes (cmd /c start re-parses & | > < ^; powershell -Command joins its args into a script and expands $(...) — Windows is now refused with a reason, as it is not a server target), two publish-path races (ensureOrigin double-bind, close/reopen), an unhandled rejection in the scan loop, a double stat per document, a frozen expiry pill, and an O(docs)-per-keystroke filter recount.
One finding was rejected after checking: the claim that a missing extensionSet breaks GFM tables — flutter_markdown_plus renders them by default (verified with a throwaway widget test).
Structural: scanWorktree is now one loop over a candidate list with the git-vs-walk fallback owned by the lister; DocRoots is a discriminated union so invalid field combinations are unrepresentable; the HTML notice takes an ordered action list instead of branching on locality; the query predicate is canonical in docs_filter.dart instead of duplicated.
Known gaps
- Reader-width toggle is inert at phone widths (680pt clamp vs a 393pt screen) — the board calls it a desktop affordance.
- Internal doc-link re-target has no test. Flutter paints links as
AXStaticTextwith no press action, so it is not drivable by UI automation; it wants a widget test. Reveal in Finder— the third action the board draws — is not implemented.- The app test suite's
: loadingflake will show red in CI regardless of this branch.
CI's fixture-sync job requires app/test/fixtures/snapshots.json and server/test/fixtures/snapshots.json to be byte-identical. The main merge resolved the two copies differently: I resolved the server one semantically (main's rewritten ports.snapshot plus our docs.snapshot frame), while the app copy took main's side and lost the docs frame. Frames 1-3 were already identical, so the app copy did carry main's updated ports.snapshot — only our docs.snapshot was missing. Copied server -> app. Caught by CI, not by either local suite: the server tests read the server copy and the app's codec_contract_test read the app copy, so each was internally consistent and neither noticed the divergence. That asymmetry is exactly what the fixture-sync job exists to catch. app/test/codec_contract_test.dart 12/12, server 1414/1414.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_18e0927a-13f7-4b37-bd84-9d44d248d046) |
One conflict: an import list in app/lib/app/router.dart. The union was wrong here, and analyze caught it. #157 replaced the "archived" sessions screen with a "closed" one and DELETED archived_screen.dart, so keeping both sides left an import of a file that no longer exists. Resolved as: our docs_screen, main's closed_screen, and the deleted archived_screen dropped — verified nothing else in app/ still references ArchivedScreen. 1445/1445 server, tsc clean, analyze clean, 2576 app tests pass (the 11 ":loading" failures are the pre-existing flake, zero non-loading). Shared snapshots fixture still byte-identical between app and server.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_2ead65fa-08da-471d-9974-aea47e44db01) |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_cc8627f1-32ac-4801-8de8-84fe148f9a63) |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_65cad140-aded-4bbd-a7c5-233ac678cdb8) |
Owner
Author
|
@coderabbitai rate limit |
Contributor
|
Your plan includes PR reviews subject to rate limits. More reviews will be available in 46 minutes. |
Owner
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 53
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/lib/desktop/chat/desktop_sidebar.dart`:
- Around line 724-727: Reattach the comments to their corresponding widgets in
the sub-row rendering block: keep the existing ports comment immediately before
_SubRowPortsGlyph, and place the docs comment immediately before
_SubRowDocsGlyph. Make no other changes to the surrounding layout or behavior.
- Around line 724-732: The _SubRowDocsGlyph and _SubRowPortsGlyph flows must
notify their host when the associated popover unmounts, including when the docs
or ports list becomes empty. Invoke onOpenChanged(false) during popover
disposal/removal, deferring the callback when required by Flutter lifecycle
constraints, so _docsOpen and the corresponding ports flag cannot remain
latched.
In `@app/lib/ui/docs/doc_preview.dart`:
- Around line 169-177: Update _openExternal to report external-link failures
instead of silently returning or swallowing launch errors, including both
canLaunchUrl false and launchUrl exceptions. Extend the onExternalLink contract
or provide a publish_sheet.dart handler that forwards the failure through
statusCenterProvider, matching the existing statusCenter behavior while keeping
the widget free of provider reads.
In `@app/lib/ui/docs/docs_filter.dart`:
- Around line 24-37: Move the documentation paragraphs describing filtering
behavior and null snapshots from the contiguous comment above docMatchesQuery
onto the filterDocs declaration. Keep the query-specific documentation
immediately above docMatchesQuery, so each function’s dartdoc describes only its
own behavior.
In `@app/lib/ui/docs/docs_screen.dart`:
- Around line 295-308: Resolve the mismatch between the _WorktreeHeader
documentation and implementation: either add and propagate an isActive flag so
only the active worktree uses cs.primary for the left border, or revise the
comment to accurately state that every worktree header uses the accent. Keep the
selected behavior consistent across all _WorktreeHeader callers.
- Around line 138-159: Update the Docs screen _body list construction to flatten
repository, worktree, and document entries into a single item sequence, then
render it with ListView.builder instead of ListView(children: ...). Preserve the
existing header ordering, counts, keys, callbacks, and styling while ensuring
only visible items build during rebuilds triggered by _SearchField.
- Around line 118-125: Memoise subtitle computation by adding or reusing an
identity-based `_subtitleCache` alongside `_countsFor`, so each `DocsSnapshot`
is processed once. Update `_subtitle` to return the cached value for the same
snapshot and update the app-bar call site in `build` to read `_subtitleCache`
instead of invoking `_subtitle(snapshot)` directly.
In `@app/lib/ui/docs/publish_sheet.dart`:
- Around line 78-98: Update _publish to capture the store controller notifier
before awaiting publishDoc, then if the sheet is no longer mounted after the
await, revoke the returned grant through that captured notifier before
returning. Preserve the existing mounted success-state update and error handling
for active sheets.
In `@app/lib/ui/home/worktree_row.dart`:
- Around line 385-395: Wrap the docs-entry InkWell in a Semantics widget with
button set to true and a label that includes the action and document count,
using singular wording for one document and plural wording otherwise. Preserve
the existing InkWell and its onTap navigation to keep the interaction available.
In `@app/test/connection_controller_test.dart`:
- Around line 345-357: Add a test alongside the existing locality cases in the
connection controller test group that sends helloAck with a non-boolean isLocal
value such as "true" or 1, then assert controller.state.serverIsLocal is false.
Reuse the existing boot, frame emission, delay, and disposal pattern to verify
malformed locality flags are treated as remote.
In `@app/test/ui/docs/doc_glyph_test.dart`:
- Around line 19-30: Update both tests around _pump and the
find.bySemanticsLabel assertions to create a SemanticsHandle with
tester.ensureSemantics(), then dispose it in a try/finally block. Keep the
existing widget and label expectations unchanged, and ensure each test releases
its handle even when an assertion fails.
In `@app/test/ui/docs/doc_preview_test.dart`:
- Around line 128-131: Add a widget test alongside the existing spinner test
that pumps DocPreview via _pump with markdownError set, then verify the “Could
not read” message is displayed and MarkdownBody is absent.
- Around line 73-76: Update the test named “a non-doc relative link is external
(fallback, not swallowed)” to use relative hrefs without a URI scheme and with
extensions other than .md or .html, so resolveDocLink reaches and verifies the
fallback branch. Replace the current scheme-based URLs while preserving the
expected DocLinkKind.external assertions.
In `@app/test/ui/docs/doc_row_test.dart`:
- Around line 121-141: Change the `docFullPath joins worktree and relPath
without a double slash` case from `testWidgets` to `test`, removing the unused
`tester` parameter while preserving both `docFullPath` assertions unchanged.
In `@app/test/ui/docs/docs_filter_test.dart`:
- Around line 64-107: Add a filterDocs test covering a null snapshot and assert
it returns an empty list. Place it in the existing filterDocs group near the
other filter behavior tests, using the DocsFilter API and preserving the
null-snapshot contract relied on by _DocsScreenState._body.
In `@app/test/ui/docs/docs_popover_test.dart`:
- Around line 138-149: Extend the docs popover widget tests with coverage for
the hover dwell, Escape dismissal, and the pinned-only outside-tap barrier. Use
a mouse gesture over DocsGlyphAnchorTapTarget to verify kDocsHoverOpenMs has not
elapsed before opening and does open afterward; add tests that exercise the
Escape binding and confirm outside taps are blocked only while the popover is
pinned.
- Around line 36-38: Update the test for kDocsHoverOpenMs to assert that it is
less than kTooltipDwell rather than matching the literal value 350. Import the
declaration containing kTooltipDwell and preserve the existing test scope.
In `@app/test/ui/docs/docs_screen_test.dart`:
- Around line 53-54: The _snap helper currently exposes scanOk without covering
the failure state. Add a DocsScreen test that pumps _snap with an empty document
list and scanOk set to false, then assert the existing failure-state rendering;
if DocsScreen has no distinct failure rendering, remove the unused scanOk
parameter from _snap instead.
In `@app/test/ui/docs/publish_test.dart`:
- Around line 7-17: Add a publish UI test that configures the DocGrant returned
by _grant with expiresAt 30 minutes after the test’s nowMs value, then assert
that _ExpiryPill renders the corresponding live “expires N min” countdown text.
Keep the existing expired-case coverage unchanged and use the established test
setup and rendering symbols.
In `@app/test/ui/home/worktree_row_docs_watch_test.dart`:
- Around line 61-76: Extend the WorktreeRow docs-watch test to mount two
WorktreeRow instances simultaneously and verify sent remains [true]. Unmount one
row and verify no additional message is sent, then unmount the second and verify
sent becomes [true, false], exercising DocsWatch reference-count collapsing.
In `@BRANCH_SUMMARY.md`:
- Line 4: Update the verification counts in BRANCH_SUMMARY.md, including the
summary and lines 41-42, to match the final run: 1414 server tests and 2442 app
tests; update the commit count to its current value, or remove the exact counts
to prevent future staleness.
- Around line 50-59: Remove the branch-scoped readiness and reviewer notes from
the repository root by deleting BRANCH_SUMMARY.md, or relocate it under docs/
with the SPEC-46 artifacts if the notes must be retained. Do not preserve short
commit SHA references as repository documentation because they may become
invalid after squash-merging.
In `@docs/specs/2026-08-09-SPEC-46-doc-preview.md`:
- Around line 86-94: Update docs/specs/2026-08-09-SPEC-46-doc-preview.md lines
86-94 to describe the rev 2 indexing behavior: by default list candidates via
git ls-files --cached --others --exclude-standard, and only walk roots when
roots is configured or git cannot answer; retain the remaining resolution steps.
Also update the example at docs/specs/2026-08-09-SPEC-46-doc-preview.md line 152
to use the plain-HTTP tailnet-address form specified by D10 rev 2 instead of the
https host.ts.net form.
In `@mockups/doc-preview.html`:
- Around line 286-311: Update the index description in the “Allowlist, not a
file tree” section to describe D1 rev 2: use git ls-files --cached --others
--exclude-standard as the primary indexing mechanism, and present the existing
root allowlist only as the fallback. Remove the outdated open-question framing
about SKILL.md while preserving the hard exclusion rules in the adjacent section
unchanged.
- Around line 314-340: Reconcile the DocDTO snippet with the shipped contract:
remove the unsupported git field and add changed?: boolean, preserving the
existing DTO fields. Update the nearby HTML grant description to reference the
publish/grant commands used by docs/publish.ts and docs/grants.ts, and replace
the open question about working-tree versus merge-base changes with the
established merge-base semantics. If this mockup is intentionally frozen,
clearly label that status at the top instead of presenting the snippet as
current protocol.ts.
In `@server/src/docs/changed.ts`:
- Around line 47-52: Update the Git invocation in the changed-document logic
around DocsService to include "-c", "core.quotePath=false" before the "diff"
subcommand, ensuring non-ASCII paths remain filesystem-compatible. Update the
exact-argument test to expect the new Git argument sequence.
In `@server/src/docs/grants.ts`:
- Around line 24-37: Reorder the declarations and doc comments in the grants
module so the idle-window comment immediately precedes DOC_GRANT_IDLE_MS, while
the ceiling comment remains immediately above MAX_LIVE_GRANTS. Keep both
constants and their values unchanged.
In `@server/src/docs/listener.test.ts`:
- Around line 110-133: Add a test covering close() during an in-flight
ensureOrigin() bind: start ensureOrigin() without awaiting it, immediately await
close(), then await the pending bind and assert isListening is false and the
opened server is no longer bound. Place it alongside the existing DocListener
lifecycle tests and use the opened server tracking to verify cleanup.
In `@server/src/docs/listener.ts`:
- Around line 106-123: Update server/src/docs/listener.ts:106-123 around
DocListener.close() and ensureOrigin() to track the in-flight close in a closing
field, await it before binding, and return the existing promise for re-entrant
close() calls. Update server/src/docs/listener.test.ts:110-133 by adding
coverage that starts ensureOrigin(), invokes close() before binding resolves,
and verifies exactly one server is created and isListening is false afterward.
- Around line 127-142: Update bind so the document server retains a persistent
error listener after successful listening; do not remove the only server “error”
handler in onListening. Preserve the existing bind-failure cleanup and null
resolution in onError, while ensuring later server errors are handled without
terminating the process.
In `@server/src/docs/publish.test.ts`:
- Around line 18-19: Remove the LAN publication path end to end: delete the lan
test fixture and related assertions, remove "lan" from DocReach, MintInput,
DocGrantDTO, and the app’s DocReach types, and eliminate LAN handling from
publishDoc and _ReachPill. Preserve tailnet-only behavior and update affected
tests to cover only supported reach values.
In `@server/src/docs/publish.ts`:
- Around line 54-74: In the publish flow, bind the non-null result of
deps.reach() to a const after the null check, then use that const for
reach.reach and the buildUrl closure’s origin reference. Keep the existing error
and loopback-only handling unchanged.
In `@server/src/docs/read.test.ts`:
- Around line 23-31: Strengthen both tests around readDocText: assert the
returned refusal reason/message in addition to r.ok being false. The HTML case
for mockups/board.html must verify the HTML-specific rejection, while the
non-resolving ../../etc/passwd case must verify the not-a-file/path-resolution
rejection, ensuring each test fails for unrelated fixture issues.
In `@server/src/docs/resolve.test.ts`:
- Around line 102-109: Expand the test covering resolveDocPath to include
accepted .markdown and .htm files, plus uppercase variants to verify
extensionOf’s case-insensitive matching; retain the existing rejection
assertions for disallowed extensions and directories.
In `@server/src/docs/roots.ts`:
- Around line 80-94: Update readConfig to bound .makit/docs.json reads using the
existing TITLE_READ_BYTES limit: import and use statSync before readFileSync,
return undefined when the file exceeds the limit, and preserve the current
handling for missing, unreadable, or malformed configuration files.
In `@server/src/docs/route.test.ts`:
- Around line 56-59: Extend the docs route tests around the req helper and
existing route cases to assert Cache-Control and X-Content-Type-Options on
successful responses, and add coverage for unsupported methods verifying the 405
status and response behavior. Preserve current status, content-type, and body
assertions while exposing the required response headers from req().
In `@server/src/docs/route.ts`:
- Around line 44-49: Update the catch handling around handle(req, res, deps) to
destroy the response socket when an error occurs after headers have been sent;
retain the existing notFound(res) behavior when headers are not sent. Ensure the
headers-sent branch explicitly terminates the underlying connection so the
response cannot remain open.
In `@server/src/docs/scan.test.ts`:
- Around line 16-36: Make the fixture precondition explicit in fixture() and the
walk-fallback tests: verify the generated temporary directory is not inside a
Git worktree before exercising walkRoots, and fail loudly with a clear assertion
if gitDocPaths could succeed there. Keep the existing fixture setup and fallback
behavior unchanged while ensuring these tests cannot silently run through the
Git-index path.
- Around line 122-151: Add an unstaged, non-ignored document in gitFixture after
git add -A so the scan must discover it through --others --exclude-standard,
then include its sorted relative path in the expected rels assertion. Keep
existing staged and ignored-file coverage unchanged.
In `@server/src/docs/scan.ts`:
- Around line 59-64: The candidate loop in the scan flow performs synchronous
filesystem work through toDoc for every non-excluded path, blocking the event
loop. Update the scan path around toDoc and its shared security checks to use
node:fs/promises without duplicating the existing boundary, or process
candidates in fixed-size chunks with yields between chunks while preserving
exclusion and document collection behavior.
In `@server/src/docs/service.test.ts`:
- Around line 181-187: Rename the test currently titled
“publish/unpublish/grants delegate to the grant store” so it accurately
describes the non-document extension refusal and empty grants assertion, unless
extending it to successfully publish an allowlisted document and then call
unpublish is required. Ensure the test name matches the behavior it actually
exercises.
- Around line 192-205: Update the test setup around DocsService and makeService:
remove the as never casts, extend makeService to accept and forward an
onGrantsChanged override, and construct this test service through makeService
instead of an ad hoc incomplete options object. Preserve the existing
signal-counting callback while relying on the typed dependency contract, without
passing unsupported emit or omitting required dependencies.
In `@server/src/docs/service.ts`:
- Around line 180-193: Update the worktree processing loop to run independently
across worktrees with Promise.all, and run scan and changedPaths concurrently
for each worktree. Preserve the existing scanOk/scanError aggregation and
grouped mtime-descending docs order by collecting each worktree’s results before
merging them in the original listWorktrees order.
- Around line 151-166: Update the catch path in runScan to assign this.cached
only when this.watchers > 0, matching the existing watcher guard on the
successful path. Continue notifying onSnapshot only for active watchers, while
preserving failure reporting for clients still watching and retaining the last
good cache when all watchers have left.
- Around line 143-148: Update runScan to record when a scan request arrives
while scanning is already true, then schedule exactly one follow-up scan after
the current doScan completes. Clear the overlap marker when consuming it,
preserve the existing watcher check, and ensure the follow-up uses the
established debounce/scheduling path without allowing concurrent scans.
In `@server/src/docs/title.test.ts`:
- Around line 108-114: The test around readDocMeta must verify the read window
is bounded, not merely that a top-of-file title is found. Update the test to
place a competing title or marker beyond TITLE_READ_BYTES and assert
readDocMeta(p, "md").title remains the expected in-window title, using the
existing TITLE_READ_BYTES symbol.
In `@server/src/docs/tracked.ts`:
- Around line 61-63: Update defaultWalk to import and reuse DEFAULT_DOC_DIRS
from roots.ts for the walk dirs instead of repeating ["mockups", "docs"], while
preserving the existing walk configuration.
In `@server/src/server.ts`:
- Around line 369-372: The comment for releaseDocsIfIdle and related
doc-listener lifecycle comments incorrectly imply immediate release after grants
expire or are reaped. Update them to state that onGrantsChanged is triggered by
DocsService.unpublish() and DocsService.grants(), with expired or idle grants
reaped lazily and the listener retained until a later grant-list or unpublish
request; preserve listener binding while live document grants remain valid.
In `@server/src/ws/commands/docs.test.ts`:
- Around line 44-58: Add a default open method to the docs test stub before the
DocsCommandPort cast, returning the controlled refusal result expected by
docs.open tests. Preserve docsOverrides so individual tests can replace this
behavior, and keep the existing read, publish, unpublish, grants, and revoked
implementations unchanged.
In `@server/src/ws/commands/docs.ts`:
- Around line 35-58: The docs command handlers require authorization and
worktree scoping. In server/src/ws/commands/docs.ts lines 35-58, update
docs.read, docs.publish, and docs.open to reject worktreePath values absent from
the indexed worktree list returned by listDocWorktrees. In
server/src/ws/commands/docs.ts lines 78-91, record ctx.client.deviceId when
minting each grant, restrict docs.grants to grants owned by that device, and
reject docs.unpublish when the requested grant is not owned by the caller.
In `@server/test/protocol/contract.test.ts`:
- Around line 230-236: Add contract assertions in the snapshot coverage near the
existing docs test to verify port 5432 preserves its Docker annotations and
reach value "exposed" in ports.snapshot. Reuse the fixture’s existing port DTO
structure and assert the expected fields directly; only omit them if the test
documents an existing equivalent coverage.
In `@server/test/ws/auth_gate.test.ts`:
- Around line 191-221: Extend the hello.ack locality tests around
AuthGate.handleHello to cover the already-trusted authenticated client without a
bearer and a remote client using the pairing path. Assert the trusted
acknowledgement includes isLocal: true and the remote pairing acknowledgement
includes isLocal: false, preserving the existing bearer and local-pairing
assertions.
In `@server/test/ws/send_message_attachments.test.ts`:
- Around line 88-96: The duplicated onDocsWatchersChanged, sendDocsSnapshot, and
docs stubs in the test should be extracted into a shared constant, following the
existing portsDepsStub pattern. Replace both inline blocks with a spread of that
constant so DocsCommandPort changes require only one update.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7a2ec8b8-b827-4f9f-b181-cf317bd53761
📒 Files selected for processing (76)
BRANCH_SUMMARY.mdapp/lib/app/router.dartapp/lib/app/routes.dartapp/lib/desktop/chat/desktop_sidebar.dartapp/lib/store/connection.dartapp/lib/store/docs.dartapp/lib/store/store.dartapp/lib/transport/codec.dartapp/lib/ui/docs/doc_glyph.dartapp/lib/ui/docs/doc_preview.dartapp/lib/ui/docs/doc_row.dartapp/lib/ui/docs/doc_vocabulary.dartapp/lib/ui/docs/docs_filter.dartapp/lib/ui/docs/docs_popover.dartapp/lib/ui/docs/docs_screen.dartapp/lib/ui/docs/publish_sheet.dartapp/lib/ui/home/worktree_row.dartapp/test/connection_controller_test.dartapp/test/fixtures/snapshots.jsonapp/test/store/docs_test.dartapp/test/ui/docs/doc_glyph_test.dartapp/test/ui/docs/doc_preview_test.dartapp/test/ui/docs/doc_row_test.dartapp/test/ui/docs/docs_filter_test.dartapp/test/ui/docs/docs_popover_test.dartapp/test/ui/docs/docs_screen_test.dartapp/test/ui/docs/publish_test.dartapp/test/ui/home/worktree_row_docs_watch_test.dartdocs/specs/2026-08-09-SPEC-46-doc-preview.mdmockups/doc-preview.htmlserver/src/docs/changed.test.tsserver/src/docs/changed.tsserver/src/docs/grants.test.tsserver/src/docs/grants.tsserver/src/docs/listener.test.tsserver/src/docs/listener.tsserver/src/docs/open.test.tsserver/src/docs/open.tsserver/src/docs/publish.test.tsserver/src/docs/publish.tsserver/src/docs/read.test.tsserver/src/docs/read.tsserver/src/docs/resolve.test.tsserver/src/docs/resolve.tsserver/src/docs/roots.test.tsserver/src/docs/roots.tsserver/src/docs/route.test.tsserver/src/docs/route.tsserver/src/docs/scan.test.tsserver/src/docs/scan.tsserver/src/docs/service.test.tsserver/src/docs/service.tsserver/src/docs/title.test.tsserver/src/docs/title.tsserver/src/docs/tracked.tsserver/src/protocol.tsserver/src/protocol/codec.tsserver/src/server.tsserver/src/ws/auth_gate.tsserver/src/ws/client.tsserver/src/ws/commands/deps.tsserver/src/ws/commands/diagnostics.test.tsserver/src/ws/commands/docs.test.tsserver/src/ws/commands/docs.tsserver/src/ws/commands/ports.test.tsserver/test/fixtures/snapshots.jsonserver/test/protocol/contract.test.tsserver/test/push_register.test.tsserver/test/reverse_rpc_wake.test.tsserver/test/ws/agents_catalog.test.tsserver/test/ws/auth_gate.test.tsserver/test/ws/command_router.test.tsserver/test/ws/pr_commands.test.tsserver/test/ws/reverse_rpc.test.tsserver/test/ws/send_message_attachments.test.tsserver/test/ws/subscription_hub.test.ts
53 review threads, verified individually rather than applied wholesale. Availability / correctness: - listener: persistent 'error' handler after bind (an EMFILE later would have crashed the process); bind is serialised against an in-flight close. - route: destroy the socket when the catch fires after headers were sent, instead of leaving the response to hang until the headers timeout. - roots: bound the user-supplied .makit/docs.json read at 64 KB. - scan: yield to the event loop every 32 candidates so a large repo's per-file sync work no longer stalls the watchers and the WSS. - changed: -c core.quotePath=false so non-ASCII paths match the ls-files list. - service: a worktree change arriving mid-walk is queued and re-run once instead of being dropped; a walk that fails after the last watcher leaves no longer overwrites a good cache; per-worktree scan + merge-base diff fan out concurrently. Scoping (SPEC-44 owner model) — docs commands constrained against server-authoritative state, not merely type-checked: - read/publish/open refuse a worktreePath the index never reported; - grants are minted with the caller's deviceId, docs.grants returns only the caller's shares, and docs.unpublish refuses a foreign id indistinguishably from an unknown one, so it cannot probe another device's grant ids. App: - publish_sheet: a grant that lands after the sheet is dismissed is revoked, so a doc cannot stay shared the user believes they cancelled. - desktop_sidebar: clear the host's docs/ports popover latch when the glyph unmounts, so the overflow menu no longer sticks in place of the diff pill. - doc_preview: an external link that fails to open now reports through StatusCenter instead of failing silently. - docs_screen: lazy ListView.builder + memoised subtitle. Tests: extracted server/test/ws/docs_deps_stub.ts so a new DocsCommandPort member breaks one place instead of four (adding isIndexedWorktree had broken all four inline copies). Plus coverage for the paths above, and the restored SPEC-42 P2c docker/reach contract test the branch had dropped. BRANCH_SUMMARY.md is removed — branch-scoped status belongs in the PR body, not the tree. Verified: server pnpm typecheck clean, 1465 tests pass; app flutter analyze clean, 2667 tests pass. Key fixes mutation-tested (latch, scoping, stub).
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_7a2031e2-9f60-49a1-b8b3-15e7625ce336) |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/src/docs/listener.ts (1)
73-79: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSerialize
ensureOrigin()after shutdown starts.Line 76 returns
bindingbefore it checksclosing. Ifclose()is waiting for that bind, a later publish can mint a grant from its origin.doClose()then closes that listener, so the returned capability URL is unavailable.After awaiting
closing, restart the state check before binding. Checkclosingbefore returningoriginorbinding. Add a test whereclose()waits on a bind and a laterensureOrigin()call must resolve to one live listener.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/docs/listener.ts` around lines 73 - 79, Update ensureOrigin() to prioritize the in-flight closing operation before returning origin or binding, then restart the state checks after awaiting closing so it reuses only the listener that remains live. Preserve single-flight binding and add coverage for close waiting on bind followed by a later ensureOrigin() resolving to one live listener.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mockups/doc-preview.html`:
- Around line 913-915: Update the capability URL example near the documented
tailnet listener to use the plain-http scheme, changing https:// to http://
while preserving the rest of the URL.
---
Outside diff comments:
In `@server/src/docs/listener.ts`:
- Around line 73-79: Update ensureOrigin() to prioritize the in-flight closing
operation before returning origin or binding, then restart the state checks
after awaiting closing so it reuses only the listener that remains live.
Preserve single-flight binding and add coverage for close waiting on bind
followed by a later ensureOrigin() resolving to one live listener.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3144155b-c67e-4aed-8cb3-6a20f084976d
📒 Files selected for processing (45)
app/lib/desktop/chat/desktop_sidebar.dartapp/lib/ui/docs/doc_preview.dartapp/lib/ui/docs/docs_filter.dartapp/lib/ui/docs/docs_screen.dartapp/lib/ui/docs/publish_sheet.dartapp/test/connection_controller_test.dartapp/test/desktop/desktop_sidebar_test.dartapp/test/ui/docs/doc_preview_test.dartapp/test/ui/docs/doc_row_test.dartapp/test/ui/docs/docs_filter_test.dartapp/test/ui/docs/docs_popover_test.dartapp/test/ui/docs/docs_screen_test.dartapp/test/ui/docs/publish_test.dartapp/test/ui/home/worktree_row_docs_watch_test.dartdocs/specs/2026-08-09-SPEC-46-doc-preview.mdmockups/doc-preview.htmlserver/src/docs/changed.test.tsserver/src/docs/changed.tsserver/src/docs/grants.test.tsserver/src/docs/grants.tsserver/src/docs/listener.test.tsserver/src/docs/listener.tsserver/src/docs/publish.test.tsserver/src/docs/publish.tsserver/src/docs/read.test.tsserver/src/docs/resolve.test.tsserver/src/docs/roots.test.tsserver/src/docs/roots.tsserver/src/docs/route.test.tsserver/src/docs/route.tsserver/src/docs/scan.test.tsserver/src/docs/scan.tsserver/src/docs/service.test.tsserver/src/docs/service.tsserver/src/docs/title.test.tsserver/src/docs/tracked.tsserver/src/server.tsserver/src/ws/commands/docs.test.tsserver/src/ws/commands/docs.tsserver/test/protocol/contract.test.tsserver/test/ws/agents_catalog.test.tsserver/test/ws/auth_gate.test.tsserver/test/ws/docs_deps_stub.tsserver/test/ws/pr_commands.test.tsserver/test/ws/send_message_attachments.test.ts
CodeRabbit caught the mockup's capability URL showing an `https://` MagicDNS
origin while the same card's prose (and D10 rev 2) says the listener is plain
HTTP. `DocListener.bindOnce` builds `http://${bindHost}:${port}`, so the example
was wrong on both the scheme and the host form.
Fixed the mockup, and the same drift in the shipped `DocReach.origin` doc
comment, which still offered `https://host.ts.net` as an example P1 can never
produce. The `https://host.ts.net` test fixtures are normalised to the real
`http://<tailnet-ip>:<port>` shape for the same reason: fixtures get read as the
canonical shape of a value.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_c5dbeb7b-acf7-4c86-9cb0-5959ed29e0de) |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
mockups/doc-preview.html (2)
985-992: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHide or disable the reader-width control on the phone frame.
Line 983 labels this frame as iPhone 375pt, but Line 991 renders
Reader widthas active. Line 1020 limits this control to desktop, and the PR identifies phone-sized behavior as inert. Remove the control from the phone mockup or mark it disabled until phone support exists.Also applies to: 1017-1022
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mockups/doc-preview.html` around lines 985 - 992, Disable or remove the active Reader width control in the iPhone 375pt mockup, including the corresponding control around the desktop-only logic at the nearby alternate occurrence. Keep the control available only for supported desktop frames and ensure the phone frame no longer presents it as interactive.
514-514: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not present
Reveal in Finderas a shipped action.The mockup renders Reveal actions at Line 514, Line 678, and Line 1085 as enabled controls. The PR identifies this action as unimplemented. If this file is the P1 design contract, remove or disable these controls, or label them as future work. Otherwise state clearly that this is an illustrative mockup.
Also applies to: 678-678, 1085-1085
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mockups/doc-preview.html` at line 514, The mockup presents the unimplemented Reveal action as enabled. Update the Reveal controls near the spans at lines 514, 678, and 1085 to be visibly disabled or clearly labeled as future work, or mark the document as illustrative if it is not a shipped design contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@mockups/doc-preview.html`:
- Around line 985-992: Disable or remove the active Reader width control in the
iPhone 375pt mockup, including the corresponding control around the desktop-only
logic at the nearby alternate occurrence. Keep the control available only for
supported desktop frames and ensure the phone frame no longer presents it as
interactive.
- Line 514: The mockup presents the unimplemented Reveal action as enabled.
Update the Reveal controls near the spans at lines 514, 678, and 1085 to be
visibly disabled or clearly labeled as future work, or mark the document as
illustrative if it is not a shipped design contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1aa1d2ba-8275-44e7-85fe-1ad3a4a9cbb7
📒 Files selected for processing (6)
mockups/doc-preview.htmlserver/src/docs/grants.test.tsserver/src/docs/publish.test.tsserver/src/docs/publish.tsserver/src/docs/service.test.tsserver/src/ws/commands/docs.test.ts
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
What & why
SPEC-46 P1: The Docs feature—browse, preview, and share docs your projects hold. Works on every device, no deps added.
Server (src/docs/):
.md/.htmlgit does not ignore (was allowlist only)docs.open(no serving), remote → tailnet publishApp (lib/ui/docs/):
isLocalfrom hello.ack)Fixes (from ocr audit):
serverIsLocalreset on reconnecthello.acksends now includeisLocal(including pairing path)scanOk:false, not unhandled rejection)Verified:
Invalid WebSocket upgradeload errors are anenvironmental
flutter_testerflake — the untouched base commit reproduces them identically,and every affected file passes when re-run)
How it was tested
cd server && pnpm test && pnpm typecheck(1465 pass, tsc clean)cd app && flutter test --no-pub && flutter analyze(2667 pass, analyze clean)Review round
CodeRabbit raised 53 threads, plus 1 follow-up on the re-review. All are addressed and
resolved — each one either fixed with a concrete change or answered with the reason no change was
right. Four were declined on the merits (
"tailnet" | "lan"is retained deliberately per D15; thedocs glyph matches its ports sibling's accessibility convention rather than inventing a new one;
testWidgetsalready enables semantics;DocsScreenhas no scan-failure state to test, so theunused fixture parameter was dropped instead).
The ones worth a reviewer's attention:
docs.read/publish/opennow refuse aworktreePaththe index never reported, and grants carry the mintingdeviceId, sodocs.grantsreturns only the caller's shares anddocs.unpublishrefuses a foreign idindistinguishably from an unknown one (it cannot be used to probe another device's grant ids).
Follows SPEC-44's existing forward-grant owner model.
that the user believes they cancelled.
'error'handler was removed afterlisten, so a laterEMFILE/ENFILEwould have taken the server down.the tree was touched again (nothing else re-arms the debounce). Now queued and re-run once.
realpath/stat/readnow yields every 32candidates, so a large repo's walk no longer blocks the watchers and the WSS.
overflow menu no longer sticks in place of the diff pill.
route.tsdestroys the socket when the catch fires after headers were sent;roots.tsbounds theuser-supplied
.makit/docs.jsonread;changed.tspassescore.quotePath=falseso non-ASCIIpaths match.
The latch fix, the scoping checks and the shared test stub were mutation-tested (removing each
fix fails exactly the tests that name it).
BRANCH_SUMMARY.mdwas dropped from the tree in thisround — branch-scoped status belongs here in the PR body, where it cannot rot after merge.
Checklist
CONTRIBUTING.mddocs.opengate, popover clamp)Notes for reviewers
isLocalfrom server: app does not infer from stored host (mDNS rediscovery can change it). Local clients get direct OS opener; remote get tailnet publish.Note
Medium Risk
Touches connection hello handling, new publish/grant flows, and sidebar/home watch lifecycles; security-sensitive sharing paths but heavily tested and defaults to remote-safe behavior when
isLocalis absent.Overview
Adds SPEC-46 P1 Docs end-to-end in the Flutter app: wire models and store plumbing for
docs.snapshot, ref-counteddocs.watch, and commands (docs.read,docs.open,docs.publish,docs.unpublish,docs.grants), plus a new/repos/docsroute.Browse: Global Docs screen (search, All/Mockups/Specs/Changed filters, repo → worktree grouping) and desktop docs popover beside the ports plug on worktree rows; phone home rows get a docs glyph that navigates to the global screen. Screens and sidebars hold
DocsWatchwhile mounted so indexing runs only when someone is looking.Preview: Shared
DocPreviewwidget (bottom sheet on mobile/desktop) — in-app markdown with front-matter chips and reader width; HTML stays out of the WebView with actions driven byserverIsLocalfromhello.ack(local → open in system browser, remote → publish).Share: Publish bottom sheet mints a tailnet/LAN grant (URL, QR, expiry, Stop sharing) and parses the nested
grantack shape.Fixes in passing: Ports/docs popover open latches clear when the glyph unmounts because the branch has no ports/docs;
serverIsLocalresets on reconnect and only treats strictisLocal: trueas local.Reviewed by Cursor Bugbot for commit 6892bd0. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit