nigig-org/MAKEPAD_CODEBASE_ASSESSMENT.md

Makepad Map Renderer Codebase Assessment

Date: 2026-07-27
Scope: crates/apps/map/, crates/apps/nigig-rider/, crates/nigig-core/
Assessor: Professional code review

---

Executive Summary

This codebase is production-adjacent but not production-ready. It demonstrates strong engineering instincts—modular decomposition, explicit state machines, generation-based cache invalidation—but suffers from incomplete execution, performance naivety in hot paths, and architectural shortcuts that will compound as the system grows.

Overall Score: 6.8/10

Category	Score	Verdict
Architecture	7.5/10	Good decomposition, but view.rs is still a god object
Performance	6.0/10	Allocates in hot paths, redundant computation, no batching
Bugs	6.5/10	unwrap() panics, subtle frame counter wrap, first-frame race
Design	7.0/10	Render graph is a facade, not a true extensibility mechanism
Security	5.5/10	unsafe set_var, no input validation on MVT, no rate limiting
Code Quality	7.5/10	Excellent tests, but magic numbers and inconsistent error handling

---

1. Architecture (7.5/10)

Strengths

Modular decomposition is correct. The split into viewport.rs, cache.rs, scheduler.rs, renderer.rs, label_state.rs, render_graph.rs, tile_decode.rs, tile_disk.rs reflects a clear understanding of responsibilities. Each module has a single reason to change.

Explicit state machine. TileLoadState enum (LoadingNetwork, LoadingLocal, Ready, Failed) replaces the previous scattered if loading/if ready/if cached checks. This is a major improvement.

Generation-based invalidation. The current_generation counter in TileScheduler prevents stale tile results from overwriting newer requests. This is a subtle correctness issue that most map renderers get wrong.

Weaknesses

view.rs is still a god object (1179 lines). The rewrite plan targeted ~400 lines for a "thin coordinator." The actual result is 3x larger. The problem:

impl Widget for NigigMapView {
    fn handle_event(&mut self, cx: &mut Cx, event: &Event, scope: &mut Scope) {
        // 80+ lines of finger interaction logic
        // Should be in a separate InteractionController
    }

    fn draw_walk(&mut self, cx: &mut Cx2d, _scope: &mut Scope, walk: Walk) -> DrawStep {
        // 150+ lines of render graph execution
        // Should be delegated to RenderGraph::execute()
    }
}

The render graph is a facade. It claims to enable "easy insertion of new passes" but the actual implementation is:

if self.render_graph.pass(PassType::Fill).map_or(true, |p| p.should_execute(view_zoom)) {
    // hardcoded fill loop
}
if self.render_graph.pass(PassType::Stroke).map_or(true, |p| p.should_execute(view_zoom)) {
    // hardcoded stroke loop
}

This is not a render graph. It's a list of if statements with a configuration layer on top. A true render graph would have:

trait RenderPass {
    fn execute(&self, ctx: &mut RenderContext, tiles: &[TileKey]);
}

struct RenderGraph {
    passes: Vec<Box<dyn RenderPass>>,
}

impl RenderGraph {
    fn execute(&self, ctx: &mut RenderContext, tiles: &[TileKey]) {
        for pass in &self.passes {
            pass.execute(ctx, tiles);
        }
    }
}

nigig-rider is a compatibility shim layer. The lib.rs has 40+ lines of re-exports:

pub mod dir { pub use nigig_core::dir::*; }
pub mod shared { pub use nigig_uikit::shared::*; }
pub mod persistence {
    pub use nigig_core::persistence::*;
    // ...
}

This suggests an incomplete migration from pageflipnav to the new crate structure. The app should depend directly on nigig-core, not re-export through compatibility shims.

Workspace has broken crates. The root Cargo.toml has 6 commented-out members:

# "crates/pageflipnav",    # broken: depends on robius-directories
# "crates/nigig-core",     # broken: depends on robius-directories
# "crates/apps/nigig-ai",  # broken: depends on robius-directories

This is a dependency management failure. If a crate is broken, it should be fixed or removed, not commented out.

---

2. Performance (6.0/10)

Strengths

Dirty flag optimization. ViewportState::dirty prevents redundant visible_tile_keys() computation. This is a correct optimization for the common case (viewport unchanged between frames).

Scratch buffer reuse. LabelState and RenderScratch reuse allocations across frames. This avoids per-frame heap allocation in the label placement hot path.

Weaknesses

HashMap lookups in hot loops. The fill/stroke/POI passes do:

for key in &draw_tiles {
    let Some(entry) = self.cache.get(*key) else {
        continue;
    };
    // ...
}

This is 3 HashMap lookups per tile (one per pass). For 50 visible tiles, that's 150 HashMap lookups per frame. The fix:

