feat(viewer): make Semantic Passport movable - #254
Merged
Merged
Conversation
…page Fixes tt-a1i#102. `readLimited(page.response, MAX_HTML_BYTES)` counted the entire HTML response against the 256 KiB cap, so a page with a tiny <head> (a handful of icon <link> tags) failed with "brand asset is too large" whenever its total body -- content the icon scan never even looks at -- pushed past the cap. Confirmed against the issue's own repro: `brands capture https://xquik.com` failed on main, succeeds after this change. Replace the html-page read with readHtmlHead(): stream the response and stop as soon as `</head>` appears in what's been read so far, before ever counting or downloading the rest of the body. Only fails closed with the existing "brand asset is too large" error if the byte budget runs out before a head close is seen -- same failure mode as before for pages whose head itself is huge or missing, per the issue's explicit requirement to keep that path failing closed. readLimited itself is unchanged and still guards image bytes. Regression coverage (three CLI-level tests, per CONTRIBUTING's "behavioral fixes need a failing regression test" -- verified failing on main via `git stash` before writing the fix): - a small head followed by a >256 KiB body now captures its icon; - a `</head>` split across two response chunks is still found; - a head that alone exceeds the cap (no `</head>` within budget) still fails closed with the same error and exit code. `npm test` (archify/): 729 passed, 0 failed, 16 skipped (real-Chrome visual-check tests, gated behind ARCHIFY_CHROME which isn't set here).
archify/renderers/shared/brand-marks.mjs changed in the previous commit, but archify.zip -- the distributable Skill package -- was not rebuilt, so it still shipped the old whole-page-capped readLimited() call. Installed Skill users would not have received the fix. Rebuilt with the canonical Node 22 toolchain via scripts/build-zip.sh. Verified: - scripts/build-zip.sh /tmp/fresh.zip; cmp -s /tmp/fresh.zip archify.zip -- exact byte match (the same comparison the zip-freshness CI job runs), confirming the build is both fresh and reproducible. - npm run check:brand-marks / check:validators -- clean, so the archive isn't stale because of an unrelated generated-artifact drift. - npm test (archify/) -- 841 passed, 0 failed, 27 skipped (unchanged). - package-smoke equivalent: unzipped archify.zip and ran scripts/package-smoke.mjs against the extracted package directly, matching the CI package-smoke job -- passed. Also rebased this branch onto the current main (several commits had landed, including the zip determinism work in tt-a1i#99-era commits this rebuild depends on) before rebuilding, so the archive reflects both this fix and everything else currently on main.
Nothing bounded an edge label rect against the viewBox on the fixed canvases. The SVG canvas clips whatever overhangs it, so architecture, sequence, data-flow, lifecycle, and fixed-v1 workflow diagrams could ship truncated label text while `validate --quality showcase` still reported 9/9 artifact checks with 0 errors: those checks read the emitted markup, and clipped text is still well-formed markup. Where the canvas is derived, the fix is to size it correctly rather than to report the author: architecture's auto viewBox now covers connection label rects, exactly as the readable-v2 workflow compiler already grows its canvas around pinned labels. Every checked-in architecture diagram renders byte-for- byte identically, because max() can only grow a bbox and their labels were already inside it. What remains is an authored viewBox and the origin side, which growth cannot reach, and that is what the new showcase rule reports — from the renderers and, for artifacts it did not produce, from `check`. The repair hints had the mirror problem: they were derived from the obstacle alone, so the validator could answer a label overlap with a fix that does not repair the document. A hint is now emitted only if applying it works. The absolute form is nudged along x and drops a vertical placement it cannot fit rather than clamping it back onto the obstacle. The relative form is measured from the document's own labelDx/labelDy against the unrounded anchor, is withheld while an authored labelAt outranks it, and is withheld when integer values cannot land the rect inside. Suggested values are stated as replacements for the authored field, never as increments. Other label surfaces, such as sequence segment titles, remain unchecked and are out of scope here. Keep the rule showcase-only: a standard document authored before it exists may overhang by a few pixels, and failing it there would break compatibility instead of repairing a diagram. Rebuild archify.zip and the Gallery: the packaged renderer bytes changed and published receipts carry the new labelCanvasOverflowIssues metric. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…abel's own rect Review follow-up for the two parity-contract violations reported on the label containment work. The standalone checker kept only width and height of the SVG viewBox, so containment was measured against [0, width] on artifacts it did not produce. A legal non-zero min-x/min-y therefore produced both failure modes at once: a label clipped past the origin edge passed 9/9 with exit 0, and a label sitting inside the offset canvas was rejected as overflow. `check` now hands all four numbers to collectLabelCanvasOverflow, which still accepts the renderers' two-value origin-zero form unchanged — the schema keeps meta.viewBox at [width, height], so renderer output stays byte-identical — and the issue record reports the origin alongside the size whenever it is non-zero. The repair helper had hard-coded its above-obstacle anchor as `obstacle.y - 4`, which silently encodes a 14px single-line rect at the -11 anchor offset. Applied to the 27px two-line forms (dataflow classification, lifecycle note) the suggested labelAt landed the rect 12px inside the obstacle it named, and the validator answered its own fix with the identical message. Both placements are now derived from the label's own rect, and a candidate whose applied rect fails the callers' own detection call — rectsOverlap at gap -2 — is withheld, so a surviving hint cannot re-raise the problem it repairs. New tests pin positive and negative origins on the check side and round-trip both two-line forms end to end. archify.zip is rebuilt with the canonical toolchain (official Node 22.23.2, zlib 1.3.1). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…repair hints Deep-review follow-up on the containment work; every behavioral defect below was reproduced by execution before it was fixed. Architecture resolved boundary-title font sizes against the label-unaware canvas width and then regrew the auto viewBox after the fact, so validate could pass an artifact that deliver/check rejects with composition/desktop-readability. Routing state is now initialized before the title convergence loop and the auto viewBox is computed once, label-aware, with the post-hoc regrow removed; every checked-in artifact renders byte-identically. suggestLabelObstacleFix filtered placements only against the obstacle it names, so a suggested labelAt could land on a neighboring node and fail validation again when applied. Callers now pass the full obstacle set. Its no-fix fallback also overclaimed: only the two vertical slots are ever tried, so the message now distinguishes "both vertical slots are blocked" from "wider than the canvas" instead of asserting that no fix exists. Also hardened while in the area: collectLabelCanvasOverflow rejects negative-size rects instead of reading a flipped interval as contained; suggestLabelObstacleFix normalizes a four-number viewBox instead of reading the origin pair as the canvas size; the check-side containment message stops advising labelAt/labelDx/labelDy on sequence artifacts whose renderer ignores those fields; formatRect is imported from geometry instead of reimplemented; and the architecture label rect formula lives once in connectionLabelBox instead of three drifting copies. archify.zip is rebuilt with the canonical toolchain. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: ketpatil77 <243740572+ketpatil77@users.noreply.github.com>
Moves the connection-routing block out of render-architecture.mjs into a `createRouter(components, connections)` factory. Function bodies are unchanged; the only edit is that `components` and `connections` arrive as arguments instead of module scope, and each router owns its own path cache and port spread. Why: render-architecture.mjs is a top-level script, so the router is reachable only by running a whole render pass. Anything that needs to ask "what would this route look like?" for a scene it is still deciding - tooling, a test, a future placement pass - currently cannot, and the alternative is reimplementing routeVia and letting the copy drift. The region was already self-contained: 309 lines with exactly two references to outer scope (`components`, and `arch.connections` for the port spread). Ten geometry imports it solely owned move with it, and no symbol it defines is used elsewhere in the file. No behavior change. test/golden.mjs byte-compares fresh renders of every checked-in example against the committed HTML and passes unchanged, which is the property this refactor is asserting. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The renderer payload gained renderers/architecture/routing.mjs, so the committed archive no longer reproduces from tracked inputs. Rebuilt with the canonical Node 22 toolchain via scripts/build-zip.sh. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
fix(renderers): align authored route checks, edge colors and lifecycle focus
…ce-display fix(delta): surface repository provenance changes
fix(ci): share required browser regressions with releases
…e-clearance perf: index label route clearance candidates
refactor(architecture): extract connection routing into routing.mjs
feat(renderers): keep edge labels inside the canvas
tt-a1i
approved these changes
Sep 16, 2026
tt-a1i
left a comment
tt-a1i
left a comment
Owner
There was a problem hiding this comment.
The bounded manual Passport move behavior is implemented in canonical Viewer sources, with native-desktop browser coverage in the shared gate. The layout-stability test race and close-button spacing regression are fixed without weakening assertions. Local focused tests and the complete decoded WebM smoke passed; final CI 35140353293 is successful across all jobs on b6467a8. Independent pointer interaction and limits are recorded in the description. Ready for dev integration and subsequent trial use.
tt-a1i
marked this pull request as ready for review
September 16, 2026 19:32
This was referenced Sep 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem and result
A Semantic Passport can cover the part of a diagram a reader wants to inspect. Add one desktop move handle so readers can place it themselves without changing the diagram or export.
Dragging starts after 3px of movement. Arrow keys move 16px, Shift+Arrow moves 4px, and Home or double-click restores automatic placement. Manual placement remains bounded to the visible reader area, survives switching a single focused node, and resets on close. Narrow/coarse-pointer readers keep automatic placement. No persistent setting or new schema field is added.
Integration and evidence
The original author commits are preserved, with the implementation ported into canonical
viewer/focus.jsandviewer/template.source.html. This branch also contains workflow reader integration #255; both target dev. Final combined head:b6467a8.0e1612a, real Chrome, zero skipped. This includes the three/five-stage workflow reader regressions from fix(workflow): fit implicit vertical stacks without inflating every lane #255.readerLayout.whenStable()fixes that race without relaxing assertions. Desktop tests reuse the existing native Blink fine-pointer fixture, including its capability assertion, so Linux CI exercises dragging rather than silently taking the coarse-pointer fallback.The first integration CI exposed a 17px close-button top gap against the existing 16px limit. Reduced header padding from 1rem to 0.9rem, retained the original assertion, and regenerated affected outputs. All 25 Passport/Focus tests pass on that correction, and the local complete WebM smoke passed, including dismissal, card exports and real video decoding (10 sampled frames, all distinct).
Final-head CI provides the remaining full-suite, platform/package, shared-browser and separate WebM decoding coverage. The focused export test verifies exclusion of the move affordance from export source; it is not by itself a decoded video check. See checks for current status.
This integrates into dev for trial use, with no main promotion or claim of real-task stability.
Fixes #245