fix: show both engines side-by-side in README benchmark table

[carlos-alm]

Summary

• README benchmark table previously showed only the preferred engine (native when available, WASM fallback) for all metrics
• Now shows both Native and WASM columns side-by-side when native is available, so users can compare engine performance directly
• All rows updated: build speed, query time, incremental rebuilds, query latency, 50k-file estimates
• Resolution precision/recall (engine-independent) shows a single value with a dash in the WASM column
• Falls back to single-column layout when only WASM is available
• Detailed per-engine breakdown still in BUILD-BENCHMARKS.md

Test plan

• Run node --import ./scripts/ts-resolve-loader.js scripts/update-benchmark-report.ts benchmark-result.json with both native and wasm data — verify README table shows | Metric | Native | WASM | header with both values in every row
• Run with native: null — verify README table shows single | Metric | Latest | column layout
• Verify BUILD-BENCHMARKS.md still contains full per-engine breakdown

[claude]

Claude finished @carlos-alm's task —— View job

---

PR Review: Fix README benchmark table display

Review Progress

• Read and analyze the changed file
• Phase 1: Root cause analysis
• Phase 2: Configuration change detection
• Phase 3: Test integrity check
• Phase 4: Code quality review
• Phase 5: Backlog compliance assessment
• Provide final rating and recommendation

Analysis Summary

File Changed: scripts/update-benchmark-report.ts (+9/-15)
Current README Performance Table: Shows separate rows for native (13.3 ms/file) and WASM (13.6 ms/file) build speeds
Proposed Change: Show only preferred engine (native when available, WASM fallback) to avoid misleading similarity

Phase 1: Root Cause Analysis ✅

Problem Being Solved: The README benchmark table showing both native and WASM build speeds side-by-side creates misleading impressions. The total build time includes many engine-independent JavaScript stages (insert, resolve, structure, roles) that dilute the native parsing advantage, making ~13.3 vs ~13.6 ms/file appear artificially similar.

Why This Approach: The change follows the existing pattern already used for rebuild and query latency rows, showing only the preferred engine. The detailed per-engine breakdown remains accessible in BUILD-BENCHMARKS.md.

Risk Assessment: Low risk. This is purely a display change that doesn't affect the underlying data collection or the detailed benchmark files.

Phase 2: Configuration Change Detection ✅

Result: No configuration changes detected. This is a pure display logic change in a reporting script.

Phase 3: Test Integrity ✅

Result: No test files modified. No assertions weakened or removed. The change only affects how benchmark data is formatted for display.

Phase 4: Code Quality Review ✅

Code Quality: Good. The change simplifies the logic by:

• Removing conditional branching (if (latest.native) { ... } else { ... })
• Consolidating to a single pref variable selection pattern
• Adding clear explanatory comments about the rationale
• Following existing patterns used elsewhere in the same file

Technical Implementation: Sound. The pref = latest.native || latest.wasm pattern correctly handles fallback logic.

Phase 5: Backlog Compliance Assessment ✅

Zero-dep: ✓ — No new dependencies added. Pure refactor of existing display logic.

Foundation-aligned: ✅ — Fully aligned. Improves user experience by reducing confusion without violating any principles.

Problem-fit: 3/5 — This addresses user confusion about benchmark presentation, which is useful but doesn't directly help AI agents navigate codebases. It's a worthwhile quality-of-life improvement.

Breaking: No — This is purely a display change. The underlying data and API remain unchanged.

Tier: 1 (zero-dep ✓ + foundation-aligned ✓)

Final Assessment

Root Cause Analysis

Problem Being Solved: Misleading benchmark presentation where native vs WASM speeds appear artificially similar due to engine-independent overhead masking the native parsing advantage.

Why This Approach: Consistent with existing patterns in the same codebase; preserves detailed data in specialized benchmark files while simplifying README presentation.

Risk Assessment: Very low risk - pure display refactor with no behavioral changes.

Backlog Compliance

• Zero-dep: ✓ — no new dependencies
• Foundation-aligned: ✓ — improves user experience without violating principles
• Problem-fit: 3/5 — useful UX improvement, tangentially related to core mission
• Breaking: No — display-only change
• Tier: 1

Critical Concerns

None. This is a straightforward improvement that addresses a real user confusion issue.

Final Recommendation

• Rating: ⭐⭐⭐⭐ (4/5)
• Action: APPROVE
• Reasoning: Well-motivated fix for misleading benchmark presentation. Clean implementation, good explanatory comments, follows existing patterns, zero risk. The only reason it's not 5 stars is that it's a relatively minor UX improvement rather than a core functionality enhancement.

---

[greptile-apps]

Greptile Summary

