[YetheSamartaka]

Adds more player detail and mission activity to playback for OCAP2/addon#128
Added

• Player profiles with gear, medical state, stamina, radios and traits.
• Loadout export, profile comparisons and radio coverage.
• Comms, ORBAT, FPS graphs, weather, bookmarks and shareable playback links.
• Zeus activity, stance, trenches, fortifications, explosions and vehicle support events.
• Faction filters for events.

Changed

• Compressed playback files, cached frontend assets and player details loaded on demand.
• Higher map zoom limits and marker labels hidden when zoomed out.

Fixed

• Friendly-fire detection after a unit changes side.
• Map scale distances and missing dashboard stats when conversion is disabled.

[fank]

Ratatoskr reviewed this pull request.

Changes requested on 6b0e9a3f. See the review.

Finished 2026-10-17 16:26 UTC.

[fank]

REQUEST_CHANGES — five actionable findings.

Review details

Reviewed head 6b0e9a3fac06612388102d5ccb4eb657e73fd89a against base ee911cb7ef3c08876567b0263c735d544cbcfd4d.

1. [P1] Restore an embeddable file for clean-checkout Go tests. Deleting internal/frontend/dist/.gitkeep leaves no tracked files matching //go:embed all:dist. The Go workflow does not build the frontend before testing. Its current run fails to compile both internal/frontend and cmd/ocap-webserver with pattern all:dist: no matching files found. Restore the placeholder or explicitly generate the embedded frontend before Go compilation.

2. [P2] Fix the introduced TypeScript errors. For example, arsenalExport.ts:8 uses String.replaceAll, but the project's configured target/library is ES2020. npx tsc --noEmit fails here and at additional new sites: the medical test's incorrect relative import, widened event discriminants in EventsTab tests, Entity/coordinate type mismatches, incomplete snapshot fixtures, demGrid.ts:202, and zeus.ts:196. These are reproducible compiler diagnostics, not merely an unexplained failed CI status. Correct the new code/fixtures against the configured types and rerun the typecheck.

3. [P2] Defer optional fixture reads until a test actually runs. convertedRecording.integration.test.ts:135–137 calls decode() inside the describe.skipIf declaration callback. Vitest still executes that callback to collect skipped tests, so absent optional recordings cause an ENOENT during collection rather than a skip. A clean npm test fails this suite. Move the reads into a hook/test that does not execute when skipped, or guard suite construction before reading the files.

4. [P2] Analyze chunks before the LRU evicts them. engine.ts:543–549 loads every chunk but discards their returned references. ChunkManager retains only three chunks. StatsTab, OrbatTab and the death heatmap then read through getStateAt/getPositionAt, silently losing the evicted portions of any recording longer than three chunks. In an isolated four-chunk fixture, after ensureAllChunks() frame 0 was null and movement totaled 895 m instead of 1,195 m. Use a streaming traversal such as visitAllFrames to build durable analysis results while each chunk is available; add coverage with the real bounded cache rather than only mocked state readers.

5. [P2] Preserve the existing positional explosion event layout. parser_v1.go:178–187 now routes every explosion through the structured-payload branch and returns immediately, even when element 2 is a numeric source ID. The existing [100, "explosion", 5, 10, 50.5] layout consequently loses its source/target/distance fields and stores just "5" as Message. CI's existing TestParserV1_parseEvent_EdgeCases/event_with_float_distance_at_index_4 confirms the distance regression (expected 50.5, actual 0). Detect structured object payloads before taking the new branch and retain the prior positional parsing for legacy events.

Verification: gh pr diff exceeded GitHub's size limit; inspected local diffs and surrounding code instead. In an isolated copy outside the reviewed checkout: npm ci --ignore-scripts succeeded; npx tsc --noEmit failed with the diagnostics above; npm test produced 102 passing files / 2,104 passing tests and one fixture-collection failure; npm run build succeeded. An additional temporary real-cache regression test reproduced finding 4. Local go test ./... could not run because Go is not installed; inspected the current Go CI failure logs, including the embed and parser failures above. No rendered-browser inspection was performed.

Discussions: GraphQL enumeration returned no review threads, with no further page. None resolved, left open, or skipped. The reviewed checkout was not modified.

[fank]

REQUEST_CHANGES — 1 correctness finding.

Review details

