feat(combat): browser grid combat-with-movement + grid-extent fix (#10)#1173
Merged
Conversation
Lane A of the S2 plan — OBJECTIVE-1's functional core, on the reliable web path: - viewer/server.py `_scene_grid_block` (#10): the board EXTENT now follows the combat tactical grid (combat.grid_width/grid_height) when combat declares one, so a token never lands off-board (a 20x20 fight previously reported a 16x10 board). Additive (no combat grid ⇒ today's 16x10 default). 5 unit tests. - viewer/openworlds/screen-combat.jsx: render a real cols×rows tactical board when the engine sends authoritative cells (grid.mode=="grid") — the slot the existing comment reserved. Click an empty cell → POST move_to_cell; click a foe → POST attack (echo turnToken, gate can_act); apply the refreshed surface the grid lane returns. The engine stays the SOLE WRITER — the board only posts intents. Verified end-to-end in the browser preview against a live 14x10 combat-on-tavern demo: click an empty cell walks the hero (engine-resolved, movement-budget-validated); click the adjacent goblin resolves an attack, advances the turn, the enemy auto-runs, the round advances. Full viewer suite 786 passed, Tier-0 GREEN. The /move sole-writer is unchanged (this adds a client, not a write path).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (5)
|
| Layer / File(s) | Summary |
|---|---|
Board extents from combat grid viewer/server.py, viewer/tests/test_move_intents.py |
_scene_grid_block keeps scene-grid fields when present and uses combat grid_width/grid_height for cols/rows; tests cover the default, malformed, oversized, and mixed scene-grid cases. |
Move and attack intents viewer/openworlds/screen-combat.jsx |
postCombatIntent posts /move with turn_token, move_to_cell, or attack, records the local intent, updates the combat surface from payload.combat or reloads it, and the screen passes the new callbacks and canAct into CombatMap. |
Grid board rendering viewer/openworlds/screen-combat.jsx |
CombatGridBoard renders tactical cells, token markers, and cell click actions, and CombatMap returns it when grid mode and dimensions are valid. |
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Confidence: 91%
Possibly related PRs
- electricsheephq/WorldOS#1046: Shares the combat grid dimension fields that
_scene_grid_blocknow uses to size the board. - electricsheephq/WorldOS#1171: Uses the same
turn_token-based/moveintent shapes that the viewer now posts.
Suggested labels
graphics, cap:c1
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly matches the main change: browser grid combat with movement plus a grid-extent fix. |
| Description check | ✅ Passed | The description covers summary and validation, but the CLA section is missing the required checkbox items from the template. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
feat/browser-combat-movement
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@viewer/openworlds/screen-combat.jsx`:
- Around line 490-528: The CombatGridBoard render logic currently treats every
empty square as a valid move target, ignoring the engine-provided terrain data.
Update CombatGridBoard to derive each cell’s walkability from grid.cells and
grid.cellDefault, then use that per-cell state in the cells map so blocked tiles
are styled differently and do not trigger onCellMove. Keep the existing
token/attack behavior intact, but only allow movement clicks on walkable cells.
- Around line 523-533: The tactical board cell handling in screen-combat.jsx is
blocking selection behind the action gate and only supports mouse clicks, unlike
the zone renderer. Update the cell interaction logic around the
clickable/onClick block so onSelect can still work even when canAct is false,
and add proper keyboard accessibility for actionable cells (button semantics or
tabIndex with Enter/Space handlers plus focus styling) so tokens remain
inspectable and usable for non-mouse users.
In `@viewer/tests/test_move_intents.py`:
- Around line 242-264: The current tests for _scene_grid_block() only cover
combat data in isolation, so add a mixed-source regression case that includes
both locations[current_location_id].scene_grid and combat in the input. In that
test, verify that combat.grid_width/grid_height still control the returned
cols/rows, while scene-grid metadata such as sceneId, cells, and cellDefault is
preserved and not overwritten by scene_grid.grid.cols/rows. Use
_scene_grid_block() and the existing scene_grid/combat merge behavior to lock in
the intended precedence.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro Plus
Run ID: 26128e0e-be9c-4006-a201-b0207840f3b7
📒 Files selected for processing (3)
viewer/openworlds/screen-combat.jsxviewer/server.pyviewer/tests/test_move_intents.py
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: test
- GitHub Check: viewer-tests
- GitHub Check: Analyze (python)
- GitHub Check: test
- GitHub Check: viewer-tests
⚠️ CI failures not shown inline (2)
GitHub Actions: LLM Quality Gate (advisory) / quality-gate: feat(combat): browser grid combat-with-movement + grid-extent fix (#10)
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1mecho "[selfcheck] bash -n on the duo runner + scorer"�[0m
�[36;1mbash -n qa/run_duo.sh�[0m
�[36;1mbash -n qa/score.sh�[0m
�[36;1mtest -x qa/run_duo.sh�[0m
�[36;1mtest -x qa/score.sh�[0m
�[36;1m�[0m
�[36;1mecho "[selfcheck] regression detector imports + answers --help (pure reader, no DB write)"�[0m
�[36;1muv run --directory servers/engine python "${GITHUB_WORKSPACE}/qa/detect_regression.py" --help >/dev/null�[0m
�[36;1m�[0m
�[36;1mecho "[selfcheck] scorer guard path emits a hashed artifact into a TEMP dir (gateway-free)"�[0m
�[36;1mtmp="$(mktemp -d)"�[0m
�[36;1mprintf '# transcript\n' > "$tmp/t.md"�[0m
�[36;1mprintf '{}\n' > "$tmp/state.json"�[0m
�[36;1mWORLDOS_SCORE_GUARD_ONLY=1 bash qa/score.sh \�[0m
�[36;1m "$tmp/t.md" "$tmp/state.json" qa/rubric.md qa/score_schema.json "$tmp/out.json" 0.01�[0m
�[36;1mtest -s "$tmp/out.json"�[0m
�[36;1mrm -rf "$tmp"�[0m
�[36;1m�[0m
�[36;1mecho "[selfcheck] jq present (scorecard tool)"�[0m
�[36;1mcommand -v jq >/dev/null || { echo "::error title=LLM Quality Gate::jq missing on runner"; exit 1; }�[0m
GitHub Actions: LLM Quality Gate (advisory) / 0_quality-gate.txt: feat(combat): browser grid combat-with-movement + grid-extent fix (#10)
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1mecho "[selfcheck] bash -n on the duo runner + scorer"�[0m
�[36;1mbash -n qa/run_duo.sh�[0m
�[36;1mbash -n qa/score.sh�[0m
�[36;1mtest -x qa/run_duo.sh�[0m
�[36;1mtest -x qa/score.sh�[0m
�[36;1m�[0m
�[36;1mecho "[selfcheck] regression detector imports + answers --help (pure reader, no DB write)"�[0m
�[36;1muv run --directory servers/engine python "${GITHUB_WORKSPACE}/qa/detect_regression.py" --help >/dev/null�[0m
�[36;1m�[0m
�[36;1mecho "[selfcheck] scorer guard path emits a hashed artifact into a TEMP dir (gateway-free)"�[0m
�[36;1mtmp="$(mktemp -d)"�[0m
�[36;1mprintf '# transcript\n' > "$tmp/t.md"�[0m
�[36;1mprintf '{}\n' > "$tmp/state.json"�[0m
�[36;1mWORLDOS_SCORE_GUARD_ONLY=1 bash qa/score.sh \�[0m
�[36;1m "$tmp/t.md" "$tmp/state.json" qa/rubric.md qa/score_schema.json "$tmp/out.json" 0.01�[0m
�[36;1mtest -s "$tmp/out.json"�[0m
�[36;1mrm -rf "$tmp"�[0m
�[36;1m�[0m
�[36;1mecho "[selfcheck] jq present (scorecard tool)"�[0m
�[36;1mcommand -v jq >/dev/null || { echo "::error title=LLM Quality Gate::jq missing on runner"; exit 1; }�[0m
🧰 Additional context used
🪛 ast-grep (0.44.0)
viewer/openworlds/screen-combat.jsx
[warning] 183-183: Avoid using the initial state variable in setState
Context: setBusyAction(label)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
[warning] 534-534: A list component should have a key to prevent re-rendering
Context: <GridCellToken t={t} selected={selected === t.id} isCurrent={t.id === curId} />
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(list-component-needs-key)
… mixed-source test (#1173) - screen-combat.jsx: derive per-cell walkability from grid.cells/cellDefault (the engine wall/prop map); blocked cells are styled + NOT move targets (no more illegal move_to_cell into a wall relying on /move rejection to correct the UI). - screen-combat.jsx: token SELECTION now bypasses the can_act gate (inspect tokens on any turn, like the zone renderer); actionable cells get button semantics (role/tabIndex + Enter/Space handler + focus ring). - test_move_intents.py: add a mixed scene_grid+combat case locking the precedence — combat owns cols/rows while the scene_grid sceneId/cells/cellDefault survive. (ast-grep nits refuted: line 183 setBusyAction(label) — `label` is a param, not the initial state var; GridCellToken "needs key" — the key is on the parent cell div, GridCellToken is a single conditional child.) Re-verified in the browser preview (board renders, no console errors); 24 move-intents tests pass.
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 `@viewer/openworlds/screen-combat.jsx`:
- Around line 500-505: The grid walkability logic in the combat screen only
tracks blocked cells, so explicit per-cell walkable:true overrides are ignored
when cellDefault.walkable is false. Update the walkability lookup around the
blocked/defaultWalkable/isWalkable logic to record both blocked and explicit
walkable cells from grid.cells, and have isWalkable(x, y) return true for
explicit walkable overrides before falling back to the grid default.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro Plus
Run ID: 83e8b804-89ee-45e4-9a32-1ca333e37b62
📒 Files selected for processing (2)
viewer/openworlds/screen-combat.jsxviewer/tests/test_move_intents.py
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: test
- GitHub Check: viewer-tests
- GitHub Check: Analyze (python)
- GitHub Check: viewer-tests
- GitHub Check: test
⚠️ CI failures not shown inline (2)
GitHub Actions: LLM Quality Gate (advisory) / 0_quality-gate.txt: feat(combat): browser grid combat-with-movement + grid-extent fix (#10)
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1mecho "[selfcheck] bash -n on the duo runner + scorer"�[0m
�[36;1mbash -n qa/run_duo.sh�[0m
�[36;1mbash -n qa/score.sh�[0m
�[36;1mtest -x qa/run_duo.sh�[0m
�[36;1mtest -x qa/score.sh�[0m
�[36;1m�[0m
�[36;1mecho "[selfcheck] regression detector imports + answers --help (pure reader, no DB write)"�[0m
�[36;1muv run --directory servers/engine python "${GITHUB_WORKSPACE}/qa/detect_regression.py" --help >/dev/null�[0m
�[36;1m�[0m
�[36;1mecho "[selfcheck] scorer guard path emits a hashed artifact into a TEMP dir (gateway-free)"�[0m
�[36;1mtmp="$(mktemp -d)"�[0m
�[36;1mprintf '# transcript\n' > "$tmp/t.md"�[0m
�[36;1mprintf '{}\n' > "$tmp/state.json"�[0m
�[36;1mWORLDOS_SCORE_GUARD_ONLY=1 bash qa/score.sh \�[0m
�[36;1m "$tmp/t.md" "$tmp/state.json" qa/rubric.md qa/score_schema.json "$tmp/out.json" 0.01�[0m
�[36;1mtest -s "$tmp/out.json"�[0m
�[36;1mrm -rf "$tmp"�[0m
�[36;1m�[0m
�[36;1mecho "[selfcheck] jq present (scorecard tool)"�[0m
�[36;1mcommand -v jq >/dev/null || { echo "::error title=LLM Quality Gate::jq missing on runner"; exit 1; }�[0m
GitHub Actions: LLM Quality Gate (advisory) / quality-gate: feat(combat): browser grid combat-with-movement + grid-extent fix (#10)
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1mecho "[selfcheck] bash -n on the duo runner + scorer"�[0m
�[36;1mbash -n qa/run_duo.sh�[0m
�[36;1mbash -n qa/score.sh�[0m
�[36;1mtest -x qa/run_duo.sh�[0m
�[36;1mtest -x qa/score.sh�[0m
�[36;1m�[0m
�[36;1mecho "[selfcheck] regression detector imports + answers --help (pure reader, no DB write)"�[0m
�[36;1muv run --directory servers/engine python "${GITHUB_WORKSPACE}/qa/detect_regression.py" --help >/dev/null�[0m
�[36;1m�[0m
�[36;1mecho "[selfcheck] scorer guard path emits a hashed artifact into a TEMP dir (gateway-free)"�[0m
�[36;1mtmp="$(mktemp -d)"�[0m
�[36;1mprintf '# transcript\n' > "$tmp/t.md"�[0m
�[36;1mprintf '{}\n' > "$tmp/state.json"�[0m
�[36;1mWORLDOS_SCORE_GUARD_ONLY=1 bash qa/score.sh \�[0m
�[36;1m "$tmp/t.md" "$tmp/state.json" qa/rubric.md qa/score_schema.json "$tmp/out.json" 0.01�[0m
�[36;1mtest -s "$tmp/out.json"�[0m
�[36;1mrm -rf "$tmp"�[0m
�[36;1m�[0m
�[36;1mecho "[selfcheck] jq present (scorecard tool)"�[0m
�[36;1mcommand -v jq >/dev/null || { echo "::error title=LLM Quality Gate::jq missing on runner"; exit 1; }�[0m
🔇 Additional comments (1)
viewer/tests/test_move_intents.py (1)
237-285: LGTM!
…oard (#1173) CodeRabbit round-2: the terrain map only recorded BLOCKED cells, so a scene with cellDefault.walkable=false + explicit walkable:true floors would wrongly treat those floors as non-walkable. Now record the full per-cell override (true OR false); isWalkable returns the override when present, else falls back to cellDefault. No current scene_grid emitter sets cellDefault.walkable=false, but the contract allows it. JSX transpiles clean (vendored babel).
coderabbitai
Bot
added
the
cap:c1
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Lane A of the S2 plan — OBJECTIVE-1's functional core (turn-based combat WITH MOVEMENT), on the reliable web path. Builds on the merged keystone (#1171): the player-turn arbiter already resolves
move_to_cell/on-turnattackover/move; this adds the grid board that consumes it + a board-extent fix so tokens never fall off-board.viewer/server.py_scene_grid_block(Epic 9: AI Companion #10): the rendered board EXTENT now follows the combat tactical grid (combat.grid_width/grid_height) when combat declares one, so a token placed on a >16×10 grid isn't clipped off-board (a 20×20 fight previously reported a 16×10 board). Additive — no explicit combat grid ⇒ today's 16×10 default / scene_grid extent. +5 unit tests.viewer/openworlds/screen-combat.jsx: render a realcols×rowstactical board when the engine sends authoritative cells (grid.mode==="grid") — the slot the existingCombatMapcomment explicitly reserved. Click an empty cell → POSTmove_to_cell; click a foe → POSTattack(echoturnToken, gate oncan_act); apply the refreshed surface the grid/movelane returns. The engine stays the SOLE WRITER — the board only posts intents; zones render unchanged for non-grid surfaces.Validation
End-to-end in the browser preview against a live 14×10 combat-on-tavern demo:
curlcross-check: surface reports the correct 14×10 grid;move_to_cellmoves the token./movesole-writer path unchanged (this adds a client + a presentation-only extent read, not a write path).Licensing / CLA
Source-available WorldOS; additive change, no new deps.