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)
Phase 8 of REVIEWS/DART_PDF_VS_MAKEPAD_PDF_GAP_ANALYSIS.md, designed in
REVIEWS/adr/0003-pdf-incremental-save.md.
The review treats Phase 8 as ten independent projects, each needing its own
design doc and merge criteria. Incremental save is taken first because it is
the only one whose prerequisites are already met, it is listed as a
prerequisite by two others (annotation editing and full AcroForm support),
and it closes a real credibility gap: DocumentFormEditor has been able to
edit form fields since Phase 3, and there was no way to save the result.
A grep for a public save API across all four crates returned nothing.
Design decision: append a revision, never rewrite. The original bytes are
copied verbatim and changed objects are appended with a new xref chained
through /Prev. A full rewrite would be easier and wrong: it would silently
discard everything this parser does not yet model (structure trees, optional
content, embedded files), and it would invalidate any signature, foreclosing
a feature listed later in the same phase. ADR 0003 records this in full.
Two latent bugs surfaced while building it, both pre-existing:
- find_xref_start searched with windows(10) for the 9-byte keyword
"startxref", so it never matched. Every parse silently fell through to a
forward scan for the first "xref" in the file. On a single-revision
document that happens to be correct; on an incrementally saved one it is
the *oldest* revision, so a saved edit read back as its pre-edit value.
This had no visible effect before because nothing produced multi-revision
files.
- XRefTable::parse read one section and ignored /Prev entirely, so a
multi-revision document lost every object the earlier revisions defined.
It now walks the chain newest-first, keeping the first definition of each
object, with a visited set against /Prev loops and bounds checks on the
offsets, which come from the file and cannot be trusted. A bad link ends
the chain instead of indexing out of bounds.
The xref unit fixture claimed startxref 408 in a 191-byte file and only ever
passed because of the windows(10) defect; it is corrected rather than
adjusted to keep passing.
Implementation:
- pdf-cos/src/incremental.rs: IncrementalUpdate builds one revision.
Recomputes stream /Length so a caller cannot write an inconsistent one,
emits xref subsections for contiguous runs, sizes /Size over the whole
chain, and is byte-reproducible for a given set of edits.
- pdf-document/src/save.rs: turns dirty AcroForm fields into a revision,
writing the new /V and a regenerated appearance stream referenced from
/AP, keyed by state name for checkboxes and radios.
Refusals rather than partial saves: an encrypted document returns
SaveError::Encrypted, because writing plaintext objects into it would
corrupt the file; a source with no startxref or no /Root is refused; and a
save with no pending edits returns the input unchanged rather than growing
the file and churning its timestamp.
Tests: 10 acceptance tests in tests/save_roundtrip.rs covering the ADR merge
criteria. The central one reparses from the written bytes rather than
reusing in-memory state, so it tests the file rather than the writer against
itself. Also asserts three chained revisions still reparse with the newest
value winning, that pages and annotations survive a save, and that saving
never panics on the malformed corpus.
Known limitations, recorded in the ADR rather than glossed: cross-reference
streams and object streams are not written, and superseded objects are not
compacted.
Validation:
TEST_TARGET=pdf ./tools/test-rust-clean.sh (307 tests)
TEST_TARGET=pdf-ui ./tools/test-rust-clean.sh (351 tests, 6 ignored)
Both rustfmt and clippy -D warnings clean.
Fix security vulnerability in recursive-descent JSON parser:
- Add MAX_JSON_DEPTH constant (128 levels)
- Add depth field to JsonParser struct
- Check depth limit in parse_value() before recursion
- Increment depth in parse_object() and parse_array()
- Decrement depth when returning from parse_object() and parse_array()
- Return error when nesting exceeds MAX_JSON_DEPTH
This prevents stack overflow attacks using deeply nested JSON structures.
Fixes: BUG-012 (Security Vulnerability in JSON Parsing)
Fix potential null pointer dereference in style evaluation functions:
- Add empty check in evaluate_color() before accessing stops.last()
- Replace unwrap() with safe last() check in evaluate_color()
- Replace unwrap() with safe last() check in evaluate_width()
- Return default values when stops is empty
This prevents panics when evaluating styles with empty stop arrays.
Fixes: BUG-009 (Null Pointer Dereference in Style Application)
Fix potential buffer overflow in MVT parser by adding overflow checks:
- Add bounds check in read_pb_len_slice() to prevent integer overflow
- Add bounds check in skip_pb_field() for wire type 2
- Check if length is unreasonably large (> bytes.len()) before adding to pos
- Prevents integer overflow when pos + len wraps around
This prevents buffer overflow vulnerabilities when parsing malformed
MVT tiles with extremely large length values.
Fixes: BUG-007 (Buffer Overflow in MVT Parser)
Fix use-after-free in geometry rendering by deferring eviction until
after rendering is complete:
- Add pending_eviction field to TileCache
- Add set_pending_eviction() method to schedule eviction
- Modify tick() to perform pending eviction at start of next frame
- Rename evict() to evict_internal() for deferred execution
- Update view.rs to call set_pending_eviction() instead of evict()
This prevents use-after-free by ensuring that Geometry objects are not
freed while the renderer is still using them. Eviction now happens at
the start of the next frame, after all rendering is complete.
Fixes: BUG-005 (Use-After-Free in Geometry Rendering)
Fix potential integer overflow when calculating tile coordinates:
- Prevent overflow when z >= 31 (would overflow i32 when cast)
- Clamp zoom level to max 30 before casting to i32
- Apply fix to all 4 locations where powi() is used with tile coordinates:
- geometry.rs: local_tile_to_lon_lat()
- mvt_parser.rs: local_tile_to_lon_lat()
- tessellation.rs: lonlat_to_tile_coords()
This prevents undefined behavior when processing tiles at very high zoom levels.
Fixes: BUG-004 (Integer Overflow in Tile Coordinate Calculation)
Enhance HTTP error handling to include detailed context for debugging:
- Add tile coordinates (z, x, y) to error messages
- Add generation number to error messages
- Log unknown request_id errors
- Improve error messages for missing response body
- Improve error messages for missing thread pool
This provides better debugging information when HTTP requests fail,
making it easier to diagnose network issues and tile loading problems.
Fixes: BUG-003 (Missing Error Handling in HTTP Requests)
Fix memory leak in cache eviction by explicitly freeing GPU resources
(Geometry objects) when tiles are evicted from the cache.
Changes:
- Add cx: &mut Cx parameter to evict() method
- Free fill_geometry and stroke_geometry before removing tiles
- Update all callers to pass cx parameter
- Update test code to create default Cx for tests
This prevents GPU memory leaks when tiles are evicted from the cache
during pan/zoom operations.
Fixes: BUG-002 (Memory Leak in Cache Eviction)
Three items were carried forward as known gaps rather than quietly dropped.
This addresses all three; two are closed outright and one is bounded by an
environment limit that is now documented rather than implied.
1. Fuzzing had never actually run (Phase 6 step 6.3).
The five cargo-fuzz targets were only compile-checked, so "zero panics on
arbitrary input" was an aspiration. They have now been run under nightly
libFuzzer:
parse_object 1,970,750 runs
parse_xref 2,471,345 runs
decode_stream 1,120,019 runs
parse_content_stream 2,655,663 runs
parse_document 2,381,367 runs
About 10.6 million executions in total, no crashes and no new findings. That
is a real result rather than a green checkmark: the three crashes the corpus
found in Phase 6 were the ones worth finding, and the fuzzer confirms the
fixes hold under adversarial input.
2. Combo dropdown overlay (Phase 4 step 4.3).
A combo box that cannot be opened is a text field with extra steps, so the
open list is real state, not a rendering detail. Clicking a combo box opens
its options; the dropdown takes a click before any field underneath it,
matching the draw order; choosing a row sets the value through
DocumentFormEditor; clicking elsewhere dismisses it without changing the
value. render_open_combo() returns placement data so the drawing code stays
trivial and the geometry is testable without a renderer.
3. Makepad event delivery.
Upstream added a makepad_test framework, so this is now testable in
principle. Adds a test host binary and six UI tests that drive the widget
through the Studio protocol: a real click on the fixture link must surface
OpenUri on the host, typing must reach the field, and a click on empty space
must emit nothing so the positive assertions are not vacuous.
They are #[ignore] by default because the Studio hub cannot start an app in
this sandbox: the harness launches with --stdin-loop, which Makepad refuses
without a Studio websocket, and the build exits 101 before startup.
Upstream own spreadsheet-ui and map UI suites fail identically here with the
same error, so this is the environment rather than this code. The tests are
checked in and compiled by cargo test so they cannot rot, CI runs them where
a hub exists, and the module documents how to run them by hand.
Getting there also fixed a real defect in the test host: it copied
ui.main_view.render() from the spreadsheet app startup hook, but a plain
View has no render method, so the app errored at startup.
Validation:
TEST_TARGET=pdf ./tools/test-rust-clean.sh (270 tests)
TEST_TARGET=pdf-ui ./tools/test-rust-clean.sh (321 tests, 6 ignored)
cargo +nightly fuzz run <target> -- -max_total_time=60 (5 targets)
Both rustfmt and clippy -D warnings clean.
