Conversation
The e2e 'the cells it reveals are interrogable' has failed in CI on every PR since #256 and never once locally. enableCellLayer waited only for the first cells to appear — ring 2's — and returned while rings 3 and 4 were still republishing and re-rendering every path on the map. Switching that layer on is a refetch, not a redraw, so the whole progressive cycle runs. The click therefore landed on a path about to be replaced, no dispatch happened, and the details panel stayed hidden with no error anywhere — the explain reply is dropped when the selection no longer matches, and that branch is silent by design. This is the third appearance of the failure waitForRefresh was written for, and its own comment names this exact case. The helper now waits for the widening, as every other refetching path already does. Not a product bug: the demo was never broken. The gate was.
📝 WalkthroughWalkthroughThe Playwright fixture now waits for cell-layer refresh completion. The timing report records the 2026-08-06 run, including updated durations, test counts, summaries, and JSON history. ChangesCell-layer refresh and timing records
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude finished @cs-util's task in 3m 30s —— View job Review — PR #262
VerdictThe fix is correct, and the mechanism holds up under checking:
One finding, posted inline: the helper's JSDoc ( Not commented on per the repo guidelines: the regenerated I did not run the e2e suite — dependencies and Playwright browsers are not installed in this checkout, and this is exactly the failure that only reproduces on a loaded CI runner, so CI is the meaningful signal here. |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
gps-plus-slam | 518fd7d | Commit Preview URL Branch Preview URL |
Aug 06 2026, 02:49 AM |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // AND THEN WAIT FOR THE WIDENING, for the reason `waitForRefresh` spells out | ||
| // above: the first cells to appear are ring 2's, and rings 3 and 4 each | ||
| // REPUBLISH a larger snapshot, re-rendering every path on the map. | ||
| // | ||
| // Switching this layer on is a REFETCH, not a redraw — cells are data-gated | ||
| // since round 10 stage B — so the whole progressive cycle runs, and a caller | ||
| // that only waited for the first cells was racing it. That is the third | ||
| // appearance of the failure `waitForRefresh` was written for: a cell clicked | ||
| // in ring 2 gets re-rendered before the click lands, the dispatch never | ||
| // happens, and the details panel silently stays hidden. | ||
| // | ||
| // It surfaced as a CI-only failure of "the cells it reveals are | ||
| // interrogable", deterministic on a slower runner and never reproducible | ||
| // locally. | ||
| await waitForRefresh(page); |
There was a problem hiding this comment.
The mechanism checks out (writeStatus runs in the same synchronous snapshot subscriber that calls drawMap, and fetchStarted fires once per cycle rather than per ring, so widening… is continuously on screen from the moment the first cells paint until the final ring) — but two comment claims above this block no longer match the code:
-
The JSDoc at line 614 now reads as a contradiction with the body: "
waitForRefreshis the wrong tool for this: it waits for the ABSENCE of 'widening' … so it returns immediately and the test races anyway." It is the wrong tool on its own; after the cell wait it is exactly right, which is what this new block does. Worth a two-word amendment so the next reader doesn't "fix" the apparent contradiction by deleting the new call. -
"Switching this layer on is a REFETCH, not a redraw" is only true when the held snapshot has no cells.
layersNeedingData(off, on, { cells: 931 })returns[](src/layers.tsviasrc/main.ts:983), so an off→on flick within one position — whichosm-demo.spec.js:3662/:3687does — is a plain redraw with no widening at all. The helper still behaves correctly there (both waits pass instantly), but the stated reason is broader than the rule.
| // AND THEN WAIT FOR THE WIDENING, for the reason `waitForRefresh` spells out | |
| // above: the first cells to appear are ring 2's, and rings 3 and 4 each | |
| // REPUBLISH a larger snapshot, re-rendering every path on the map. | |
| // | |
| // Switching this layer on is a REFETCH, not a redraw — cells are data-gated | |
| // since round 10 stage B — so the whole progressive cycle runs, and a caller | |
| // that only waited for the first cells was racing it. That is the third | |
| // appearance of the failure `waitForRefresh` was written for: a cell clicked | |
| // in ring 2 gets re-rendered before the click lands, the dispatch never | |
| // happens, and the details panel silently stays hidden. | |
| // | |
| // It surfaced as a CI-only failure of "the cells it reveals are | |
| // interrogable", deterministic on a slower runner and never reproducible | |
| // locally. | |
| await waitForRefresh(page); | |
| // AND THEN WAIT FOR THE WIDENING, for the reason `waitForRefresh` spells out | |
| // above: the first cells to appear are ring 2's, and rings 3 and 4 each | |
| // REPUBLISH a larger snapshot, re-rendering every path on the map. | |
| // | |
| // Switching this layer on is a REFETCH rather than a redraw WHENEVER THE HELD | |
| // SNAPSHOT HAS NO CELLS — they are data-gated since round 10 stage B, and | |
| // `layersNeedingData` refetches only in that case — so the whole progressive | |
| // cycle runs and a caller that only waited for the first cells was racing it. | |
| // (After an off/on flick within one position the cells are already held, no | |
| // refetch happens, and both waits below pass immediately.) That is the third | |
| // appearance of the failure `waitForRefresh` was written for: a cell clicked | |
| // in ring 2 gets re-rendered before the click lands, the dispatch never | |
| // happens, and the details panel silently stays hidden. | |
| // | |
| // It surfaced as a CI-only failure of "the cells it reveals are | |
| // interrogable", deterministic on a slower runner and never reproducible | |
| // locally. | |
| // | |
| // The paragraph above about `waitForRefresh` being the wrong tool is about | |
| // using it ALONE: after the cell wait its widening check is exactly right. | |
| await waitForRefresh(page); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@GpsPlusSlamJs_OsmDemo/playwright-tests/fixtures.js`:
- Line 640: Update the refresh flow around waitForRefresh(page) to bind
completion to the asynchronous `#layer-cells` update rather than the generic
visible status. Require a layer-specific loading/status transition before
validating width completion, or remove this duplicate wait if that transition is
already awaited elsewhere.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 923fb605-4e4e-4123-adf3-55c301c76681
📒 Files selected for processing (2)
GpsPlusSlamJs_OsmDemo/docs/test-timings.mdGpsPlusSlamJs_OsmDemo/playwright-tests/fixtures.js
| // It surfaced as a CI-only failure of "the cells it reveals are | ||
| // interrogable", deterministic on a slower runner and never reproducible | ||
| // locally. | ||
| await waitForRefresh(page); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify that enabling the layer produces a refresh-specific status transition
# before the first cell path appears, and that completion follows the last ring.
rg -n -C 25 \
'waitForRefresh|enableCellLayer|layer-cells|writeStatus|widening|snapshot\.radius|affordance-cell' \
GpsPlusSlamJs_OsmDemoRepository: cs-util-com/location-based-webxr
Length of output: 50389
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Relevant test helper definitions"
sed -n '550,660p' GpsPlusSlamJs_OsmDemo/playwright-tests/fixtures.js
echo
echo "## Application source files that may contain layer/status/ring code"
git ls-files GpsPlusSlamJs_OsmDemo | rg '\.(js|ts|mjs)$' | sed -n '1,120p'
echo
echo "## Precise searches in source files"
rg -n -C 8 -g '*.js' -g '*.ts' -g '*.mjs' \
'layer-cells|`#layer-cells`|writeStatus|status\.innerHTML|affordance-cell|snapshot\.radius|widening|fetchWorkingSet|refresh\(' GpsPlusSlamJs_OsmDemo | sed -n '1,260p'Repository: cs-util-com/location-based-webxr
Length of output: 35199
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Read relevant fixtures helper and nearby callers"
sed -n '550,720p' GpsPlusSlamJs_OsmDemo/playwright-tests/fixtures.js
echo
echo "## Narrow application search with file filters"
rg -n -C 6 -g '*.js' -g '*.ts' -g '*.mjs' \
'layer-cells|`#layer-cells`|writeStatus|innerHTML|affordance-cell|snapshot\.radius|widening|fetchWorkingSet|refresh\(\)' GpsPlusSlamJs_OsmDemo | sed -n '1,320p'Repository: cs-util-com/location-based-webxr
Length of output: 33508
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## waitForRefresh definition"
sed -n '220,280p' GpsPlusSlamJs_OsmDemo/playwright-tests/fixtures.js
echo
echo "## refresh-cycle relevant implementation"
sed -n '1,260p' GpsPlusSlamJs_OsmDemo/src/refresh-cycle.ts
echo
echo "## status/statusReady/status text writers"
rg -n -C 6 -g '*.ts' -g '*.js' 'statusReady|writeStatus|snapshot\.radius|radius\s*:' GpsPlusSlamJs_OsmDemo/src | sed -n '1,260p'Repository: cs-util-com/location-based-webxr
Length of output: 36412
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## main.ts status/loading reducers and snapshot subscriber"
sed -n '240,310p' GpsPlusSlamJs_OsmDemo/src/main.ts
rg -n -C 8 -g '*.ts' 'snapshotReady|writeStatus|writingPhase|radius|isFinalRing' GpsPlusSlamJs_OsmDemo/src/main.ts GpsPlusSlamJs_OsmDemo/src/osm-store.ts GpsPlusSlamJs_OsmDemo/src/main.ts
echo
echo "## main.ts writeStatus implementation"
sed -n '765,830p' GpsPlusSlamJs_OsmDemo/src/main.ts
echo
echo "## osm-store slice around loading/snapshotReady"
sed -n '1,220p' GpsPlusSlamJs_OsmDemo/src/osm-store.ts
echo
echo "## layer toggles implementation"
rg -n -C 8 -g '*.ts' 'layer-cells|cells:' GpsPlusSlamJs_OsmDemo/src | sed -n '1,260p'Repository: cs-util-com/location-based-webxr
Length of output: 50388
Bound the refresh wait to #layer-cells.
waitForRefresh(page) only waits for a visible final status and no "widening" text. Since enabling #layer-cells is asynchronous, that count can reflect the initial refresh. The helper should require a cell-layer-specific loading/status change before waiting for the width completion, or skip this duplicate wait.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@GpsPlusSlamJs_OsmDemo/playwright-tests/fixtures.js` at line 640, Update the
refresh flow around waitForRefresh(page) to bind completion to the asynchronous
`#layer-cells` update rather than the generic visible status. Require a
layer-specific loading/status transition before validating width completion, or
remove this duplicate wait if that transition is already awaited elsewhere.
1 participant
Fixes the e2e race that has made CI red on every PR since #256, and never once locally.
Summary by CodeRabbit
Documentation
Bug Fixes