Reviewed head 9e53433abbcaaf40e1f5a8928036bb8c17406cb7 against base ee911cb7ef3c08876567b0263c735d544cbcfd4d.

Required change

[P2] Preserve shared playback positions outside the saved focus range — ui/src/pages/recording-playback/RecordingPlayback.tsx:186–189.

A viewer can select FULL, seek outside the recording's saved focus, and copy the new shareable URL. Opening that URL starts with showFullTimeline = false, however, so the focus-constraining effect at lines 252–265 immediately overrides the restored frame. With focus [20, 80], opening ?t=90 lands at 80, not 90; the URL synchronization then replaces the requested position. Restore full-timeline mode when an explicit linked frame lies outside the focus (or encode/restore that mode), and add a component-level regression test for both sides of the focus range. The existing deep-link helper tests do not exercise this interaction.

Confirmed using an isolated Vitest test outside the checkout, reusing the existing RecordingPlayback test harness: expected frame 90, received 80.

Verification

• cd ui && npm test: 106 files passed, 1 skipped; 2,132 tests passed, 13 skipped.
• cd ui && npm run test:coverage: same passing test totals.
• cd ui && ./node_modules/.bin/vitest run --config /tmp/pr556-review/vitest.config.mjs -t 'review: restores': diagnostic regression failed as described above.
• go test ./...: unavailable locally (go: command not found).
• Inspected failed GitHub checks rather than treating status alone as a defect: Go fails in coverage tooling because go-test-coverage/v2@v2.20.0 requires Go >=1.27 while CI uses 1.26.5; UI CI reports a Zeus synthesis assertion failure, which did not reproduce in the local full test/coverage runs.
• No real-browser visual inspection or end-to-end playback verification performed; component tests are not a visual check.
• GraphQL review-thread enumeration returned zero threads, with no further page. No earlier discussions to resolve or continue; no thread mutations performed.
• Reviewed checkout left unchanged; no commits, pushes, or merge.

[fank]

Ratatoskr reviewed this pull request.

Changes requested on 701fa315. See the review.

Finished 2026-10-17 20:08 UTC.

[fank]

REQUEST_CHANGES — 2 correctness findings.

Review details

Reviewed head 701fa315aeb6771388fdab3c5f71b8556f5e6267 against base ee911cb7ef3c08876567b0263c735d544cbcfd4d.

1. [P2] Prefer the current player entity over its longer-recorded corpse. ui/src/pages/recording-playback/orbat.ts:112–115 deduplicates a named slot solely by total recording span. A corpse can remain recorded longer than the player's subsequent respawn. Reproduction: same-name/same-group entities spanning frames 0–100 (dead at frame 60, currently AI-flagged) and 50–100 (alive and player-controlled). At frame 60, buildOrbat selects the corpse, reports zero alive members, and exposes the wrong entity for navigation. Prefer the active player/living replacement using state at the requested frame rather than total lifespan, and add a regression covering this case; the existing duplicate-slot test only covers the favorable span ordering.

2. [P2] Preserve the terrain tiles' geographic extent in the heightmap fallback. ui/src/playback/radioRange/demGrid.ts:350–361 treats the assembled whole-tile rectangle as though it starts at world (0,0) and exactly spans worldSize. Web Mercator tile boundaries do not match the world bounds. Additionally, maxY = latToTile(0, n) includes the tile immediately south of the equator; when absent, its transparent canvas pixels decode as −10,000 m. In an isolated reproduction using a 12,800 m world and a zoom-11 terrain tile of constant 100 m elevation, the fallback produces a 256×512 grid and sampleDem(grid, 100, 100) returns −10,000 m. This misplaces terrain and invalidates terrain-aware radio coverage/reachability for existing heightmap-only maps. Crop/resample using the actual tile coordinates and geographic world bounds, exclude the out-of-bounds southern tile, and handle missing/transparent pixels explicitly. Add a fallback-path regression, not just binary DEM tests.

Verification