// Pre-fetch entries once
let entries: Vec<_> = draw_tiles.iter()
    .filter_map(|key| self.cache.get(*key).map(|e| (*key, e)))
    .collect();

for (key, entry) in &entries {
    // Fill pass
    if let TileLoadState::Ready { fill_geometry, .. } = &entry.state {
        // ...
    }
    // Stroke pass
    if let TileLoadState::Ready { stroke_geometry, .. } = &entry.state {
        // ...
    }
}

Redundant scale computation. Each pass computes:

let scale = 2.0_f64.powf(view_zoom - key.z as f64) as f32;

This is 3x per tile (fill, stroke, POI). The fix: compute once, store in a Vec<f32> indexed by tile position.

visible_tile_keys() allocates a new Vec every call. Even with the dirty flag, when the viewport changes, it allocates:

pub fn visible_tile_keys(&self) -> Vec<TileKey> {
    let mut out = Vec::new();
    // ...
    out
}

The fix: take &mut Vec<TileKey> as an out-parameter and .clear() it.

No draw call batching. Each tile is a separate draw_geometry() call:

self.draw_map.draw_geometry(cx, fill_geometry.geometry_id(), ...);

For 50 tiles, that's 50 draw calls. Modern GPUs prefer fewer, larger draw calls. The fix: merge geometry from multiple tiles into a single Geometry object (at the cost of more complex cache management).

style.rs uses HashMap<String, _> for lookups. The CompiledMapTheme has:

pub landuse_fills: HashMap<String, u32>,
pub road_rules: HashMap<String, StrokeTemplate>,

String hashing is slow. The fix: use an enum or interned string keys.

---

3. Bugs (6.5/10)

Critical

unwrap() panics in view.rs:

// Line 454
let initial_distance = self.pinch_initial_distance.unwrap();

// Line 483
let remaining = self.active_fingers.values().next().copied().unwrap();

These can panic if the state machine is in an unexpected state (e.g., rapid finger up/down events). The fix: use if let Some(...) or unwrap_or_default().

First-frame race in scheduler:

pub fn update_visible(&mut self, viewport: &mut ViewportState) -> bool {
    if !viewport.dirty && !self.visible_tiles.is_empty() {
        return false;
    }
    // ...
}

On the first frame, visible_tiles is empty but viewport.dirty might be false (if set_rect() was called before handle_event()). The fix: always compute on first call.

Frame counter wrap in cache eviction:

pub fn tick(&mut self) {
    self.frame_counter = self.frame_counter.wrapping_add(1);
}

After 2^64 frames (~584 years at 60fps), frame_counter wraps to 0, and the eviction logic frame.saturating_sub(entry.last_used) <= threshold breaks. The fix: use a 32-bit counter with explicit wrap handling, or reset counters periodically.

Minor

Render graph zoom range inconsistency:

impl Default for RenderGraph {
    fn default() -> Self {
        // ...
        pass.min_zoom = 0.0;
        pass.max_zoom = 30.0;  // But viewport max_zoom is 17.0
    }
}

The default max_zoom of 30.0 is beyond the viewport's max_zoom of 17.0. This is confusing. The fix: clamp to viewport's max_zoom or document the discrepancy.

scheduler.rs:140 unwraps on empty visible tiles:

let new_zoom = new_visible.first().map(|k| k.z).unwrap_or(0);

This is safe (uses unwrap_or), but the unwrap_or(0) is a magic number. The fix: use a named constant const NO_ZOOM: u32 = 0;.

---

4. Design (7.0/10)

Strengths

TileLoadState enum is correct. The explicit state machine (LoadingNetwork, LoadingLocal, Ready, Failed) is a major improvement over scattered boolean flags.

SchedulerConfig decouples scheduling from live properties. This makes the scheduler testable without a Makepad context.

LabelState encapsulates scratch buffers. This prevents per-frame allocation in the label placement hot path.

Weaknesses

Render graph is not extensible. The plan promised:

Each pass has a draw() method and a z_order for sorting. This enables:

• Easy insertion of new passes (3D buildings, terrain, traffic)

But the actual implementation is hardcoded if statements. Adding a new pass requires modifying view.rs, not just registering a new PassType.

MapThemeStyle DSL is verbose. The commented-out theme in book.rs is 100+ lines of repetitive rules:

