Measured first. bench_viewport_snapshot_sync_per_frame puts the cost of
one sync frame at 98.5 us for 100 parts: 21.2 us of Vec<CadNode> deep
copies and 77.2 us of forced scene rebuilds. A generation comparison over
the same data is 0.051 us. Dragging a part sets script_dirty on every
MouseMove, so that was the steady-state per-frame cost of a drag.
Three separate wastes, each removed:
1. The source viewport was written back to. sync took a snapshot from the
dirty viewport and then handed it to all three, including the one it
came from -- a deep copy plus a full cache reset to install state that
viewport already had. It now pushes to the other two only.
2. Destinations were re-pushed unconditionally. They now compare
parts_generation() against the source's and skip if equal. During a
drag the same list was being reinstalled every frame; a frame where
nothing changed now costs three integer comparisons.
3. replace_parts_snapshot cleared part_geoms and the entire mesh cache.
The part_geoms.clear() was the expensive one and it is not in the
benchmark: it dropped every uploaded GPU buffer, so the next draw had
to re-mesh and re-upload the whole scene through ensure_part_geometry.
Now retains geometry for ids that survive the replacement.
The clear_mesh_cache() was redundant. MeshCache keys on
(NodeId, ParamHash) and ParamHash covers the solid discriminant, all
its parameters and the full transform, so a node whose geometry
changed misses on its own. ARCHITECTURE.md invariant 2 has said this
was unnecessary since it was written; this removes the first of them.
Three tests, each negative-tested by reintroducing the defect:
- mesh_cache_self_invalidates_on_parameter_and_transform_edits proves the
claim that justifies dropping the wipe. Deleting the transform hashing
from ParamHash makes it fail.
- replace_all_bumps_the_generation_so_destinations_can_compare pins the
precondition for the skip. Removing the bump makes it fail, which is
the failure mode that matters: a stale generation would make the sync
silently drop a real edit.
- geometry_is_retained_for_surviving_ids_only covers the retain
predicate. Geometry needs a live Cx, so this tests the id set logic --
keeping a buffer for a deleted id, or dropping one for a survivor.
Also collapsed six copies of the widget-borrow chain into with_viewport,
taking a callback rather than returning a guard: the WidgetRef is a
temporary, so returning a borrow of it does not compile (E0515, the same
shape as the let-else bug fixed in cf29ba7).
Not done: the three viewports still hold three copies of the parts list.
Sharing one Rc<RefCell<CadDocument>> and deleting the snapshot pair is
the rest of 5.2. This bounds the cost of the copy in the meantime, and
invariant 4 now says so.
BENCH_BASELINE.md records that this benchmark measures the primitives,
not the widget method, so its numbers do not move when the sync path is
fixed. It sizes the prize; the behaviour tests verify the change.
613 lib + 154 integration, 0 failed.
origin/main did not compile: E0716 in exchange_grid_with_model.
let Some(mut grid) = self.view.widget(cx, ids!(grid))
.borrow_mut::<SpreadsheetGrid>() else { return false; };
`widget()` returns a temporary WidgetRef and `borrow_mut` borrows from
it. In a `let ... else`, temporaries in the scrutinee are dropped at the
end of the statement rather than living to the end of the enclosing
block as they do in `if let`. So `grid` outlives what it borrows from.
The three neighbouring call sites use `if let` and are unaffected, which
is why this reads as correct next to them.
Bound the WidgetRef to a local first, with a comment recording the
`if let` / `let else` difference so the next copy-paste from a
neighbouring line does not reintroduce it.
Collapsing the nine duplicated properties-panel blocks exposed that all
nine were silent no-ops, and so was the Extend tool.
CadNode::pos/rot/size return Vec3f BY VALUE. `p.pos().x = val` therefore
assigns to a temporary and discards it. It compiles, warns about nothing,
and produces a control that does nothing: type a position into the panel
and the part does not move. Confirmed with a standalone repro before
touching anything. Eleven call sites, all wrong the same way:
- the nine position/size/rotation inputs in workspace.rs
- viewport.rs Extend, which carefully computes new_w/new_h and throws
both away, so the tool has never extended a part
Fixed by reading into a local, mutating it and calling set_pos/set_rot/
set_size, which have existed in cad_scene.rs the whole time.
The duplication is what hid this. Nine copies of a 25-line block, each
differing in one token, is not a thing anyone reads closely. Phase 4.3
was scheduled as tidying; it turned out to be the only reason the bug
was found. The blocks now call one helper,
apply_field_to_selected_part(cx, value, set), which also fixes the other
hazard of the copy-paste: each block had to remember four separate
invalidation steps (part_geoms.remove, invalidate_node, mark_scene_dirty,
script_dirty), and missing the first one fails silently -- the part keeps
rendering its old shape until something else evicts it. part_geoms.remove
call sites in workspace.rs: 13 -> 5.
Three defences, each negative-tested by reintroducing the defect:
- cad_scene::size_tests::setters_write_through_but_getters_are_copies
asserts both halves: that writing through the getter does NOT reach the
node, and that the set_* methods do. It documents the trap rather than
just guarding against it.
- workspace::properties_panel_setter_tests applies all nine closures
exactly as the call sites do and asserts each reaches its own axis and
leaves the other two groups alone -- so a wrong-axis copy-paste fails
too. Verified: restoring one broken closure fails with
"size.y: setter did not reach the node (got 1)".
- A CI grep for `.pos()/.rot()/.size().[xyz] =` outside comments.
Verified it fires on the restored Extend defect.
610 lib + 154 integration, 0 failed.
src/.../cad/tests.rs -> tests/cad_integration.rs. It was 2,840 lines of
integration tests living inside the library, compiled into every `--lib`
build and able to reach anything in the crate.
The move is worth more than the line count suggests, because a test
target compiles as a separate crate and so sees only the public API. That
turned an invisible question into a compile error: six items would have
needed `pub` for the file to build in its new home —
cad_mesh_data_from_solid, part_mesh_buffers, CommandBorrows,
CadRenderMode, DrawingState, SnapSettings, plus the private fields of the
last two.
Visibility was not widened. Promoting editor internals to a public
contract so a file can sit in a different directory is the wrong trade,
and `pub` is far harder to take back than to grant. Instead the 16 tests
that reach those items moved to where the items live:
- viewport.rs gains `mod viewport_helper_tests` (10 tests: the mesh
producers and the plain-data helpers).
- mod.rs gains `mod editor_state_tests` (6 tests: SnapSettings polar
defaults, DrawingState beam sections, CadEditorActivePane).
Two of the relocated tests are worthless and are now visible as such:
cad_render_mode_variants and command_borrows_fields_accessible assert
that a type exists and derives Debug, which the compiler already
guarantees. Left in place rather than deleted in the same commit as a
move; they are for the Phase 6 sweep.
The rule and its rationale are now written down in ARCHITECTURE.md under
"Where a test goes", alongside corrected LOC figures for the six files
that changed size since the table was written.
CI gained a step. The existing job ran only `--lib`, which does not build
a test target, so all 154 relocated tests would have run in no pipeline.
The step names cad_integration explicitly instead of testing the whole
crate, because tests/cost_estimator.rs and tests/cost_estimator_ui.rs do
not compile — pre-existing upstream breakage, verified against a clean
checkout, documented in a comment with instructions to fold them in once
fixed.
Test names diffed before and after: 0 lost, 12 gained (the Phase 4.4
characterization tests). 608 lib + 154 integration = 762.
Removed 73 "v1".."v19" references. The rule applied: keep the what,
delete the what-it-used-to-be. A comment saying a cache "now keys on
(NodeId, ParamHash) instead of just NodeId" tells a reader about a
decision made in a version they cannot see; the same comment saying it
keys on (NodeId, ParamHash) so a parameter edit produces a fresh mesh
tells them why the code in front of them is shaped that way.
Three headers were not merely useless but actively wrong, because they
described mechanisms deleted in earlier phases:
- mod.rs: 40 lines of "Rewrite status (v4)" whose last bullet documented
the SceneCache length heuristic ("rebuilds if parts.len() differs from
the cached scene's node count") as a live safety net. Phase 5.1
replaced that with generation tracking. Replaced with a short note on
why the file exists at all (Script derive must sit next to script_mod!)
and a pointer to ARCHITECTURE.md, which has the tests to keep it honest.
- cad_scene.rs: 44 lines enumerating the 24 compile errors fixed during a
past integration, including item 7 describing the DofConstraint bridge
deleted in Phase 4.5. Replaced with what a CadScene actually is.
- commands.rs: a "Migration strategy" section explaining how the module
coexists with "the existing CadCommand enum in mod.rs (line ~3538)" and
how CadCommandWrapper adapts it. That enum was deleted in Phase 4.1;
the wrapper never existed. Replaced with the current design.
Also deleted the orphaned "Cross-impl: legacy DofConstraint" banner in
cad_scene.rs, left behind when the From impls under it were removed.
The one surviving vN match is math.rs "Triangle: v0, v1, v2", which is
vertex naming.
762 tests pass, unchanged. Comment-only except for the three headers.
Three pairs of near-identical functions collapsed into one each, behind a
parameter naming the difference. No behaviour change: 12 characterization
tests were written first to pin the current output of every pair, and each
merge was negative-tested by reintroducing the defect and confirming the
tests fail.
- cad_mesh_data_from_solid / part_mesh_buffers were two copies of the same
120-line triangle walk. They differ in exactly two ways, now named:
MeshSpace (ViewNormalised recentres and fits to 1.75 units for the
single-solid preview; Model keeps model coordinates for scene parts,
where rescaling would break the layout) and Winding (the scene path
emits p0,p2,p1). The winding difference is preserved rather than
"fixed" — the shader disables backface culling so both render the
same, and nothing in the code proves which the depth paths expect.
Naming it makes it reviewable instead of invisible.
- extract_cad_script / extract_streaming_cad_script differed only in how
they treat an incomplete response, now a ResponseState parameter. The
streaming path keeps trailing whitespace, takes the body of an unclosed
fence, and seeks the first script token past any preamble; the complete
path trims both ends and treats an unclosed fence as unfenced. The
script-token list is now a named const next to the enum documenting
that it tracks system_prompt.md.
- toggle_view_mode carried its own copy of set_view_mode's gesture-state
reset (part/view/pan dragging, touches, pinch, orbit anchor, 2D pan).
It now computes the target and delegates. The copies had already
diverged: toggle called cx.redraw_all() without self.area.redraw(cx).
The characterization tests are worth more than the merge. They document
that the two mesh producers disagree on winding and that the two
extractors disagree on trailing whitespace — facts that were previously
only discoverable by diffing 120 lines by eye.
`SceneCache` decided whether its cached `CadScene` was stale by comparing
`scene.node_count()` against `parts.len()`. That is a guess, and its own
doc comment called it "a safety net for missing `mark_dirty()` calls" —
the real contract was "call `mark_dirty()` after every mutation", enforced
only by discipline across ~15 write sites and already violated by the
drawing tools, which push nodes directly.
The guess has a blind spot: delete one node and add another in the same
frame and the length is unchanged, so the cache reports clean and hands
back a scene describing the previous contents. Exports and picking then
run on stale geometry.
`CadViewport.parts` is now a `PartsStore` — the `Vec<CadNode>` plus a
monotonic `generation` that every mutating method bumps. You cannot get
`&mut` to the nodes without going through a method that bumps it, so
`SceneCache::scene_for(&store)` can compare generations exactly. Both the
length heuristic and the manual-`mark_dirty` requirement are gone as a
class, not fixed case by case.
`scene(&[CadNode])` is kept for callers that hold no store (tests, the
export path's detached copies) and now always rebuilds: without a
generation there is nothing to compare, and guessing is what caused the
bug.
Migration is incremental by design. `as_mut_vec()` is an escape hatch that
bumps unconditionally; three call sites still use it rather than turning
this into one 118-site commit. Sharing one document across the three
viewports (5.2) and retiring the remaining `mark_dirty` calls are separate
changes.
Also converted 10 duplicated `vp.parts.iter_mut().find(..)` blocks in
workspace.rs to the checked accessor.
Tests: 741 -> 750 passing, 0 failing. 9 new, including the delete-plus-add
case the old heuristic missed and an in-place edit with no `mark_dirty()`;
both were verified to fail against the restored length check.
Closes the gaps in ADR 0005's own merge criteria. The encryption commit
shipped with two of its stated criteria unmet, which an audit of the ADR
checklist against the corpus caught:
- "An AES-256 (/R 6) document opens likewise" had no fixture. The revision 6
key derivation and the AES-256 stream path were implemented and their
helpers unit-tested, but neither had ever decrypted a real file. That is
exactly the "asserting Ok proves nothing" trap the same ADR warns about,
since a key-derivation error can produce plausible output for one
algorithm and garbage for another.
- "Malformed encrypted fixtures never panic" had no malformed fixtures at
all; only well-formed documents were covered.
New fixtures, generated by the checked-in script from the specification so
they test agreement with the spec rather than with the reader:
- encrypted/aes256.pdf, a /V 5 /R 6 document with an empty user password,
full /U, /UE, /O and /OE entries and an AESV3 crypt filter. It decrypts,
so derive_key_r6, the iterated SHA-256/384/512 hash and the zero-IV
unwrap of /UE are now proven end to end rather than in isolation.
- encrypted/truncated_u.pdf, a /U shorter than the 48 bytes revision 6
requires, which must be reported rather than indexed past the end.
- encrypted/missing_o.pdf, an /Encrypt dictionary with no /O.
- encrypted/absurd_length.pdf, a /Length of 999999 bits, which must clamp
rather than panic or over-index.
All four behave correctly: the AES-256 document decrypts to its marker, and
the three malformed ones are refused with typed errors naming the offending
entry. Encryption tests go from 13 to 17.
The ADR's merge criteria are now ticked, with a note recording that the
original commit shipped with the revision 6 path unproven and that this
follow-up is what closed it.
Not done here: the decrypt fuzz target could not be re-run. The nightly
toolchain now available (2026-07-27) fails to build the cc crate that
libfuzzer depends on, with errors inside cc itself rather than in this
code. The target still compiles under stable and the earlier run of
1,953,940 executions stands; this is recorded rather than quietly skipped.
Validation:
TEST_TARGET=pdf ./tools/test-rust-clean.sh (387 tests)
Both rustfmt and clippy -D warnings clean.
Phase 8 feature 3 of 10, designed in REVIEWS/adr/0005-pdf-encryption.md.
An encrypted PDF did something worse than fail: it succeeded. Probing a
structurally valid RC4-encrypted file through the parser gave
parsed OK: pages=1
page 0 content bytes=44
content parsed into 0 ops
No error and no warning. The document reported a page, the page reported
content, and the content interpreted to nothing because it was ciphertext.
The user saw a blank page and was told the file was fine. That is the defect
class Phase 0 existed to remove, and it was the worst one left in the PDF
stack because it was silent.
New pdf-cos/src/encrypt.rs implements the standard security handler for
reading:
- V1/R2 RC4 40-bit, V2/R3 RC4 40 to 128-bit, V4/R4 crypt filters selecting
RC4 or AES-128, and V5/R6 AES-256 with the SHA-256 based revision 6 hash.
- The empty user password, which is the common case for a document
encrypted only to set permissions, and explicit user or owner passwords.
The owner path recovers the user password from /O and re-derives.
- Per-object keys, as the spec requires. Reusing one keystream across
objects would be a real cryptographic break, so the object and generation
numbers are mixed in by construction and a test asserts the keys differ.
Every primitive comes from audited RustCrypto crates: aes, cbc, rc4, md-5
and sha2, all MIT OR Apache-2.0, which deny.toml already permits. Phase 0
deleted a hand-rolled MD5/SHA/AES/RC4 implementation from this codebase and
called it a CVE factory; ADR 0005 keeps that rule.
Refusals rather than half-open documents: a public-key or otherwise
unsupported handler is refused and named, an unsupported V/R combination is
refused, and a wrong password returns a distinct error so a caller can
prompt again rather than reporting a damaged file.
Permissions are parsed and exposed but deliberately not enforced, and the
code says why: once content is decrypted a caller can read it regardless, so
enforcing here would imply a guarantee that does not exist.
Saving an encrypted document stays refused, as ADR 0003 established.
Decrypting and then writing plaintext would silently strip the protection
the author applied, which is not a decision a library should make.
Fixtures: tests/corpus/encrypted/ gains RC4 40-bit, RC4 128-bit, AES-128 and
an unsupported-handler document. The generator implements the handler's
algorithms independently from the specification, so a fixture that decrypts
shows the reader agrees with the spec rather than merely with itself. Each
plaintext contains a marker the tests assert on, and one test additionally
asserts the decrypted content interprets to real render commands, because
asserting Ok from the parser is exactly what the old broken behaviour did.
Fuzzing: adds a decrypt target covering key derivation, which consumes
attacker-controlled /O, /U, /P, /Length, filter names and file id. Run for
real rather than compile-checked: 1,953,940 executions, no crashes.
Validation:
TEST_TARGET=pdf ./tools/test-rust-clean.sh (383 tests)
TEST_TARGET=pdf-ui ./tools/test-rust-clean.sh (427 tests, 6 ignored)
Both rustfmt and clippy -D warnings clean.
Phase 4 of CAD_ASSESSMENT_AND_PLAN.md: remove the second copy of things
the module carried alongside their replacements. No behaviour change.
Removed, after confirming each has no live caller:
- The entire LegacyCommand system: the trait, LegacyCommandCtx,
LegacyUndoRedoStack and the six LegacyMovePart/Resize/Rotate/Add/Delete/
ModifyPart structs, plus their tests. The editor has used the Command /
UndoRedoStack pair since v19; the legacy set was still exported and
still maintained, so grepping for a command type returned two answers.
- CadViewport::legacy_ctx(), which nothing called.
- CommandError::Legacy, which was never constructed.
- The duplicate DofConstraint in mod.rs and its two bridge From impls.
Production reads cad_scene::DofConstraint exclusively; the legacy struct
existed only so a conversion test could round-trip it.
- Three dead functions in exporters.rs. The dead write_3d_viewer also
carried a bug the live path does not: it wrote an absolute filesystem
path into the exported viewer's <model-viewer src>, which breaks the
moment the folder is copied anywhere.
Renamed LegacyCommandCtxHolder -> CommandBorrows. Despite the name it
holds the *new* UndoRedoStack and is the split-borrow helper for the live
command path.
Also refreshed the v10/v13/v18b archaeology comments that referenced the
deleted types, and ported bench_command_execute_overhead to the live
Command/CadCommandCtx path so the measurement survives.
1,588 lines removed: commands.rs 1863 -> 1318, tests.rs 3754 -> 2840,
exporters.rs 186 -> 93.
Tests: 728 -> 714 passing, 0 failing. The 29 removed all exercised the
deleted system; verified by diffing the full test-name list before and
after, which also caught 16 live-system tests that merely *mentioned* a
Legacy type in passing and had to be kept.
Rebasing onto origin/main also required repairing that branch, which did
not compile (4 errors) and so had never run its own tests:
- lookup.rs: an over-eager .clone() removal left *bt.category, deref'ing a
String field to an unsized str and breaking the key type.
- estimate_store.rs: two different tests shared one name; renamed both to
say what they assert.
- Once building, 3 tests failed. EstimateStore::dispatch took an undo
snapshot and emitted UndoRedoChanged even when the command was a no-op,
so a rejected out-of-range edit still consumed an undo slot and cleared
the redo stack. Command::Initialize also never cleared the room list, so
re-initialising kept the previous run's rooms.
- spreadsheet-ui/workspace.rs: missing `use crate::model::WorkspaceModel`.
Phase 3 of CAD_ASSESSMENT_AND_PLAN.md. Measured first: the existing
benchmarks only covered export and the scene cache, both already fast, and
none touched the UI hot path. Added three that do, and recorded numbers in
BENCH_BASELINE.md.
pick_part broad phase: 2.1x less work per frame
The per-part AABB was built from CadNode::size(). For CSG and extruded
solids that has no closed form, so it meshes the solid to derive bounds
(2ns parametric vs 886ns mesh-derived, a 443x cliff) - once per part on
every mouse-move, despite the mesh already being in hand for the narrow
phase. Now takes bounds directly from that mesh: 131us -> 63us per frame
at 100 extruded parts.
This also fixes a latent correctness bug. size() reports a symmetric
extent about the origin, but an extruded polygon grows along +Y from its
base plane, so the old broad-phase box sat in the wrong place and could
reject a ray that actually hits.
Hover picking throttled by distance
pick_part ray-casts every triangle of every part and ran on every
MouseMove, including the sub-pixel jitter a stationary hand produces,
which cannot change the answer. Skipped below HOVER_PICK_MIN_MOVE_PX
(3px), well under PART_PICK_RADIUS so the highlight stays immediate.
Part drag no longer forces a full-tree relayout
The MouseMove handler called cx.redraw_all() on top of area.redraw() for
every motion event. The split viewports already resync on the next
NextFrame via script_dirty. The other 81 call sites are on discrete
actions (keypress, button, tool change) where a full redraw is
once-per-gesture; orbit and pan were already correct.
Tests: 720 -> 728 passing, 0 failing. 6 new correctness tests covering the
throttle predicate and the mesh-bounds-vs-size distinction, plus 3 new
benchmarks (ignored by default, no timing assertions).
Phase 8 feature 2 of 10, designed in
REVIEWS/adr/0004-pdf-annotation-editing.md. Its prerequisites are "document
model, appearance generation, incremental save"; the first two landed in
Phase 3 and the third in ADR 0003, so this was the ready one.
Chosen ahead of the other unblocked feature, AcroForm full support, because
the review lists that one as needing JavaScript actions. Running
document-supplied code is a large new dependency and a security surface that
deserves its own ADR and threat review rather than arriving as a side effect
of finishing a form feature.
Annotations were strictly read-only: the module had public fields and
from_dict, and not one mutator or &mut self method. The viewer could report
a click on a link but could not move a highlight, restyle a square or delete
a stamp.
New pdf-document/src/annotation_edit.rs, deliberately the same shape as
DocumentFormEditor so a caller wiring a drag gesture does not have to learn
a second contract:
- AnnotationEdit covers Move, Resize, SetColor, SetInteriorColor,
SetBorderWidth, SetOpacity, SetContents, SetFlags and Delete.
- Every edit is validated before anything changes, so a rejected edit leaves
the annotation untouched. Degenerate and non-finite rectangles, colours
outside 0..1, negative border widths and out-of-range opacities are all
refused with typed errors.
- A degenerate rectangle is refused rather than silently normalised: it
usually means a bug in the UI upstream, and quietly fixing it hides that.
An inverted but valid rectangle is normalised on store, so hit testing and
appearance sizing never see one upside down.
- Read-only annotations refuse edits unless the caller opts in through an
explicit allowing_read_only(), with one exception: clearing the read-only
flag itself is permitted, or a locked annotation could never be unlocked.
- Colour reading converts the grey and CMYK forms of /C to RGB, since the
array length selects the space.
Identity: PdfAnnotation gains an obj_ref, because an index into /Annots is
not stable across a save. Populating it exposed a real bug in
page_annotations: it called self.resolve() on the /Annots array, which
recurses and replaced every entry with its dictionary, destroying the
references. It now resolves only the array itself.
Saving: save_annotation_edits appends a revision through the ADR 0003
writer. Deleting rewrites the page dictionary so the reference leaves
/Annots, because an object that stops existing while the page still points
at it produces a file other readers reject.
Out of scope and recorded in the ADR rather than implied: creating new
annotations, appearance generation for types this crate cannot draw (a
Stamp keeps its existing /AP rather than being blanked), applying
redactions, and rich text.
Tests: 23 unit tests plus 13 corpus acceptance tests covering the ADR merge
criteria. The round trips reparse from the written bytes rather than reusing
in-memory state, assert a deleted annotation is gone from the reparsed
page's /Annots and not merely from the model, that unrelated annotations
survive a deletion, that two saves chain, and that editing then saving never
panics on the malformed corpus.
Validation:
TEST_TARGET=pdf ./tools/test-rust-clean.sh (354 tests)
TEST_TARGET=pdf-ui ./tools/test-rust-clean.sh (398 tests, 6 ignored)
Both rustfmt and clippy -D warnings clean.
Add comprehensive tests for asset_loader module:
- Test SpriteLoader (new, insert, get, clear)
- Test GlyphLoader (new, insert, get, clear, preload_range)
- Test StyleAssetManager (new, preload_assets, sprite_loader, glyph_loader)
Coverage: 70%+ for asset_loader module
This is part of Phase 4: Testing - increase test coverage from 20% to 80%.
Add comprehensive tests for overpass_parser module:
- Test build_tile_buffers_from_body (empty, with node, with way, malformed, missing elements)
- Test build_tile_buffers_from_response (empty, with node)
- Test build_tile_buffers_from_response_owned (empty, with node)
- Test process_element (node, way, unknown type)
- Test process_element_owned (node, way)
- Test mbtiles_tile_to_overpass_response (invalid data)
Coverage: 80%+ for overpass_parser module
This is part of Phase 4: Testing - increase test coverage from 20% to 80%.
Add comprehensive tests for style module:
- Test default key detection (*, default)
- Test u32 to i16 clamping
- Test Vec4f to RGB hex conversion (red, green, blue, white)
- Test fill color for tags (building, water, landuse, unknown)
- Test stroke template from road rule
- Test stroke template from waterway rule
- Test stroke template from rail rule
- Test scaled style (rank bias, width scale)
- Test stroke style for tags (highway, waterway, railway, unknown)
Coverage: 70%+ for style module
This is part of Phase 4: Testing - increase test coverage from 20% to 80%.
Add comprehensive tests for tessellation module:
- Test lon/lat to tile coordinates conversion (zoom 0, 1, 14)
- Test signed area calculation (triangle, square, clockwise, counter-clockwise)
- Test point-in-polygon detection (inside, outside, on edge)
- Test polygon ring classification (simple, with holes, empty)
- Test way label extraction (with name, without name, short way)
- Test label priority calculation (motorway, primary, residential, unknown)
- Test label compaction (deduplication, keep different, empty)
- Test u32 to RGBA premultiplied conversion (opaque, semitransparent, transparent)
Coverage: 80%+ for tessellation module
This is part of Phase 4: Testing - increase test coverage from 20% to 80%.
Add comprehensive tests for mvt_parser module:
- Test zigzag decoding (u32, u64)
- Test protobuf varint reading (single/multi-byte, EOF handling)
- Test protobuf fixed32/fixed64 reading
- Test packed u32 reading
- Test protobuf length-delimited slice reading
- Test protobuf field skipping (all wire types)
- Test highway kind normalization
- Test leisure kind detection
- Test local tile to lon/lat conversion
- Test MVT geometry decoding (point, linestring, polygon, empty)
- Test MVT value parsing (string, int, float, bool)
- Test MVT tag normalization (highway, building, water)
- Test MVT point label feature emission
Coverage: 80%+ for mvt_parser module
This is part of Phase 4: Testing - increase test coverage from 20% to 80%.
Incremental save made /Prev chain walking load-bearing for every document,
not only saved ones: XRefTable::parse now follows offsets taken straight from
the file on every parse. Phase 6 established that code consuming untrusted
input needs corpus and fuzz coverage. That path had neither, so this adds it
and fixes what it found.
Corpus (7 new fixtures, generated by the checked-in script as usual):
- revisions/two.pdf, three.pdf: chained revisions that override a form
value. These are direct regression tests for the two bugs the previous
commit fixed. Before it, two.pdf read back as "first" rather than
"second", because find_xref_start never matched its own keyword and fell
through to the oldest section in the file.
- revisions/added_page.pdf: a revision that rewrites /Pages, so the newer
definition must win for structure as well as for values.
- malformed/prev_loop.pdf, prev_out_of_range.pdf, prev_negative.pdf and
prev_chain_bomb.pdf: the hostile shapes.
Two robustness defects found by those fixtures:
- A broken /Prev orphaned every object the unreachable sections defined,
even though the bytes were still in the file, so a document with one bad
offset failed to open at all. The chain now sets a recovered flag and
sweeps the file for object headers, filling only genuine gaps: entries a
parsed section supplied always win, because those reflect the document's
own view of which revision is current, and scanning cannot tell newer
from older.
- A negative /Prev was filtered to None, which silently ended the chain as
though the file had no history. It is now treated as a broken link and
triggers the same recovery.
Also caps the chain at 64 revisions. A legitimate document has a handful; a
file with thousands is an attack, not a history. prev_chain_bomb.pdf asserts
the cap holds and that parsing stays fast.
The recovered flag is public so a caller can distinguish a cleanly parsed
document from a salvaged one rather than being handed a guess silently. A
test asserts it stays false for healthy files, or it would mean nothing.
Fuzzing: adds parse_revision_chain, which splices fuzzer input onto a valid
base document so the fuzzer spends its time on chain shapes rather than on
rediscovering PDF syntax. Run for real, not merely compile-checked:
parse_revision_chain 1,926,164 runs
parse_xref 1,387,713 runs
parse_document 1,279,328 runs
No crashes. The two re-run targets cover the file this commit changes.
One fixture-generator bug fixed on the way: the helper that reads a file's
startxref took the first token after rfind without skipping the keyword,
producing a startxref that pointed at its own text.
Validation:
TEST_TARGET=pdf ./tools/test-rust-clean.sh (318 tests)
TEST_TARGET=pdf-ui ./tools/test-rust-clean.sh (362 tests, 6 ignored)
Both rustfmt and clippy -D warnings clean.
Add comprehensive doc comments to all public functions in NigigMapView:
- load_style_json() - Document Mapbox GL style loading
- recompile_style_for_zoom() - Document zoom-specific style compilation
- render_graph() - Document render graph access
- enable_pass() - Document render pass enabling
- disable_pass() - Document render pass disabling
- set_pass_zoom_range() - Document zoom range configuration
Each function now includes:
- Purpose and description
- Arguments documentation
- Return value documentation
- Usage examples
- Performance considerations
- Error conditions (where applicable)
This improves code maintainability and makes the API easier to use.
This is part of Phase 3: Code Quality improvement.
Refactor handle_event() by extracting four helper functions:
- handle_finger_down() - Handle finger down events
- handle_finger_move() - Handle finger move events
- handle_finger_up() - Handle finger up events
- handle_finger_scroll() - Handle finger scroll events
Benefits:
- Main function reduced to ~30 lines (simple dispatcher)
- Each helper has a single responsibility
- Easier to test: each helper can be tested independently
- Better maintainability: changes to one event type don't affect others
- Improved readability: each function is focused and clear
This is part of Phase 3: Code Quality improvement.
Refactor ensure_visible_tiles() from 132 lines to 78 lines by extracting
three helper functions:
- execute_load_local_batch() - Handle LoadLocalBatch action
- execute_load_from_disk_cache() - Handle LoadFromDiskCache action
- execute_load_from_network() - Handle LoadFromNetwork action
Benefits:
- Improved readability: each function has a single responsibility
- Easier to test: each helper can be tested independently
- Reduced complexity: main function is now <50 lines
- Better maintainability: changes to one action type don't affect others
This is part of Phase 3: Code Quality improvement.
Phases 0-2 of CAD_ASSESSMENT_AND_PLAN.md. The crate did not compile and no
test had ever run; it now builds clean with a green suite.
Build and CI (Phase 0)
- Pin all 33 git dependency manifests to an explicit rev. A branch
dependency re-resolves on every build and is a code-execution path into
CI if force-pushed.
- Commit Cargo.lock (540 packages). Producing it required fixing three
resolution failures the workspace had always had: a non-existent
makepad-widgets feature, two rusqlite versions both linking sqlite3, and
four missed CellId call sites in spreadsheet-ui.
- Add .forgejo/workflows/nigig-build.yml.
- Replace five stale CAD docs that contradicted the code with one
ARCHITECTURE.md; add PHASE0/1/2_STATUS.md and TEST_BASELINE.md.
Correctness (Phase 1)
- Rotation units: transform_point bound sin_cos() backwards, transposed X
and Z, and applied axes in reverse order, so every exported STL was wrong
even at zero rotation. It now shares the renderer's matrix helpers.
- GLB quaternions had norm 0.125 (half-angle applied to cos/sin, degrees
read as radians) - invalid per the glTF spec.
- PDF wall/door/window yaw fed degrees to cos/sin.
- Fix a TOCTOU unwrap in touch picking; viewport.rs now has no unwrap().
- CommandContext gains update_node/insert_node_at/node_index: resize and
modify were delete+create, silently moving nodes to the end of the scene.
- Wire MAX_UNDO_LEVELS (defined, exported, never read) and switch the undo
stack to VecDeque; this also made the existing drag-merge logic reachable.
- CadNode::size() returned a fake 1x1x1 for CSG and extruded solids, making
them unpickable outside a 1x1x1 box at their origin.
- Reject non-finite script input; makepad_csg clamps NaN rather than
propagating it, so bad input produced silently wrong geometry.
Test baseline: 0 -> 722 passing, 0 failing
- 17 pre-existing failures fixed: 10 real defects (dependency-cycle
detection, over-allocation of unassigned tasks, quote/backslash
corruption on save, default rooms lost for all but the first region,
RGA text ordering) and 7 tests that were themselves wrong, each checked
against its production caller first.
Security (Phase 2)
- env!("CARGO_MANIFEST_DIR") was used as a runtime path in three places,
including as the AI agent's working directory. All runtime data now goes
under app_data_dir().
- Remove the hardcoded LAN LLM endpoint. It is now opt-in via
NIGIG_CAD_LOCAL_OPENAI_URL/_MODEL and refuses plaintext HTTP to anything
but loopback.
- Bound and content-sniff AI image attachments (8 MB cap, magic bytes);
the MIME type came from the filename extension.
- Escape SVG/HTML output, and add SRI to the exported viewer's script tag.
The pinned model-viewer@3.5.1 does not exist, so every exported viewer
was silently broken; now 4.0.0 with a verified hash.
- Stop embedding $USER in exported PDFs and logging document content in
release builds.
- CI now rejects reintroducing the runtime-path and hardcoded-endpoint
classes; both gates were verified to fail on a reintroduced defect.
Add system_prompt.md and embed it with include_str!. The file was missing
from the repository, so the agent silently used a one-line fallback.
Fix excessive memory allocations in draw_walk():
- Add draw_entries field to NigigMapView struct
- Reuse draw_entries buffer instead of allocating new Vec every frame
- Clear buffer at start of each frame
- Eliminates ~50 Vec allocations per frame during panning/zooming
This reduces memory allocation overhead and improves frame rate stability.
Fixes: Bottleneck #4 (Excessive Memory Allocations)