• cd ui && npm test -- --run: passed; 106 test files passed, 1 skipped; 2,139 tests passed, 13 skipped.
• cd ui && npx tsc --noEmit: passed.
• cd ui && npx vitest run --config /tmp/pr556-review/vitest.config.mts: both additional regression tests fail as described above. Harness and log are outside the checkout; PMTiles and canvas are mocked, not a rendered-UI inspection.
• go test ./...: unavailable (go: command not found).
• Inspected failed CI diagnostics with gh run view 39064940071 --repo OCAP2/web --log-failed and gh run view 39064939949 --repo OCAP2/web --log-failed. Go CI fails in the coverage tool because go-test-coverage/v2@v2.20.0 requires Go ≥1.27 but the runner uses 1.26.5 (GOTOOLCHAIN=local); this is a tooling mismatch, not evidence of a playback defect. UI CI reports a failed Zeus empty-entity-hole synthesis assertion; that test passed in the local full suite, so its CI-only cause is unverified and is not counted as an additional finding.
• No rendered UI or live-system testing performed. Checkout left unchanged.

Earlier discussions: GraphQL enumeration returned no review threads (pagination complete); none resolved, left open, or skipped.

[fank]

Ratatoskr reviewed this pull request.

Changes requested on bf241c96. See the review.

Finished 2026-10-17 20:21 UTC.

[fank]

REQUEST_CHANGES — 1 correctness finding.

Review details

Reviewed head bf241c96f614904640a315951ff832260c4bd974.

[P2] Preserve roles in protobuf ORBAT

ui/src/pages/recording-playback/orbat.ts:103 reads only state.role, falling back to an empty string. However, protobuf EntityState has no role field, and ProtobufDecoder.convertEntityState never supplies one. Consequently, converted recordings show no ORBAT role labels or leader indicators, cannot be searched by role, and sort leaders alphabetically instead of first—even though the manifest retains each unit's role.

Reproduced with a decoded protobuf chunk containing Aaron (manifest role Rifleman) and Zulu (Squad Leader): both ORBAT roles became "", with Aaron listed first. Fall back to the unit's recorded role when the frame lacks one; preserve per-frame roles through conversion if dynamic roles are intended. Add a protobuf-path regression test with the leader alphabetically after another player. The existing sorts leaders first, then players, then by name test does not pin leader priority: Boss already sorts before Zulu.

Verification

• npm --prefix ui test: 2,151 passed, 13 skipped.
• npm --prefix ui run test:coverage: 2,151 passed, 13 skipped.
• ui/node_modules/.bin/tsc --project ui/tsconfig.json --noEmit: passed.
• npm --prefix ui run build -- --outDir /tmp/ocap556-build: passed, with a bundle-size warning.
• go test ./...: unavailable locally (go: command not found).
• GitHub UI CI reports a Zeus synthesis assertion failure; both the targeted test and full suite, including coverage, passed locally. GitHub Go CI fails in the coverage-report action because its tool requires Go >=1.27 while the runner uses 1.26.5; this is a toolchain failure, not evidence of a playback defect. Neither CI status is the basis of this verdict.
• No browser/rendered-UI inspection performed; the ORBAT reproduction exercised decoded data and the actual roster builder, not presentation.
• GraphQL review-thread enumeration returned no threads; none required replies or resolution.

The reviewed checkout was not edited or committed.

[fank]

Ratatoskr reviewed this pull request.

Changes requested on 666d1452. See the review.

Finished 2026-10-17 20:40 UTC.

[fank]

REQUEST_CHANGES — 2 findings requiring correction.

Review details

Reviewed head 666d1452c618305d6a8b222330f1c8c5490a5c95 against base dcb58a1a9e948a4bdae56745f1772d2de2d4d209.

Findings

1. [P2] TFAR coverage is clipped despite an increased range multiplier. ui/src/playback/radioRange/tfarCoverage.ts:147–154 caps the search at rangeMeters, but effectiveDistance divides distance by globalRadioRangeCoef. On a flat DEM with a 5,000 m radio and coefficient 2, I measured every polygon vertex at approximately 4,999 m, while inTfarRange correctly accepts a receiver 7,500 m away. The profile coverage therefore contradicts the reachability calculation. Expand the search bound to account for the recorded multiplier, and add a polygon-level regression test with a coefficient greater than 1; the existing multiplier test exercises only effectiveDistance.

2. [P2] Reconcile Zeus placeholder replacement with the failing synthesis test. ui/src/playback/engine.ts:441–458 attempts to overwrite a nameless placeholder through addEntity, but EntityManager.addEntity keeps the existing entity when its lifetime is longer or equal. The PR's new synthesizes VIRTUAL Zeus when curatorId 0 is an empty entity hole test reproduces this: entity 0 remains a Vehicle, not the expected VIRTUAL Unit. This fails both locally and in UI CI. If this remains a supported engine input, explicitly replace the placeholder; if decoder filtering makes it unsupported, revise the test to exercise the actual supported loading path. Do not leave the new regression test asserting behavior the implementation cannot provide. I have not established that decoder-filtered production recordings still reach this placeholder case.

