Draft: add viewer session action model context - #201
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Cache: Disabled due to Reviews > Disable Cache setting 📝 WalkthroughWalkthroughServer now projects writeLane, enabledActions/blockedActions, and actionContext into session surfaces/action models. Client ScreenTable reads these fields, validates action availability by id before POSTing to writeLane.endpoint (fallback /move), and renders consequence-due badges. Tests cover surface shape and client JSX usage. ChangesAction Availability Gating and Write-Lane Routing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
viewer/openworlds/screen-table.jsx (1)
136-136:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winError message references hardcoded
/movebut endpoint is configurable.The endpoint is now
writeLane.endpoint(line 124), but this error fallback still says/move. If the server configures a different endpoint, the message will be misleading.Suggested fix
- toast({ kind: "danger", title: "Move not sent", body: error?.message || "The viewer could not reach /move." }); + toast({ kind: "danger", title: "Move not sent", body: error?.message || `The viewer could not reach ${writeLane.endpoint || "/move"}.` });🤖 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 `@viewer/openworlds/screen-table.jsx` at line 136, The error toast message incorrectly hardcodes "/move"; update the toast call that currently uses toast({ kind: "danger", title: "Move not sent", body: error?.message || "The viewer could not reach /move." }) to reference the configured endpoint variable (writeLane.endpoint) in its fallback text so it reads something like "The viewer could not reach {writeLane.endpoint}." and retain error?.message when available; ensure you locate this change at the toast invocation in the same component where writeLane.endpoint is defined/used.
🤖 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/tests/test_session_surface.py`:
- Line 255: The test is fragile because it indexes blockedActions[0]; instead,
locate the blocked action by its action ID and assert its disabled_reason, e.g.
build or use the existing mapping pattern used earlier (like lines that access
action maps) to find blocked_action = blocked_actions_by_id[<ACTION_ID>] or use
a comprehension/next(...) to find the entry where ["id"] == "<ACTION_ID>", then
assert blocked_action["disabled_reason"] == "no live move sink"; update
references to use the `surface` variable and its `actionModel["blockedActions"]`
list and the exact action ID string used elsewhere in the test.
---
Outside diff comments:
In `@viewer/openworlds/screen-table.jsx`:
- Line 136: The error toast message incorrectly hardcodes "/move"; update the
toast call that currently uses toast({ kind: "danger", title: "Move not sent",
body: error?.message || "The viewer could not reach /move." }) to reference the
configured endpoint variable (writeLane.endpoint) in its fallback text so it
reads something like "The viewer could not reach {writeLane.endpoint}." and
retain error?.message when available; ensure you locate this change at the toast
invocation in the same component where writeLane.endpoint is defined/used.
🪄 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
Run ID: 66a1d6ea-e7f4-433c-b81f-9fc01afb8def
📒 Files selected for processing (4)
viewer/openworlds/screen-table.jsxviewer/server.pyviewer/tests/test_openworlds_static.pyviewer/tests/test_session_surface.py
📜 Review details
🧰 Additional context used
🧬 Code graph analysis (1)
viewer/server.py (1)
viewer/tests/test_session_surface.py (1)
build_session_surface(91-91)
🔇 Additional comments (12)
viewer/server.py (6)
854-865: LGTM!
868-876: LGTM!
879-910: LGTM!
913-932: LGTM!
1065-1072: LGTM!Also applies to: 1089-1089, 1108-1119
3666-3671: LGTM!viewer/tests/test_session_surface.py (1)
193-254: LGTM!Also applies to: 256-261
viewer/tests/test_openworlds_static.py (1)
131-143: LGTM!viewer/openworlds/screen-table.jsx (4)
26-31: LGTM!
38-41: LGTM!
148-148: LGTM!Also applies to: 158-158, 171-171
340-342: LGTM!
|
Addressed the current-head CodeRabbit review items in a2f5935.\n\nValidation from /Volumes/LEXAR/repos/ClawDnD-session-action-model:\n- python3 -m unittest viewer.tests.test_session_surface -q: OK (6 tests)\n- python3 -m unittest viewer.tests.test_openworlds_static -q: OK (16 tests)\n- python3 -m py_compile viewer/server.py: OK\n- python3 scripts/license_check.py: OK\n- git diff --check: OK\n\nKept the PR in draft; no merge requested. |
a2f5935 to
eb4c026
Compare
1 participant
Summary
Refs #115, #61.
This is the early visible Lane D draft for the viewer session action model. It keeps the engine as state authority and preserves the existing
/movewrite lane, while making the/session-surfaceread model more explicit for the OpenWorlds Table.Changes:
writeLanemetadata to the session/action read model (/move,POST, engine authority, player move intent, no campaign snapshot writes)enabledActionsandblockedActionsbuckets with concise disabled reasons, while keeping the existingavailableActionsshape for compatibilityactionContextfor scene, active quests, and consequence timing counts/signals without leaking notes, DM fields, private lore, or consequence notesNotes
This intentionally does not change engine state authority or add browser-side snapshot writes. Unsupported/non-live actions remain disabled with reasons.
PR #150 is intentionally out of scope.
Validation
Ran locally from
/Volumes/LEXAR/repos/ClawDnD-session-action-model:All commands exited 0.
Summary by CodeRabbit
New Features
Improvements
Tests