M .beads/interactions.jsonl => .beads/interactions.jsonl +1 -0
@@ 13,3 13,4 @@
{"id":"int-70f93fbf5d69de0ca9f334d61c671867","kind":"field_change","created_at":"2026-07-24T19:44:55.309486Z","actor":"Eugene Blikh","issue_id":"spec-by6.3.4","extra":{"field":"status","new_value":"closed","old_value":"open","reason":"spec_comment: list threads with anchor state + reply; no-resolve enforced by interface. 9 tests green, no DB needed. Committed f82d90a."}}
{"id":"int-6f8f60b7e8df8f63b87952e47015d2d5","kind":"field_change","created_at":"2026-07-24T19:59:27.168688Z","actor":"Eugene Blikh","issue_id":"spec-by6.3.3","extra":{"field":"status","new_value":"closed","old_value":"open","reason":"Context blocks + anchor ids + thread UI; lost-anchor area; stale-block 409. Committed 472bcb1."}}
{"id":"int-e8dbd67090e659150fb378d33b692d30","kind":"field_change","created_at":"2026-07-24T19:59:29.109943Z","actor":"Eugene Blikh","issue_id":"spec-by6.3","extra":{"field":"status","new_value":"closed","old_value":"open","reason":"All four children done: core+db anchors (1a25367), service API + auto-merge gate (c6e5d66), mcpsrv spec_comment (f82d90a), web commentable diff (472bcb1)."}}
+{"id":"int-e7b60421ba107f7bcf5b5551535f1218","kind":"field_change","created_at":"2026-08-05T01:11:46.647218Z","actor":"Eugene Blikh","issue_id":"spec-by6.3.5","extra":{"field":"status","new_value":"closed","old_value":"open","reason":"Closed"}}
M .beads/issues.jsonl => .beads/issues.jsonl +3 -0
@@ 1,5 1,6 @@
{"_type":"issue","id":"spec-zqb","title":"Phase 3: write plane — agents propose","description":"The agent half of the loop, and the service's whole premise: an agent proposes a document change, gets back a link, a human approves in a browser. Until this ships, agents can only read.","design":"Scope: proposals (branch proposals/\u003cid\u003e + Postgres row, state open-\u003emerged|rejected); the If-Match tree-splice merge with the four staleness cases and the ancestry check gitx flagged; the single agent token + mandatory provenance trailers; spec_propose / spec_comment over REST + MCP; every write response returns {proposal, url} so the agent can hand over a link. Prereqs found during Phases 1-2: (1) service.ListProposals(space, state) — graph proposal listing is stubbed and db only has ListProposalsByState; (2) IsAncestor(proposalHead, H) check before Merge so an already-merged proposal reports 'already merged' not a confusing 409; (3) auto-merge policy evaluation (.spec.yml AutoMerges) which the reconciler's 'approval=policy' inference also wants. Dispatch like Phases 1-2: proposal orchestration in service/ first and committed, then surface tools fan out. This is where the merge model and reconciler first run under real proposals rather than on paper.","status":"closed","priority":1,"issue_type":"feature","assignee":"Eugene Blikh","owner":"bigbes@gmail.com","created_at":"2026-07-23T04:08:58Z","created_by":"Eugene Blikh","updated_at":"2026-07-23T05:29:29Z","started_at":"2026-07-23T04:52:59Z","closed_at":"2026-07-23T05:29:29Z","close_reason":"Write plane shipped: service.Propose/Merge/Reject/ListProposals with provenance trailers, auto-merge policy, If-Match staleness + already-merged ancestry check; surfaces graph (Proposals port), mcpsrv spec_propose, and new api/ REST PUT — every write returns {proposal, url}. Validated end-to-end against Postgres. spec_comment deliberately deferred to Phase 5 (spec-by6), which gates the comment schema on the Phase 4 review UI; the approve/reject browser UI is Phase 4 (spec-3vz).","dependencies":[{"issue_id":"spec-zqb","depends_on_id":"spec-ejq","type":"parent-child","created_at":"2026-07-23T07:09:24Z","created_by":"Eugene Blikh","metadata":"{}"}],"dependency_count":0,"dependent_count":1,"comment_count":0}
{"_type":"issue","id":"spec-ejq","title":"spec.sr.ht — reviewable document storage for humans and agents","description":"A third custom Go service on the self-hosted SourceHut instance: agents propose documents, a human reviews and curates, agents read the approved text. One loop — bot produces, human curates, bots consume. Runs at https://spec.srht.bigb.es, deployed on phoebe as srht-spec-1. Code: git.sourcecraft.dev/bigbes/sr-ht-spec. Design: docs/DESIGN.md.","design":"DONE and deployed (Phases 0-2): core domain, gitx (bare repos + id-keyed tree-splice merge), db (Postgres schema + global ID registry), authn (unified-login cookie + agent tokens + provenance), the three receive hooks + hook RPC + daemon, service layer + reconciler, doc (warren vault/render on git objects), search (one global bleve index, per-line ru/en routing), projects (saved filter, not container), web read UI, mcpsrv read tools, graph read schema, prosediff (Phase 0 gate PASSED), specsrht space create/list, push-\u003ereindex. Live corpus: ~bigbes/rfcs holds SPEC-0001 and NOTE-0001. Verified end-to-end: validating push path, fail-closed reads, id-addressing, bilingual search, GraphQL. NOT DONE: the agent half of the loop (write plane), review UI, comments.","status":"open","priority":1,"issue_type":"epic","owner":"bigbes@gmail.com","created_at":"2026-07-23T04:08:44Z","created_by":"Eugene Blikh","updated_at":"2026-07-23T04:08:44Z","dependency_count":0,"dependent_count":0,"comment_count":0}
+{"_type":"issue","id":"spec-by6.3.5","title":"Port review UI to a line-numbered unified prose diff","description":"Replace the block-card review renderer with a real unified diff: line-numbered gutter, drag-select a line range, comment composer inline. Owner reviewed the card version against a live proposal and rejected it — labels ('ADDED PARAGRAPH') outweighed content on every row, every block carried identical chrome, and on a new file the whole page is green so the cards add noise and no signal. Split view was considered and dropped: prose lines are long and unified reads better for reflowed text. Approved prototype: https://claude.ai/code/artifact/a45e45cb-16b5-42ec-8108-84767ef9f465","design":"SELECTION IS BY LINE, ANCHORING IS BY BLOCK. Lines are what the cursor lands on; block hashes are what survive a reflow. The web layer maps a selected line range to its enclosing prosediff block and stores the existing core.CommentAnchor unchanged. service/, db/ and core/ do not move. The composer states what it will anchor to, so the indirection is visible rather than magic.\n\nTHE BLOCKING PROBLEM — word marks cannot be placed on a line for free.\nprosediff.Span is {Op, Text, Space} with no source offsets, and Tokenize\ndeliberately drops whitespace ('\\n' and ' ' both collapse to Space=true) —\nwhich is exactly what makes a rewrap invisible to the differ. So for a\nChangeModify block there is no stored answer to 'which line did this word\nchange on'.\n\nRecoverable, not free: the span script consumes the old token sequence in order\n(Equal+Delete) and the new one (Equal+Insert). Re-tokenize each source line of\nBlock.Lines, walk the script, and split a span where it crosses a line\nboundary. Needs a mapper of roughly 100 lines plus tests. Equal/Insert/Delete\nblocks need none of this — their lines map 1:1 and carry exact numbers.\n\nFALLBACK IF THE MAPPER IS NOT WANTED: render a modified block as a paired\nold/new region labelled by line RANGE rather than per line, keeping the word\nmarks. Honest and much smaller, but does not match the approved prototype.\n\nGUTTER DETAILS SETTLED IN REVIEW: one shared rail ground for both number\ntracks with a single hairline against the content (a border per cell drew a\ncage); one --row-lh shared by gutter and prose so numerals sit on the text\nbaseline rather than floating; 34px tracks; the add/delete tint starts at the\nsign column so the gutter never looks part of the change.\n\nHEADING PATH: no per-hunk breadcrumb. It restated headings visible a few rows\nabove, always so on a new file. Replaced by one sticky section readout that\nappears only once its heading has scrolled past. The path stays in the composer\nand in each thread's anchor note, where the heading is usually off screen.","acceptance_criteria":"A reviewer drags across line numbers, comments, and the thread stores an anchor identical to the one the block-card UI would have stored. Word-level marks still appear inside a modified paragraph. Unchanged lines render as context and collapse. An outdated comment still appears and is never attached to a neighbouring block.","status":"closed","priority":2,"issue_type":"feature","owner":"bigbes@gmail.com","created_at":"2026-07-25T11:40:01Z","created_by":"Eugene Blikh","updated_at":"2026-08-05T01:11:47Z","closed_at":"2026-08-05T01:11:47Z","close_reason":"Closed","dependencies":[{"issue_id":"spec-by6.3.5","depends_on_id":"spec-by6.3","type":"parent-child","created_at":"2026-07-25T14:40:01Z","created_by":"Eugene Blikh","metadata":"{}"}],"dependency_count":0,"dependent_count":0,"comment_count":0}
{"_type":"issue","id":"spec-ejq.2","title":"CI publish task fails: build secret apk-ci-s3 is missing","description":"Every recent builds.sr.ht run fails at the publish task while every other task passes. Confirmed on jobs #244 (e97532c), #245 (2dc6b71) and #246 (c2dd1ef): 'scss keygen version build' all SUCCESS, 'publish' FAILED. Pre-existing — #244 and #245 predate the Phase 5b work, so no code change caused it. Consequence: the apk never reaches the Garage repo bucket, so apk-mirror on phoebe has nothing new to re-index and the deployed service cannot be upgraded from CI output.","design":"EVIDENCE. .build.yml declares 'secrets: [apk-ci-s3]', documented as a File secret installed at ~/.apk-ci.env carrying APK_CI_S3_ACCESS_KEY / APK_CI_S3_SECRET_KEY for the Garage repo bucket. The publish task's first real line is '. ~/.apk-ci.env'. But 'hut builds secret list' returns exactly four secrets and none is apk-ci-s3: agent1-deploy (SSH key), bencher-api-key, s3-cache-key-secret, s3-cache-key-id. So the file the task sources is never installed.\n\nNOT VERIFIED: the exact failure text. The raw log endpoint (https://builds.srht.bigb.es/query/log/246/publish/log) needs a Bearer token, and reading hut's credential file was correctly refused, so the diagnosis rests on the secret list plus the manifest rather than on the log line itself. Read the log to confirm before acting.\n\nTWO POSSIBILITIES, needs the owner to distinguish:\n1. The secret was deleted or never created — fix is to create a File secret named apk-ci-s3 at ~/.apk-ci.env (mode 600) holding the two S3 keys.\n2. builds.sr.ht resolves manifest secrets by UUID, not by name — fix is to replace the name with the secret's UUID in .build.yml.\n\nSAME BUG IN THE SIBLING: ~/data/home/sourcehut-compare/.build.yml carries a byte-identical secrets block, so compare.sr.ht's publish is broken the same way and both fix together.","acceptance_criteria":"A push to master produces a build whose publish task succeeds and uploads the .apk to the Garage repo bucket; apk-mirror on phoebe re-indexes it within 15 minutes.","status":"open","priority":2,"issue_type":"bug","owner":"bigbes@gmail.com","created_at":"2026-07-24T21:16:35Z","created_by":"Eugene Blikh","updated_at":"2026-07-24T21:16:35Z","dependencies":[{"issue_id":"spec-ejq.2","depends_on_id":"spec-ejq","type":"parent-child","created_at":"2026-07-25T00:16:35Z","created_by":"Eugene Blikh","metadata":"{}"}],"dependency_count":0,"dependent_count":0,"comment_count":0}
{"_type":"issue","id":"spec-by6.3.3","title":"web: commentable diff — context blocks, anchor ids, comment UI","description":"renderDocDiff currently skips ChangeEqual ('the review shows only what changed', web/diff.go:61). It must now render unchanged blocks as commentable context — collapsed or dimmed so the diff still reads as a diff — because the settled scope is that any block of a proposed document can be commented on. Every rendered block gains a stable DOM id derived from the anchor tuple. Then the comment UI itself: existing comments rendered beside their block with their anchor state (moved / outdated shown honestly), a compose form, reply and resolve. Threading is flat per anchor.","status":"closed","priority":2,"issue_type":"task","owner":"bigbes@gmail.com","created_at":"2026-07-24T18:57:14Z","created_by":"Eugene Blikh","updated_at":"2026-07-24T19:59:27Z","closed_at":"2026-07-24T19:59:27Z","close_reason":"Context blocks + anchor ids + thread UI; lost-anchor area; stale-block 409. Committed 472bcb1.","dependencies":[{"issue_id":"spec-by6.3.3","depends_on_id":"spec-by6.3.2","type":"blocks","created_at":"2026-07-24T21:57:29Z","created_by":"Eugene Blikh","metadata":"{}"},{"issue_id":"spec-by6.3.3","depends_on_id":"spec-by6.3","type":"parent-child","created_at":"2026-07-24T21:57:14Z","created_by":"Eugene Blikh","metadata":"{}"}],"dependency_count":1,"dependent_count":0,"comment_count":0}
{"_type":"issue","id":"spec-by6.3.2","title":"service: comment API + policy auto-merge gate","description":"service.Comment type, Comment/ListComments/ReplyComment/ResolveComment, and the anchor resolution pass that takes a revision's prosediff blocks and reports each comment as anchored, moved or outdated. Wire the merge gate: an unresolved comment suppresses policy auto-merge (not manual approve) — the check belongs beside the existing auto-merge policy evaluation, and needs a test that a commented proposal stops auto-merging while Merge with human approval still succeeds.","status":"closed","priority":2,"issue_type":"task","assignee":"Eugene Blikh","owner":"bigbes@gmail.com","created_at":"2026-07-24T18:57:05Z","created_by":"Eugene Blikh","updated_at":"2026-07-24T19:15:24Z","started_at":"2026-07-24T19:09:52Z","closed_at":"2026-07-24T19:15:24Z","close_reason":"service.CommentOn/ReplyTo/ResolveThread/Threads + AnchorThreads/AnchorOf; auto-merge gate in autoMerges. 10 tests, PG-backed ones green against real PG 16.","dependencies":[{"issue_id":"spec-by6.3.2","depends_on_id":"spec-by6.3","type":"parent-child","created_at":"2026-07-24T21:57:05Z","created_by":"Eugene Blikh","metadata":"{}"},{"issue_id":"spec-by6.3.2","depends_on_id":"spec-by6.3.1","type":"blocks","created_at":"2026-07-24T21:57:28Z","created_by":"Eugene Blikh","metadata":"{}"}],"dependency_count":1,"dependent_count":2,"comment_count":0}
@@ 8,6 9,8 @@
{"_type":"issue","id":"spec-43w","title":"Stale apk pins: paste.sr.ht 0.19.3-r0 and hub.sr.ht 0.29.4-r0 aged off the mirror","description":"Pre-existing, not caused by spec.sr.ht — but a FULL stack rebuild (post_push) would fail on these two apk layers and leave the stack un-deployable (the 2026-07-11 incident pattern). The self-hosted append-only repo.bigb.es mirror only preserves what it has seen, and it started after these aged off; old versions are unrecoverable.","design":"spec.sr.ht was deployed AROUND this with labng push --no-restart + a single-service 'docker compose build spec', so nothing else rebuilt. To fix properly: bump the two pins in versions.env to what the mirror currently ships (paste 0.19.4-r0, hub 0.31.1-r0 as of 2026-07-22). hub 0.29.4-\u003e0.31.1 crosses two minors with forward-only migrations — pg_dumpall first per the stack CLAUDE.md. Kept separate from the spec deploy deliberately.","status":"closed","priority":2,"issue_type":"bug","owner":"bigbes@gmail.com","created_at":"2026-07-23T04:09:54Z","created_by":"Eugene Blikh","updated_at":"2026-07-23T04:52:46Z","closed_at":"2026-07-23T04:52:46Z","close_reason":"Stale apk pins already bumped in the stack versions.env (paste 0.19.4-r0, hub 0.31.1-r0); fixed out of band.","dependencies":[{"issue_id":"spec-43w","depends_on_id":"spec-ejq","type":"parent-child","created_at":"2026-07-23T07:09:54Z","created_by":"Eugene Blikh","metadata":"{}"}],"dependency_count":0,"dependent_count":0,"comment_count":0}
{"_type":"issue","id":"spec-ar4","title":"Restart other SourceHut services so spec.sr.ht appears in their nav","description":"The switcher entry is config-driven: each service builds its own nav from config sections ending in .sr.ht. spec.sr.ht is in the shared config.ini now, but the OTHER services (git, meta, todo, ...) only pick up the new entry on restart. Not done unprompted because it briefly bounces every service. On phoebe: docker compose restart \u003cthe sourcehut services\u003e.","notes":"2026-07-24: verified still needed, but BLOCKED on host access from this machine.\n\nVerified over HTTPS (LAN, read-only):\n- spec.srht.bigb.es -\u003e HTTP 200, the service itself is up.\n- git / meta / todo .srht.bigb.es nav all still MISSING a spec entry, so the restart has not happened and is still the remaining step.\n\nAccess attempts: 'phoebe' does not resolve; srht.bigb.es domains resolve to 192.168.88.18, which answers on :22 but refuses blikh (publickey,password). The working git remote is the restricted 'git' user shell, not a login. No deploy repo here carries an SSH path — deploys to phoebe are pull-based (apk-mirror re-indexes within 15 min), so there is no push-SSH route configured on this Mac.\n\nNeeds the owner to run it on phoebe directly:\n docker compose restart\nThen confirm with:\n curl -s https://git.srht.bigb.es | grep -c spec.srht.bigb.es # expect \u003e0","status":"in_progress","priority":2,"issue_type":"task","assignee":"Eugene Blikh","owner":"bigbes@gmail.com","created_at":"2026-07-23T04:09:53Z","created_by":"Eugene Blikh","updated_at":"2026-07-24T20:23:14Z","started_at":"2026-07-24T20:21:42Z","dependencies":[{"issue_id":"spec-ar4","depends_on_id":"spec-ejq","type":"parent-child","created_at":"2026-07-23T07:09:53Z","created_by":"Eugene Blikh","metadata":"{}"}],"dependency_count":0,"dependent_count":0,"comment_count":0}
{"_type":"issue","id":"spec-3vz","title":"Phase 4: review plane — approve/reject in a browser","description":"Proposal pages at stable URLs (returned by every write), the inbox, prose diff, approve (merges immediately) / reject, status lifecycle, and the digest of policy-merged firehose content.","design":"Productionise prosediff (the Phase 0 spike) into the review UI. CRITICAL requirement from the Phase 0 verdict: 13% of real prose modifications shred into interleaved fragments (similarity \u003c= 0.73), so the UI MUST switch to a two-column old/new view below ~0.75 and inline word diffs above it — building only the inline renderer makes one review in eight unreadable. The prosediff package already emits the (Hash, HeadingPath, Ordinal) comment-anchor tuple. Reads a proposal branch via service.ReadDocumentAtRef (the deliberately-awkward bypass), never the normal read path.","status":"closed","priority":2,"issue_type":"feature","assignee":"Eugene Blikh","owner":"bigbes@gmail.com","created_at":"2026-07-23T04:09:25Z","created_by":"Eugene Blikh","updated_at":"2026-07-23T06:52:55Z","started_at":"2026-07-23T06:31:53Z","closed_at":"2026-07-23T06:52:55Z","close_reason":"Review plane shipped: proposal page at /~owner/space/p/\u003cid\u003e with the prose diff (inline word-diff above 0.75 similarity, two-column old/new below — the Phase 0 verdict's hard requirement, in web/diff.go), approve(merge)/reject as owner-only CSRF-guarded POSTs, status badges, and the /inbox review queue + policy-merged digest. service.ProposalDiff/InboxProposals/DigestProposals/MergeHuman back it; web.Reader extended. Diff reads the proposal branch by resolving its tip to a sha (pinned read) rather than the ReadDocumentAtRef bypass — cleaner and immutable. Validated: diff renderer unit tests (inline/two-column/escaping), proposal-page + approve/reject handler tests (CSRF, owner-only, stale-\u003e409, wrong-space-\u003e404), inbox tests, and PG-backed service tests. Follow-up: digest_mark 'since last seen' tracking.","dependencies":[{"issue_id":"spec-3vz","depends_on_id":"spec-ejq","type":"parent-child","created_at":"2026-07-23T07:09:25Z","created_by":"Eugene Blikh","metadata":"{}"},{"issue_id":"spec-3vz","depends_on_id":"spec-zqb","type":"blocks","created_at":"2026-07-23T07:09:26Z","created_by":"Eugene Blikh","metadata":"{}"}],"dependency_count":1,"dependent_count":1,"comment_count":0}
+{"_type":"issue","id":"spec-by6.5","title":"prosediff: an equal span's Space flag is read from the old side only","description":"prosediff/token.go:207, spans(): Span.Space is taken from toks[0].Space, and for an equal run those tokens come from the OLD side. An insertion at the head of a block therefore emits\n\n op=insert space=false text=\"w16 w17\"\n op=equal space=false text=\"w12 w13\"\n\nand any renderer joining the script writes the two runs together with no space: {+w16 w17+}w12 w13.\n\nNot reachable from the web layer today — the new side's separator is simply not in the data the web layer is handed, and web/diffrows.go's sideSpans already carries a dropped span's separator onto the next kept one for the region rows. It affected the old block-card renderer identically, so it is not a regression from the line-numbered diff port (spec-by6.3.5).\n\nFix shape: OR the two sides' leading-token flags when building an equal span. Found by the property test added in web/diffrows_test.go, which is deliberately blind to spacing (it compares nonce word sequences) and says so.","status":"open","priority":3,"issue_type":"bug","owner":"bigbes@gmail.com","created_at":"2026-08-05T01:10:14Z","created_by":"Eugene Blikh","updated_at":"2026-08-05T01:10:14Z","dependencies":[{"issue_id":"spec-by6.5","depends_on_id":"spec-by6","type":"parent-child","created_at":"2026-08-05T04:10:14Z","created_by":"Eugene Blikh","metadata":"{}"}],"dependency_count":0,"dependent_count":0,"comment_count":0}
+{"_type":"issue","id":"spec-by6.4","title":"A code fence whose only change is its info string renders as unchanged","description":"prosediff.Block.Lines excludes the fence delimiters, so a change from ```go to ```python produces no line-script difference. Stats counts the block as modified (BlocksModified: 1) while the unified diff table renders every one of its lines as ph-r-eq context. The page therefore says 'this document changed' and then shows nothing that changed.\n\nFound by adversarial verification of spec-by6.3.5; pre-existing in the segmenter, not introduced by the line-numbered diff port. It matters more now than it did under the block-card UI, because the table's whole contract is that a row states what happened to that line.\n\nFix is in prosediff: either carry the info string into the block's diffed content, or have the web layer render an explicit marker row when Block.Info differs across a ChangeModify.","status":"open","priority":3,"issue_type":"bug","owner":"bigbes@gmail.com","created_at":"2026-08-05T00:49:06Z","created_by":"Eugene Blikh","updated_at":"2026-08-05T00:49:06Z","dependencies":[{"issue_id":"spec-by6.4","depends_on_id":"spec-by6","type":"parent-child","created_at":"2026-08-05T03:49:05Z","created_by":"Eugene Blikh","metadata":"{}"}],"dependency_count":0,"dependent_count":0,"comment_count":0}
{"_type":"issue","id":"spec-by6.3.4","title":"mcpsrv: spec_comment tool (read + reply, no resolve)","description":"The agent half of the loop. spec_comment lets an agent list the comments on a proposal it authored and reply to them; it must NOT resolve one, since an agent marking its own critique resolved defeats the auto-merge gate. Reuses the Phase 3 provenance path (agent + agent_session) for reply authorship, and the existing owner+agents read gate from spec-jjo. mcpsrv/mcpsrv.go:39 still documents spec_comment as deliberately absent — update that comment when it lands.","notes":"Implemented. Divergence from this issue's original text, accepted: reads are NOT narrowed to 'a proposal the agent authored'. Agent identity is self-declared (X-Agent/X-Agent-Session headers) and all agents share one token, so an authorship check would gate on a string the caller chooses — stricter on paper than the owner+agents read plane (spec-jjo) it sits in, while enforcing nothing. Revisit if/when per-agent token scoping lands (spec-by6 backlog).\n\nNo-resolve is enforced by TYPE: Commenter names only Threads/ReplyTo/GetProposal/ProposalDiff, so service.CommentOn and service.ResolveThread are unreachable from the handler however service/ grows. Writer is now Proposer+Commenter.\n\nExtra beyond the issue: replying requires proposal+thread and checks membership, because a thread id is global and service.ReplyTo takes only the id — a mistyped id would otherwise reply on a stranger's proposal.","status":"closed","priority":3,"issue_type":"task","owner":"bigbes@gmail.com","created_at":"2026-07-24T18:57:18Z","created_by":"Eugene Blikh","updated_at":"2026-07-24T19:44:55Z","closed_at":"2026-07-24T19:44:55Z","close_reason":"spec_comment: list threads with anchor state + reply; no-resolve enforced by interface. 9 tests green, no DB needed. Committed f82d90a.","dependencies":[{"issue_id":"spec-by6.3.4","depends_on_id":"spec-by6.3.2","type":"blocks","created_at":"2026-07-24T21:57:30Z","created_by":"Eugene Blikh","metadata":"{}"},{"issue_id":"spec-by6.3.4","depends_on_id":"spec-by6.3","type":"parent-child","created_at":"2026-07-24T21:57:17Z","created_by":"Eugene Blikh","metadata":"{}"}],"dependency_count":1,"dependent_count":0,"comment_count":0}
{"_type":"issue","id":"spec-by6.2","title":"Attachments: image embeds render as visibly missing","description":"gitx walks .md only, so an archive built from a git tree has no attachment index and ![[diagram.png]] resolves to a visibly-missing link. doc.FromPages already accepts an assets map — the gap is producing one. Two options: (a) gitx.WalkBlobs + an asset index threaded through service.Archive, plus a read-plane route serving the blob; (b) adopt Mermaid-by-convention and document that binary attachments are out of scope. Carried over from spec-wcr item 7 (triaged 2026-07-24: a feature, not a latent bug).","status":"open","priority":3,"issue_type":"feature","owner":"bigbes@gmail.com","created_at":"2026-07-24T18:12:24Z","created_by":"Eugene Blikh","updated_at":"2026-07-24T18:12:24Z","dependencies":[{"issue_id":"spec-by6.2","depends_on_id":"spec-by6","type":"parent-child","created_at":"2026-07-24T21:12:24Z","created_by":"Eugene Blikh","metadata":"{}"}],"dependency_count":0,"dependent_count":0,"comment_count":0}
{"_type":"issue","id":"spec-45j","title":"Phase 5a-2: webhook tables, SDL, resolvers, delivery on proposal open/merge/reject","description":"On top of 5a-1: gql_proposal_wh_sub/_delivery tables + migration, WebhookSubscription/Delivery/Payload SDL, create/delete mutations + webhooks queries, and fire via webhooks.Schedule on proposal lifecycle events (service event hook -\u003e queue). Reference pages.sr.ht webhooks/webhooks.go + graph/model/webhooks.go.","status":"closed","priority":3,"issue_type":"feature","owner":"bigbes@gmail.com","created_at":"2026-07-24T02:05:29Z","created_by":"Eugene Blikh","updated_at":"2026-07-24T09:50:16Z","closed_at":"2026-07-24T09:50:16Z","close_reason":"Webhook tables/SDL/resolvers/delivery all shipped and verified end-to-end.","dependencies":[{"issue_id":"spec-45j","depends_on_id":"spec-3m9","type":"parent-child","created_at":"2026-07-24T05:05:30Z","created_by":"Eugene Blikh","metadata":"{}"}],"dependency_count":0,"dependent_count":0,"comment_count":0}
M prosediff/segment.go => prosediff/segment.go +92 -2
@@ 115,7 115,12 @@ func Segment(src []byte) []Block {
// paragraph, which diffs badly and is not what the block is.
func splitFrontmatter(src []byte) (body []byte, fm []string, lineOffset int) {
s := string(src)
- if !strings.HasPrefix(s, "---\n") && s != "---" {
+ // The opening fence must be a whole line. A document that is nothing but
+ // "---" used to be admitted here as well and then sliced at [4:] on three
+ // bytes; it is not frontmatter under any reading — there is no line after it
+ // to hold a key and no closing fence — it is a thematic break, and goldmark
+ // is left to say so.
+ if !strings.HasPrefix(s, "---\n") {
return src, nil, 0
}
rest := s[4:]
@@ 148,6 153,10 @@ type segmenter struct {
blocks []Block
// headingStack holds (level, text) of the currently open headings.
headingStack []headingEntry
+ // ruleFrom is the byte offset just past the last thematic break located —
+ // see thematicBreakLine, which has no other way to tell two adjacent rules
+ // apart.
+ ruleFrom int
}
type headingEntry struct {
@@ 252,7 261,7 @@ func (s *segmenter) node(n ast.Node, c ctx) {
})
case *ast.ThematicBreak:
- line := s.lineOf(nodeStart(n))
+ line := s.thematicBreakLine(n)
s.emit(Block{
Kind: KindThematicBreak,
QuoteDepth: c.quoteDepth,
@@ 376,6 385,87 @@ func (s *segmenter) segLines(segs *text.Segments) (int, int) {
return s.lineOf(segs.At(0).Start), s.lineOf(segs.At(segs.Len()-1).Stop - 1)
}
+// thematicBreakLine finds the source line a rule sits on.
+//
+// A thematic break is the one block with nothing in it: goldmark records no
+// text segment and no line segment for it, because there is no text to record.
+// nodeStart therefore answered 0 for every rule in a document, and every rule
+// reported line 1 — harmless while the renderers printed no line numbers, and a
+// lie the moment one of them did. Two rules in a document then also shared a
+// number, which is the one thing a line-numbered view must never do.
+//
+// So the position is recovered from the source instead, bounded by the two
+// neighbours that do carry offsets: the search starts after whatever the
+// previous sibling covered and stops where the next one begins, and takes the
+// first line in that window that is a rule. ruleFrom carries the floor forward
+// across a run of consecutive rules, which have no offsets of their own to tell
+// them apart.
+//
+// A rule the scan cannot place reports line 0, not line 1. A renderer shows an
+// empty gutter cell for 0; showing 1 would invite a comment onto whatever is at
+// the top of the document.
+func (s *segmenter) thematicBreakLine(n ast.Node) int {
+ from := s.ruleFrom
+ if prev := n.PreviousSibling(); prev != nil {
+ if _, stop := nodeSpan(prev); stop > from {
+ from = stop
+ }
+ }
+ to := len(s.src)
+ if next := n.NextSibling(); next != nil {
+ if start, _ := nodeSpan(next); start >= 0 && start < to {
+ to = start
+ }
+ }
+
+ for i := sort.SearchInts(s.lineStarts, from+1) - 1; i >= 0 && i < len(s.lineStarts); i++ {
+ start := s.lineStarts[i]
+ if start > to {
+ break
+ }
+ stop := len(s.src)
+ if i+1 < len(s.lineStarts) {
+ stop = s.lineStarts[i+1] - 1
+ }
+ if !isThematicBreakLine(string(s.src[start:stop])) {
+ continue
+ }
+ // Past the newline, not at it: an offset inside a line resolves back to
+ // that same line, and the next rule would find this one again.
+ s.ruleFrom = stop + 1
+ return i + 1 + s.lineOffset
+ }
+ return 0
+}
+
+// isThematicBreakLine recognises the line goldmark has already decided is a
+// rule: three or more of -, _ or * with only spaces between them, under any
+// number of blockquote markers.
+func isThematicBreakLine(line string) bool {
+ t := strings.TrimSpace(line)
+ for strings.HasPrefix(t, ">") {
+ t = strings.TrimSpace(t[1:])
+ }
+ if t == "" {
+ return false
+ }
+ c := t[0]
+ if c != '-' && c != '_' && c != '*' {
+ return false
+ }
+ n := 0
+ for i := 0; i < len(t); i++ {
+ switch t[i] {
+ case c:
+ n++
+ case ' ', '\t':
+ default:
+ return false
+ }
+ }
+ return n >= 3
+}
+
// sourceLineRange expands a byte span to whole source lines, which is how a
// table row (whose AST node carries only inline segments) recovers the pipe
// syntax the reviewer actually wrote.
M prosediff/segment_test.go => prosediff/segment_test.go +63 -0
@@ 148,3 148,66 @@ func TestKindProse(t *testing.T) {
assert.False(t, k.Prose(), "%s should diff as code", k)
}
}
+
+// A thematic break carries no text and therefore no text segment, so its
+// position has to be recovered from the source rather than read off the AST.
+// Every rule used to report line 1 — including two rules in one document, which
+// is the one thing a line-numbered renderer must never be told.
+func TestThematicBreakReportsItsOwnLine(t *testing.T) {
+ src := "# One\n\npara\n\n---\n\nmore\n\n---\n\n> quoted\n>\n> ---\n\ntail\n"
+ var rules []Block
+ for _, b := range Segment([]byte(src)) {
+ if b.Kind == KindThematicBreak {
+ rules = append(rules, b)
+ }
+ }
+ require.Len(t, rules, 3)
+ assert.Equal(t, []int{5, 9, 13}, []int{rules[0].StartLine, rules[1].StartLine, rules[2].StartLine})
+ for _, r := range rules {
+ assert.Equal(t, r.StartLine, r.EndLine, "a rule occupies one line")
+ }
+}
+
+// Consecutive rules have nothing but their order to tell them apart, and are
+// the case a naive "scan forward for the next rule" gets wrong by reporting the
+// first one twice.
+func TestAdjacentThematicBreaksDoNotShareALine(t *testing.T) {
+ var lines []int
+ for _, b := range Segment([]byte("a\n\n---\n\n---\n\n---\n\nb\n")) {
+ if b.Kind == KindThematicBreak {
+ lines = append(lines, b.StartLine)
+ }
+ }
+ assert.Equal(t, []int{3, 5, 7}, lines)
+}
+
+// Line numbers are metadata, not identity: a block's hash covers its structure
+// and its content and nothing about where it sits. Moving a rule down the
+// document must therefore leave every anchor pointing where it did, which is
+// what makes fixing the line number above safe for stored comments.
+func TestThematicBreakHashIgnoresItsPosition(t *testing.T) {
+ near := Segment([]byte("a\n\n---\n"))
+ far := Segment([]byte("a\n\nb\n\nc\n\n---\n"))
+ require.Equal(t, KindThematicBreak, near[len(near)-1].Kind)
+ require.Equal(t, KindThematicBreak, far[len(far)-1].Kind)
+ assert.Equal(t, near[len(near)-1].Hash, far[len(far)-1].Hash)
+ assert.NotEqual(t, near[len(near)-1].StartLine, far[len(far)-1].StartLine)
+}
+
+// A document that is nothing but "---" was admitted as frontmatter and then
+// sliced at [4:] on three bytes. The review page renders whatever is on a
+// branch, so this was a panic reachable from any one-line file.
+func TestBareRuleDocumentIsNotFrontmatter(t *testing.T) {
+ got := Segment([]byte("---"))
+ require.Len(t, got, 1)
+ assert.Equal(t, KindThematicBreak, got[0].Kind)
+ assert.Equal(t, 1, got[0].StartLine)
+
+ // The neighbouring shapes that must keep working.
+ assert.NotPanics(t, func() { Segment([]byte("---\n")) })
+ assert.NotPanics(t, func() { Segment([]byte("--")) })
+ assert.NotPanics(t, func() { Segment([]byte("----")) })
+ fm := Segment([]byte("---\nid: X\n---\n\nbody\n"))
+ require.Len(t, fm, 2)
+ assert.Equal(t, KindFrontmatter, fm[0].Kind)
+}
M scss/main.scss => scss/main.scss +400 -110
@@ 112,120 112,346 @@
// ---- Prose diff -----------------------------------------------------------
//
-// The renderer (web/diff.go) emits this markup: a hunk per heading path, a
-// block per change, and inline <del>/<ins> spans or a two-column view depending
-// on how badly the block was rewritten. Colours follow Bootstrap's success and
-// danger so added and removed read the same here as everywhere else.
+// A unified, line-numbered diff table (web/diffrows.go emits it): four columns
+// — old number, new number, sign, text — one row per source line, plus a
+// full-width `ph-notes` row per block carrying that block's review threads.
//
-// Every block of the proposed document is on the page, because every block can
-// be commented on — so the visual weight is what keeps this a diff rather than a
-// document dump. A changed block is bordered, tinted and full size; an unchanged
-// one is a collapsed, dimmed, borderless line. That contrast is load-bearing:
-// remove it and a reviewer can no longer see at a glance what the agent did.
+// This replaced a block-card renderer, and the reasons it was rejected are the
+// reasons these rules look the way they do. The cards labelled every block
+// ("ADDED PARAGRAPH", "MODIFIED PARAGRAPH") and boxed it; on a new document the
+// whole page is one change, so every card carried identical chrome and the
+// labels outweighed the prose they described. Two columns of digits say the
+// same thing and then get out of the way, which is the whole point of a gutter.
+//
+// The markup is a TABLE on purpose. Prose lines wrap — often to three or four
+// visual lines — and a line's number has to stay on the FIRST of them. Only a
+// table cell does that for free (`vertical-align: top`); a flex row centres the
+// number and a grid row stretches it.
.prosediff {
+ // ONE row metric for the whole diff, in absolute units. The gutter is small
+ // tabular monospace and the prose is larger body text; if each side kept its
+ // own unitless line-height, the two would build different row boxes and the
+ // numerals would drift off the text baseline the moment the sizes diverged.
+ // Everything that participates in a row reads this variable and nothing sets
+ // its own line-height.
+ --row-lh: 1.4rem;
+
+ // Settled in the bead's review: 34px number tracks. Four digits of tabular
+ // figures fit; anything wider and the gutter stops being a margin.
+ --num-w: 34px;
+ --sign-w: 1.25rem;
+
+ // The rail ground and its single hairline are named once here, so light and
+ // dark differ in one place rather than in a dozen nested media queries.
+ --rail-bg: #{$gray-100};
+ --rail-line: #{$gray-300};
+ --num-fg: #{$gray-600};
+
+ // Change tints have to be re-mixed for dark: an alpha that reads as a wash
+ // over white is invisible over near-black.
+ --ins-tint: #{rgba($success, 0.1)};
+ --del-tint: #{rgba($danger, 0.1)};
+ --move-tint: #{rgba($info, 0.08)};
+ --sel-tint: #{rgba($info, 0.26)};
+
font-size: 0.95rem;
- .ph-hunk {
- margin-bottom: 1rem;
+ @media (prefers-color-scheme: dark) {
+ --rail-bg: #{$gray-800};
+ --rail-line: #{$gray-700};
+ --num-fg: #{$gray-500};
+ --ins-tint: #{rgba($success, 0.22)};
+ --del-tint: #{rgba($danger, 0.22)};
+ --move-tint: #{rgba($info, 0.18)};
+ --sel-tint: #{rgba($info, 0.3)};
+ }
+
+ .ph-diff {
+ width: 100%;
+ // Fixed layout pins the three narrow columns to the widths declared below
+ // instead of letting the longest line in the document negotiate them, and
+ // it is what makes the rail the same width in every table on the page.
+ table-layout: fixed;
+ border-collapse: collapse;
+ margin: 0;
}
- .ph-path {
+ // Cells never get their own vertical rhythm: they inherit the row metric and
+ // hang from the top, which is what keeps a number level with the first visual
+ // line of the wrapped line beside it. `height` on a table cell behaves as a
+ // minimum, so a blank source line still occupies a full row instead of
+ // collapsing to nothing.
+ .ph-row > td {
+ padding: 0;
+ line-height: var(--row-lh);
+ height: var(--row-lh);
+ vertical-align: top;
+ }
+
+ // ---- the rail ----
+ //
+ // The review's exact complaint about the first attempt was that a border on
+ // each cell drew a CAGE. So: one ground colour behind BOTH number tracks
+ // (they are adjacent collapsed cells of the same colour, so there is no seam
+ // to see) and exactly one hairline, on the right edge of the new-number
+ // track, dividing the rail from the content. No horizontal rules at all — the
+ // line height is what separates the rows.
+ .ph-n {
+ width: var(--num-w);
+ padding: 0 0.35rem 0 0.15rem;
+ background-color: var(--rail-bg);
+ color: var(--num-fg);
font-family: $font-family-monospace;
- font-size: 0.8rem;
- color: $gray-600;
- padding: 0.15rem 0;
+ font-size: 0.78rem;
+ // Digits must not change width between rows, or the right-aligned column
+ // jitters as the numbers grow.
+ font-variant-numeric: tabular-nums;
+ text-align: right;
+ // The numbers are chrome, not content: leaving them selectable means every
+ // copied diff arrives with the gutter interleaved into the prose. The sign
+ // and the text stay selectable, so a copied selection is still a usable
+ // diff.
+ -webkit-user-select: none;
+ user-select: none;
+ cursor: default;
+ }
+
+ .ph-n-new {
+ border-right: 1px solid var(--rail-line);
+ }
+
+ .ph-sign {
+ width: var(--sign-w);
+ padding: 0 0.25rem;
+ font-family: $font-family-monospace;
+ font-size: 0.78rem;
+ text-align: center;
+ }
- @media (prefers-color-scheme: dark) {
- color: $gray-400;
+ .ph-text {
+ padding: 0 0.6rem;
+ // Fixed table layout has no escape hatch for an unbreakable token, so a
+ // pasted URL would otherwise push the text column past the page.
+ overflow-wrap: break-word;
+ }
+
+ // Code fences, frontmatter and raw HTML blocks: significant whitespace, and
+ // read line by line rather than as a measure of prose.
+ .ph-mono {
+ font-family: $font-family-monospace;
+ font-size: 0.85em;
+ white-space: pre-wrap;
+ }
+
+ // ---- change tints ----
+ //
+ // The tint starts at the SIGN column and never touches the gutter, so the
+ // rail reads as the frame of the diff rather than as part of the change. This
+ // is also why the tint is set on the two content cells and not on the row.
+ .ph-r-ins {
+ > .ph-sign,
+ > .ph-text {
+ background-color: var(--ins-tint);
+ }
+
+ > .ph-sign {
+ color: $success;
+ }
+ }
+
+ .ph-r-del {
+ > .ph-sign,
+ > .ph-text {
+ background-color: var(--del-tint);
+ }
+
+ > .ph-sign {
+ color: $danger;
}
}
- .ph-block {
- padding: 0.35rem 0.6rem;
- margin: 0.2rem 0;
- border-left: 3px solid transparent;
- border-radius: 2px;
+ // A move is not an edit: the text is unchanged, only its position moved, so
+ // it gets the informational colour and no red or green.
+ .ph-r-move {
+ > .ph-sign,
+ > .ph-text {
+ background-color: var(--move-tint);
+ }
+
+ > .ph-sign {
+ color: $info;
+ }
+
+ > .ph-text {
+ color: $gray-700;
+
+ @media (prefers-color-scheme: dark) {
+ color: $gray-400;
+ }
+ }
}
- .ph-label {
- display: block;
- font-size: 0.72rem;
- text-transform: uppercase;
- letter-spacing: 0.03em;
+ // A marker row is the renderer speaking: "paragraph moved here (was line 3)".
+ // It sits in the text column, in the same face as the prose it introduces, and
+ // a reviewer skimming a moved block would otherwise read it as the block's
+ // first sentence. Italic and quiet is enough to place it outside the document
+ // without giving it the weight of a heading.
+ .ph-marker > .ph-text {
+ font-style: italic;
+ font-size: 0.85em;
color: $gray-600;
- margin-bottom: 0.15rem;
@media (prefers-color-scheme: dark) {
color: $gray-400;
}
}
- .ph-insert {
- border-left-color: $success;
- background: rgba($success, 0.08);
+ // The region fallback (a modified block the word mapper could not attribute
+ // per line) states a line RANGE in the gutter — "12–15" — because a single
+ // number there would be a guess. Five monospace glyphs do not fit 34px, and
+ // the track cannot grow, so the range drops to the proportional face, which
+ // is narrow enough for a two-plus-two-digit range on one line. A four-digit
+ // document will still overflow that, so wrapping stays allowed: the range
+ // then breaks after its dash and its two ends land on the row's first two
+ // lines. Either way the whole range is in the cell's title.
+ .ph-region .ph-n {
+ font-family: inherit;
+ font-size: 0.62rem;
+ padding-left: 0;
+ padding-right: 0.1rem;
+ white-space: normal;
+ overflow-wrap: anywhere;
+ }
+
+ // ---- sticky section readout ----
+ //
+ // There is no per-hunk breadcrumb any more: it restated a heading that was
+ // visible a few rows above, and on a new file it restated it on every hunk.
+ // Instead the heading row itself pins, so the section is only ever named when
+ // its heading has scrolled off. The background must be opaque or the rows
+ // scroll through it, and it is the rail colour so the band reads as one strip
+ // rather than as a heading with a differently-coloured gutter.
+ //
+ // The ROW is sticky as well as its cells, and it carries a background of its
+ // own, because sticky cells alone do not survive a second heading. Every
+ // heading pins at top: 0 and none of them ever unpins — the containing block
+ // of a table part is the whole table, not the section — so from the second
+ // heading onwards two of them share the same 22px band, and the newest one
+ // has to cover the rest. Measured in Chrome 141: with sticky cells only, the
+ // earlier heading's TEXT paints above the later heading's cell background and
+ // the two section names render on top of each other ("## Storalge"); a sticky
+ // row without a background does the same; a sticky row WITH a background is
+ // the one combination that hides the older headings completely. The cell rule
+ // stays underneath it because Safari sticks table cells but not table rows.
+ tr.ph-head {
+ position: sticky;
+ top: 0;
+ z-index: 2;
+ background-color: var(--rail-bg);
+ }
+
+ .ph-head > td {
+ position: sticky;
+ top: 0;
+ z-index: 2;
+ background-color: var(--rail-bg);
+ font-weight: 700;
}
- .ph-delete {
- border-left-color: $danger;
- background: rgba($danger, 0.08);
+ // ...but a sticky heading that is also an insert has to keep its tint, or an
+ // added section stops looking added the moment it pins.
+ .ph-head.ph-r-ins > .ph-sign,
+ .ph-head.ph-r-ins > .ph-text {
+ background-image: linear-gradient(var(--ins-tint), var(--ins-tint));
}
- .ph-modify {
- border-left-color: $gray-400;
- background: rgba($gray-500, 0.06);
+ .ph-head.ph-r-del > .ph-sign,
+ .ph-head.ph-r-del > .ph-text {
+ background-image: linear-gradient(var(--del-tint), var(--del-tint));
}
- .ph-move {
- border-left-color: $info;
- color: $gray-600;
- font-style: italic;
+ // ---- selection ----
+ //
+ // Set by diff.js when the reviewer drags across the gutter. Unlike a change
+ // tint this one DOES cover the number cells: the selection is about the lines
+ // themselves, so the rail is part of what is selected. `background-image`
+ // rather than `background-color` so it paints over both the rail ground and
+ // the change tint without having to out-specify either.
+ .ph-sel > td {
+ background-image: linear-gradient(var(--sel-tint), var(--sel-tint));
}
- // Unchanged text: present, addressable, and deliberately quiet. It is a
- // <details> collapsed to one line, so a long untouched section costs a few
- // dim rows rather than screens of prose between the changes.
- .ph-context {
- padding: 0;
- margin: 0;
- border-left-color: transparent;
- color: $gray-600;
+ // diff.js gives the table a single tab stop and walks the rows with the
+ // arrow keys, so the focused row has to be visible as a row — inset so the
+ // ring does not overlap the neighbouring line's tint.
+ .ph-row:focus-visible {
+ outline: 2px solid $info;
+ outline-offset: -2px;
+ }
- @media (prefers-color-scheme: dark) {
- color: $gray-500;
- }
+ // ---- folding ----
+ //
+ // A long run of untouched context collapses behind a checkbox, not behind a
+ // script: folding is the one interaction a reviewer needs before deciding
+ // whether the page is worth trusting, so it works with JavaScript off.
+ .ph-folded {
+ display: none;
+ }
- > .ph-context-label {
- padding: 0.1rem 0.6rem;
- font-size: 0.85rem;
- cursor: pointer;
- list-style-position: outside;
- white-space: nowrap;
- overflow: hidden;
- text-overflow: ellipsis;
- }
+ .ph-fold:has(.ph-fold-cb:checked) .ph-folded {
+ display: table-row;
+ }
- // Expanded, it indents under its own summary and stays uncoloured: it is
- // still context, even while being read.
- > .ph-body,
- > .ph-code,
- > .ph-threads,
- > .ph-form {
- margin-left: 1.2rem;
- }
+ .ph-fold-head > td {
+ padding: 0.1rem 0.6rem;
+ background-color: var(--rail-bg);
+ border-top: 1px solid var(--rail-line);
+ border-bottom: 1px solid var(--rail-line);
+ font-size: 0.8rem;
+ color: var(--num-fg);
+ }
- &[open] > .ph-context-label {
- color: $gray-700;
+ // Hidden from sight, NOT from the tab order: `display: none` on the checkbox
+ // would take the only control of the fold away from the keyboard. This is the
+ // clip-rect pattern, which leaves the input focusable, and the focus ring is
+ // drawn on the label so there is something visible to focus.
+ .ph-fold-cb {
+ position: absolute;
+ width: 1px;
+ height: 1px;
+ padding: 0;
+ margin: -1px;
+ overflow: hidden;
+ clip: rect(0, 0, 0, 0);
+ border: 0;
+ }
- @media (prefers-color-scheme: dark) {
- color: $gray-400;
- }
- }
+ .ph-fold-cb + label {
+ display: inline-block;
+ margin: 0;
+ padding: 0 0.2rem;
+ cursor: pointer;
+ text-decoration: underline dotted;
}
- // Inline marks: a deletion is struck through in danger, an insertion is
- // underlined in success. Both keep a faint background so a one-word change is
- // visible without reading the colour.
+ .ph-fold-cb:focus-visible + label {
+ outline: 2px solid $info;
+ outline-offset: 1px;
+ }
+
+ .ph-fold-cb:checked + label {
+ text-decoration: none;
+ opacity: 0.7;
+ }
+
+ // ---- word-level marks ----
+ //
+ // A deletion is struck through in danger; an insertion keeps the browser's
+ // underline off and is coloured in success instead, because a line of
+ // underlined prose is harder to read than the same line in green. Both carry
+ // a faint background so a one-word change inside a long line is visible
+ // without reading the colour.
del {
text-decoration: line-through;
color: $danger;
@@ 243,46 469,110 @@
}
}
- // The two-column fallback for shredded blocks: old on the left, new on the
- // right, stacking on a narrow screen so it never overflows the page.
- .ph-cols {
- display: flex;
- flex-wrap: wrap;
- gap: 0.75rem;
- }
-
- .ph-col {
- flex: 1 1 20rem;
- min-width: 0;
- padding: 0.4rem 0.6rem;
- border-radius: 2px;
+ // ---- the block's review threads ----
+ //
+ // A full-width cell under the block it belongs to. It is indented to the text
+ // column so a thread lines up with the prose it criticises, and it carries no
+ // rail: the rail numbers lines, and this row is not a line.
+ .ph-notes > td {
+ padding: 0.25rem 0.6rem 0.4rem
+ calc(var(--num-w) * 2 + var(--sign-w) + 0.6rem);
}
- .ph-old {
- background: rgba($danger, 0.06);
+ // On a phone that indent is a quarter of the screen, and a review comment is
+ // the one thing on this page that has to stay readable at any width. Below
+ // Bootstrap's sm breakpoint the alignment gives way to the text.
+ @media (max-width: 575.98px) {
+ .ph-notes > td {
+ padding-left: 0.6rem;
+ }
}
- .ph-new {
- background: rgba($success, 0.06);
+ // ---- the composer, once JavaScript is in ----
+ //
+ // The server renders a shut `<details class="ph-compose">Comment on this
+ // block</details>` under EVERY block, and it has to: with diff.js absent that
+ // control is the only way to comment, so removing it from the template would
+ // take commenting away from anyone without JavaScript. Drawn as-is, though, a
+ // seventeen-block document shows seventeen identical "Comment on this block"
+ // summaries, each on its own full-width row with an empty gutter beside it.
+ // That is the same identical-chrome-under-every-block that got the block-card
+ // renderer rejected, and it chops the continuous run of numbered lines this
+ // table exists to be into seventeen pieces.
+ //
+ // So the composer is hidden only when there is another way to reach it.
+ // diff.js marks each table it has successfully wired `.ph-js`, and under that
+ // class a notes row is drawn only if it has something to show: a thread
+ // (someone's critique is content, not chrome), an OPEN composer — which is
+ // how selecting lines brings it back, since diff.js sets `open` on the
+ // composer of the block whose lines were picked, by mouse or by keyboard —
+ // or a composer marked `.ph-draft`. Selecting the lines is the point of the
+ // port; diff.js also writes the one `.ph-hint` line below that says the
+ // gutter is draggable.
+ //
+ // `.ph-draft` is diff.js saying "there are words in this textarea that have
+ // not been posted", and it is what stops this rule from eating a review
+ // someone is in the middle of writing. Without it, collapsing the composer
+ // from its own summary hides the row, and the disclosure control the reviewer
+ // would click to get their text back goes with it. An empty composer is
+ // chrome and this rule may hide it; one holding a draft is content, exactly
+ // as a posted thread is, and stays drawn until it is posted or emptied.
+ //
+ // DO NOT "tidy this up" by deleting the composer from threads.html, and do
+ // not drop the `.ph-js` qualifier: with JavaScript off no table is marked,
+ // this rule matches nothing, and every block keeps its visible composer,
+ // which is the only reason hiding it here is honest.
+ .ph-diff.ph-js tr.ph-notes:not(:has(.ph-threads)):not(:has(.ph-compose[open])):not(:has(.ph-compose.ph-draft)) {
+ display: none;
+ }
+
+ // ...and the same draft outranks the fold. A notes row inside a collapsed run
+ // carries `.ph-folded` like the lines around it, so closing the fold again
+ // hides it — including when the reviewer opened its composer from inside the
+ // fold and typed into it. Measured in Chrome 150 before this rule: the
+ // composer stayed `open` with the text intact, its row computed `display:
+ // none` and the textarea measured 0px tall, so a half-written critique was
+ // still in the document with nothing on the page to say so.
+ //
+ // The rejected fix was to refuse the fold's second click while a composer was
+ // open inside it. That makes a control that does nothing when clicked, and it
+ // needs script to enforce, which would take folding away from the no-JS path
+ // it was built for. Letting the fold close and keeping the one row that holds
+ // typed text is the honest half: the lines go, the words stay, and the range
+ // diff.js prefixed onto the note ("Lines 16–17 · Anchors to …") is what says
+ // which folded lines they were about.
+ .ph-diff.ph-js tr.ph-notes.ph-folded:has(.ph-compose.ph-draft) {
+ display: table-row;
+ }
+
+ // The hint diff.js writes above the table, standing in for those seventeen
+ // summaries. It has to weigh less than any one of them, so it takes the
+ // gutter's size and colour: one quiet line, above the diff and outside it,
+ // read once and then ignored.
+ .ph-hint {
+ margin: 0 0 0.25rem;
+ font-size: 0.8rem;
+ color: var(--num-fg);
}
+}
- // Code / frontmatter line diffs keep their wrapping and are read line by line.
- .ph-code {
- margin: 0;
- padding: 0.25rem 0;
- background: transparent;
- white-space: pre-wrap;
- word-break: break-word;
+// The composer and each thread state which block they attach to, because
+// selection is by line while anchoring is by block — the indirection is real,
+// so it is written down rather than left for the reviewer to infer. Server
+// rendered, so it is honest with JavaScript off; diff.js only prefixes the
+// selected line range onto it. Outside .prosediff because a thread whose anchor
+// was lost is rendered on the page itself, away from any diff table.
+.ph-anchor-note {
+ margin: 0 0 0.3rem;
+ font-size: 0.8rem;
+ color: $gray-600;
- .ph-line-del {
- display: block;
- background: rgba($danger, 0.1);
- }
+ code {
+ font-size: inherit;
+ }
- .ph-line-ins {
- display: block;
- background: rgba($success, 0.1);
- }
+ @media (prefers-color-scheme: dark) {
+ color: $gray-400;
}
}
M => +25 -0
@@ 55,6 55,13 @@ type threadPanel struct {
// leaves it empty — the document heading it renders under already says it.
DocPath string
// AnchorPath and AnchorIndex name the block this thread is drawn against,
// display-only. They are set for a thread the diff placed and left empty for
// one it could not: a lost thread's anchor is precisely what did not resolve,
// and printing the path it used to name would read as a place it still sits.
AnchorPath string
AnchorIndex int
CanReply bool
CanResolve bool
ActionBase string
@@ 82,6 89,24 @@ type composeForm struct {
Ordinal int
Side core.CommentSide
Hash string
// AnchorPath and AnchorIndex are shown, not submitted: the reviewer selects
// lines and the form stores a block, and this is where that indirection is
// stated instead of being magic. AnchorIndex is the anchor's own index —
// 0-based, within the heading path — because a number invented for display
// would not be the number the comment stores.
AnchorPath string
AnchorIndex int
}
// anchorPathLabel names a block's section for a reader: its enclosing headings,
// outermost first, or the phrase for a block that sits above the document's
// first heading. Empty would leave the composer saying "Anchors to" and nothing.
func anchorPathLabel(path []string) string {
if s := strings.Join(path, " › "); s != "" {
return s
}
return "(document preamble)"
}
// blockComments is what the "blockthreads" template renders for one block: the
M web/diff.go => web/diff.go +201 -256
@@ 16,7 16,7 @@ import (
// inlineSimilarityThreshold is the Phase 0 verdict's presentation switch: a
// modified prose block whose token similarity is at or above it renders as an
-// inline word diff, and one below it renders as a two-column old/new view.
+// inline word diff, and one below it renders as a paired old/new region.
//
// 13% of real prose modifications shred into interleaved fragments — those
// paragraphs really were rewritten sentence by sentence — and every one of them
@@ 26,11 26,6 @@ import (
// itself; this is where the decision is made.
const inlineSimilarityThreshold = 0.75
-// contextPreviewRunes bounds the one-line preview a collapsed unchanged block
-// shows: long enough to recognise the paragraph, short enough that a screen of
-// them still reads as a list of blocks rather than as the document itself.
-const contextPreviewRunes = 90
-
// diffView is the whole rendered diff of one document, ready for the proposal
// template. Unchanged reports the degenerate case — a proposal that touches a
// document without changing it — so the page can say so rather than show an
@@ 73,15 68,20 @@ type blockKey struct {
}
// renderDocDiff diffs the approved (old) and proposed (new) source of one
-// document and renders it to HTML, with every block's review threads and, for
-// the owner, the form that opens a new one.
+// document and renders it as a line-numbered unified diff, with every block's
+// review threads and, for the owner, the form that opens a new one.
+//
+// The page is a table because the gutter has to be a gutter: two number tracks
+// that stay aligned with the first visual line of a prose line that wraps three
+// times. Selection is by line and anchoring is by block, so every row carries
+// the anchor of the block it belongs to and the block's first row carries the id
+// a comment link scrolls to.
//
-// It walks prosediff's block-change model rather than its text renderer: the
-// text renderer is for a terminal, and the review page needs headings grouped,
-// word edits marked with <ins>/<del>, and — the load-bearing part — the
-// two-column fallback for shredded blocks. Every piece of document content is
-// HTML-escaped before it reaches the output; the only markup this produces is
-// its own structure and the thread markup html/template escapes for it.
+// The arithmetic — which number belongs in which track, and which lines a fold
+// may hide — is diffrows.go's, deliberately kept out of here. What is left is
+// escaping and concatenation: every piece of document content passes through
+// template.HTMLEscapeString, and the only markup this produces is its own
+// structure plus the thread markup html/template escapes for it.
func renderDocDiff(in docDiff) diffView {
d := prosediff.Compare(in.Base, in.Proposed)
view := diffView{Stats: d.Stats, Unchanged: !d.Stats.Changed()}
@@ 96,61 96,42 @@ func renderDocDiff(in docDiff) diffView {
}
var b strings.Builder
- b.WriteString(`<div class="prosediff">`)
- lastPath := "\x00" // impossible path, so the first real one always prints
- inHunk := false
- closeHunk := func() {
- if inHunk {
- b.WriteString(`</div></section>`)
- inHunk = false
- }
+ b.WriteString(`<div class="prosediff"><table class="ph-diff">`)
+ for _, g := range groupRows(buildRows(d.Changes, r.blockInfo)) {
+ r.writeGroup(&b, g)
}
+ b.WriteString(`</table></div>`)
- for _, c := range d.Changes {
- blk := c.New
- if blk == nil {
- blk = c.Old
- }
- if path := strings.Join(blk.HeadingPath, " › "); path != lastPath {
- closeHunk()
- lastPath = path
- shown := path
- if shown == "" {
- shown = "(document preamble)"
- }
- b.WriteString(`<section class="ph-hunk"><div class="ph-path">`)
- b.WriteString(template.HTMLEscapeString(shown))
- b.WriteString(`</div><div class="ph-blocks">`)
- inHunk = true
- }
- r.writeBlock(&b, c)
- }
- closeHunk()
- b.WriteString(`</div>`)
view.HTML = template.HTML(b.String())
view.Unplaced = r.unplaced()
return view
}
-// docRenderer renders the blocks of one document's diff. It holds the anchor
+// docRenderer renders the rows of one document's diff. It holds the anchor
// numbering of both revisions and the threads still waiting for a block: a
-// block claims its threads as it is written, and whatever is left over at the
-// end never had a block on the page.
+// block claims its threads as its notes row is written, and whatever is left
+// over at the end never had a block on the page.
type docRenderer struct {
in docDiff
// anchors is each side's blocks, numbered within their heading path.
anchors map[core.CommentSide][]core.AnchorBlock
pending map[blockKey][]service.Thread
+ // foldPrefix scopes this document's fold checkbox ids. A proposal page
+ // renders several documents into one HTML document, and two folds sharing an
+ // id would toggle each other.
+ foldPrefix string
}
func newDocRenderer(in docDiff, d *prosediff.Diff) *docRenderer {
+ sum := sha256.Sum256([]byte(in.Path))
r := &docRenderer{
in: in,
anchors: map[core.CommentSide][]core.AnchorBlock{
core.SideNew: blockAnchors(d.NewBlocks),
core.SideOld: blockAnchors(d.OldBlocks),
},
- pending: make(map[blockKey][]service.Thread, len(in.Threads)),
+ pending: make(map[blockKey][]service.Thread, len(in.Threads)),
+ foldPrefix: hex.EncodeToString(sum[:])[:8],
}
// A thread is placed by (side, block ordinal) and by nothing else. The
// anchor resolution already decided which block it belongs to, and any
@@ 219,189 200,216 @@ func (r *docRenderer) target(c prosediff.BlockChange) (core.CommentAnchor, block
return anchor, blockKey{side, blk.Ordinal}, true
}
-// writeBlock renders one block of the diff and the comment UI hanging off it.
-// Insert/delete/move show the whole block; a modify chooses among a code line
-// diff, an inline word diff and the two-column view, on the rules the design
-// pins; an unchanged block renders as collapsed context.
-func (r *docRenderer) writeBlock(b *strings.Builder, c prosediff.BlockChange) {
- if c.Kind == prosediff.ChangeMoveOut {
- fmt.Fprintf(b, `<div class="ph-block ph-move"><span class="ph-label">%s moved away (now line %d)</span></div>`,
- template.HTMLEscapeString(c.Old.Label()), c.New.StartLine)
- return
+// blockInfo answers, for the row builder, the two questions about a change that
+// are not in the change: what it anchors to, and whether it needs a notes row.
+//
+// It counts this block's pending threads without claiming them — claiming
+// happens when the notes row is written, which is the one place that can
+// guarantee they were actually rendered. A block the anchor numbering does not
+// cover is still given rows, because losing document content would be worse
+// than losing its comment affordance; it just carries no id and offers no form.
+func (r *docRenderer) blockInfo(c prosediff.BlockChange) blockInfo {
+ anchor, key, ok := r.target(c)
+ if !ok {
+ return blockInfo{}
+ }
+ threads := len(r.pending[key]) > 0
+ return blockInfo{
+ Anchor: anchor,
+ Key: key,
+ Commentable: true,
+ HasThreads: threads,
+ Notes: threads || r.in.Controls.Owner,
}
+}
- anchor, key, commentable := r.target(c)
- var threads []service.Thread
- if commentable {
- threads = r.pending[key]
- delete(r.pending, key)
- }
+// writeGroup renders one <tbody>. A fold group opens with the checkbox and
+// label that reveal it: a real form control rather than a script-driven button,
+// so an unchanged run can be opened with JavaScript off.
+func (r *docRenderer) writeGroup(b *strings.Builder, g rowGroup) {
+ if !g.Fold {
+ b.WriteString(`<tbody>`)
+ } else {
+ id := fmt.Sprintf("fold-%s-%d", r.foldPrefix, g.Index)
+ b.WriteString(`<tbody class="ph-fold"><tr class="ph-fold-head"><td colspan="4">`)
+ fmt.Fprintf(b, `<input type="checkbox" class="ph-fold-cb" id="%s"><label for="%s">%d unchanged lines</label>`,
+ id, id, g.Hidden)
+ b.WriteString(`</td></tr>`)
+ }
+ for _, row := range g.Rows {
+ r.writeRow(b, row)
+ }
+ b.WriteString(`</tbody>`)
+}
- if c.Kind == prosediff.ChangeEqual {
- r.writeContext(b, c, anchor, key, threads, commentable)
+// writeRow renders one row of the table: two number cells, a sign, and the
+// line.
+func (r *docRenderer) writeRow(b *strings.Builder, row diffRow) {
+ if row.Kind == rowNotes {
+ r.writeNotesRow(b, row)
return
}
- class, label := changedPresentation(c)
- // A block the anchor numbering does not cover is still shown — losing
- // document content would be worse than losing its comment affordance — it
- // just carries no id and offers no form. Nothing in prosediff produces one
- // today; this is the branch that keeps that assumption from being fatal.
- fmt.Fprintf(b, `<div class="ph-block %s"%s><span class="ph-label">%s</span>`,
- class, idAttr(anchor, commentable), template.HTMLEscapeString(label))
- writeChangedBody(b, c)
- if commentable {
- r.writeComments(b, anchor, key, threads)
+ b.WriteString(`<tr class="ph-row ph-r-`)
+ b.WriteString(string(row.Kind))
+ if row.Start {
+ b.WriteString(" ph-blk-start")
}
- b.WriteString(`</div>`)
-}
-
-// idAttr renders the id attribute of a commentable block, and nothing at all
-// for one that carries no anchor: an empty id="" is a fragment that matches
-// every such block at once.
-func idAttr(a core.CommentAnchor, commentable bool) string {
- if !commentable {
- return ""
+ if row.Block.Heading {
+ b.WriteString(" ph-head")
}
- return ` id="` + blockDOMID(a) + `"`
-}
+ if row.Region {
+ b.WriteString(" ph-region")
+ }
+ if row.Folded {
+ b.WriteString(" ph-folded")
+ }
+ // A marker row is the renderer talking, not the document: "paragraph moved
+ // here (was line 3)" sits in the same column as the prose around it, and
+ // without a class of its own it reads as a sentence someone wrote.
+ if row.Note != "" {
+ b.WriteString(" ph-marker")
+ }
+ b.WriteString(`"`)
+ // The id goes on the block's first row and only there: an id repeated down a
+ // block would give one anchor several places to scroll to, and an empty id=""
+ // on an uncommentable row is a fragment that matches every such row at once.
+ if row.Start && row.Block.Commentable {
+ b.WriteString(` id="` + blockDOMID(row.Block.Anchor) + `"`)
+ }
+ writeBlockAttrs(b, row.Block)
+ b.WriteString(`>`)
-// writeContext renders an unchanged block.
-//
-// The rule used to be that the review shows only what changed, and it is now
-// that any block of a proposed document can be commented on — a reviewer's
-// objection is as often to the paragraph the agent left alone as to the one it
-// touched, and a block that is not on the page cannot be pointed at. So
-// unchanged text is rendered, but subordinate: collapsed behind a one-line
-// preview, so a screenful of context never competes with the marked-up blocks
-// and the page still reads at a glance as a diff. A block that already carries a
-// comment opens by default — it is no longer merely context once someone has
-// said something about it.
-func (r *docRenderer) writeContext(b *strings.Builder, c prosediff.BlockChange,
- anchor core.CommentAnchor, key blockKey, threads []service.Thread, commentable bool) {
-
- open := ""
- if len(threads) > 0 {
- open = " open"
- }
- fmt.Fprintf(b, `<details class="ph-block ph-context"%s%s><summary class="ph-context-label">%s</summary>`,
- idAttr(anchor, commentable), open, template.HTMLEscapeString(blockPreview(c.New)))
- writeBody(b, c.New.Text, c.New.Kind.Prose())
- if commentable {
- r.writeComments(b, anchor, key, threads)
- }
- b.WriteString(`</details>`)
+ writeNumCell(b, "ph-n-old", row.OldNum, row.OldEnd)
+ writeNumCell(b, "ph-n-new", row.NewNum, row.NewEnd)
+ b.WriteString(`<td class="ph-sign">` + signOf(row.Kind) + `</td>`)
+
+ b.WriteString(`<td class="ph-text`)
+ if row.Block.Mono {
+ b.WriteString(" ph-mono")
+ }
+ b.WriteString(`">`)
+ if row.Note != "" {
+ // Renderer-authored text, but escaped all the same: a block's label
+ // carries a code fence's info string, which the document wrote.
+ b.WriteString(template.HTMLEscapeString(row.Note))
+ } else {
+ writeInlineSpans(b, row.Spans)
+ }
+ b.WriteString(`</td></tr>`)
}
-// writeComments appends one block's threads and, for the owner, the form that
-// opens a new one.
+// writeNotesRow renders a block's threads and, for the owner, the form that
+// opens a new one, in a full-width row under the block's lines.
//
// It renders through html/template rather than by hand because everything here
// is prose someone else wrote; contextual auto-escaping is what keeps a comment
// body text. The template is executed into a buffer first, for the reason
// Server.render uses one: a template that fails halfway must not leave its
// half-written markup inside the diff.
-func (r *docRenderer) writeComments(b *strings.Builder, anchor core.CommentAnchor,
- key blockKey, threads []service.Thread) {
+//
+// This is also where a block claims its threads. The row model only emits a
+// notes row for a block that has something to put in it, so there is no case
+// here for an empty one.
+func (r *docRenderer) writeNotesRow(b *strings.Builder, row diffRow) {
+ blk := row.Block
+ threads := r.pending[blk.Key]
+ delete(r.pending, blk.Key)
+
+ // The anchor note states what a comment written here will attach to. The
+ // selection a reviewer makes is by line and the anchor stored is by block, so
+ // without this the indirection would be invisible — and it is server-rendered
+ // rather than filled in by script, because it has to be readable before the
+ // reviewer decides to type.
+ path, index := anchorPathLabel(blk.Anchor.HeadingPath), blk.Anchor.Index
data := blockComments{}
for _, t := range threads {
- data.Threads = append(data.Threads, threadPanelOf(t, r.in.Controls))
+ p := threadPanelOf(t, r.in.Controls)
+ p.AnchorPath, p.AnchorIndex = path, index
+ data.Threads = append(data.Threads, p)
}
if r.in.Controls.Owner {
data.Compose = &composeForm{
- ActionBase: r.in.Controls.ActionBase,
- DocPath: r.in.Path,
- Ordinal: key.ordinal,
- Side: key.side,
- Hash: anchor.BlockHash,
+ ActionBase: r.in.Controls.ActionBase,
+ DocPath: r.in.Path,
+ Ordinal: blk.Key.ordinal,
+ Side: blk.Key.side,
+ Hash: blk.Anchor.BlockHash,
+ AnchorPath: path,
+ AnchorIndex: index,
}
}
- if len(data.Threads) == 0 && data.Compose == nil {
- return
- }
var buf bytes.Buffer
if err := blockThreadsTmpl.Execute(&buf, data); err != nil {
log.Printf("web: rendering comments on %s: %v", r.in.Path, err)
return
}
- b.Write(buf.Bytes())
-}
-
-// changedPresentation is the class and the label of a changed block, decided
-// together: the two-column fallback changes both, and deciding it twice is how
-// a block ends up labelled "rewritten" while rendering inline.
-func changedPresentation(c prosediff.BlockChange) (class, label string) {
- switch c.Kind {
- case prosediff.ChangeInsert:
- return "ph-insert", "added " + c.New.Label()
- case prosediff.ChangeDelete:
- return "ph-delete", "removed " + c.Old.Label()
- case prosediff.ChangeMoveIn:
- return "ph-move", fmt.Sprintf("%s moved here (was line %d)", c.New.Label(), c.Old.StartLine)
- }
-
- label = "changed " + c.New.Label()
- if c.StructureOnly {
- label = fmt.Sprintf("changed %s → %s (structure)", c.Old.Label(), c.New.Label())
- }
- if c.Moved {
- label += fmt.Sprintf(" (moved from line %d)", c.Old.StartLine)
+ b.WriteString(`<tr class="ph-notes`)
+ // A notes row hides with the block it belongs to. Dropping the flag the row
+ // model already set left a collapsed run displaying the compose forms of the
+ // very blocks it had just hidden.
+ if row.Folded {
+ b.WriteString(" ph-folded")
}
- if twoColumn(c) {
- return "ph-modify ph-columns", label + " (rewritten)"
- }
- return "ph-modify", label
-}
-
-// twoColumn reports whether a modified block falls back to the old/new columns:
-// a prose block (no line script) rewritten past the point where inline marks
-// stay readable.
-func twoColumn(c prosediff.BlockChange) bool {
- return len(c.Lines) == 0 && c.Similarity < inlineSimilarityThreshold
+ b.WriteString(`"`)
+ writeBlockAttrs(b, blk)
+ b.WriteString(`><td colspan="4">`)
+ b.Write(buf.Bytes())
+ b.WriteString(`</td></tr>`)
}
-// writeChangedBody renders the body of a changed block. A code, frontmatter or
-// HTML block has a line-oriented edit script and renders line by line; a prose
-// block has a word edit script and renders inline when it stayed similar enough
-// to follow, and two-column when it did not. An inserted, deleted or moved block
-// has no edit script and shows its whole text.
+// writeBlockAttrs writes the one data attribute a row of a commentable block
+// carries: which block it belongs to.
//
-// A move-in used to render as a bare "moved here" marker. It shows its text now
-// because it is commentable, and a comment control on text the reviewer cannot
-// see is a control on nothing.
-func writeChangedBody(b *strings.Builder, c prosediff.BlockChange) {
- switch c.Kind {
- case prosediff.ChangeInsert, prosediff.ChangeMoveIn:
- writeBody(b, c.New.Text, c.New.Kind.Prose())
- return
- case prosediff.ChangeDelete:
- writeBody(b, c.Old.Text, c.Old.Kind.Prose())
+// It used to write the block's heading path and index alongside, so that a
+// script could read a row's section without walking back up the table. No
+// script reads them, and an attribute pair repeated on every row of every diff
+// for a reader that does not exist is the speculative chrome this port set out
+// to remove. The heading path a person sees is in the composer and the thread
+// header, where it is read.
+//
+// A row with no anchor gets no attribute at all rather than an empty one. The
+// selection script clamps a drag to rows sharing a data-anchor, and rows that
+// all carried data-anchor="" would look to it like one enormous block.
+func writeBlockAttrs(b *strings.Builder, blk *rowBlock) {
+ if !blk.Commentable {
return
}
+ b.WriteString(` data-anchor="` + blockDOMID(blk.Anchor) + `"`)
+}
- if len(c.Lines) > 0 {
- b.WriteString(`<pre class="ph-code">`)
- writeLineSpans(b, c.Lines)
- b.WriteString(`</pre>`)
- return
- }
- if !twoColumn(c) {
- b.WriteString(`<div class="ph-body ph-inline">`)
- writeInlineSpans(b, c.Words)
- b.WriteString(`</div>`)
- return
+// writeNumCell writes one of the two gutter tracks.
+//
+// A zero number is an empty cell: the row has no line number on this side, and
+// the design's rule is that a number the renderer had to guess is never shown.
+// A region row states the range it stands for instead, in the cell and in a
+// title so it is readable when the track is too narrow for it.
+func writeNumCell(b *strings.Builder, side string, n, end int) {
+ b.WriteString(`<td class="ph-n ` + side + `"`)
+ switch {
+ case n == 0:
+ b.WriteString(`></td>`)
+ case end > n:
+ fmt.Fprintf(b, ` title="lines %d–%d">%d–%d</td>`, n, end, n, end)
+ default:
+ fmt.Fprintf(b, `>%d</td>`, n)
}
+}
- // The two-column fallback: the block was rewritten enough that inline marks
- // would shred it. The old column keeps deletions, the new keeps insertions,
- // each still marked, so a reviewer reads two coherent paragraphs side by side.
- b.WriteString(`<div class="ph-cols"><div class="ph-col ph-old">`)
- writeColumnSpans(b, c.Words, true)
- b.WriteString(`</div><div class="ph-col ph-new">`)
- writeColumnSpans(b, c.Words, false)
- b.WriteString(`</div></div>`)
+// signOf is the one character the sign column shows. It is where the add/delete
+// tint starts, so the gutter never reads as part of the change.
+func signOf(kind rowKind) string {
+ switch kind {
+ case rowInsert:
+ return "+"
+ case rowDelete:
+ return "-"
+ case rowMove:
+ return "≡"
+ }
+ return ""
}
// blockAnchors numbers a revision's blocks the way the anchor model does:
@@ 448,20 456,6 @@ func blockDOMID(a core.CommentAnchor) string {
return "b-" + hex.EncodeToString(h.Sum(nil))[:16]
}
-// blockPreview is the one line a collapsed context block shows: its text with
-// whitespace collapsed and truncated, or its structural label when it has no
-// text to show (a thematic break, an empty block).
-func blockPreview(blk *prosediff.Block) string {
- text := strings.Join(strings.Fields(blk.Text), " ")
- if text == "" {
- return blk.Label()
- }
- if runes := []rune(text); len(runes) > contextPreviewRunes {
- return string(runes[:contextPreviewRunes]) + "…"
- }
- return text
-}
-
// sideOf defaults an empty side to the new one, the way service.CommentOn does
// when it stores a comment: an anchor read back without a side is a comment on
// the text under review.
@@ 472,22 466,9 @@ func sideOf(s core.CommentSide) core.CommentSide {
return core.SideNew
}
-// writeBody renders whole-block text: escaped, in a <pre> when it is not prose.
-func writeBody(b *strings.Builder, text string, prose bool) {
- if prose {
- b.WriteString(`<div class="ph-body">`)
- b.WriteString(template.HTMLEscapeString(text))
- b.WriteString(`</div>`)
- return
- }
- b.WriteString(`<pre class="ph-code">`)
- b.WriteString(template.HTMLEscapeString(text))
- b.WriteString(`</pre>`)
-}
-
-// writeInlineSpans renders a word edit script inline, marking deletions and
-// insertions where they sit. The Space flag reproduces prosediff's own spacing:
-// a span that replaces the one before it carries Space=false, so a one-word
+// writeInlineSpans renders a row's content, marking deletions and insertions
+// where they sit. The Space flag reproduces prosediff's own spacing: a span
+// that replaces the one before it carries Space=false, so a one-word
// substitution does not render with a gap in the middle.
func writeInlineSpans(b *strings.Builder, spans []prosediff.Span) {
emitted := false
@@ 500,27 481,6 @@ func writeInlineSpans(b *strings.Builder, spans []prosediff.Span) {
}
}
-// writeColumnSpans renders one side of the two-column view: the old side keeps
-// equal and deleted words, the new side keeps equal and inserted ones. Both
-// still mark their changes, so each column is a readable paragraph that also
-// shows what moved.
-func writeColumnSpans(b *strings.Builder, spans []prosediff.Span, old bool) {
- emitted := false
- for _, s := range spans {
- keep := s.Op == prosediff.OpEqual ||
- (old && s.Op == prosediff.OpDelete) ||
- (!old && s.Op == prosediff.OpInsert)
- if !keep {
- continue
- }
- if s.Space && emitted {
- b.WriteByte(' ')
- }
- emitted = true
- writeSpan(b, s)
- }
-}
-
// writeSpan writes one span's escaped text, wrapped in <del> or <ins> for a
// change and bare for an equal run.
func writeSpan(b *strings.Builder, s prosediff.Span) {
@@ 538,18 498,3 @@ func writeSpan(b *strings.Builder, s prosediff.Span) {
b.WriteString(esc)
}
}
-
-// writeLineSpans renders a line-oriented edit script (a code fence, frontmatter
-// or HTML block) one line per row inside a <pre>, each line class-marked.
-func writeLineSpans(b *strings.Builder, spans []prosediff.Span) {
- for _, s := range spans {
- class := "ph-line-eq"
- switch s.Op {
- case prosediff.OpDelete:
- class = "ph-line-del"
- case prosediff.OpInsert:
- class = "ph-line-ins"
- }
- fmt.Fprintf(b, `<span class="%s">%s</span>`+"\n", class, template.HTMLEscapeString(s.Text))
- }
-}
M web/diff_internal_test.go => web/diff_internal_test.go +143 -30
@@ 1,6 1,7 @@
package web
import (
+ "fmt"
"strings"
"testing"
@@ 8,6 9,11 @@ import (
"sourcecraft.dev/bigbes/sr-ht-spec/service"
)
+// These tests are about the markup: the classes and attributes the stylesheet
+// and the selection script are pinned to, and the escaping discipline. What a
+// row *says* — which number lands in which track — is diffrows_test.go's, on
+// the model rather than through a layer of angle brackets.
+
// render is renderDocDiff for a document nobody has commented on, which is what
// the presentation tests are about.
func render(oldSrc, newSrc []byte) diffView {
@@ 53,15 59,16 @@ func TestRenderDocDiffInlineWordChange(t *testing.T) {
if !strings.Contains(html, "<ins>red</ins>") {
t.Errorf("missing inline insertion of 'red'; got:\n%s", html)
}
- if strings.Contains(html, "ph-columns") {
- t.Errorf("a one-word edit rendered as two columns; got:\n%s", html)
+ if strings.Contains(html, "ph-region") {
+ t.Errorf("a one-word edit fell back to a line range; got:\n%s", html)
}
}
-// TestRenderDocDiffTwoColumnBelowThreshold proves a block rewritten enough to
-// fall below the similarity threshold renders as the two-column old/new view —
-// the Phase 0 verdict's hard requirement.
-func TestRenderDocDiffTwoColumnBelowThreshold(t *testing.T) {
+// TestRenderDocDiffRegionFallbackBelowThreshold proves a block rewritten enough
+// to fall below the similarity threshold renders as the paired old/new region
+// rows — the Phase 0 verdict's hard requirement, and the one place the gutter
+// states a range instead of a line number.
+func TestRenderDocDiffRegionFallbackBelowThreshold(t *testing.T) {
// A block rewritten to ~0.58 similarity: paired as a modify (above the 0.40
// pairing floor) but shredded enough to fall below the 0.75 inline switch.
old := []byte("# Title\n\nThe committee approved the annual budget after a long and " +
@@ 70,11 77,14 @@ func TestRenderDocDiffTwoColumnBelowThreshold(t *testing.T) {
"quiet discussion that ended early in the afternoon.\n")
v := render(old, nw)
html := string(v.HTML)
- if !strings.Contains(html, "ph-columns") {
- t.Fatalf("a wholesale rewrite did not render as two columns; got:\n%s", html)
+ if !strings.Contains(html, `ph-r-del ph-blk-start ph-region"`) ||
+ !strings.Contains(html, `ph-r-ins ph-region"`) {
+ t.Fatalf("a wholesale rewrite did not render as a region pair; got:\n%s", html)
}
- if !strings.Contains(html, "ph-col ph-old") || !strings.Contains(html, "ph-col ph-new") {
- t.Errorf("two-column view missing an old or new column; got:\n%s", html)
+ // The old side keeps its deletions and the new side its insertions, so each
+ // row is a paragraph a reviewer can read straight through.
+ if !strings.Contains(html, "<del>approved</del>") || !strings.Contains(html, "<ins>rejected</ins>") {
+ t.Errorf("the region rows lost their word marks; got:\n%s", html)
}
}
@@ 93,8 103,8 @@ func TestRenderDocDiffEscapesContent(t *testing.T) {
}
}
-// TestRenderDocDiffEscapesUnchangedContent proves the context blocks the review
-// now renders are escaped too — both the collapsed preview and the body.
+// TestRenderDocDiffEscapesUnchangedContent proves the context rows the review
+// now renders are escaped too, not only the changed ones.
func TestRenderDocDiffEscapesUnchangedContent(t *testing.T) {
const untouched = "A <b>bold</b> claim nobody edited.\n"
old := []byte("# Title\n\n" + untouched + "\nfirst.\n")
@@ 103,30 113,34 @@ func TestRenderDocDiffEscapesUnchangedContent(t *testing.T) {
if strings.Contains(html, "<b>bold</b>") {
t.Fatalf("unescaped markup from an unchanged block reached the output; got:\n%s", html)
}
- if want := strings.Count(html, "<b>bold</b>"); want != 2 {
- t.Errorf("escaped markup appears %d times, want 2 (the preview and the body); got:\n%s", want, html)
+ if want := strings.Count(html, "<b>bold</b>"); want != 1 {
+ t.Errorf("escaped markup appears %d times, want 1 (the context row); got:\n%s", want, html)
}
}
-// TestRenderDocDiffInsertAndDelete proves an added and a removed block are each
-// shown whole, labelled.
+// TestRenderDocDiffInsertAndDelete proves an added and a removed line are each
+// marked as such, in the class and in the sign column.
func TestRenderDocDiffInsertAndDelete(t *testing.T) {
old := []byte("# Title\n\nKept paragraph.\n\nDoomed paragraph.\n")
nw := []byte("# Title\n\nKept paragraph.\n\nBrand new paragraph.\n")
v := render(old, nw)
html := string(v.HTML)
- if !strings.Contains(html, "ph-insert") {
- t.Errorf("missing an inserted block; got:\n%s", html)
+ if !strings.Contains(html, "ph-r-ins") {
+ t.Errorf("missing an inserted line; got:\n%s", html)
+ }
+ if !strings.Contains(html, "ph-r-del") {
+ t.Errorf("missing a deleted line; got:\n%s", html)
}
- if !strings.Contains(html, "ph-delete") {
- t.Errorf("missing a deleted block; got:\n%s", html)
+ if !strings.Contains(html, `<td class="ph-sign">+</td>`) ||
+ !strings.Contains(html, `<td class="ph-sign">-</td>`) {
+ t.Errorf("the sign column does not say what happened; got:\n%s", html)
}
}
-// TestUnchangedBlocksRenderAsSubordinateContext proves the reversal this phase
-// makes: an unchanged block is on the page — every block of a proposed document
-// must be commentable — but as collapsed context, so a changed block still reads
-// as the thing that changed.
+// TestUnchangedBlocksRenderAsSubordinateContext proves an unchanged block is on
+// the page — every block of a proposed document must be commentable — but as
+// context: no tint class of its own, no sign, so a changed row still reads as
+// the thing that changed.
func TestUnchangedBlocksRenderAsSubordinateContext(t *testing.T) {
old := []byte("# Title\n\nUntouched paragraph.\n\nOld wording here.\n")
nw := []byte("# Title\n\nUntouched paragraph.\n\nNew wording here.\n")
@@ 135,19 149,118 @@ func TestUnchangedBlocksRenderAsSubordinateContext(t *testing.T) {
if !strings.Contains(html, "Untouched paragraph.") {
t.Fatalf("the unchanged block is not on the page; got:\n%s", html)
}
- if !strings.Contains(html, `<details class="ph-block ph-context"`) {
- t.Errorf("the unchanged block is not rendered as collapsed context; got:\n%s", html)
+ if !strings.Contains(html, `class="ph-row ph-r-eq ph-blk-start" id="b-`) {
+ t.Errorf("the unchanged block is not rendered as a context row; got:\n%s", html)
}
- // The changed block must stay plainly changed: its own class, its inline
- // marks, and no <details> wrapper hiding it behind a click.
- if !strings.Contains(html, `<div class="ph-block ph-modify"`) {
+ // The changed block must stay plainly changed: its rows marked, its word
+ // marks intact, and both sides of the edit numbered from their own revision.
+ if !strings.Contains(html, "ph-r-del") || !strings.Contains(html, "ph-r-ins") {
t.Errorf("the changed block lost its own presentation; got:\n%s", html)
}
- if !strings.Contains(html, "<ins>New</ins>") {
+ if !strings.Contains(html, "<ins>New</ins>") || !strings.Contains(html, "<del>Old</del>") {
t.Errorf("the changed block lost its inline marks; got:\n%s", html)
}
}
+// TestLongContextRunCollapses proves the other half of "subordinate": a screen
+// of unchanged lines folds away behind a checkbox, and the fold is a real form
+// control so it opens with JavaScript off.
+func TestLongContextRunCollapses(t *testing.T) {
+ var src strings.Builder
+ src.WriteString("# Title\n")
+ for i := 1; i <= 12; i++ {
+ fmt.Fprintf(&src, "\nContext paragraph %d.\n", i)
+ }
+ html := string(render([]byte(src.String()+"\nOld tail.\n"), []byte(src.String()+"\nNew tail.\n")).HTML)
+
+ if !strings.Contains(html, `<tbody class="ph-fold">`) {
+ t.Fatalf("a run of unchanged lines did not fold; got:\n%s", html)
+ }
+ if !strings.Contains(html, `<input type="checkbox" class="ph-fold-cb" id="fold-`) {
+ t.Errorf("the fold is not a checkbox, so it needs JavaScript to open; got:\n%s", html)
+ }
+ if !strings.Contains(html, "ph-folded") {
+ t.Errorf("the fold hides nothing; got:\n%s", html)
+ }
+ // Hidden is not absent: the text of a folded row is still in the document,
+ // so browser search and a text-mode reader still find it.
+ if !strings.Contains(html, "Context paragraph 6.") {
+ t.Errorf("a folded line was dropped from the page rather than hidden; got:\n%s", html)
+ }
+}
+
+// TestEverythingInAFoldIsHidden proves a collapsed run collapses whole. The
+// owner gets a compose form on every block, context ones included, and a
+// comment control drawn for lines the fold has just hidden is a control on
+// nothing — on the page it appears as an orphan row under the fold's own
+// toggle.
+func TestEverythingInAFoldIsHidden(t *testing.T) {
+ var src strings.Builder
+ src.WriteString("# Title\n")
+ for i := 1; i <= 12; i++ {
+ fmt.Fprintf(&src, "\nContext paragraph %d.\n", i)
+ }
+ view := renderDocDiff(docDiff{
+ DocID: "SPEC-0007", Path: "specs/0007-storage.md",
+ Base: []byte(src.String() + "\nOld tail.\n"),
+ Proposed: []byte(src.String() + "\nNew tail.\n"),
+ Controls: reviewControls{Owner: true, Reply: true, ActionBase: "/s/p/1"},
+ })
+ html := string(view.HTML)
+
+ open := strings.Index(html, `<tbody class="ph-fold">`)
+ if open < 0 {
+ t.Fatalf("no fold in a diff with twelve unchanged paragraphs; got:\n%s", html)
+ }
+ fold := html[open : open+strings.Index(html[open:], "</tbody>")]
+ for _, piece := range strings.Split(fold, "<tr")[1:] {
+ row := "<tr" + piece[:strings.IndexByte(piece, '>')]
+ if strings.Contains(row, "ph-fold-head") {
+ continue // the toggle itself is the one row a fold shows
+ }
+ if !strings.Contains(row, "ph-folded") {
+ t.Errorf("a row inside the fold is not hidden by it: %s", row)
+ }
+ }
+ if !strings.Contains(fold, `class="ph-notes ph-folded"`) {
+ t.Errorf("no folded compose row in this fold, so the test proves nothing; got:\n%s", fold)
+ }
+}
+
+// TestFoldIDsAreScopedToTheirDocument proves two documents on one review page
+// cannot toggle each other's folds. The ids are per-document by construction;
+// this is what says so.
+func TestFoldIDsAreScopedToTheirDocument(t *testing.T) {
+ var src strings.Builder
+ src.WriteString("# Title\n")
+ for i := 1; i <= 12; i++ {
+ fmt.Fprintf(&src, "\nContext paragraph %d.\n", i)
+ }
+ base, proposed := []byte(src.String()+"\nOld tail.\n"), []byte(src.String()+"\nNew tail.\n")
+
+ one := renderDocDiff(docDiff{DocID: "A", Path: "specs/a.md", Base: base, Proposed: proposed})
+ two := renderDocDiff(docDiff{DocID: "B", Path: "specs/b.md", Base: base, Proposed: proposed})
+
+ ids := func(html string) []string {
+ var out []string
+ for _, rest := range strings.Split(html, ` id="fold-`)[1:] {
+ out = append(out, rest[:strings.IndexByte(rest, '"')])
+ }
+ return out
+ }
+ a, b := ids(string(one.HTML)), ids(string(two.HTML))
+ if len(a) == 0 || len(b) == 0 {
+ t.Fatalf("expected a fold in each document, got %v and %v", a, b)
+ }
+ for _, x := range a {
+ for _, y := range b {
+ if x == y {
+ t.Fatalf("two documents share the fold id %q", x)
+ }
+ }
+ }
+}
+
// TestEveryRenderedBlockCarriesAnID proves a comment has something to point at
// and a link has something to scroll to, on changed and unchanged blocks alike.
func TestEveryRenderedBlockCarriesAnID(t *testing.T) {
A web/diffrows.go => web/diffrows.go +622 -0
@@ 0,0 1,622 @@
+package web
+
+import (
+ "fmt"
+ "strings"
+
+ "sourcecraft.dev/bigbes/sr-ht-spec/core"
+ "sourcecraft.dev/bigbes/sr-ht-spec/prosediff"
+)
+
+// This file is the row model of the unified diff: the arithmetic of which line
+// number goes in which gutter track, and nothing else. It emits no HTML on
+// purpose. Line attribution is the part of this renderer that can be quietly
+// wrong — a number off by one invites a reviewer to comment on text that was
+// never there — and a model built out of structs can be tested by reading its
+// fields instead of by matching substrings of markup.
+
+// rowKind is what one row says happened to its line. The values are the
+// suffixes of the ph-r-* classes the markup contract pins, so the HTML writer
+// concatenates rather than translates.
+type rowKind string
+
+const (
+ rowEqual rowKind = "eq"
+ rowInsert rowKind = "ins"
+ rowDelete rowKind = "del"
+ rowMove rowKind = "move"
+ // rowNotes is not a line. It marks the place in the stream where a block's
+ // threads and its compose form belong. It lives in the model rather than in
+ // the writer because where it goes — after the last row of its block, before
+ // the first row of the next — is a statement about order, and because the
+ // folder has to know a block's comment UI is there before it hides the block.
+ rowNotes rowKind = "notes"
+)
+
+// Folding thresholds. A run of unchanged lines is collapsed so a screenful of
+// context never competes with what changed, but collapsing is not free: it
+// costs a click and it hides text a reviewer may want to point at.
+//
+// foldMinRun and foldMinHidden are two gates, not one. A run of exactly
+// foldMinRun rows keeps foldKeepEdge rows at each end and would therefore hide
+// two, which is a bad trade — one toggle row replacing two lines of prose — so
+// the second gate rejects it and folding effectively starts at seven rows. The
+// markup contract states both numbers and they do not quite agree at the
+// boundary; the resolution here is the conservative one, because a fold that
+// saves nothing is a click that buys nothing.
+const (
+ foldMinRun = 6
+ foldKeepEdge = 2
+ foldMinHidden = 3
+)
+
+// rowBlock is what every row of one block shares. Rows point at it rather than
+// copying it so that the folder can ask a question about the *block* — does
+// anyone have a comment on it — while walking rows.
+type rowBlock struct {
+ // Anchor and Key are the block's comment identity; both are zero when
+ // Commentable is false, which is the move-out marker and nothing else today.
+ Anchor core.CommentAnchor
+ Key blockKey
+ Commentable bool
+
+ // Notes says a notes row was emitted for this block: it has threads, or the
+ // viewer is the owner and gets a compose form. A block with neither gets no
+ // row, because an empty one would be a gap in the table for no reason.
+ Notes bool
+ // HasThreads is only about folding: an unchanged block someone has already
+ // commented on is no longer merely context.
+ HasThreads bool
+
+ // Context marks a block that is unchanged on both sides. It, and not the row
+ // kind, is what the folder groups by — a block's notes row is not a ph-r-eq
+ // row but belongs inside the fold with the lines it hangs off.
+ Context bool
+ // Heading drives the sticky section readout; Mono drives the monospaced text
+ // cell of a code fence, frontmatter or HTML block.
+ Heading bool
+ Mono bool
+}
+
+// diffRow is one <tr> of the unified diff, before it is one.
+//
+// The two number fields are independent on purpose: a row states a number for
+// the side it came from and leaves the other side empty. That is what makes it
+// impossible to attribute a line number to the wrong revision — the alternative,
+// carrying one number plus a side flag, puts the decision in the writer, where
+// a rewrapped block would have to guess.
+type diffRow struct {
+ Kind rowKind
+ Block *rowBlock
+
+ // OldNum and NewNum are 1-based source line numbers, or zero for "this side
+ // has no number for this row". Zero renders as an empty cell and never as a
+ // 0: an honest blank beats a number nobody can defend.
+ OldNum, NewNum int
+ // OldEnd and NewEnd close a line range on a region row, and are zero
+ // everywhere else.
+ OldEnd, NewEnd int
+ // Region marks the fallback row that stands for a whole block rather than
+ // for one line — see regionRows.
+ Region bool
+
+ // Spans is the row's content as an edit script: one equal span for an
+ // untouched line, several for a line carrying word-level marks. Empty when
+ // Note is set.
+ Spans []prosediff.Span
+ // Note is renderer-authored text rather than document content — the move
+ // markers. It is still escaped on the way out, because a block label can
+ // carry a code fence's info string, which the document wrote.
+ Note string
+
+ // Start marks the first row of a block: the row that carries the id a
+ // comment link scrolls to.
+ Start bool
+ // Folded marks a row hidden until its fold is opened.
+ Folded bool
+}
+
+// rowGroup is one <tbody>. Grouping exists only to make folding a pure CSS
+// affordance: a fold group holds the rows a toggle hides, and everything else
+// accumulates into plain groups. Block identity is never carried by a group —
+// a fold boundary can and does cut a block in half.
+type rowGroup struct {
+ Fold bool
+ // Index numbers the fold groups of one document, so their checkboxes get
+ // distinct ids on a page that renders several documents.
+ Index int
+ // Hidden is how many *lines* the fold hides, which is what its label says.
+ // Notes rows are hidden with them but are not lines and are not counted.
+ Hidden int
+ Rows []diffRow
+}
+
+// blockInfo is what the row builder cannot work out for itself: the comment
+// identity of a change and whether anything has been said about it. It is
+// passed in as a function so the model can be built — and tested — without a
+// thread store, a document path or a template behind it.
+type blockInfo struct {
+ Anchor core.CommentAnchor
+ Key blockKey
+ Commentable bool
+ HasThreads bool
+ Notes bool
+}
+
+// buildRows turns a document's block changes into the unified row stream, in
+// document order.
+//
+// Every change contributes at least one row. A block that produced none would
+// be document content that silently left the page, which is worse than a row
+// that only says the block is empty.
+func buildRows(changes []prosediff.BlockChange, info func(prosediff.BlockChange) blockInfo) []diffRow {
+ var rows []diffRow
+ for _, c := range changes {
+ blk := newRowBlock(c, info(c))
+ at := len(rows)
+ rows = append(rows, changeRows(c, blk)...)
+ if len(rows) == at {
+ continue
+ }
+ rows[at].Start = true
+ if blk.Notes {
+ rows = append(rows, diffRow{Kind: rowNotes, Block: blk})
+ }
+ }
+ return rows
+}
+
+// newRowBlock derives the per-block facts every row of a change shares.
+//
+// The structural facts come from the side the rows are drawn from — the old
+// block for a deletion and for a move-out marker, the new one otherwise — so a
+// block that changed kind (a paragraph promoted to a heading) is described by
+// the revision the reader is looking at.
+func newRowBlock(c prosediff.BlockChange, info blockInfo) *rowBlock {
+ src := c.New
+ if c.Kind == prosediff.ChangeDelete || c.Kind == prosediff.ChangeMoveOut {
+ src = c.Old
+ }
+ blk := &rowBlock{
+ Anchor: info.Anchor,
+ Key: info.Key,
+ Commentable: info.Commentable,
+ Notes: info.Notes,
+ HasThreads: info.HasThreads,
+ Context: c.Kind == prosediff.ChangeEqual,
+ }
+ if src != nil {
+ blk.Heading = src.Kind == prosediff.KindHeading
+ blk.Mono = !src.Kind.Prose()
+ }
+ return blk
+}
+
+// changeRows renders one block change into rows. The five kinds that carry a
+// whole block map onto their lines directly and exactly; only a modification
+// has to recover which line a word edit fell on, which is modifyRows' problem.
+func changeRows(c prosediff.BlockChange, blk *rowBlock) []diffRow {
+ switch c.Kind {
+ case prosediff.ChangeEqual:
+ return equalRows(c, blk)
+ case prosediff.ChangeInsert:
+ return wholeBlockRows(c.New, rowInsert, false, blk)
+ case prosediff.ChangeDelete:
+ return wholeBlockRows(c.Old, rowDelete, true, blk)
+ case prosediff.ChangeMoveIn:
+ // The marker first, then the text. A move-in is commentable — the block
+ // is at its new position and this is where a reviewer objects to it — so
+ // it shows its lines; a comment control on text the reviewer cannot see
+ // is a control on nothing.
+ note := diffRow{
+ Kind: rowMove,
+ Block: blk,
+ Note: fmt.Sprintf("%s moved here (was line %d)", c.New.Label(), c.Old.StartLine),
+ }
+ return append([]diffRow{note}, wholeBlockRows(c.New, rowMove, false, blk)...)
+ case prosediff.ChangeMoveOut:
+ // One marker and no text: the block is rendered in full at its new
+ // position, and showing it twice would give one paragraph two places to
+ // be commented on.
+ return []diffRow{{
+ Kind: rowMove,
+ Block: blk,
+ OldNum: c.Old.StartLine,
+ Note: fmt.Sprintf("%s moved away (now line %d)", c.Old.Label(), c.New.StartLine),
+ }}
+ case prosediff.ChangeModify:
+ return modifyRows(c, blk)
+ }
+ return nil
+}
+
+// equalRows renders an unchanged block as context.
+//
+// A row states an old line number only when the old revision really does hold
+// this text on that line. The rule used to be that the two sides' line *counts*
+// agreeing was proof enough of a 1:1 correspondence, and it is not: the block is
+// equal at the token level, which is what makes a rewrap invisible to the
+// differ, so words can move across the line breaks while the count stays the
+// same. "alpha beta / gamma delta" rewrapped to "alpha / beta gamma delta" is
+// two lines before and after, and pairing them by position numbered a row 2
+// whose text was never on old line 2 — a reviewer selecting it would have
+// commented on text that does not exist in that revision, which is the exact
+// failure prosediff.WordsByLine refuses to risk.
+//
+// So each row is checked on its own, and an unpaired row leaves the old cell
+// empty exactly as the count-mismatch case already did. Blank beats fabricated.
+func equalRows(c prosediff.BlockChange, blk *rowBlock) []diffRow {
+ nw := blockLines(c.New)
+ old := blockLines(c.Old)
+ prose := c.New.Kind.Prose()
+
+ rows := make([]diffRow, len(nw))
+ for i, ln := range nw {
+ rows[i] = diffRow{
+ Kind: rowEqual,
+ Block: blk,
+ NewNum: c.New.StartLine + i,
+ Spans: plainSpans(ln),
+ }
+ if i < len(old) && sameSourceLine(old[i], ln, prose) {
+ rows[i].OldNum = c.Old.StartLine + i
+ }
+ }
+ return rows
+}
+
+// sameSourceLine reports whether two revisions' copies of a line hold the same
+// text, by the same yardstick finishBlock uses to hash the block: prose
+// compares normalized, because the tokenizer is what the differ ran on and the
+// space between two words is not a difference anyone can see; everything else
+// compares verbatim, because in a code fence it is.
+func sameSourceLine(old, nw string, prose bool) bool {
+ if !prose {
+ return old == nw
+ }
+ return prosediff.Normalize(old) == prosediff.Normalize(nw)
+}
+
+// wholeBlockRows renders every line of a block on one side of the diff: an
+// insertion, a deletion, or the body of a move-in.
+func wholeBlockRows(src *prosediff.Block, kind rowKind, old bool, blk *rowBlock) []diffRow {
+ lines := blockLines(src)
+ rows := make([]diffRow, len(lines))
+ for i, ln := range lines {
+ rows[i] = diffRow{Kind: kind, Block: blk, Spans: plainSpans(ln)}
+ if old {
+ rows[i].OldNum = src.StartLine + i
+ } else {
+ rows[i].NewNum = src.StartLine + i
+ }
+ }
+ return rows
+}
+
+// modifyRows renders an edited block, choosing among the three presentations
+// the design pins.
+//
+// A code fence, frontmatter or HTML block already has a line-oriented script
+// and needs no recovery. A prose block that stayed similar enough to follow has
+// its word script spread back over its source lines. A prose block rewritten
+// past that point — or one whose lines and text disagree, so the spreading
+// cannot be trusted — falls back to a pair of region rows.
+func modifyRows(c prosediff.BlockChange, blk *rowBlock) []diffRow {
+ if len(c.Lines) > 0 {
+ return lineScriptRows(c, blk)
+ }
+ if c.Similarity >= inlineSimilarityThreshold {
+ if old, nw, ok := prosediff.WordsByLine(c); ok {
+ return mergeLineWords(old, nw, blk)
+ }
+ }
+ return regionRows(c, blk)
+}
+
+// lineScriptRows renders a modified non-prose block. prosediff.DiffLines emits
+// exactly one span per line, so the two counters walk the two revisions in step
+// and every row's number is read off, not derived.
+func lineScriptRows(c prosediff.BlockChange, blk *rowBlock) []diffRow {
+ oldNo, newNo := c.Old.StartLine, c.New.StartLine
+ rows := make([]diffRow, 0, len(c.Lines))
+ for _, s := range c.Lines {
+ row := diffRow{Block: blk, Spans: plainSpans(s.Text)}
+ switch s.Op {
+ case prosediff.OpDelete:
+ row.Kind, row.OldNum = rowDelete, oldNo
+ oldNo++
+ case prosediff.OpInsert:
+ row.Kind, row.NewNum = rowInsert, newNo
+ newNo++
+ default:
+ row.Kind, row.OldNum, row.NewNum = rowEqual, oldNo, newNo
+ oldNo++
+ newNo++
+ }
+ rows = append(rows, row)
+ }
+ return rows
+}
+
+// mergeLineWords interleaves the two sides of a spread word script into one
+// unified column.
+//
+// The two sides are separate sequences of lines with no correspondence stored
+// between them — prosediff.WordsByLine hands back the old block's lines and the
+// new block's lines, each carrying its own share of the script — so the order
+// they appear in is this function's choice. Two cursors walk them:
+//
+// 1. a line neither side marked, with the same text on both, is one context
+// row carrying both numbers;
+// 2. otherwise an old line carrying a deletion is emitted alone, old track only;
+// 3. otherwise a new line carrying an insertion is emitted alone, new track only;
+// 4. otherwise the two lines differ without either being marked, which is a
+// rewrap: the words did not change but the lines did, so the pair is emitted
+// as a removed line followed by an added one, adjacent.
+//
+// Case 4 is the judgement call. The alternative was to emit the pair as one
+// context row and let the gutter show both numbers, which reads better but says
+// two lines are the same line when their text differs; in a table whose whole
+// contract is "the number in the gutter is the number in the file", that is the
+// wrong lie to tell. The alternative to case 2 before 3 — pairing a marked old
+// line with a marked new line on one row — was rejected because it re-invents a
+// correspondence the differ deliberately did not compute.
+//
+// Whatever the interleaving does, a row's number always comes from its own
+// side's LineWords, so an imperfect order costs readability and never
+// correctness.
+func mergeLineWords(old, nw []prosediff.LineWords, blk *rowBlock) []diffRow {
+ var rows []diffRow
+ delRow := func(l prosediff.LineWords) diffRow {
+ return diffRow{Kind: rowDelete, Block: blk, OldNum: l.Line, Spans: l.Spans}
+ }
+ insRow := func(l prosediff.LineWords) diffRow {
+ return diffRow{Kind: rowInsert, Block: blk, NewNum: l.Line, Spans: l.Spans}
+ }
+
+ i, j := 0, 0
+ for i < len(old) && j < len(nw) {
+ o, n := old[i], nw[j]
+ switch {
+ case !marked(o.Spans, prosediff.OpDelete) && !marked(n.Spans, prosediff.OpInsert) &&
+ lineText(o) == lineText(n):
+ rows = append(rows, diffRow{
+ Kind: rowEqual, Block: blk,
+ OldNum: o.Line, NewNum: n.Line, Spans: n.Spans,
+ })
+ i++
+ j++
+ case marked(o.Spans, prosediff.OpDelete):
+ rows = append(rows, delRow(o))
+ i++
+ case marked(n.Spans, prosediff.OpInsert):
+ rows = append(rows, insRow(n))
+ j++
+ default:
+ rows = append(rows, delRow(o), insRow(n))
+ i++
+ j++
+ }
+ }
+ for ; i < len(old); i++ {
+ rows = append(rows, delRow(old[i]))
+ }
+ for ; j < len(nw); j++ {
+ rows = append(rows, insRow(nw[j]))
+ }
+ return rows
+}
+
+// regionRows is the honest fallback: one row for the old side of the block and
+// one for the new, each labelled by the line range it covers rather than by a
+// line number.
+//
+// It fires for a block rewritten past the point where inline marks stay
+// readable — the Phase 0 verdict's one review in eight — and for a block whose
+// word script could not be spread back over its lines. In both cases a per-line
+// number would be a guess, and the design's rule is that a range the reader can
+// check beats a number they cannot.
+func regionRows(c prosediff.BlockChange, blk *rowBlock) []diffRow {
+ return []diffRow{
+ {
+ Kind: rowDelete, Block: blk, Region: true,
+ OldNum: c.Old.StartLine, OldEnd: c.Old.EndLine,
+ Spans: sideSpans(c.Words, true),
+ },
+ {
+ Kind: rowInsert, Block: blk, Region: true,
+ NewNum: c.New.StartLine, NewEnd: c.New.EndLine,
+ Spans: sideSpans(c.Words, false),
+ },
+ }
+}
+
+// sideSpans keeps one side of a word script: the old side keeps equal and
+// deleted words, the new side keeps equal and inserted ones. Both keep their
+// marks, so each region row is a readable paragraph that also shows what moved.
+//
+// The separator of a dropped span moves onto the next kept one. Span.Space says
+// a space preceded that span *in the combined rendering*, and an insertion that
+// directly replaces a deletion carries Space=false because the deletion in front
+// of it already carried the space — prosediff's own note on the matter. Split
+// onto one side that deletion is gone, and without this the row would read "the
+// committeerejected the budget".
+func sideSpans(spans []prosediff.Span, old bool) []prosediff.Span {
+ out := make([]prosediff.Span, 0, len(spans))
+ space := false
+ for _, s := range spans {
+ switch {
+ case s.Op == prosediff.OpEqual,
+ old && s.Op == prosediff.OpDelete,
+ !old && s.Op == prosediff.OpInsert:
+ default:
+ space = space || s.Space
+ continue
+ }
+ s.Space = s.Space || space
+ space = false
+ out = append(out, s)
+ }
+ return out
+}
+
+// groupRows cuts the row stream into <tbody> groups, collapsing long runs of
+// context.
+//
+// A run is delimited by blocks, not by rows: a block is foldable when it is
+// unchanged and carries no threads, and then all of its rows fold, its notes
+// row included. Keying on the row kind instead would let a changed block's
+// compose form — which is not a ph-r-eq row but sits between two of them —
+// either break every run or be swallowed into a fold it does not belong to.
+func groupRows(rows []diffRow) []rowGroup {
+ var groups []rowGroup
+ var plain []diffRow
+ folds := 0
+
+ flush := func() {
+ if len(plain) > 0 {
+ groups = append(groups, rowGroup{Rows: plain})
+ plain = nil
+ }
+ }
+
+ for i := 0; i < len(rows); {
+ if !foldable(rows[i]) {
+ plain = append(plain, rows[i])
+ i++
+ continue
+ }
+ j := i
+ for j < len(rows) && foldable(rows[j]) {
+ j++
+ }
+ run := rows[i:j]
+ i = j
+
+ lo, hi, ok := foldWindow(run)
+ if !ok {
+ plain = append(plain, run...)
+ continue
+ }
+ plain = append(plain, run[:lo]...)
+ flush()
+
+ hidden := make([]diffRow, hi-lo)
+ lines := 0
+ for k, row := range run[lo:hi] {
+ row.Folded = true
+ hidden[k] = row
+ if row.Kind != rowNotes {
+ lines++
+ }
+ }
+ groups = append(groups, rowGroup{Fold: true, Index: folds, Hidden: lines, Rows: hidden})
+ folds++
+ plain = append(plain, run[hi:]...)
+ }
+ flush()
+ return groups
+}
+
+// foldable reports whether a row may be hidden inside a fold. A block someone
+// has commented on never is: it stopped being context the moment somebody had
+// something to say about it, and a comment behind a closed fold is a comment
+// nobody reads.
+func foldable(row diffRow) bool {
+ return row.Block.Context && !row.Block.HasThreads
+}
+
+// foldWindow picks the half-open range of a context run to hide: everything
+// between the first foldKeepEdge lines and the last foldKeepEdge lines, which
+// keeps a couple of lines of orientation on each side of the gap. The window is
+// measured in rows so that a notes row falling inside the gap is hidden with
+// it, and counted in lines so that both gates judge the same thing the label
+// will report.
+//
+// The rule the window has to respect is that a block's notes row is hidden
+// exactly when the whole block is. A compose form for lines nobody can see is a
+// control on nothing — the same reason a move-in renders its text — and a
+// compose form hidden away from lines that *are* visible is worse still,
+// because the block looks uncommentable. Only the opening edge can break it: a
+// window that starts inside a block would keep that block's first lines visible
+// and swallow the trailer that follows its last one, so in that case the window
+// opens after the trailer instead. The closing edge cannot break it, because hi
+// is a line row by construction and a trailer always directly follows its
+// block's last line.
+func foldWindow(run []diffRow) (lo, hi int, ok bool) {
+ var lines []int
+ for i, row := range run {
+ if row.Kind != rowNotes {
+ lines = append(lines, i)
+ }
+ }
+ if len(lines) < foldMinRun || len(lines)-2*foldKeepEdge < foldMinHidden {
+ return 0, 0, false
+ }
+ lo, hi = lines[foldKeepEdge], lines[len(lines)-foldKeepEdge]
+
+ if !run[lo].Start {
+ for k := lo; k < hi && !run[k].Start; k++ {
+ if run[k].Kind == rowNotes {
+ lo = k + 1
+ break
+ }
+ }
+ }
+ // Giving that trailer back can leave too little to be worth a click, so the
+ // gate is asked again about what is actually left.
+ hidden := 0
+ for _, i := range lines {
+ if i >= lo && i < hi {
+ hidden++
+ }
+ }
+ if hidden < foldMinHidden {
+ return 0, 0, false
+ }
+ return lo, hi, true
+}
+
+// blockLines is a block's source lines, with the block's whole text as the one
+// line of a block that records none. Nothing in the segmenter produces such a
+// block today; this is what keeps that assumption from silently deleting a
+// block from the page if one ever does.
+func blockLines(src *prosediff.Block) []string {
+ if len(src.Lines) > 0 {
+ return src.Lines
+ }
+ return []string{src.Text}
+}
+
+// plainSpans is a line with no word-level marks on it, as a one-span script, so
+// every row's content has the same shape whatever produced it.
+func plainSpans(text string) []prosediff.Span {
+ return []prosediff.Span{{Op: prosediff.OpEqual, Text: text}}
+}
+
+// marked reports whether a line's share of a word script carries an op.
+func marked(spans []prosediff.Span, op prosediff.Op) bool {
+ for _, s := range spans {
+ if s.Op == op {
+ return true
+ }
+ }
+ return false
+}
+
+// lineText rebuilds a spread line's text for comparison.
+//
+// It compares reconstructed tokens rather than the source lines because that is
+// what "the same line" has to mean here: the tokenizer is what the diff ran on,
+// so two lines differing only in how much whitespace separates their words are
+// the same line to every part of this package.
+func lineText(l prosediff.LineWords) string {
+ var b strings.Builder
+ for i, s := range l.Spans {
+ if s.Space && i > 0 {
+ b.WriteByte(' ')
+ }
+ b.WriteString(s.Text)
+ }
+ return b.String()
+}
A web/diffrows_test.go => web/diffrows_test.go +900 -0
@@ 0,0 1,900 @@
+package web
+
+import (
+ "fmt"
+ "math/rand"
+ "regexp"
+ "strings"
+ "testing"
+
+ "sourcecraft.dev/bigbes/sr-ht-spec/core"
+ "sourcecraft.dev/bigbes/sr-ht-spec/prosediff"
+)
+
+// These tests read the row model directly rather than the markup it becomes.
+// The thing that can be quietly wrong here is arithmetic — which line number
+// goes in which gutter track — and a test that matched HTML would be asserting
+// the arithmetic through a layer that can also change for cosmetic reasons.
+
+// stubInfo gives every change its own commentable block. The row model never
+// looks inside an anchor, only at whether there is one, so these are simply
+// distinct; threads names the changes (by position) that carry a comment.
+func stubInfo(threads map[int]bool, owner bool) func(prosediff.BlockChange) blockInfo {
+ n := -1
+ return func(c prosediff.BlockChange) blockInfo {
+ n++
+ // A move-out marker is the one change the renderer offers no anchor for.
+ if c.Kind == prosediff.ChangeMoveOut {
+ return blockInfo{}
+ }
+ has := threads[n]
+ return blockInfo{
+ Anchor: core.CommentAnchor{DocID: "D", Index: n, BlockHash: fmt.Sprint(n)},
+ Key: blockKey{core.SideNew, n},
+ Commentable: true,
+ HasThreads: has,
+ Notes: has || owner,
+ }
+ }
+}
+
+// rowsOf builds the row model of two revisions, as a reviewer with no comments
+// and no compose form sees it.
+func rowsOf(oldSrc, newSrc string) []diffRow {
+ d := prosediff.Compare([]byte(oldSrc), []byte(newSrc))
+ return buildRows(d.Changes, stubInfo(nil, false))
+}
+
+// modifyRowsOf isolates the rows of the single modified block in a diff, so a
+// test about line attribution is not also a test about what the aligner paired.
+func modifyRowsOf(t *testing.T, oldSrc, newSrc string) []diffRow {
+ t.Helper()
+ for _, c := range prosediff.Compare([]byte(oldSrc), []byte(newSrc)).Changes {
+ if c.Kind == prosediff.ChangeModify {
+ return buildRows([]prosediff.BlockChange{c}, stubInfo(nil, false))
+ }
+ }
+ t.Fatalf("no modified block in the diff of:\n%s\n---\n%s", oldSrc, newSrc)
+ return nil
+}
+
+// summarize renders rows as "<kind> <old>/<new> <text>", with an interpunct for
+// a track this row states no number for and inline brackets for the word marks,
+// so an expectation reads as what the reviewer would see in the gutter.
+func summarize(rows []diffRow) []string {
+ out := make([]string, len(rows))
+ for i, row := range rows {
+ out[i] = fmt.Sprintf("%s %s/%s %s", row.Kind,
+ track(row.OldNum, row.OldEnd), track(row.NewNum, row.NewEnd), rowText(row))
+ }
+ return out
+}
+
+func track(n, end int) string {
+ switch {
+ case n == 0:
+ return "·"
+ case end > n:
+ return fmt.Sprintf("%d–%d", n, end)
+ default:
+ return fmt.Sprint(n)
+ }
+}
+
+func rowText(row diffRow) string {
+ if row.Note != "" {
+ return row.Note
+ }
+ var b strings.Builder
+ for i, s := range row.Spans {
+ if s.Space && i > 0 {
+ b.WriteByte(' ')
+ }
+ switch s.Op {
+ case prosediff.OpDelete:
+ b.WriteString("[-" + s.Text + "-]")
+ case prosediff.OpInsert:
+ b.WriteString("{+" + s.Text + "+}")
+ default:
+ b.WriteString(s.Text)
+ }
+ }
+ return b.String()
+}
+
+func checkRows(t *testing.T, got []diffRow, want []string) {
+ t.Helper()
+ summary := summarize(got)
+ if len(summary) != len(want) {
+ t.Fatalf("rows =\n %s\nwant\n %s", strings.Join(summary, "\n "), strings.Join(want, "\n "))
+ }
+ for i := range want {
+ if summary[i] != want[i] {
+ t.Errorf("row %d = %q, want %q", i, summary[i], want[i])
+ }
+ }
+}
+
+// An inserted block numbers the new revision and leaves the old track empty:
+// those lines exist in one revision only, and a number in the other track would
+// name a line that was never there.
+func TestInsertedBlockNumbersOnlyTheNewSide(t *testing.T) {
+ const old = "# H\n\nkept.\n"
+ const nw = "# H\n\nkept.\n\nfirst added line\nsecond added line\n"
+
+ var ins []diffRow
+ for _, row := range rowsOf(old, nw) {
+ if row.Kind == rowInsert {
+ ins = append(ins, row)
+ }
+ }
+ checkRows(t, ins, []string{
+ "ins ·/5 first added line",
+ "ins ·/6 second added line",
+ })
+}
+
+// A deleted block is the mirror image, and is numbered against the base
+// revision because that is the only revision it is in.
+func TestDeletedBlockNumbersOnlyTheOldSide(t *testing.T) {
+ const old = "# H\n\nkept.\n\ndoomed line one\ndoomed line two\n"
+ const nw = "# H\n\nkept.\n"
+
+ var del []diffRow
+ for _, row := range rowsOf(old, nw) {
+ if row.Kind == rowDelete {
+ del = append(del, row)
+ }
+ }
+ checkRows(t, del, []string{
+ "del 5/· doomed line one",
+ "del 6/· doomed line two",
+ })
+}
+
+// An unchanged block occupying the same lines in both revisions states both
+// numbers: that is the ordinary context row, and the reviewer can check either
+// against the file.
+func TestEqualBlockNumbersBothSidesWhenTheLineCountsAgree(t *testing.T) {
+ const src = "# H\n\nfirst context line\nsecond context line\n"
+ rows := buildRows(
+ prosediff.Compare([]byte(src+"\ntail.\n"), []byte(src+"\ntail, edited.\n")).Changes,
+ stubInfo(nil, false))
+
+ var eq []diffRow
+ for _, row := range rows {
+ if row.Kind == rowEqual && row.Block.Context {
+ eq = append(eq, row)
+ }
+ }
+ // The heading row carries "H" and not "# H": prosediff records a heading's
+ // text without its markers, and a row shows the block's own lines.
+ checkRows(t, eq, []string{
+ "eq 1/1 H",
+ "eq 3/3 first context line",
+ "eq 4/4 second context line",
+ })
+}
+
+// A block that was only rewrapped is unchanged prose sitting on a different
+// number of lines. The new side is numbered and the old side is left blank,
+// because there is no line-to-line correspondence to state and interpolating
+// one would be inventing evidence.
+func TestRewrappedEqualBlockLeavesTheOldTrackEmpty(t *testing.T) {
+ const old = "# H\n\nthe quick brown fox\njumps over the lazy dog\n\ntail.\n"
+ const nw = "# H\n\nthe quick brown fox jumps\nover\nthe lazy dog\n\ntail, edited.\n"
+
+ var eq []diffRow
+ for _, row := range rowsOf(old, nw) {
+ if row.Kind == rowEqual && row.Block.Context && row.OldNum != 1 {
+ eq = append(eq, row)
+ }
+ }
+ checkRows(t, eq, []string{
+ "eq ·/3 the quick brown fox jumps",
+ "eq ·/4 over",
+ "eq ·/5 the lazy dog",
+ })
+}
+
+// The same rule when the rewrap happens to preserve the line count — the case
+// equal counts were once taken as proof of a 1:1 correspondence.
+//
+// "alpha beta / gamma delta" becoming "alpha / beta gamma delta" is two lines
+// either way and unchanged prose either way, but nothing on new line 2 was on
+// old line 2. Numbering it 2 offered the reviewer a drag that would have
+// anchored a comment to text that revision never contained.
+func TestRewrapKeepingTheLineCountStillLeavesTheOldTrackEmpty(t *testing.T) {
+ const old = "alpha beta\ngamma delta\n\nzzz trigger\n"
+ const nw = "alpha\nbeta gamma delta\n\nzzz triggered\n"
+
+ var eq []diffRow
+ for _, row := range rowsOf(old, nw) {
+ if row.Kind == rowEqual && row.Block.Context {
+ eq = append(eq, row)
+ }
+ }
+ checkRows(t, eq, []string{
+ "eq ·/1 alpha",
+ "eq ·/2 beta gamma delta",
+ })
+}
+
+// Pairing is decided per row, not per block, so the lines a rewrap did not
+// touch keep their old numbers. Blanking the whole block whenever any line
+// moved would be honest but needlessly lossy: the old track is where a reviewer
+// looks to find the text in the base revision.
+func TestUnmovedLinesOfARewrappedBlockKeepTheirOldNumbers(t *testing.T) {
+ const old = "one two\nthree four\nfive six seven\n\nzzz trigger\n"
+ const nw = "one two\nthree four\nfive six\nseven\n\nzzz triggered\n"
+
+ var eq []diffRow
+ for _, row := range rowsOf(old, nw) {
+ if row.Kind == rowEqual && row.Block.Context {
+ eq = append(eq, row)
+ }
+ }
+ checkRows(t, eq, []string{
+ "eq 1/1 one two",
+ "eq 2/2 three four",
+ "eq ·/3 five six",
+ "eq ·/4 seven",
+ })
+}
+
+// A modified prose block similar enough to follow is spread back over its own
+// lines: the change is a row of its own on each side, numbered from that side,
+// and the lines around it stay context rows carrying both numbers.
+func TestModifiedProseGoesPerLine(t *testing.T) {
+ const old = "# H\n\nalpha beta gamma\ndelta epsilon zeta\neta theta iota\n"
+ const nw = "# H\n\nalpha beta gamma\ndelta CHANGED zeta\neta theta iota\n"
+
+ checkRows(t, modifyRowsOf(t, old, nw), []string{
+ "eq 3/3 alpha beta gamma",
+ "del 4/· delta [-epsilon-] zeta",
+ "ins ·/4 delta {+CHANGED+} zeta",
+ "eq 5/5 eta theta iota",
+ })
+}
+
+// A block rewritten past the similarity threshold falls back to one row per
+// side, labelled by the line range it covers. Per-line numbers here would be a
+// guess, and the design's rule is that a range beats a number nobody can check.
+func TestShreddedBlockFallsBackToRegionRows(t *testing.T) {
+ const old = "# H\n\nThe committee approved the annual budget\nafter a long and contentious debate that\nlasted well into the evening.\n"
+ const nw = "# H\n\nThe committee rejected the annual budget\nafter a brief and quiet discussion that\nended early in the afternoon.\n"
+
+ rows := modifyRowsOf(t, old, nw)
+ for _, row := range rows {
+ if !row.Region {
+ t.Fatalf("a shredded block rendered per line:\n %s", strings.Join(summarize(rows), "\n "))
+ }
+ }
+ if len(rows) != 2 {
+ t.Fatalf("region fallback produced %d rows, want 2:\n %s", len(rows), strings.Join(summarize(rows), "\n "))
+ }
+ if got := track(rows[0].OldNum, rows[0].OldEnd); got != "3–5" {
+ t.Errorf("old region covers %q, want the whole block's range 3–5", got)
+ }
+ if rows[0].NewNum != 0 || rows[1].OldNum != 0 {
+ t.Errorf("a region row numbered the side it does not describe: %v", summarize(rows))
+ }
+ if got := track(rows[1].NewNum, rows[1].NewEnd); got != "3–5" {
+ t.Errorf("new region covers %q, want 3–5", got)
+ }
+ // Each side keeps its own words and its own marks: two readable paragraphs.
+ if got := rowText(rows[0]); !strings.Contains(got, "[-approved-]") || strings.Contains(got, "{+") {
+ t.Errorf("old region = %q, want the deletions and no insertions", got)
+ }
+ if got := rowText(rows[1]); !strings.Contains(got, "{+rejected+}") || strings.Contains(got, "[-") {
+ t.Errorf("new region = %q, want the insertions and no deletions", got)
+ }
+ // The space that belonged to the deletion this insertion replaced has to
+ // survive dropping it, or the row runs two words together.
+ if got := rowText(rows[1]); !strings.Contains(got, "committee {+rejected+}") {
+ t.Errorf("new region = %q, want a separator before the replacement", got)
+ }
+}
+
+// A code fence has a line-oriented script already, so its rows are read off it
+// rather than recovered: equal lines carry both numbers, and the two counters
+// walk their own revisions.
+func TestModifiedCodeFenceFollowsTheLineScript(t *testing.T) {
+ const old = "# H\n\n```go\na := 1\nb := 2\nc := 3\n```\n"
+ const nw = "# H\n\n```go\na := 1\nb := 20\nc := 3\n```\n"
+
+ // The fence delimiters are not part of the block, so the rows start at the
+ // first line of code — line 4 of the document.
+ rows := modifyRowsOf(t, old, nw)
+ checkRows(t, rows, []string{
+ "eq 4/4 a := 1",
+ "del 5/· b := 2",
+ "ins ·/5 b := 20",
+ "eq 6/6 c := 3",
+ })
+ for _, row := range rows {
+ if !row.Block.Mono {
+ t.Fatalf("a code fence row is not marked monospaced: %+v", row)
+ }
+ }
+}
+
+// A move renders as a marker where the block was and the block itself where it
+// now is. The marker at the old position is not commentable: the text is on the
+// page once, and anchoring it twice would give one paragraph two places to be
+// argued about.
+func TestMovedBlockPair(t *testing.T) {
+ old := &prosediff.Block{
+ Kind: prosediff.KindParagraph, Text: "the travelling paragraph",
+ Lines: []string{"the travelling paragraph"}, StartLine: 3, EndLine: 3,
+ }
+ nw := &prosediff.Block{
+ Kind: prosediff.KindParagraph, Text: "the travelling paragraph",
+ Lines: []string{"the travelling paragraph"}, StartLine: 12, EndLine: 12,
+ }
+ rows := buildRows([]prosediff.BlockChange{
+ {Kind: prosediff.ChangeMoveOut, Old: old, New: nw},
+ {Kind: prosediff.ChangeMoveIn, Old: old, New: nw},
+ }, stubInfo(nil, false))
+
+ checkRows(t, rows, []string{
+ "move 3/· paragraph moved away (now line 12)",
+ "move ·/· paragraph moved here (was line 3)",
+ "move ·/12 the travelling paragraph",
+ })
+ if rows[0].Block.Commentable {
+ t.Errorf("the move-out marker offers an anchor; it points at text rendered elsewhere")
+ }
+ if !rows[1].Block.Commentable || !rows[1].Start {
+ t.Errorf("the move-in is not an anchorable block starting at its marker row")
+ }
+}
+
+// Every block starts exactly one row, and its notes row closes it: a comment
+// belongs under the lines it is about, and the id a link scrolls to belongs on
+// the first of them and nowhere else.
+func TestEachBlockStartsOnceAndItsNotesRowComesLast(t *testing.T) {
+ const old = "# H\n\ncontext.\n\nold wording.\n"
+ const nw = "# H\n\ncontext.\n\nnew wording.\n"
+
+ rows := buildRows(prosediff.Compare([]byte(old), []byte(nw)).Changes, stubInfo(nil, true))
+ starts, notes := 0, 0
+ for i, row := range rows {
+ if row.Start {
+ starts++
+ if row.Kind == rowNotes {
+ t.Errorf("row %d: a notes row opened a block", i)
+ }
+ }
+ if row.Kind == rowNotes {
+ notes++
+ if i+1 < len(rows) && rows[i+1].Block == row.Block {
+ t.Errorf("row %d: the notes row is not the block's last row", i)
+ }
+ }
+ }
+ // Heading, context paragraph, modified paragraph.
+ if starts != 3 || notes != 3 {
+ t.Errorf("%d block starts and %d notes rows, want 3 and 3", starts, notes)
+ }
+}
+
+// contextChanges is n unchanged one-line blocks, which is the shape the folder
+// is about.
+func contextChanges(n int) []prosediff.BlockChange {
+ out := make([]prosediff.BlockChange, n)
+ for i := range out {
+ blk := &prosediff.Block{
+ Kind: prosediff.KindParagraph,
+ Text: fmt.Sprintf("context line %d", i+1),
+ Lines: []string{fmt.Sprintf("context line %d", i+1)},
+ StartLine: i + 1, EndLine: i + 1,
+ }
+ out[i] = prosediff.BlockChange{Kind: prosediff.ChangeEqual, Old: blk, New: blk}
+ }
+ return out
+}
+
+func foldGroups(groups []rowGroup) []rowGroup {
+ var out []rowGroup
+ for _, g := range groups {
+ if g.Fold {
+ out = append(out, g)
+ }
+ }
+ return out
+}
+
+// checkNotesFollowTheirBlock asserts the rule a fold must never break: a
+// block's notes row is hidden exactly when every line of that block is. A
+// compose form offered for text nobody can see is a control on nothing, and one
+// hidden away from text that is on screen makes a commentable block look
+// uncommentable.
+func checkNotesFollowTheirBlock(t *testing.T, groups []rowGroup) {
+ t.Helper()
+ visible := make(map[*rowBlock]bool)
+ for _, g := range groups {
+ for _, row := range g.Rows {
+ if row.Kind != rowNotes && !row.Folded {
+ visible[row.Block] = true
+ }
+ }
+ }
+ for _, g := range groups {
+ for _, row := range g.Rows {
+ if row.Kind != rowNotes {
+ continue
+ }
+ if row.Folded == visible[row.Block] {
+ t.Errorf("block %q: notes row folded=%v, block has visible lines=%v",
+ row.Block.Anchor.BlockHash, row.Folded, visible[row.Block])
+ }
+ }
+ }
+}
+
+// checkFoldGroupsAreWhollyHidden asserts the other half: everything inside a
+// fold's <tbody> is hidden by it. A visible row in there renders between the
+// fold's toggle and the next line — which is exactly how the orphaned compose
+// forms appeared on the page.
+func checkFoldGroupsAreWhollyHidden(t *testing.T, groups []rowGroup) {
+ t.Helper()
+ for _, g := range groups {
+ if !g.Fold {
+ continue
+ }
+ for _, row := range g.Rows {
+ if !row.Folded {
+ t.Errorf("a %s row sits inside a fold group unhidden: %q", row.Kind, rowText(row))
+ }
+ }
+ }
+}
+
+// paragraphChanges is n unchanged blocks of lines lines each, so a test can put
+// a block boundary where it wants one relative to the fold's edges.
+func paragraphChanges(n, lines int) []prosediff.BlockChange {
+ out := make([]prosediff.BlockChange, n)
+ at := 1
+ for i := range out {
+ blk := &prosediff.Block{
+ Kind: prosediff.KindParagraph, StartLine: at, EndLine: at + lines - 1,
+ }
+ for j := 0; j < lines; j++ {
+ blk.Lines = append(blk.Lines, fmt.Sprintf("block %d line %d", i+1, j+1))
+ }
+ blk.Text = strings.Join(blk.Lines, "\n")
+ out[i] = prosediff.BlockChange{Kind: prosediff.ChangeEqual, Old: blk, New: blk}
+ at += lines + 1
+ }
+ return out
+}
+
+// A long run of context collapses and a short one does not. Six is the run
+// length the markup contract names, but keeping two rows at each end would
+// leave a fold hiding two lines behind a click that costs one row to draw, so
+// the second gate — at least three hidden lines — is what actually decides.
+func TestContextRunFoldsOnlyWhenItSavesSomething(t *testing.T) {
+ for _, tc := range []struct {
+ run int
+ hidden int // 0 for "does not fold"
+ }{
+ {run: 5, hidden: 0},
+ {run: 6, hidden: 0},
+ {run: 7, hidden: 3},
+ {run: 10, hidden: 6},
+ } {
+ rows := buildRows(contextChanges(tc.run), stubInfo(nil, false))
+ groups := groupRows(rows)
+ folds := foldGroups(groups)
+
+ if tc.hidden == 0 {
+ if len(folds) != 0 {
+ t.Errorf("a run of %d folded; want it left alone", tc.run)
+ }
+ continue
+ }
+ if len(folds) != 1 {
+ t.Fatalf("a run of %d produced %d folds, want 1", tc.run, len(folds))
+ }
+ if folds[0].Hidden != tc.hidden || len(folds[0].Rows) != tc.hidden {
+ t.Errorf("a run of %d hides %d rows (label says %d), want %d",
+ tc.run, len(folds[0].Rows), folds[0].Hidden, tc.hidden)
+ }
+ checkFoldGroupsAreWhollyHidden(t, groups)
+ checkNotesFollowTheirBlock(t, groups)
+ // Nothing is dropped on the way into a group, and the order is the
+ // document's: a fold hides rows, it does not remove them.
+ var back []diffRow
+ for _, g := range groups {
+ back = append(back, g.Rows...)
+ }
+ if len(back) != len(rows) {
+ t.Errorf("grouping a run of %d yielded %d rows, want %d", tc.run, len(back), len(rows))
+ }
+ for i := range back {
+ if rowText(back[i]) != rowText(rows[i]) {
+ t.Fatalf("grouping reordered the rows at %d", i)
+ }
+ }
+ }
+}
+
+// A block someone has commented on is not context any more, so it is never
+// hidden — and it breaks the run around it rather than being skipped over,
+// because a fold that stepped over it would hide the lines the comment is
+// about.
+func TestACommentedBlockKeepsItsRunOpen(t *testing.T) {
+ rows := buildRows(contextChanges(8), stubInfo(map[int]bool{3: true}, false))
+ groups := groupRows(rows)
+
+ if folds := foldGroups(groups); len(folds) != 0 {
+ t.Fatalf("a run split by a commented block still folded: %+v", folds)
+ }
+ for _, g := range groups {
+ for _, row := range g.Rows {
+ if row.Folded {
+ t.Errorf("a row was hidden anyway: %+v", row)
+ }
+ }
+ }
+}
+
+// The compose form the owner sees hangs off every block, including a context
+// one, and it folds with the lines it belongs to rather than breaking the run.
+// The fold's label counts lines, though: a notes row is not a line.
+func TestNotesRowsFoldWithTheirBlockAndAreNotCountedAsLines(t *testing.T) {
+ rows := buildRows(contextChanges(8), stubInfo(nil, true))
+ groups := groupRows(rows)
+ folds := foldGroups(groups)
+ if len(folds) != 1 {
+ t.Fatalf("the owner's compose forms broke the run: %d folds, want 1", len(folds))
+ }
+ if folds[0].Hidden != 4 {
+ t.Errorf("the fold says it hides %d lines, want 4", folds[0].Hidden)
+ }
+ lines, notes := 0, 0
+ for _, row := range folds[0].Rows {
+ if row.Kind == rowNotes {
+ notes++
+ } else {
+ lines++
+ }
+ }
+ if lines != 4 || notes != 4 {
+ t.Errorf("the fold holds %d lines and %d notes rows, want 4 and 4", lines, notes)
+ }
+ // Each of those four blocks is hidden whole: its line and its compose form.
+ checkFoldGroupsAreWhollyHidden(t, groups)
+ checkNotesFollowTheirBlock(t, groups)
+}
+
+// A fold whose opening edge lands inside a block gives that block back rather
+// than hiding the compose form belonging to lines still on screen. The fold
+// starts at the next block instead, hiding less and staying honest about what
+// can be commented on.
+func TestFoldOpensSoThatNoVisibleBlockLosesItsComposer(t *testing.T) {
+ // Two six-line paragraphs: keeping two lines at each end puts the raw
+ // opening edge in the middle of the first one, three rows above its notes row.
+ rows := buildRows(paragraphChanges(2, 6), stubInfo(nil, true))
+ groups := groupRows(rows)
+ folds := foldGroups(groups)
+
+ if len(folds) != 1 {
+ t.Fatalf("got %d folds, want 1", len(folds))
+ }
+ checkFoldGroupsAreWhollyHidden(t, groups)
+ checkNotesFollowTheirBlock(t, groups)
+
+ // The fold gave back the whole first paragraph and hides the second's first
+ // four lines, which is where a block boundary actually falls.
+ if got := summarize(folds[0].Rows); len(got) != 4 || got[0] != "eq 8/8 block 2 line 1" {
+ t.Errorf("the fold hides %v, want the second block's first four lines", got)
+ }
+ if folds[0].Hidden != 4 {
+ t.Errorf("the fold says it hides %d lines, want 4", folds[0].Hidden)
+ }
+ for _, g := range groups {
+ for _, row := range g.Rows {
+ if row.Folded && strings.HasPrefix(rowText(row), "block 1") {
+ t.Errorf("a line of the first block was hidden: %q", rowText(row))
+ }
+ }
+ }
+}
+
+// The rule holds when the last block of a run is only partly hidden too: its
+// trailing lines and its composer stay together below the fold.
+func TestFoldClosingEdgeKeepsAPartlyVisibleBlocksComposer(t *testing.T) {
+ rows := buildRows(paragraphChanges(3, 4), stubInfo(nil, true))
+ groups := groupRows(rows)
+ if len(foldGroups(groups)) != 1 {
+ t.Fatalf("got %d folds, want 1", len(foldGroups(groups)))
+ }
+ checkFoldGroupsAreWhollyHidden(t, groups)
+ checkNotesFollowTheirBlock(t, groups)
+}
+
+// ---- property test --------------------------------------------------------
+//
+// Everything above is a case someone thought of. The defect this section
+// exists for was not: pairing a rewrapped block's lines by position was
+// reviewed, unit-tested and wrong, because the test that covered it chose an
+// example where the line counts differed. Line attribution is exactly the kind
+// of invariant that hand-written cases bless and generated ones break, so the
+// generator stays.
+//
+// The invariant it checks is the one the whole gutter rests on: whatever text a
+// row renders is text that actually sits on the source lines that row's numbers
+// name. Every generated line carries a nonce word found nowhere else, so a row
+// that names a neighbouring line cannot pass by resembling it.
+
+// propCases is the budget per seed. Small documents diff in microseconds, so a
+// few thousand cases cost less than the package's HTTP tests and run on every
+// `go test ./...` rather than on a fuzzing run nobody remembers to start.
+const propCases = 700
+
+// genBlock is one generated block: the kind decides how its lines are written
+// into the document, and lines are the words a reader would see.
+type genBlock struct {
+ kind string // "heading", "para", "rule", "code"
+ lines []string
+}
+
+// source writes a document, one blank line between blocks — always, because
+// "text" directly above "---" is a setext heading rather than a paragraph and a
+// rule, and the generator is not trying to test goldmark.
+func source(blocks []genBlock) string {
+ var b strings.Builder
+ for i, blk := range blocks {
+ if i > 0 {
+ b.WriteString("\n")
+ }
+ switch blk.kind {
+ case "heading":
+ b.WriteString("# " + blk.lines[0] + "\n")
+ case "rule":
+ b.WriteString("---\n")
+ case "code":
+ b.WriteString("```\n")
+ for _, ln := range blk.lines {
+ b.WriteString(ln + "\n")
+ }
+ b.WriteString("```\n")
+ default:
+ for _, ln := range blk.lines {
+ b.WriteString(ln + "\n")
+ }
+ }
+ }
+ return b.String()
+}
+
+// nonces hands out a word that appears exactly once in the pair of documents,
+// which is what turns "the row renders text from the line it names" into a test
+// an off-by-one cannot survive.
+type nonces struct{ n int }
+
+func (g *nonces) word() string {
+ g.n++
+ return fmt.Sprintf("w%d", g.n)
+}
+
+func (g *nonces) line(rng *rand.Rand) string {
+ words := []string{g.word()}
+ for i := rng.Intn(4); i > 0; i-- {
+ words = append(words, g.word())
+ }
+ rng.Shuffle(len(words), func(i, j int) { words[i], words[j] = words[j], words[i] })
+ return strings.Join(words, " ")
+}
+
+func (g *nonces) block(rng *rand.Rand) genBlock {
+ switch rng.Intn(10) {
+ case 0:
+ return genBlock{kind: "heading", lines: []string{g.line(rng)}}
+ case 1:
+ return genBlock{kind: "rule", lines: []string{"---"}}
+ case 2, 3:
+ blk := genBlock{kind: "code"}
+ for i := rng.Intn(3) + 1; i > 0; i-- {
+ blk.lines = append(blk.lines, g.line(rng))
+ }
+ return blk
+ default:
+ blk := genBlock{kind: "para"}
+ for i := rng.Intn(4) + 1; i > 0; i-- {
+ blk.lines = append(blk.lines, g.line(rng))
+ }
+ return blk
+ }
+}
+
+func (g *nonces) document(rng *rand.Rand) []genBlock {
+ var out []genBlock
+ for i := rng.Intn(5) + 1; i > 0; i-- {
+ out = append(out, g.block(rng))
+ }
+ return out
+}
+
+// mutate is the proposal an agent might have pushed: a rewrap that the differ
+// is designed not to see, a word changed, a block added, removed or moved.
+func (g *nonces) mutate(rng *rand.Rand, in []genBlock) []genBlock {
+ out := append([]genBlock(nil), in...)
+ for i := range out {
+ out[i].lines = append([]string(nil), out[i].lines...)
+ }
+ for n := rng.Intn(3) + 1; n > 0 && len(out) > 0; n-- {
+ at := rng.Intn(len(out))
+ switch rng.Intn(6) {
+ case 0: // rewrap: the same words, different line breaks
+ if out[at].kind != "para" {
+ continue
+ }
+ words := strings.Fields(strings.Join(out[at].lines, " "))
+ var lines []string
+ for len(words) > 0 {
+ take := rng.Intn(len(words)) + 1
+ lines = append(lines, strings.Join(words[:take], " "))
+ words = words[take:]
+ }
+ out[at].lines = lines
+ case 1: // one word replaced, in place
+ i := rng.Intn(len(out[at].lines))
+ words := strings.Fields(out[at].lines[i])
+ if len(words) == 0 || out[at].kind == "rule" {
+ continue
+ }
+ words[rng.Intn(len(words))] = g.word()
+ out[at].lines[i] = strings.Join(words, " ")
+ case 2: // a block appears
+ blk := g.block(rng)
+ out = append(out[:at], append([]genBlock{blk}, out[at:]...)...)
+ case 3: // a block goes away
+ out = append(out[:at], out[at+1:]...)
+ case 4: // a block moves, which is what produces move markers
+ if len(out) < 2 {
+ continue
+ }
+ blk := out[at]
+ out = append(out[:at], out[at+1:]...)
+ to := rng.Intn(len(out) + 1)
+ out = append(out[:to], append([]genBlock{blk}, out[to:]...)...)
+ case 5: // a line appears inside a block
+ if out[at].kind == "rule" || out[at].kind == "heading" {
+ continue
+ }
+ i := rng.Intn(len(out[at].lines) + 1)
+ out[at].lines = append(out[at].lines[:i],
+ append([]string{g.line(rng)}, out[at].lines[i:]...)...)
+ }
+ }
+ return out
+}
+
+// TestRowNumbersNameTheLinesTheyRender is the property. It builds documents and
+// edits from fixed seeds — reproducible, so a failure is a case anyone can
+// replay — and asserts of every rendered row that its text really is on the
+// lines its gutter claims.
+func TestRowNumbersNameTheLinesTheyRender(t *testing.T) {
+ for _, seed := range []int64{1, 7, 1979, 20260805} {
+ rng := rand.New(rand.NewSource(seed))
+ for i := 0; i < propCases; i++ {
+ g := &nonces{}
+ base := source(g.document(rng))
+ proposed := source(g.mutate(rng, parseBack(base)))
+
+ d := prosediff.Compare([]byte(base), []byte(proposed))
+ rows := buildRows(d.Changes, stubInfo(nil, false))
+ checkRowsNameTheirText(t, rows, base, proposed, seed, i)
+ if t.Failed() {
+ return // one reproduction is enough; the rest would be noise
+ }
+ }
+ }
+}
+
+// parseBack recovers the generated blocks from a rendered document, so mutate
+// works on the same structure the differ will see rather than on a shape only
+// the generator knows about.
+func parseBack(src string) []genBlock {
+ var out []genBlock
+ for _, chunk := range strings.Split(strings.TrimRight(src, "\n"), "\n\n") {
+ lines := strings.Split(chunk, "\n")
+ switch {
+ case strings.HasPrefix(lines[0], "# "):
+ out = append(out, genBlock{kind: "heading", lines: []string{strings.TrimPrefix(lines[0], "# ")}})
+ case lines[0] == "---":
+ out = append(out, genBlock{kind: "rule", lines: []string{"---"}})
+ case lines[0] == "```":
+ out = append(out, genBlock{kind: "code", lines: lines[1 : len(lines)-1]})
+ default:
+ out = append(out, genBlock{kind: "para", lines: lines})
+ }
+ }
+ return out
+}
+
+func checkRowsNameTheirText(t *testing.T, rows []diffRow, base, proposed string, seed int64, iter int) {
+ t.Helper()
+ baseLines := strings.Split(base, "\n")
+ propLines := strings.Split(proposed, "\n")
+
+ for _, row := range rows {
+ // A notes row holds no document text, and a move marker's text is the
+ // renderer's own sentence rather than the document's.
+ if row.Kind == rowNotes || row.Note != "" {
+ continue
+ }
+ got := noncesOf(rowPlainText(row))
+ check := func(side string, lines []string, from, to int) {
+ if from == 0 {
+ return
+ }
+ if to < from {
+ to = from
+ }
+ if from < 1 || to > len(lines) {
+ t.Errorf("seed %d case %d: row names %s lines %d–%d of a %d-line revision\nbase:\n%s\nproposed:\n%s",
+ seed, iter, side, from, to, len(lines), base, proposed)
+ return
+ }
+ want := noncesOf(strings.Join(lines[from-1:to], " "))
+ if !inOrderSubsequence(got, want) {
+ t.Errorf("seed %d case %d: row %q claims %s line(s) %d–%d, which hold %q\nbase:\n%s\nproposed:\n%s",
+ seed, iter, rowText(row), side, from, to, strings.Join(lines[from-1:to], " | "), base, proposed)
+ }
+ }
+ check("old", baseLines, row.OldNum, row.OldEnd)
+ check("new", propLines, row.NewNum, row.NewEnd)
+ }
+}
+
+// noncePattern finds the generated words in a string.
+var noncePattern = regexp.MustCompile(`w[0-9]+`)
+
+// noncesOf reduces text to the sequence of generated words in it.
+//
+// The comparison is on the nonces rather than on the tokens because the two
+// questions are separable and only one of them is this test's. Whether a
+// separator lands between two words is prosediff's Span.Space, which has a
+// known defect — an equal run takes its flag from the old revision's token, so
+// an insertion at the head of a block renders as "{+new+}old" with the words
+// run together — and it is a defect in a package this test cannot reach.
+// Whether a row's words are the words of the line it names is this test's whole
+// point, and it survives that defect: two nonces merged into one string still
+// yield both nonces, in order, while a row that reached for a neighbouring line
+// yields the wrong ones. Nonces also let the check ignore the syntax a block
+// legitimately drops — a heading's "#", a fence's backticks.
+func noncesOf(s string) []string {
+ return noncePattern.FindAllString(s, -1)
+}
+
+// rowPlainText is the text a row puts on the page, without the [-…-] / {+…+}
+// markers summarize adds for readability — those are the test's punctuation,
+// and tokenizing them would compare the test against itself.
+func rowPlainText(row diffRow) string {
+ var b strings.Builder
+ for i, s := range row.Spans {
+ if s.Space && i > 0 {
+ b.WriteByte(' ')
+ }
+ b.WriteString(s.Text)
+ }
+ return b.String()
+}
+
+// inOrderSubsequence reports whether every token of sub appears in super, in
+// order. Subsequence rather than equality because a row legitimately drops
+// syntax the line carries — a heading's "#", a list item's marker — and never
+// legitimately adds a word.
+func inOrderSubsequence(sub, super []string) bool {
+ i := 0
+ for _, s := range super {
+ if i < len(sub) && sub[i] == s {
+ i++
+ }
+ }
+ return i == len(sub)
+}
A web/static/diff.js => web/static/diff.js +642 -0
@@ 0,0 1,642 @@
+// diff.js — line selection for the unified prose diff.
+//
+// PROGRESSIVE ENHANCEMENT, NOT AN APPLICATION. Everything the review page must
+// do, it does server-side or in CSS: folding is a checkbox, commenting is a
+// per-block <details class="ph-compose"> holding a plain form POSTing to
+// /<space>/p/<id>/comment. This file adds one thing on top — picking the lines
+// you are talking about — and if it fails to load, nothing above is lost.
+//
+// SELECTION IS BY LINE, ANCHORING IS BY BLOCK. Lines are what the cursor lands
+// on; a line number, though, is worthless the moment the document is edited
+// again, so a comment is stored against its block's content hash (see
+// core.CommentAnchor). The two-level scheme is why the selection is CLAMPED to
+// a single block: a range spanning two blocks would have to be anchored to one
+// of them, and picking one would be a guess presented as a fact. The composer
+// keeps saying which block it will anchor to, and this file only prefixes the
+// lines you actually chose onto that sentence.
+//
+// Markup contract: table.ph-diff with tr.ph-row rows, td.ph-n gutter cells,
+// data-anchor on every commentable row, and a tr.ph-notes[data-anchor] per
+// block holding that block's threads and composer.
+//
+// THE TABLE IS MARKED `ph-js` ONCE IT IS WIRED, and the stylesheet keys one
+// rule off that class: a notes row holding nothing but a shut composer stops
+// being drawn (see "the composer, once JavaScript is in" in scss/main.scss).
+// The server has to render that composer under EVERY block, because with this
+// file absent it is the only way to comment; with this file present it is
+// sixteen identical "Comment on this block" rows cutting the numbered listing
+// into pieces — the same "identical chrome on every block" that got the earlier
+// block-card UI rejected. So the enhancement is: pick the lines, and the
+// composer for their block opens by itself. Both paths below do that.
+//
+// ONE COMPOSER AT A TIME, AND TYPED TEXT IS NEVER HIDDEN. Those two rules
+// settle every "should this close now" question in this file. The first is what
+// keeps the hiding rule above worth having: opening a composer per selection
+// and never closing the last one walks the page back to a comment box under
+// every block within a dozen clicks, which is the state the rule exists to
+// prevent. The second outranks the first wherever they meet — an empty composer
+// is chrome and may be closed, folded away or otherwise tidied up, but a
+// composer holding a half-written critique is the reviewer's own work, and
+// neither this file nor the stylesheet may take it off the screen. A stale
+// empty box is noise; a discarded critique is lost work, and the reviewer has
+// no way of knowing it is still in the DOM. `ph-draft`, below, is how the
+// stylesheet is told which of the two it is looking at.
+
+(function () {
+ "use strict";
+
+ var EN_DASH = "–";
+
+ // Every controller on the page, so a selection in one document's table can
+ // clear the selection in another's: the location hash is global, and two
+ // highlighted ranges would claim to be one.
+ var controllers = [];
+
+ // The composer this file has opened, page-wide rather than per table. The
+ // selection is page-wide already — a mousedown in one document clears the
+ // other's — so the composer that selection opened has to travel with it, or
+ // picking a line in the second document leaves a comment box standing under
+ // the first.
+ var opened = null;
+
+ // Text the reviewer has typed and not yet posted. This is the one question
+ // asked before anything here closes a composer or lets the stylesheet stop
+ // drawing one.
+ function hasDraft(compose) {
+ var body = compose ? compose.querySelector("textarea") : null;
+ return !!(body && body.value.trim() !== "");
+ }
+
+ // `ph-draft` is how the stylesheet learns that a composer is holding words:
+ // a notes row carrying it stays drawn even with the composer shut, and stays
+ // drawn when the fold around it is closed again (see "the composer, once
+ // JavaScript is in" in scss/main.scss).
+ //
+ // CSS could very nearly read the textarea itself, with
+ // `:has(textarea:not(:placeholder-shown))`, and then it could never fall out
+ // of step with the DOM. It is not used because the placeholder lives in
+ // threads.html: the day someone edits that attribute away, the stylesheet
+ // starts hiding drafts and nothing in either file says why. A class this file
+ // sets is a contract between the two files that both of them name.
+ function markDraft(compose) {
+ compose.classList.toggle("ph-draft", hasDraft(compose));
+ }
+
+ // Shut a composer — unless it is holding a draft, in which case it stays open
+ // and stays the reviewer's to dispose of. Returns whether it actually went,
+ // so callers can keep `opened` honest.
+ function dismissComposer(compose) {
+ if (!compose || hasDraft(compose)) {
+ return false;
+ }
+ compose.open = false;
+ return true;
+ }
+
+ // Open one composer and, in the same move, put the last one away. Every
+ // opening goes through here, so "one composer at a time" holds without any
+ // caller having to remember it. A previous composer with a draft in it does
+ // not go: two boxes are on the page then, but one of them has the reviewer's
+ // words in it, which makes it content in the same way a posted thread is.
+ function showComposer(compose) {
+ if (opened && opened !== compose) {
+ dismissComposer(opened);
+ }
+ opened = compose;
+ compose.open = true;
+ }
+
+ // A gutter cell holds either one number ("12") or, for a region-fallback row,
+ // a range ("12–15"). Reading every integer out of it covers both without the
+ // caller having to know which kind of row it is looking at.
+ function cellNumbers(cell) {
+ if (!cell) {
+ return [];
+ }
+ var found = cell.textContent.match(/\d+/g);
+ return found ? found.map(Number) : [];
+ }
+
+ function rowNumbers(row, track) {
+ return cellNumbers(row.querySelector(track === "D" ? ".ph-n-old" : ".ph-n-new"));
+ }
+
+ // The extent of a selection, preferring the new side. A row that exists only
+ // on the old side (a deletion) has no new number at all, so a delete-only
+ // selection is reported on the old track and marked "D" — never renumbered
+ // onto the new side, which would point at a line the reviewer did not pick.
+ function extent(rows) {
+ var news = [];
+ var olds = [];
+ rows.forEach(function (row) {
+ news = news.concat(rowNumbers(row, "L"));
+ olds = olds.concat(rowNumbers(row, "D"));
+ });
+ var used = news.length ? news : olds;
+ if (!used.length) {
+ return null;
+ }
+ return {
+ track: news.length ? "L" : "D",
+ lo: Math.min.apply(null, used),
+ hi: Math.max.apply(null, used)
+ };
+ }
+
+ function hashFor(ext) {
+ var one = ext.track + ext.lo;
+ return "#" + (ext.hi > ext.lo ? one + "-" + ext.track + ext.hi : one);
+ }
+
+ function labelFor(ext) {
+ var what = ext.track === "D" ? "Removed line" : "Line";
+ if (ext.hi > ext.lo) {
+ return what + "s " + ext.lo + EN_DASH + ext.hi;
+ }
+ return what + " " + ext.lo;
+ }
+
+ function parseHash(hash) {
+ var m = /^#([LD])(\d+)(?:-[LD]?(\d+))?$/.exec(hash);
+ if (!m) {
+ return null;
+ }
+ var lo = Number(m[2]);
+ var hi = m[3] === undefined ? lo : Number(m[3]);
+ return { track: m[1], lo: Math.min(lo, hi), hi: Math.max(lo, hi) };
+ }
+
+ // replaceState, not location.hash: assigning the hash would push a history
+ // entry per drag and make the back button undo highlights instead of leaving
+ // the page. The URL still round-trips, which is the link-sharing path.
+ function writeHash(hash) {
+ if (!window.history || !window.history.replaceState) {
+ return;
+ }
+ window.history.replaceState(null, "", hash || window.location.pathname + window.location.search);
+ }
+
+ // One line above the diff saying the gutter is draggable.
+ //
+ // It is the price of hiding the per-block composers: a control that is only
+ // reachable by an interaction nobody has been told about is not reachable.
+ // ONCE PER DIFF, never per block — a hint repeated under every block would be
+ // the very thing it replaced. It is written by this file rather than by the
+ // template because with JavaScript off it would be false: there is no
+ // selection then, and the composers are all still on the page.
+ //
+ // The wording follows what the reader can actually do: without a composer
+ // (they are not the proposal's owner) selecting lines only writes the hash,
+ // which is the link-sharing path, and promising them a comment box would be a
+ // lie.
+ function addHint(table, canComment) {
+ var hint = document.createElement("p");
+ hint.className = "ph-hint";
+ hint.textContent = canComment
+ ? "Drag across the line numbers to comment on those lines — or tab into the diff, walk it with ↑ ↓, and press Enter."
+ : "Drag across the line numbers to select them — or tab into the diff and walk it with ↑ ↓. The address bar keeps the range.";
+ table.parentNode.insertBefore(hint, table);
+ }
+
+ function setup(table) {
+ var rows = Array.prototype.slice.call(table.querySelectorAll("tr.ph-row"));
+ var notes = Array.prototype.slice.call(table.querySelectorAll("tr.ph-notes"));
+ var order = new Map();
+ rows.forEach(function (row, i) {
+ order.set(row, i);
+ });
+
+ var selected = [];
+ var origin = null; // the row the current selection was started from
+ var dragging = false;
+
+ function commentable(row) {
+ return !!(row && row.dataset && row.dataset.anchor);
+ }
+
+ // Rows that carry no anchor (a move-out marker, say) are not part of any
+ // block, so there is nothing a comment on them could attach to.
+ var pickable = rows.filter(commentable);
+
+ function composerFor(anchor) {
+ var row = null;
+ notes.forEach(function (n) {
+ if (n.dataset.anchor === anchor) {
+ row = n;
+ }
+ });
+ return row ? row.querySelector("details.ph-compose") : null;
+ }
+
+ // The range is prefixed as its own node rather than by rewriting the note's
+ // text, because the server-rendered note contains markup (<code>Goals ›
+ // Non-goals</code>) that a textContent assignment would flatten. That is
+ // also why putting it back is a removal of the two nodes this file added
+ // and not a restore from a saved copy of the note's text: writing such a
+ // copy back would have to go through textContent, which would cost the
+ // <code> element the note is restored to.
+ function noteOf(compose) {
+ return compose ? compose.querySelector(".ph-anchor-note") : null;
+ }
+
+ function prefixNote(compose, text) {
+ var note = noteOf(compose);
+ if (!note) {
+ return;
+ }
+ var mark = note.querySelector("strong.ph-sel-range");
+ if (!mark) {
+ mark = document.createElement("strong");
+ mark.className = "ph-sel-range";
+ note.insertBefore(document.createTextNode(" · "), note.firstChild);
+ note.insertBefore(mark, note.firstChild);
+ }
+ mark.textContent = text;
+ }
+
+ function restoreNotes() {
+ table.querySelectorAll(".ph-anchor-note strong.ph-sel-range").forEach(function (mark) {
+ // A composer holding a draft keeps its range: the reviewer is writing
+ // about those lines right now, and the sentence saying which lines they
+ // are is as much part of the unposted comment as the text is. Every
+ // other note goes back to the server's wording.
+ var compose = mark.closest("details.ph-compose");
+ if (compose && hasDraft(compose)) {
+ return;
+ }
+ var sep = mark.nextSibling;
+ if (sep && sep.nodeType === Node.TEXT_NODE) {
+ sep.parentNode.removeChild(sep);
+ }
+ mark.parentNode.removeChild(mark);
+ });
+ }
+
+ // The selection is over: the highlight, the range on the note and the
+ // composer that selection opened all go. The composer is part of it because
+ // it was opened by the selection and by nothing else — leaving it behind is
+ // how the page ends up with a box under every block again.
+ function clear() {
+ selected.forEach(function (row) {
+ row.classList.remove("ph-sel");
+ });
+ selected = [];
+ origin = null;
+ if (dismissComposer(opened)) {
+ opened = null;
+ }
+ restoreNotes();
+ }
+
+ // Mark the run from `a` to `b`, dropping every row that belongs to another
+ // block. This is the clamp, and it is deliberately silent: the reviewer's
+ // drag simply stops adding rows at the block boundary.
+ function mark(a, b) {
+ var anchor = a.dataset.anchor;
+ var i = order.get(a);
+ var j = order.get(b);
+ if (i > j) {
+ var t = i;
+ i = j;
+ j = t;
+ }
+ selected.forEach(function (row) {
+ row.classList.remove("ph-sel");
+ });
+ selected = [];
+ for (var k = i; k <= j; k++) {
+ if (rows[k].dataset.anchor !== anchor) {
+ continue;
+ }
+ rows[k].classList.add("ph-sel");
+ selected.push(rows[k]);
+ }
+ }
+
+ // Called once the selection has settled (mouse up, Enter, shift-click).
+ // Opening the composer is the whole point of selecting: the reviewer picked
+ // lines in order to say something about them.
+ function settle(openComposer) {
+ if (!selected.length) {
+ return;
+ }
+ var ext = extent(selected);
+ // Some rows carry no number on either track — the "paragraph moved here
+ // (was line 43)" marker is one, and it belongs to a block, so it is
+ // selectable and does open that block's composer. It just has no line
+ // range to name. The hash is then LEFT ALONE rather than emptied:
+ // clearing the address is a thing the reviewer asks for with Escape, and
+ // a row with nothing to say about line numbers must not silently throw
+ // away a hash it did not write — which may be a range someone shared, or
+ // may not be this file's at all.
+ if (ext) {
+ writeHash(hashFor(ext));
+ }
+ var compose = composerFor(selected[0].dataset.anchor);
+ if (!compose) {
+ return; // no composer: a reader without write access, link-sharing only
+ }
+ if (ext) {
+ prefixNote(compose, labelFor(ext));
+ }
+ if (!openComposer) {
+ return;
+ }
+ showComposer(compose);
+ compose.scrollIntoView({ block: "nearest" });
+ var body = compose.querySelector("textarea");
+ if (body) {
+ body.focus();
+ }
+ }
+
+ // Roving tabindex: one stop for the whole table rather than one per line.
+ // Tab reaches the diff, the arrows walk it, Enter comments on the line you
+ // stopped at — a gutter with a thousand tab stops in it is not accessible,
+ // it is a trap.
+ pickable.forEach(function (row, i) {
+ row.tabIndex = i === 0 ? 0 : -1;
+ });
+
+ function focusRow(row) {
+ if (!row) {
+ return;
+ }
+ pickable.forEach(function (other) {
+ other.tabIndex = other === row ? 0 : -1;
+ });
+ row.focus();
+ }
+
+ function step(row, delta) {
+ var i = pickable.indexOf(row);
+ if (i < 0) {
+ return null;
+ }
+ return pickable[i + delta] || null;
+ }
+
+ // A folded row cannot be scrolled to or read, so anything that points at
+ // one opens its fold first. The fold is a checkbox, so "open it" is exactly
+ // that — no separate JS state to keep in step with the CSS.
+ //
+ // Nothing here ever CLOSES a fold or refuses to let one close, including
+ // when the reviewer has a composer open inside it. Refusing was considered
+ // — a fold that shuts over an open composer used to take a half-written
+ // comment off the screen with it — and lost: a control that ignores the
+ // click it was given is worse than one whose effect can be seen, and
+ // policing the checkbox would put the fold's state in two places, which is
+ // exactly what this function avoids. The composer survives because a draft
+ // keeps its row drawn through a closed fold (see the `ph-draft` rule in
+ // scss/main.scss); an empty one is chrome and folds away with the lines,
+ // still open, and comes back with them.
+ function reveal(row) {
+ if (!row.classList.contains("ph-folded")) {
+ return;
+ }
+ var body = row.closest("tbody.ph-fold");
+ var cb = body ? body.querySelector(".ph-fold-cb") : null;
+ if (cb) {
+ cb.checked = true;
+ }
+ }
+
+ table.addEventListener("mousedown", function (ev) {
+ var cell = ev.target.closest ? ev.target.closest("td.ph-n") : null;
+ if (!cell || ev.button !== 0) {
+ return;
+ }
+ var row = cell.closest("tr.ph-row");
+ if (!commentable(row)) {
+ return;
+ }
+ // Suppress the browser's own text selection: dragging down the gutter
+ // would otherwise sweep the prose beside it into a native selection.
+ ev.preventDefault();
+ controllers.forEach(function (c) {
+ if (c.table !== table) {
+ c.clear();
+ }
+ });
+ if (ev.shiftKey && origin) {
+ mark(origin, row);
+ } else {
+ restoreNotes();
+ origin = row;
+ mark(row, row);
+ }
+ dragging = true;
+ focusRow(row);
+ });
+
+ table.addEventListener("mousemove", function (ev) {
+ if (!dragging || !origin) {
+ return;
+ }
+ var row = ev.target.closest ? ev.target.closest("tr.ph-row") : null;
+ if (commentable(row)) {
+ mark(origin, row);
+ }
+ });
+
+ // Every keystroke in a composer, so `ph-draft` is true the moment there is
+ // something to lose rather than at the next selection. `input` bubbles, so
+ // one listener per table covers every composer in it.
+ table.addEventListener("input", function (ev) {
+ var compose = ev.target.closest ? ev.target.closest("details.ph-compose") : null;
+ if (compose) {
+ markDraft(compose);
+ }
+ });
+
+ // Closing a composer from its own summary. The stylesheet stops drawing a
+ // notes row whose composer is shut and which has no threads, so that click
+ // takes the summary away with it: the control deletes the control. Read as
+ // a deliberate cancel it becomes honest instead — the selection, the range
+ // on the note and the hash go at the same moment, so the row leaving is
+ // plainly the consequence of the click, and re-selecting the lines brings
+ // it all back. The alternative was to keep the row drawn while its block is
+ // selected; it was dropped because it puts a shut "Comment on this block"
+ // summary back on the page, which is the chrome the hiding rule exists to
+ // remove, and because it leaves the reviewer with a cancel that cancels
+ // nothing.
+ //
+ // The default action has to go: `details` toggles itself AFTER the click is
+ // dispatched, so shutting it here and letting the browser proceed would
+ // toggle it straight back open.
+ //
+ // A composer with a draft in it is not touched — it collapses natively,
+ // keeps its text, and `ph-draft` keeps its row (and so its summary) drawn,
+ // which is the only reason collapsing it is not the same trap.
+ table.addEventListener("click", function (ev) {
+ var summary = ev.target.closest ? ev.target.closest("summary") : null;
+ var compose = summary ? summary.parentNode : null;
+ if (!compose || !compose.classList.contains("ph-compose") || !compose.open) {
+ return; // opening one, or a reply form's summary: not this
+ }
+ if (compose !== opened || hasDraft(compose)) {
+ return; // not the one the selection opened, or it is holding words
+ }
+ ev.preventDefault();
+ compose.open = false;
+ opened = null;
+ clear();
+ writeHash("");
+ });
+
+ // On the document, not the table: a drag that ends past the last row still
+ // has to end, or the next mouse move keeps painting.
+ document.addEventListener("mouseup", function () {
+ if (!dragging) {
+ return;
+ }
+ dragging = false;
+ settle(true);
+ });
+
+ table.addEventListener("keydown", function (ev) {
+ var row = ev.target.closest ? ev.target.closest("tr.ph-row") : null;
+ if (!commentable(row)) {
+ return;
+ }
+ var next = null;
+ if (ev.key === "ArrowDown") {
+ next = step(row, 1);
+ } else if (ev.key === "ArrowUp") {
+ next = step(row, -1);
+ } else if (ev.key === "Enter" || ev.key === " ") {
+ ev.preventDefault();
+ // Enter on a row that is already part of the selection means "comment
+ // on what I picked", not "start again from here" — otherwise a range
+ // built with Shift+Arrow collapses to one line the moment the reviewer
+ // asks to comment on it.
+ if (selected.indexOf(row) < 0) {
+ restoreNotes();
+ origin = row;
+ mark(row, row);
+ }
+ settle(true);
+ return;
+ } else {
+ return;
+ }
+ if (!next) {
+ return;
+ }
+ ev.preventDefault();
+ reveal(next);
+ focusRow(next);
+ if (ev.shiftKey) {
+ // Shift+Arrow with nothing selected yet starts the selection at the row
+ // the reviewer was standing on, the way a text cursor does. Without
+ // this the keyboard path has no way in: it can only extend a selection
+ // that the mouse made.
+ if (!origin) {
+ origin = row;
+ }
+ mark(origin, next);
+ settle(false);
+ }
+ });
+
+ function restoreFromHash() {
+ var want = parseHash(window.location.hash);
+ if (!want) {
+ return false;
+ }
+ var hit = [];
+ var anchor = null;
+ rows.forEach(function (row) {
+ if (!commentable(row)) {
+ return;
+ }
+ var covered = rowNumbers(row, want.track).some(function (n) {
+ return n >= want.lo && n <= want.hi;
+ });
+ if (!covered) {
+ return;
+ }
+ if (anchor === null) {
+ anchor = row.dataset.anchor;
+ }
+ // The clamp applies to a restored selection too: a hash naming lines
+ // that straddle two blocks is not a selection this UI could have made.
+ if (row.dataset.anchor === anchor) {
+ hit.push(row);
+ }
+ });
+ if (!hit.length) {
+ return false;
+ }
+ hit.forEach(reveal);
+ origin = hit[0];
+ mark(hit[0], hit[hit.length - 1]);
+ // The range is put on the composer's note, but the composer is left shut:
+ // arriving from a link is reading, not yet writing.
+ settle(false);
+ hit[0].scrollIntoView({ block: "center" });
+ focusRow(hit[0]);
+ return true;
+ }
+
+ // `ph-draft` is derived from the textarea, so it is read off the textarea
+ // here rather than assumed to start false. A browser may bring back what
+ // was typed into a form when the page is reloaded, and it does that without
+ // firing an input event — Chrome 150 measurably does not restore this
+ // particular form, but the class is supposed to be the answer to "is there
+ // text in there", and one pass over sixteen elements is cheaper than
+ // depending on which browsers restore what.
+ table.querySelectorAll("details.ph-compose").forEach(markDraft);
+
+ // Last, once every listener above is attached and nothing has thrown: the
+ // table declares itself enhanced. This is what lets the stylesheet stop
+ // drawing the per-block composers, so it must not be set on a table whose
+ // selection never got wired — that table's readers would be left with no
+ // way to comment at all. A table with no pickable row (a diff that is one
+ // uncommentable move marker, say) gets neither the class nor the hint,
+ // because there is nothing there to select.
+ if (pickable.length) {
+ table.classList.add("ph-js");
+ addHint(table, !!table.querySelector("details.ph-compose"));
+ }
+
+ var controller = { table: table, clear: clear, restoreFromHash: restoreFromHash };
+ controllers.push(controller);
+ return controller;
+ }
+
+ function init() {
+ document.querySelectorAll("table.ph-diff").forEach(setup);
+
+ // Escape drops the selection: the highlight goes, the note goes back to
+ // what the server wrote, the composer the selection opened shuts unless it
+ // is holding a draft, and the URL stops claiming a range.
+ document.addEventListener("keydown", function (ev) {
+ if (ev.key !== "Escape") {
+ return;
+ }
+ controllers.forEach(function (c) {
+ c.clear();
+ });
+ writeHash("");
+ });
+
+ controllers.some(function (c) {
+ return c.restoreFromHash();
+ });
+ window.addEventListener("hashchange", function () {
+ controllers.forEach(function (c) {
+ c.clear();
+ });
+ controllers.some(function (c) {
+ return c.restoreFromHash();
+ });
+ });
+ }
+
+ if (document.readyState === "loading") {
+ document.addEventListener("DOMContentLoaded", init);
+ } else {
+ init();
+ }
+})();
M web/templates/proposal.html => web/templates/proposal.html +10 -0
@@ 1,3 1,13 @@
+{{/*
+ Line selection in the prose diff. Deferred and unconditional: the script is
+ pure enhancement (folding is a checkbox, commenting is a <details> with a
+ plain form), so nothing on this page waits for it and nothing breaks if it
+ never arrives.
+*/}}
+{{define "head"}}
+<script src="/static/diff.js" defer></script>
+{{end}}
+
{{define "content"}}
{{$p := .Data.Proposal}}
<div class="row">
M web/templates/threads.html => web/templates/threads.html +23 -0
@@ 17,6 17,18 @@
{{if .Resolved}}<span class="badge badge-success">resolved</span>{{end}}
{{if .Note}}<span class="badge badge-warning" title="{{.NoteWhy}}">{{.Note}}</span>{{end}}
{{if .DocPath}}<code class="ph-when">{{.DocPath}}</code>{{end}}
+ {{/*
+ What this thread is attached to, spelled out. The reviewer selected lines;
+ the comment stores a block under a heading path, and by the time anyone
+ reads the thread the heading is usually off screen.
+
+ It comes last and behind a separator because the badge in front of it is a
+ warning about fit while this is a locator. Run together they read as one
+ sentence, and "block edited since Storage › Goals" is a phrase the reader
+ has to un-parse before noticing it is two unrelated facts.
+ */}}
+ {{if .AnchorPath}}<span class="ph-when">·</span>
+ <code class="ph-anchor-path" title="the block this comment anchors to">{{.AnchorPath}} ¶{{.AnchorIndex}}</code>{{end}}
</header>
<div class="ph-comment-body">{{.Body}}</div>
@@ 65,6 77,17 @@
{{if .Compose}}
<details class="ph-form ph-compose">
<summary>Comment on this block</summary>
+ {{/*
+ Selection is by line, anchoring is by block, so the composer states which
+ block it will anchor to before anything is typed. It is rendered here, on the
+ server, rather than filled in by the selection script: with JavaScript off
+ there is no selection and this sentence is the whole truth about where the
+ comment lands. The script prefixes the selected line range onto it as its own
+ element and removes that element again on Escape, so there is nothing here to
+ repeat as a plain-text copy for restoring: writing such a copy back would
+ have to go through textContent, which would flatten the code element below.
+ */}}
+ <p class="ph-anchor-note">Anchors to <code>{{.Compose.AnchorPath}}</code> ¶{{.Compose.AnchorIndex}}</p>
<form method="POST" action="{{.Compose.ActionBase}}/comment">
<input type="hidden" name="doc" value="{{.Compose.DocPath}}">
<input type="hidden" name="block" value="{{.Compose.Ordinal}}">