Verification

• cd ui && npm test: failed — 2,154 passed, 1 failed (Zeus case above), 13 skipped.
• cd ui && npx tsc --noEmit: passed.
• cd ui && npm run build -- --outDir /tmp/ocap556-ui-build: passed.
• node /tmp/ocap556-radio-repro.cjs: reproduced the polygon/reachability discrepancy using the checked-out TypeScript modules and a synthetic flat DEM.
• go test ./...: unavailable locally (go: command not found). The Go CI failure inspected is in coverage tooling: go-test-coverage@v2.20.0 requires Go >=1.27 while the runner uses 1.26.5 with GOTOOLCHAIN=local; this is not evidence of a playback defect.
• No rendered-browser or end-to-end check was performed. Findings above are source/test and numerical reproductions, not visual observations.

Earlier discussions

GraphQL enumeration returned no review threads (hasNextPage: false), so there were no comments to paginate or discussions to resolve, retain, or skip. No thread mutations were made. The PR head was rechecked and still matches the reviewed commit. The checkout was not modified, committed, or pushed.

[fank]

Ratatoskr is reviewing this pull request automatically again.

It had stopped at its limit of 10 review slots. The limit is now 20, so automated reviews resumed.

[fank]

Ratatoskr reviewed this pull request.

Changes requested on 8d3c3014. See the review.

Finished 2026-10-17 21:09 UTC.

[fank]

REQUEST_CHANGES — 1 correctness finding.

Review details

Reviewed head 8d3c301445fb6ad01f39d02daff5c4b19cb8bbc8 against base dcb58a1a9e948a4bdae56745f1772d2de2d4d209, focusing on storage/conversion, HTTP delivery, playback state, and the new event/profile/link paths. GitHub's diff endpoint exceeded its 20,000-line limit, so the local base-to-head diff was used.

Required change

[P2] Center shared-link targets after their chunk loads — ui/src/pages/recording-playback/RecordingPlayback.tsx:196–199.

For a protobuf recording, startup loads only chunk 0. Opening a link such as ?t=650&u=1 (300-frame chunks) starts an asynchronous seek and immediately calls followEntity(1). That method pans only if the target snapshot already exists. When chunk 2 arrives, the engine recomputes snapshots but does not pan; camera-follow updates otherwise happen only in the playing tick. The link therefore remains on the initial map view while paused, rather than showing the shared player/location. Wait for the target frame to load before the initial pan, or recenter the followed entity when the asynchronous seek completes. Add a regression test covering a paused link into an initially unloaded chunk.

Reproduced with an isolated Vitest test outside the checkout: after seeking to frame 650 and following unit 1, its asynchronously loaded snapshot was [9000,8000], but the renderer's setView had zero calls. This is a state-level reproduction, not a rendered visual check.

Verification

• With Node 24.21.0 on PATH, npm --prefix ui test: 108 test files passed, 1 skipped; 2,161 tests passed, 13 skipped.
• ./ui/node_modules/.bin/tsc --noEmit -p ui/tsconfig.json: passed.
• With Go 1.26.6 on PATH, go test ./...: not a clean pass. SQLite-backed server/convertcli tests fail because CGO is disabled and no C compiler is installed. Conversion, storage, maptool, maptoolcli, precompresscli, and command-package tests passed. These environment failures are not treated as PR defects.
• From ui/, ./node_modules/.bin/vitest run --config /tmp/ocap556-review-tests/vitest.config.mjs: 1 expected regression-test failure, described above.
• Inspected failed GitHub Go-job diagnostics: the coverage action selects go-test-coverage@v2.20.0, which requires Go >=1.27, while the runner uses Go 1.26.5 with GOTOOLCHAIN=local. This tooling mismatch is not the basis for this verdict.
• No browser-rendered inspection performed. Checkout left unchanged; no commits or pushes.

Earlier discussions

GraphQL enumeration returned zero review threads, with no additional page, both initially and before publication. Resolved: 0; still-open earlier discussions: 0; skipped: 0.

[fank]

