feat(web): read-only browser mirror of one live review (Phase 5, PR 1) - #746
Open
benvinegar wants to merge 24 commits into
Open
feat(web): read-only browser mirror of one live review (Phase 5, PR 1)#746benvinegar wants to merge 24 commits into
benvinegar wants to merge 24 commits into
Conversation
…dentity, and reconnect timing Phase 5 PR 1 needs four facts that already exist in the repo but not in a place a browser client may reach. Extracting them first keeps the client from being the second implementation of any of them (docs/browser-review-seam-audit.md). - E1: `reviewFileStatBadges` in `core/review/presentation.ts` decides badge text and states the two policies behind it — hide a zero, mark truncation once. The sidebar's private `formatSidebarStat` is deleted and tombstoned, so churn cannot read one way in a terminal and another in a browser. - G1(a): `REVIEW_VIEW_OPTION_LOCUS` in `core/review/viewOptions.ts` says which options belong to the review and which to one client, as a table over the option schema rather than as a consequence of which module happens to read a value. Totality is a `Record` over the keys. - D4 reader half: `reviewFileContentIdentityOf` recomputes a projected file's identity from the file itself, so a reader can check that what arrived still describes the content it declares without carrying a field list of its own. Projection now hashes the file it built rather than a parallel description of it, so there is one derivation. - C5: `createReconnectScheduler` in `@hunk/session-broker-core` owns "one pending attempt, a delay, an unref'd timer". `SessionBrokerConnection` and `HunkSessionBrokerClient` both drop their own copy; their fixed 3s timing is unchanged (the default factor is 1), and a browser client can now ask the same scheduler for backoff. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018L6h5GBz6RAxRXbgUS4mx4
…urces, event stream The browser client's transport half. It composes the Phase 4 contracts rather than restating them: routes and authorization from `reviewHttpProtocol`, frame names and envelopes from `reviewEventProtocol`, chunk verification from the shared `ReviewChunkAssembler`, wording from `reviewErrorCatalog`. That is what closes C4's client half and C2's browser site — the prototype re-declared all of it and its client bounds were compatible with the server's only by luck (docs/browser-review-seam-audit.md). - `reviewApiClient.ts` reads the publication, reads a resource as several bounded windows joined by the shared assembler, and follows the event stream with `fetch` — not `EventSource`, which cannot present the capability header, and whose built-in reconnect would be a second scheduler (C5). - `reviewMirror.ts` orders every arriving publication with `classifyReviewPublication` and nothing else, resyncs on `gap` only (a generation's document is immutable), and abandons a load a newer generation superseded. It verifies each canonical file by recomputing its content identity rather than checking a field list of its own (D4). - `reviewDigest.ts` is a synchronous SHA-256, because the shared assembler verifies inline as chunks arrive and Web Crypto is async-only. Platform hashing is one of the things the audit deliberately does not unify; the test holds it to the session's own digest. One gap in the Phase 4 surface had to close for any of this to be verifiable: a published catalog describes resources the producer has not measured yet, so a reader could not learn what to verify a read against. Every resource response now states the whole resource's size and digest in headers declared beside the routes, and the daemon keeps the digest its own assembly verified against instead of rehashing. `scripts/review-vocabulary.test.ts` gains the browser tier, so the client cannot re-declare a constant it could import. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018L6h5GBz6RAxRXbgUS4mx4
…ierre The browser's rendering half. `pierreDocument.ts` turns a semantic file into what Pierre draws from and refuses to compute anything core already owns — hunk extents, gap addresses, note and reveal targets, expansion side, source splitting, row totals — which is every A-finding the prototype's own adapter got wrong (docs/browser-review-seam-audit.md, A3/A4/A5/A6/A7/A8/A10). - The stream renders one Pierre view per hunk rather than per file, so a collapsed-region strip appears where the collapsed region is. Isolating a hunk is the shared re-basing walk, and the lines are sliced with the indices it reports rather than by a second count (A6). - Gap strips are labelled from `reviewGapAddress`, and opening one reveals lines split by `normalizedReviewSourceLines` over the source resource — no `\r` glyphs, no phantom last line (A4). Which gaps are open is client-local presentation state in a read-only mirror; PR 2 lowers it to the shared `expansion/toggle` intent. - File and hunk anchors are `formatReviewAddress`, giving G3's grammar its first consumer, and stat badges are the shared formatter's (E1). - `viewOptions.ts` resolves only the options `REVIEW_VIEW_OPTION_LOCUS` calls per-client, over the host's resolved defaults when the page carries them (G1). Theme is deliberately absent: E2 is undecided, so the client renders in Pierre's own palette. - `main.tsx` imports `core/fileLanguage` for its side effect, so `.mts` and `.cts` highlight in a browser as they do in a terminal (A11). The boundary gate gains a named list of browser-safe core modules for exactly that import — it refused it first, which is the gate working. Three browser consumers join the conformance corpus and run the whole thing: the projection against the geometry fixtures, the mirror against the ordering fixtures, and the client's event reader against the C4 framing fixtures, which is the sender and the reader on one corpus. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018L6h5GBz6RAxRXbgUS4mx4
…it settled The audit doc is the ledger this phase is measured against, so the browser sites it closes are written down where the finding lives rather than in a PR description: A6, A7, A11, C1, C2, C3, C4's client half, C5, D4, E1, G4, and G1 part (a), with A3/A4/A5/A8/A10 and B6's geometry half now answered by the browser projection beside the terminal. Two entries are answers rather than repayments, and both change what PR 2 has to do. The publication a client reads carries a review's content and nothing about its semantic position, so every note-shaped browser finding waits on review state reaching the wire — a read-only client cannot mirror a selection by reading harder, and one that derived its own would be the duplication this seam exists to prevent. And C3 closes by there being one supersede machine rather than by unifying two: the runtime reload queue it was paired with never landed in the rebuild. E2 stays open on purpose — the client renders in Pierre's palette and imports no terminal theme, so nothing has been decided by accident. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018L6h5GBz6RAxRXbgUS4mx4
…inned too `ReviewStream` and every model module had colocated coverage; the shell that puts them together did not. A static render is as far as this can go without a DOM, which is enough for what matters here: the file list is in review order, links through the shared address grammar, stays absent until a document exists, and reports a failure in the words the mirror handed it rather than wording invented at the edge (G4). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018L6h5GBz6RAxRXbgUS4mx4
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
The Package gate regenerates nix/bun.lock.nix and fails on drift; this is the entry its own regeneration printed for the new devDependency, applied verbatim since this environment has no nix to run the generator. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018L6h5GBz6RAxRXbgUS4mx4
Contributor
Greptile SummaryThe PR adds a read-only browser client that authenticates with a fragment capability, mirrors live review publications and resources, and renders the review through Pierre. It also adds shared reconnect scheduling, resource measurement headers, browser boundary gates, conformance consumers, and extensive tests.
Confidence Score: 4/5The malformed-path crash should be fixed before merging so invalid or hand-edited review links fail gracefully instead of aborting page initialization. The new location parser can throw URIError on malformed percent encoding, and its uncaught production caller consequently fails to mount the browser review page. Files Needing Attention: src/web/reviewApiClient.ts, src/web/main.tsx Important Files Changed
Sequence DiagramsequenceDiagram
participant Browser as Browser Review
participant Daemon as Session Daemon
participant Session as Live Hunk Session
Browser->>Daemon: GET event stream + capability header
Daemon-->>Browser: Complete publication and resource catalog
Browser->>Daemon: Range requests for canonical files
Daemon->>Session: Load resource chunks
Session-->>Daemon: Digest-verified chunks
Daemon-->>Browser: Byte windows + size/digest headers
Browser->>Browser: Assemble, verify, and render with Pierre
Daemon-->>Browser: Later publication event
Browser->>Browser: Order publication or resync generation
Prompt To Fix All With AI### Issue 1
src/web/reviewApiClient.ts:135-136
**Malformed paths abort page initialization**
When a review URL has a valid capability fragment but contains malformed percent encoding in any pathname segment, `decodeURIComponent` throws instead of returning an invalid-location result, causing the uncaught exception in `mountReviewPage` to abort page initialization rather than display the invalid-link message.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "test(web): cover the review page shell, ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…owing decodeURIComponent throws URIError on truncated percent-encoding, and parseReviewLocation's job is to answer whether a URL is a review link — a hand-edited one should land on the invalid-link message, not abort the mount. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018L6h5GBz6RAxRXbgUS4mx4
… PR 2 Theme: the browser follows the terminal's theme as a wire-delivered host default (client-overridable) — shared by wire, not by import, so no second palette exists to drift. Selection: per-client with an optional follow-the-terminal mode, so the B11 cursor fight cannot recur and actions keep carrying explicit targets. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018L6h5GBz6RAxRXbgUS4mx4
…tream The mirror conflated "where the review is" with "what the document was read for", so a load that failed had no way back: every later publication of the same generation classified stale or accepted and never re-read it. A dropped stream took the same failure path over a document the review never invalidated, and the resync that followed was a replay, so the page stayed failed with a perfectly good diff behind it. Ordering stays the shared classifier's alone; load state becomes its own fact (the generation the document on screen was read for). A publication is read again when it opens a new generation, or when the mirror has no document for the generation it names and none is on the way — which is the retry. A publication over a document the mirror still holds now restores ready, so a reconnect recovers without re-reading a generation that cannot have changed, and a stream that drops over a good document degrades to a new reconnecting status instead of hiding the review. Two lifetime bugs fall out of the same seam. A finished load published the address it captured at its start, walking the position back past a revision accepted while it read; it now publishes whichever of the two the classifier puts further along. And stop() was permanent — the stopped flag refused every later start() — so React's double-mount left the page dead; stop() now cancels rather than stops the shared scheduler, abandons the in-flight load by token, and start() attaches again unless the session already said goodbye.
The page read a file's whole source on every gap toggle and stored whatever came back under the file key. Three problems in one callback: a read issued before a reload landed after it, so gen-A text drew gen-B's gaps; a refused read was dropped on the floor, leaving the gap claiming forever that it was loading; and every toggle re-read, re-assembled, and re-hashed the same megabytes on the main thread. The lifecycle moves into a ReviewSourceStore the page watches like the mirror: it captures the generation at request time and drops what lands for another, reads once per file, and keeps a refusal as a state so the expanded gap can say — in the shared catalog's words — why there are no lines. Retry stays the reader's gesture: opening the gap again asks again, and nothing retries on its own. The request also moves out of the state updater React may run twice.
…e header one GapStrip took one flag and used it for both questions it answers, so the lines an opened gap reveals carried numbers only when hunk headers were on — a reader with line numbers on and headers off got unnumbered lines, and one with the opposite pair got numbers they had turned off. The two view options are now plumbed separately. The file's render model also moves behind a memo keyed on the file it is built from. It depends on content alone — width reaches Pierre, not the model — so every resize event was rebuilding each file's model and handing Pierre fresh object identities to re-highlight against.
…settling The mirror now re-reads a generation it holds no document for, which is how a failed load is retried. That makes the adapter's "a read means gap" inference depend on the first read still being in flight, so the reason is written down where an editor would otherwise add an await.
…screen A new generation's document reaches the stream one render before the effect that clears the source store can run, so an open gap could paint the previous file's lines under the new file's geometry for a frame. The store now carries the generation its entries were read for, and the page hands the stream nothing when that generation is not the one it is drawing.
…-phase-5-mirror # Conflicts: # AGENTS.md
The diff header and the static pager hand-rolled their own stat badges, so the same file read `+1 -0` there and `+1` in the sidebar, which already took its text from the shared review formatter. Both now call `reviewFileStatBadges`, and a zero count stops being a badge everywhere. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018L6h5GBz6RAxRXbgUS4mx4
The `id:`/`event:`/`data:` prefixes, the slice offsets behind them, and the blank-line record boundary were declared four times — once implicitly by `encodeReviewEventFrame` and three more times by readers that had to guess compatible offsets (C4). `ReviewEventSseDecoder` now sits beside the writer, holds a partial record until the read that completes it, and every reader imports it instead of re-deriving the grammar. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018L6h5GBz6RAxRXbgUS4mx4
Three copies of "run this under a limit" had already drifted: the mirror's clamp let a limit of zero start no workers at all, while the broker's guarded it. `inBoundedParallel` and `ConcurrencyGate` now live together in the browser-safe broker core, with the guarded clamp, and the mirror, the broker state, and the producer's resource store all use them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018L6h5GBz6RAxRXbgUS4mx4
…oks like The client re-declared the wire failure type field for field, rebuilt the server's body-and-wording policy beside it, and read a bare 416 through a literal while the code-to-status table stayed server-only. The failure builder and the status table now live in `reviewHttpProtocol` with the rest of the HTTP contract, and both ends import them; the client reads a body-less refusal through the table's unambiguous inverse instead of a hardcoded status. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018L6h5GBz6RAxRXbgUS4mx4
The page's "incomplete link" text was the catalog's `unauthorized` remedy pasted in without its statement, and eight other sites replaced the catalog sentence with wording invented in this tier — so a browser explained the same failure differently from the terminal beside it (G4). Each now states the catalog's sentence and appends what only this client knows in parentheses, which is what `transportFailure` already did. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018L6h5GBz6RAxRXbgUS4mx4
…inverse Core owned "which gap does this line belong to" but not "which lines does this gap reveal", so the terminal, the browser, and the conformance model each paired the two sides' ranges with the expansion side themselves (A6) — one offset apart from mislabelling every expanded line on a side. `reviewExpandedGapLines` states it beside `resolveReviewExpandedLine`, with a round trip between the two, and all three read it from there. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018L6h5GBz6RAxRXbgUS4mx4
Three small re-declarations: the client asked for an event stream by a literal it could import, a conformance adapter spelled the canonical-file content type out by hand, and the browser's view options declared their own `auto | split | stack` beside the terminal's `LayoutMode`. The layout union now lives with the shared view-option classification, which both tiers already reach, and the two content types come from the modules that define them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018L6h5GBz6RAxRXbgUS4mx4
Three tiers share this vocabulary and only the import path said which one a name came from. Under the repo owner's rule — `Browser*` runs in a browser, `Web*` is the daemon operating the web path, bare names are the shared language both speak — every `src/web` export takes the `Browser` prefix, and the daemon's HTTP surface becomes `webReviewServer` / `WebReviewServer`. File names, the shared protocol modules, and the browser-safe gate lists are unchanged; so is behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018L6h5GBz6RAxRXbgUS4mx4
Every other browser/web-path file in the repo already carries its runtime prefix; after the symbol rename, src/web's bare file names were the odd ones out and no longer matched their primary exports. main.tsx keeps its bundler-conventional name. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018L6h5GBz6RAxRXbgUS4mx4
…-phase-5-mirror # Conflicts: # bun.lock # package.json
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
2 participants
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.
Phase 5, PR 1 of the browser-review rebuild (
docs/browser-review-rebuild.md): the read-only browser mirror. A browser client that authenticates with the capability from the URL fragment, reads one live review over the Phase 4 HTTP surface, and renders it as a read-only review stream with Pierre. No actions, no note editing, no selection sync — that is PR 2. The browser page itself has no runtime entry until Phase 6; the one user-visible change is apatchchangeset for a stat-badge consistency fix the drift audit surfaced (below).What
New browser tier (
src/web/) — Node-free by gate, every export named for its runtime (Browser*= runs in the browser;Web*= server code operating the web path; bare = shared language):reviewApiClient.ts(BrowserReviewApiClient) — publication reads, Range-windowed resource reads reassembled by the sharedReviewChunkAssembler, and the SSE stream read withfetchstreaming (the capability travels as a header, whichEventSourcecannot carry), decoded byreviewEventProtocol's own record decoder and frame parsers. Failures are the shared wire failure shape with catalog wording.reviewMirror.ts(BrowserReviewMirror) — holds one publication; orders candidates withclassifyReviewPublicationonly; recovers from failed loads and dropped streams (reconnectingstatus over a held document); never moves position backwards; verifies every canonical file by recomputing its content identity.pierreDocument.ts— projectsReviewFileV1into the per-file render model from the shared geometry/anchors/expansion modules — nosideRange, no baresplit("\n"), and expanded-gap lines now come from core'sreviewExpandedGapLines, beside its inverse.reviewSources.ts(BrowserReviewSourceStore) — generation-tagged source reads: stale reads dropped at resolve and render, failures surface catalog wording, one read per file, reopening a failed gap is the retry.reviewDigest.ts(browserReviewDigest) — the sanctioned synchronous SHA-256 platform carve-out, pinned differentially against the Node implementation.viewOptions.ts/ReviewStream.tsx/ReviewApp.tsx/main.tsx(BrowserReviewStream,BrowserReviewApp,mountBrowserReviewPage) — client-locus view options over host defaults; one Pierre render per hunk with gap strips; page shell in review order.Shared additions (each one extraction, adopted by every consumer, old copies deleted):
core/review/presentation.ts—reviewFileStatBadges, now the only stat-badge formatter: sidebar, diff header, static pager, and browser all consume it (E1, completed by the drift audit).core/review/viewOptions.ts—REVIEW_VIEW_OPTION_LOCUS(G1a) and the singleLayoutModeunion.core/review/expansion.ts—reviewExpandedGapLines, the forward gap-row derivation besideresolveReviewExpandedLine, consumed by terminal, browser, and conformance.core/review/document.ts—reviewFileContentIdentityOf; the projection hashes the file it built.session/reviewEventProtocol.ts—ReviewEventSseDecoder, the SSE record grammar colocated with its encoder; five former hand-rolled copies deleted.session/reviewHttpProtocol.ts—REVIEW_ERROR_STATUS,reviewHttpFailure(),reviewErrorCodeForStatus(): one owner for the HTTP failure contract, consumed by server and client.packages/session-broker-core—createReconnectScheduler(C5) andconcurrency.ts(inBoundedParallel+ConcurrencyGate), adopted by broker, browser, and producer.Phase 4 surface completed: resource responses carry whole-resource size/digest headers (colocated writer/parser pair, round-trip tested) so a client can verify reads; the server module is renamed for what it is — daemon code operating the web path:
src/session/broker/webReviewServer.ts(WebReviewServer).Gates extended: browser-tier boundary walk (
BROWSER_SAFE_CORE_MODULES), andscripts/review-vocabulary.test.tsscanssrc/webfor re-declared shared constants and hand-written digest patterns.Conformance
Real consumers registered for geometry (browser projection), ordering (the real mirror), and events (the client reader against a real HTTP surface). Navigation and wire deliberately wait for PR 2 — a read-only client plans no moves and sends no actions; recorded in the audit doc.
Findings repaid
A6, A7, A11, C1/C2 (browser), C3 (closed), C4 (client), C5, D4 (browser), E1 (completed across all four surfaces), G1(a), G4 (browser adoption); G3 gains its first consumer. E2 and G2 are decided and recorded in the audit doc: theme follows the terminal as a wire-delivered host default with client override (shared by wire, not by import); selection is per-client with optional follow-the-terminal. Both land as implementation in PR 2.
Post-open hardening
Two dedicated review passes ran against this branch after it opened, and everything they found is fixed here:
patchchangeset), the re-declared SSE record grammar (5 copies → 1 decoder), duplicated bounded-concurrency helpers (3 copies → 1 shared module), the client's re-declared HTTP failure shape and status mapping, invented wording for cataloged error codes, the third copy of the expanded-gap derivation (now in core beside its inverse), and inline content-type/layout-union literals.src/webexport carriesBrowser*/browser*; the daemon's serving module isWeb*. One behavior-free commit; the targeted suite is byte-identical across it.Gates
bun run typecheck,bun run lint(0/0),bun run format— clean;patchchangeset (file change stats render consistently across sidebar, diff header, pager, and browser, hiding zero counts everywhere).bun testat head vs. baseline: failure set no worse (rotating PTY parallel-load flakes that pass in isolation — a fulltest/pty/re-run gives 111/0 — plus the two pre-existing website spec errors). Targeted suites at final head: 1,003 tests / 0 fail across web, conformance, gates, core review, broker-core, and the producer→daemon→HTTP→client integration test, independently re-run at review time.src/ui/+src/opentui/in full: 1,031/0 (the stat-badge change is TUI-visible and deliberately pinned).Residuals for PR 2
Review semantic state (selection, filter, expansion, notes) on the wire — the blocker for every note-shaped browser finding. Then: action dispatch, note editing (draft-body intent), selection sync per the decided G2 policy, theme-as-host-default per the decided E2 policy, key bindings and the command palette from the shared catalog, A9 STML parser relocation, navigation + wire conformance consumers. Recorded low-severity audit notes (gap-strip wording,
rangeLabelformat,reviewSourcesdescriptor lookup) carry to PR 2's cleanup.Generated by Claude Code