Move engineering docs to spec/, resync with the implementation, and group the tree#122
Merged
Merged
Conversation
Merging this PR will not alter performance
Comparing Footnotes
|
masenf
reviewed
Jul 21, 2026
| tests/ # Docs, links, live-preview, and integration tests | ||
| ../ # Public Markdown sources | ||
| ../engineering/ # Internal project docs; never published | ||
| ../../spec/ # Internal project docs; never published |
Contributor
There was a problem hiding this comment.
doesn't seem like this is useful now that spec is in the root
The move in this PR promoted docs/engineering/ to spec/ and CLAUDE.md declares that tree the source of truth. An audit of all 24 documents against the code found the tree had drifted at PR #96: five behavior PRs (#103, #105, #106, #108, #115) had landed with no spec update, and ~100 claims contradicted shipped code. Accuracy: correct claims that no longer match the implementation, including the Rust ABI version (v31 -> v36), the "zero-crate cdylib" claim (png is a real dependency), the pyramid ABI function names, the panic-shield status, the R2/VAO claim in the renderer doc, the WASM/Web-Worker architecture in dossier §3/§8 (superseded by §32's C-ABI cdylib), the count-only tier rule, the matplotlib shim overhead figures (the cited guardrail says +60% at 10k, not +9%), and the Plotly Scattergl 10M figure that appeared as two different numbers from the same run. Coverage: add spec/api/export.md, spec/api/interaction.md and spec/design/wire-protocol.md for three surfaces that had no coverage at all, plus the config.py threshold table, full JS and Rust module inventories, and the hexbin centers-only wire contract that three renderers must expand identically. Structure: group 15 top-level files into api/, design/, matplotlib/, benchmarks/ and process/, and rewrite spec/README.md as a real index (it previously listed only the two SVG assets, leaving 21 of 22 docs unreachable). All 150 consumer references repointed, including the CI --emit-md target and the compat-matrix generator. Guard: verify_sdist.py pinned only 5 top-level spec files, so the sdist could silently lose spec/design/ or spec/assets/ and still pass; it now asserts markdown under every subdirectory and both SVGs.
Greptile SummaryThis PR moves internal engineering documentation into a root-level specification tree. The main changes are:
Confidence Score: 5/5This looks safe to merge. No blocking issues found in the changed code.
What T-Rex did
Important Files Changed
Reviews (1): Last reviewed commit: "Resync spec/ with the implementation and..." | Re-trigger Greptile |
Alek99
marked this pull request as ready for review
July 21, 2026 05:43
Alek99
added a commit
that referenced
this pull request
Jul 21, 2026
Rebasing onto main pulled in #122, which moved engineering docs to spec/ and added spec/api/interaction.md — the declared authority on interaction switches and event payloads — describing the pre-#117 contract. That now contradicts the shipped, tested per-axis implementation. Bring the spec and public docs back in line with the code (docs only, no behavior change): - interaction.md: drop the removed `view_change` switch; add the source toggles (wheel_zoom/box_zoom/zoom_buttons/double_click_reset) and a new §2.1 for the non-boolean axis-policy/zoom-limit keys; rewrite the event surface to {ranges, source, axes, phase, interaction_id} with x0/x1/y0/y1 aliases; fix the source vocabulary, the double-click gate (navigation + double_click_reset, not zoom), and the linked-view semantics (a navigation-locked receiver still follows its group). - wire-protocol.md, chart-grammar.md, reflex-shaped-api.md, events-and-callbacks.md, chart-kind-contract.md: the same view_change -> ranges and switch-removal corrections at their altitude.
Alek99
added a commit
that referenced
this pull request
Jul 21, 2026
* Add axis-scoped zoom and chart examples * Implement pan and zoom configuration contract * Format pan and zoom implementation * Reconcile interaction spec with the per-axis pan/zoom contract Rebasing onto main pulled in #122, which moved engineering docs to spec/ and added spec/api/interaction.md — the declared authority on interaction switches and event payloads — describing the pre-#117 contract. That now contradicts the shipped, tested per-axis implementation. Bring the spec and public docs back in line with the code (docs only, no behavior change): - interaction.md: drop the removed `view_change` switch; add the source toggles (wheel_zoom/box_zoom/zoom_buttons/double_click_reset) and a new §2.1 for the non-boolean axis-policy/zoom-limit keys; rewrite the event surface to {ranges, source, axes, phase, interaction_id} with x0/x1/y0/y1 aliases; fix the source vocabulary, the double-click gate (navigation + double_click_reset, not zoom), and the linked-view semantics (a navigation-locked receiver still follows its group). - wire-protocol.md, chart-grammar.md, reflex-shaped-api.md, events-and-callbacks.md, chart-kind-contract.md: the same view_change -> ranges and switch-removal corrections at their altitude. * Fix wheel-gesture end race and post-rebase CI failures Greptile P1 (verified): a wheel event landing just before the 90 ms end-of-gesture timer let "end" fire — and the apply rAF, queued earlier, then overwrite it in the view-event coalescer — while the final delta was still pending, so a continuous wheel gesture could finish without an "end" carrying the final committed range (spec §11.3/§15). _queueWheelZoom now clears the end timer whenever it queues a delta; only the applying frame re-arms it. CI, browser lifecycle smoke: the new x-only-zoom latency histogram is lognormal, so at wide canvases the center-sampled lit-pixel probe landed entirely in the near-empty tail and read 0. Give the example a home domain of (0, 250) framing the bulk — which also demonstrates domain-as- home from the pan/zoom spec; the tail stays reachable by panning and the default zoom limit clamps zoom-out at this window. Also chunk the smoke's iframe shell to 16 charts per mount: the five examples this PR adds took the static fleet to 21 concurrent WebGL contexts, past Chrome's 16-context LRU cap, which evicted the earliest slots' contexts mid-probe. CI, benchmark methodology: bench_interaction still passed the removed view_change chart kwarg; drop it — xy:view_change DOM events are always emitted and the harness listens on the chart root. Regenerate the example chart assets, which also picks up the #118 density-flicker fixes merged during the rebase (the committed assets still embedded the pre-rebase client). * Lock the wheel-gesture terminal event with a regression test The end-timer cancellation moved to the queue path in b642cec, but nothing covered it. Because view events coalesce into one animation frame, a timer surviving into the next delta does not just fire early: the apply frame queued behind it overwrites the pending "end", so the gesture delivers no terminal event at all and consumers that commit on "end" wait on a stale "update". Drive the interleave in the real client behind a deterministic clock and frame queue, placing a delta 1 ms before the deadline, and assert the consumer-visible `xy:view_change` stream still ends in exactly one "end" carrying the committed range. Verified the test fails against the pre-fix client (endCount == 0). Also null the timer handle alongside clearTimeout, matching how 50_chartview.js pairs them. * Contain pan-locked axes to their home window Excluding an axis from pan_axes while zoom could still navigate it was a hollow guarantee: cursor-anchored zoom is a scaling composed with a translation (dcenter = span * (anchor - 1/2) * (1 - f)), so a zoom-in/out chain at two cursor positions panned the "locked" axis exactly, box zoom translated it in one drag, and the default zoom_limits only cap span, not position. Exclusion now means containment. The shared clamp tightens the positional envelope (axis bounds intersected with the home window) for any axis zoom can navigate but pan cannot, so its window never extends past home on any mutation path -- pan, wheel, box, and button zoom, linked ranges, and programmatic updates. Inside the envelope the axis stays live: plain drag includes contained axes, so a zoomed-in window slides within home and pins flush at the extent, while at home magnification it cannot move at all. With navigation off nothing is contained, keeping read-only follower charts free. Spec: interaction.md gains the containment invariant (§2.2); the design doc folds it through the pipeline, precedence, wheel/box, zoom-limit, programmatic, and linked-charts sections, and notes that action-specific axis policy is only independent because of this clamp. The containment probe drives the real client in headless Chromium with pan_axes=("x",): the wheel-chain escape stays inside home on y while free x genuinely escapes, programmatic sets slide back, a zoomed-in drag moves y within home and pins at the edge, and pan_drag end events report the contained axis. Verified fail-first against the pre-change client.
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
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.
Summary
docs/engineering/to a root-levelspec/directory, and define that tree inCLAUDE.mdas the source of truth that must stay synchronized with the implementationapi/,design/,matplotlib/,benchmarks/andprocess/, and rewritespec/README.mdas a real indexWhy
The engineering documents are internal specifications rather than public docs-site content. Moving them to a top-level
spec/directory makes that role explicit.But promoting a tree to "source of truth" only helps if it is true. Auditing all 24 documents against the implementation found the tree had drifted at PR #96 — five behavior PRs (#103, #105, #106, #108, #115) landed with no spec update. Three architecture documents described systems that no longer exist. Shipping the rename without that resync would have made the tree authoritative and wrong at the same time, so the first contributor to follow it would write wrong code.
Accuracy
Representative corrections, each verified against code:
spec/design/rust-engine.md— ABI v31 → v36; "zero-crate cdylib" →pngis a real dependency; pyramid ABI names (xy_pyramid_tile/_polldo not exist; shipped names are_count/compose/append); the panic shield was listed as pending but landed 29 ABI bumps ago asffi_guardspec/design-dossier.md§3/§8 — described a Rust→WASM/Web-Worker/SharedArrayBuffer core; the shipped core is a C-ABI cdylib loaded by ctypes, as the same document's §32 already said. Reconciled, with the dropped WASM path recorded so the history is not lostspec/design/renderer-architecture.md— R2 claimed "No VAOs" via a_bindScalarAttrthat does not exist;gl.createVertexArray()shipsspec/matplotlib/compat.md— claimed the shim adds "+9% at 10k"; the guardrail it cites as proof says +60%spec/benchmarks/results.md— PlotlyScatterglat 10M appeared as both 54,064 ms and 33,907 ms from the same run; the derived headline multiplier was recomputed from the correct figurety checkgate status, browser gate count, and the CI-enforced bundle budget corrected to match what CI actually doesCoverage
New documents for surfaces with zero prior coverage:
spec/api/export.md— the static export surface shipped in Add export format parity and a unified export API (ENG-10447) #115 (to_image/write_image/write_images, five formats,Engine.autorouting). Records screen/export theme parity as an open gap rather than implying it is solvedspec/api/interaction.md—interaction_config(), the event payloads, linking, and the gesture map (Add pan/zoom interaction switches for static-viewport charts #103/facet_chart: add opt-in linked panel interactions #105)spec/design/wire-protocol.md— the eight client↔Python request kinds and the packed-vs-split first-paint layoutAlso added: the
config.pythreshold table (CLAUDE.md§28 promises every tier decision is recorded in the spec), full JS and Rust module inventories, and the hexbin centers-only wire contract that three renderers must expand identically — previously bound only by code comments, and once the cause of a CI-red payload regression.Structure
spec/README.mdpreviously listed only the two SVG assets, leaving 21 of 22 documents unreachable from any entry point; it is now a full index.scripts/verify_sdist.pypinned only 5 top-levelspec/*.md, so the sdist could have silently lostspec/design/orspec/assets/and still passed. It now asserts markdown under every subdirectory and both SVG snapshots.Validation
Run locally on this branch:
tests/andtests/pyplot)format --checkandcheckboth cleansync_matplotlib_compat.py --checkexits 0 after the generator was repointed to the new location and link depthverify_sdist.pypasses against a fresh sdist; the sdist contains all 27spec/files, verified by diffing its member list against the working tree in both directionscheck_claim_guardrails.pyandverify_ci_workflow.pyexit 0spec/resolves (170 targets, checked withtest -efrom each containing file); zero repo-wide hits for any old spec path or fordocs/engineeringNot verified locally: the benchmark regression gate.
check_regressions.pyneeds fresh bench JSONs from a built engine plus Chrome/CDP, which do not run in this environment — CI has to confirm it. The--emit-mdtarget and artifact upload path inci.ymlwere repointed tospec/benchmarks/metrics.mdand cross-checked againstverify_ci_workflow.py's assertions, and a repo-wide grep confirms no writer still targets the old path, but the gate itself is unproven until CI runs.