This PR refactors the README benchmark table in update-benchmark-report.ts to show Native and WASM columns side-by-side when both engine results are present, replacing the previous single-column layout that only showed the preferred engine. The null-access bug on latest.wasm flagged in the prior review cycle is correctly resolved: hasBoth = hasNative && latest.wasm != null guards every wasm dereference in the README patch block, and all three cases (both engines, native-only, WASM-only) produce consistent column counts between the table header and rows.

Confidence Score: 5/5

Safe to merge — all prior P1 concerns resolved, no new issues found

The hasBoth flag correctly guards every wasm dereference in the README patch block. All three cases (both engines, native-only, WASM-only) produce consistent column counts between header and rows. No logic errors, security issues, or correctness problems remain.

No files require special attention

Important Files Changed

Filename	Overview
scripts/update-benchmark-report.ts	Adds dual-column Native/WASM README table; hasBoth flag correctly guards all wasm dereferences across every branch

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Read benchmark JSON] --> B{latest.native != null?}
    B -- Yes --> C{latest.wasm != null?}
    B -- No --> G[WASM-only: single-column rows]
    C -- Yes --> D[hasBoth=true: Native + WASM two-column rows]
    C -- No --> E[native-only: single-column rows]
    D --> H[tableHeader: Metric | Native | WASM]
    E --> I[tableHeader: Metric | Latest]
    G --> I
    H --> J[Patch README performance section]
    I --> J

Loading

_{Reviews (4): Last reviewed commit: "fix: guard against null wasm data in REA..." | Re-trigger Greptile}

[carlos-alm]

Addressing Greptile review feedback (ca6a828):

P1 — Crash when latest.wasm is absent (line 203):
Fixed. Added optional chaining (latest.wasm?.perFile) and guarded all four WASM estimate rows with estWasm ? ... : 'n/a', matching the existing pattern used for the native side.

P2 — Missing engine label on query latency rows (lines 349-350):
Fixed. Both Query: fn-deps and Query: path rows now append prefLabel (e.g. (native)) for consistent labeling with all other metric rows.

@greptileai

[carlos-alm]

Latest commit 97b2cf4 changes the approach: instead of showing only the preferred engine with an engine label suffix, it now uses a proper two-column table layout (Native | WASM) when both engines are available, falling back to a single-column layout for WASM-only environments. This is cleaner and makes the side-by-side comparison more readable.

The wasm null guards from ca6a828 are preserved in BUILD-BENCHMARKS.md sections (using optional chaining). The README section uses hasNative branching — when hasNative is true, latest.wasm is always expected to be present since the benchmark harness runs both engines.

@greptileai

[greptile-apps]

Null access on latest.wasm when native is present but WASM is absent

hasNative is only latest.native != null, but every code path inside the if (hasNative) block directly dereferences latest.wasm (lines 332, 333, 342, 346, 362, 378–379) without guarding against latest.wasm being null. If a benchmark run produces native data but WASM data is absent (e.g. the WASM grammar failed to build), this throws TypeError: Cannot read properties of null. The immediately preceding commit (ca6a828) was literally titled "fix: guard wasm null access", so this scenario is known to happen in practice.

Use a hasBoth flag instead:

const hasNative = latest.native != null;
const hasBoth   = hasNative && latest.wasm != null;

let rows = '';
if (hasBoth) {
    rows += `| Build speed | **${latest.native.perFile.buildTimeMs} ms/file** | **${latest.wasm.perFile.buildTimeMs} ms/file** |\n`;
    rows += `| Query time | **${formatMs(latest.native.queryTimeMs)}** | **${formatMs(latest.wasm.queryTimeMs)}** |\n`;
} else if (hasNative) {
    rows += `| Build speed | **${latest.native.perFile.buildTimeMs} ms/file** |\n`;
    rows += `| Query time | **${formatMs(latest.native.queryTimeMs)}** |\n`;
} else {
    rows += `| Build speed | **${latest.wasm.perFile.buildTimeMs} ms/file** |\n`;
    rows += `| Query time | **${formatMs(latest.wasm.queryTimeMs)}** |\n`;
}

And replace every downstream if (hasNative) guard (incremental rebuild rows, query latency rows, 50k-file estimate, resolution rows, and tableHeader) with if (hasBoth) so that WASM data is only shown when it is actually present.

[carlos-alm]

Fixed in 8a83fe5. Introduced a hasBoth flag (hasNative && latest.wasm != null) and replaced every if (hasNative) guard in the README-patch block with if (hasBoth). The two-column layout now only activates when both engines have data. When only native is present (no WASM), falls back to single-column layout using a pref = latest.native || latest.wasm pattern — same approach used for the existing WASM-only fallback. The hasNative variable is retained only for the intermediate native-only branch in the build speed / query time rows.

[carlos-alm]

@greptileai