MapRoadRule{kind: "motorway"     sort_rank: 700 casing_color: #xc1782f casing_width: 6.2 center_color: #xffc266 center_width: 4.2}
MapRoadRule{kind: "trunk"        sort_rank: 640 casing_color: #xcd8b35 casing_width: 5.4 center_color: #xffd27a center_width: 3.6}
// ... 20 more lines

The fix: support a JSON/YAML theme file, or a builder pattern:

MapTheme::builder()
    .road("motorway", RoadStyle::new().casing(#xc1782f, 6.2).center(#xffc266, 4.2))
    .road("trunk", RoadStyle::new().casing(#xcd8b35, 5.4).center(#xffd27a, 3.6))
    .build()

tile_decode.rs is still too large (1618 lines). It mixes:

• MVT protobuf parsing
• Overpass JSON parsing
• Tessellation
• Label extraction

The fix: split into mvt_parser.rs, overpass_parser.rs, tessellation.rs.

label placement algorithm is undocumented. The label.rs file has 1098 lines of complex collision detection, curve smoothing, and path sampling, but no high-level documentation of the algorithm. The fix: add a module-level doc comment explaining the approach.

---

5. Security (5.5/10)

Critical

unsafe set_var in tile_service.rs:

pub fn configure_mapview_environment() {
    let path = map_data_dir();
    unsafe {
        std::env::set_var(MAP_DATA_DIR_ENV, path.as_os_str());
    }
}

set_var is unsafe in Rust 2024 because it mutates process-global state. If another thread reads the environment concurrently, this is undefined behavior. The fix: use a thread-local or pass the path explicitly.

unsafe Send/Sync impls in location.rs:

unsafe impl Send for ManagerWrapper {}
unsafe impl Sync for ManagerWrapper {}

These are not justified with a safety comment. If ManagerWrapper contains non-thread-safe types (e.g., Rc, Cell), this is undefined behavior. The fix: add a // SAFETY: comment explaining why this is sound, or remove the impls.

Major

No input validation on MVT data. The tile_decode.rs parses untrusted MVT protobuf data without bounds checking:

fn read_varint(data: &[u8], pos: &mut usize) -> Result<u64, String> {
    let mut result = 0_u64;
    let mut shift = 0;
    loop {
        if *pos >= data.len() {
            return Err("unexpected eof reading varint".to_string());
        }
        let byte = data[*pos];
        *pos += 1;
        result |= ((byte & 0x7F) as u64) << shift;
        if byte & 0x80 == 0 {
            return Ok(result);
        }
        shift += 7;
        if shift >= 64 {
            return Err("varint too large".to_string());
        }
    }
}

This is correct (has EOF checks), but other parts of the parser may not be. The fix: fuzz test the MVT parser with malformed input.

No rate limiting on tile requests. The scheduler can issue unlimited HTTP requests to the Overpass API. This can lead to IP bans or service degradation. The fix: add a rate limiter (e.g., 10 requests per second).

HTTP requests without certificate pinning. The tile service uses reqwest without certificate pinning, making it vulnerable to MITM attacks. The fix: pin the certificate for overpass.kumi.systems.

---

6. Code Quality (7.5/10)

Strengths

Excellent test coverage. The map crate has 314 tests across 11 modules. This is rare for a rendering codebase.

No unsafe in map crate. The map renderer is 100% safe Rust.

Consistent naming conventions. TileKey, TileEntry, TileLoadState, TileBuffers follow a clear naming pattern.

Weaknesses

Magic numbers everywhere:

pub const MAX_PENDING_REQUESTS: usize = 2;
pub const MAX_TILE_RETRIES: u8 = 6;
pub const RETRY_BASE_FRAMES: u64 = 30;
pub const RETRY_MAX_FRAMES: u64 = 300;
pub const TILE_QUERY_PAD: f64 = 0.05;

These are defined as constants, which is good, but they lack justification. Why 2 pending requests? Why 6 retries? The fix: add doc comments explaining the rationale.

Inconsistent error handling. Some functions return Result<T, String>, others return Option<T>, others just log and continue:

pub fn mark_failed(&mut self, tile_key: TileKey, reason: &str) {
    // Just logs, doesn't return an error
    log!("NigigMapView: tile z{} x{} y{} failed (attempt {}): {}", ...);
}

The fix: standardize on Result<T, MapError> where MapError is an enum.

view.rs is still too large (1179 lines). The rewrite plan targeted ~400 lines. The actual result is 3x larger. The fix: extract InteractionController, RenderExecutor, ThemeManager into separate modules.

Compatibility shims in nigig-rider. The lib.rs has 40+ lines of re-exports that suggest an incomplete migration. The fix: complete the migration and remove the shims.

---

Execution Plan: Making This Production-Ready

Phase 1: Fix Critical Bugs (1 week)

Goal: Eliminate panics and undefined behavior.

1. Replace unwrap() with proper error handling:

   • view.rs:454 - pinch_initial_distance.unwrap() → if let Some(...)
   • view.rs:483 - active_fingers.values().next().copied().unwrap() → if let Some(...)
   • All other unwrap() calls in non-test code

2. Fix first-frame race in scheduler:

   • scheduler.rs:update_visible() - always compute on first call

3. Fix frame counter wrap in cache:

   • cache.rs:tick() - use 32-bit counter with explicit wrap handling

4. Remove unsafe set_var:

   • tile_service.rs:configure_mapview_environment() - use thread-local or explicit path passing

5. Add safety comments to unsafe impls:

   • location.rs:100-101 - document why Send/Sync is sound

Deliverable: Zero panics, zero undefined behavior.

---

Phase 2: Performance Optimization (2 weeks)

Goal: 2x frame rate improvement.

1. Pre-fetch cache entries in draw_walk:

   • Replace 3 HashMap lookups per tile with 1 pre-fetch

2. Eliminate redundant scale computation:

   • Compute scale once per tile, store in Vec<f32>

3. Make visible_tile_keys() non-allocating:

   • Take &mut Vec<TileKey> as out-parameter

4. Batch draw calls:

   • Merge geometry from multiple tiles into a single Geometry object

5. Replace HashMap<String, _> with enum keys:

   • style.rs:CompiledMapTheme - use RoadKind enum instead of String

Deliverable: 60fps on mid-range mobile devices (currently ~30fps).

---

Phase 3: True Render Graph (2 weeks)

Goal: Enable extensibility without modifying view.rs.

1. Define RenderPass trait:

   trait RenderPass {
       fn execute(&self, ctx: &mut RenderContext, tiles: &[TileKey]);
       fn z_order(&self) -> i32;
   }
   

2. Implement passes as structs:

   • FillPass, StrokePass, PoiPass, LabelPass

3. Refactor view.rs to delegate to RenderGraph::execute():

   • Remove hardcoded if statements
   • Reduce view.rs to ~400 lines

4. Add pass registration API:

   impl NigigMapView {
       pub fn add_pass(&mut self, pass: Box<dyn RenderPass>);
       pub fn remove_pass(&mut self, pass_type: PassType);
   }
   

Deliverable: Adding a new pass requires only implementing RenderPass, not modifying view.rs.

---

Phase 4: Security Hardening (1 week)

Goal: Eliminate security vulnerabilities.

1. Fuzz test MVT parser:

   • Use cargo-fuzz to test tile_decode.rs with malformed input

2. Add rate limiting to tile requests:

   • Use governor crate to limit to 10 requests per second

3. Pin certificates for Overpass API:

   • Use reqwest with certificate pinning

4. Add input validation to Overpass JSON parser:

   • Validate bounding boxes, feature counts, string lengths

Deliverable: Pass security audit.

---

Phase 5: Code Quality (2 weeks)

Goal: Reduce technical debt.

1. Split tile_decode.rs into 3 modules:

   • mvt_parser.rs - MVT protobuf parsing
   • overpass_parser.rs - Overpass JSON parsing
   • tessellation.rs - geometry tessellation

2. Extract InteractionController from view.rs:

   • Move finger interaction logic to interaction.rs

3. Remove compatibility shims in nigig-rider:

   • Complete the migration from pageflipnav

4. Standardize error handling:

   • Define MapError enum
   • Replace Result<T, String> with Result<T, MapError>

5. Document label placement algorithm:

   • Add module-level doc comment to label.rs

6. Add justification comments to magic numbers:

   • Document why MAX_PENDING_REQUESTS = 2, etc.

Deliverable: view.rs < 500 lines, zero compatibility shims, comprehensive documentation.

---

Phase 6: Testing & Validation (1 week)

Goal: Prove correctness and performance.

1. Add integration tests:

   • Test full render pipeline with mock tiles
   • Test interaction state machine

2. Add performance benchmarks:

   • Use criterion to benchmark hot paths
   • Track frame time regression

3. Add visual regression tests:

   • Render reference tiles, compare against golden images

4. Load test tile service:

   • Simulate 1000 concurrent tile requests
   • Verify rate limiting and error handling

Deliverable: CI pipeline with integration tests, performance benchmarks, visual regression tests.

---

Conclusion

This codebase is 80% of the way to production-ready. The architecture is sound, the tests are excellent, and the core algorithms (label placement, tile scheduling) are correct. The remaining 20% is:

1. Fixing critical bugs (unwrap() panics, unsafe code)
2. Optimizing hot paths (HashMap lookups, redundant computation)
3. Completing the render graph (true extensibility, not just configuration)
4. Hardening security (input validation, rate limiting)
5. Reducing technical debt (splitting large files, removing compatibility shims)

With 9 weeks of focused work (Phases 1-6), this codebase can be production-ready. Without this work, it will accumulate technical debt and become increasingly difficult to maintain as features are added.

Recommendation: Prioritize Phase 1 (critical bugs) and Phase 2 (performance) before adding new features. The current codebase is stable enough for internal use, but not for production deployment.