Ratatoskr reviewed this pull request.

Changes requested on af4c2b56. See the review.

Finished 2026-10-17 21:41 UTC.

[fank]

REQUEST_CHANGES — 1 blocking correctness finding.

Review details

Reviewed head af4c2b569f06825284bc16ddc9c0988623d7e414 against base dcb58a1a9e948a4bdae56745f1772d2de2d4d209.

Required change

[P2] Preserve the missing-Stop cap when another Start arrives — ui/src/playback/comms.ts:310–316.

The second Start unconditionally replaces the previous transmission's capped endFrame with the new Start's frame. If a Stop is missing and that player next transmits on the same net much later, this invents continuous activity throughout the gap. Reproduced directly against the current module: Starts at frames 0 and 3600, with no Stops, produce an initial span ending at 3600, despite TRANSMISSION_MAX_FRAMES = 60; transmissionsAtFrame(spans, 1800) incorrectly returns that transmission. Both the Comms tab and map overlay consume these spans, so they display a live transmission while nobody is transmitting.

Keep the earlier capped endpoint when closing an unmatched Start (for example, take the minimum of its existing endpoint and the new Start frame). Add a regression covering a second Start after the cap, while retaining the existing behavior for a second Start before the cap.

Verification

• cd ui && npm test: 108 test files passed, 1 skipped; 2,164 tests passed, 13 skipped. jsdom emitted unsupported canvas/scroll warnings.
• cd ui && npx tsc --noEmit: passed.
• cd ui && npm run build -- --outDir /tmp/ocap556-ui-build: passed; output kept outside the checkout.
• go test ./internal/server -run '^(TestAcceptedEncodings|TestAcceptsEncoding|TestDataContentType|TestServePrecompressed_.*|TestStaticCacheControl|TestSpaFileServer_.*|TestGetDataServesPlayerSnapshotSidecar|TestIntegration_PrecompressedArtifactServing)$': passed with installed Go 1.27.2.
• go test ./...: not a clean pass locally. Server/convertcli database tests fail because this environment compiles without CGO and go-sqlite3 requires it; no C compiler is available. Storage, conversion, maptool, maptoolcli, precompresscli and command-package tests passed.
• Inspected the failed GitHub Go job: the coverage action invokes go-test-coverage@v2.20.0, which requires Go >=1.27, under Go 1.26.5 with GOTOOLCHAIN=local. This tooling incompatibility is not the basis for the requested changes.
• Reproduced the finding through Vite's ssrLoadModule using the current comms.ts; this was a logic check, not a rendered UI check. No browser/E2E verification performed.

Earlier discussions

GraphQL review-thread enumeration returned no threads (hasNextPage: false), so there were no existing discussions to continue or resolve. No thread mutations performed. The checkout was not edited, committed or pushed.

[fank]

Ratatoskr reviewed this pull request.

Approved on 80fe9caf. See the review.

Finished 2026-10-17 21:54 UTC.

[fank]

APPROVE — 0 blocking findings.

Review details

Reviewed head 80fe9cafd13187f2eddbf84481addbb952322a7d against base dcb58a1a9e948a4bdae56745f1772d2de2d4d209, starting from the local diff because GitHub rejected the oversized PR diff. No actionable blocking defect found in the reviewed conversion/snapshot sidecar, compression delivery, stats backfill, decoder, event attribution, playback/deep-link, and player/comms/ORBAT paths and their tests.

Verification:

• cd ui && npm test: 108 test files passed, 1 skipped; 2,166 tests passed, 13 skipped.
• cd ui && npx tsc --noEmit: passed.
• cd ui && npx eslint .: exit 0; two unused-variable warnings, no errors.
• cd ui && npm run build: passed; bundle-size warning.
• go test ./...: unavailable locally (go: command not found). GitHub's Go job reports its test and build steps succeeded. Its overall failure is in coverage tooling: go-test-coverage v2.20.0 requires Go >=1.27 while the runner uses Go 1.26.5 with GOTOOLCHAIN=local. This is not evidence of a playback defect; the coverage check remains unsuccessful.
• No browser-rendered UI or end-to-end verification performed; presentation remains unverified.

Discussions: GraphQL enumeration returned no review threads and no additional thread pages. Resolved: 0; still open: 0; skipped: 0. PR head rechecked before publication; tracked checkout unchanged. No commits, pushes, or merge performed.