[codex] deterministic decoupling: ui, sim runtime, and persistence boundaries#53
Open
Vannon0911 wants to merge 4 commits intomainfrom
Open
[codex] deterministic decoupling: ui, sim runtime, and persistence boundaries#53Vannon0911 wants to merge 4 commits intomainfrom
Vannon0911 wants to merge 4 commits intomainfrom
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughWalkthroughThis pull request refactors the codebase to establish canonical module locations for simulation runtime logic and restructure persistence driver architecture. Simulation runtime implementations move from Changes
Sequence DiagramsequenceDiagram
participant App as App/Test Entry
participant Kernel as Kernel Store
participant Driver as Storage Driver
participant Persist as Persistence Layer
App->>Kernel: createStore(manifest, {reducer, simStep}, {storageDriver: getDefaultWebDriver()})
Kernel->>Driver: new driver instance (WebDriver or NullDriver)
Kernel->>Persist: Initialize persistence module
Persist->>Driver: Call driver.load()
Driver->>Persist: Return stored document or null
Persist->>Kernel: Provide hydrated or initial state
App->>Kernel: dispatch(actions...)
Kernel->>Driver: On state changes, call driver.save(doc)
Driver->>Persist: Write document to localStorage
Persist->>Driver: Confirm save complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![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
🧹 Nitpick comments (4)
src/platform/persistence/webDriver.js (2)
53-53: Minor:exportcreates a throwaway driver instance.
createWebDriver().exportinstantiates a new driver just to borrow itsexportmethod. This works but creates an unnecessary closure on each call. Consider extracting the export logic to a shared function.♻️ Optional: Extract shared export function
+const downloadExport = (doc) => { + const blob = new Blob([JSON.stringify(doc, null, 2)], { type: "application/json" }); + const url = URL.createObjectURL(blob); + const a = document.createElement("a"); + a.href = url; + a.download = "state_export.json"; + document.body.appendChild(a); + a.click(); + document.body.removeChild(a); + URL.revokeObjectURL(url); +}; + export const createWebDriver = (key = "llm_kernel_state") => ({ // ... - export: (doc) => { - const blob = new Blob([JSON.stringify(doc, null, 2)], { type: "application/json" }); - // ... - } + export: downloadExport, }); export const createMetaOnlyWebDriver = (key = "llm_kernel_meta_v1") => ({ // ... - export: createWebDriver().export, + export: downloadExport, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/persistence/webDriver.js` at line 53, The export property currently calls createWebDriver().export which constructs a throwaway driver to borrow its export method; refactor by extracting the export logic into a standalone function (e.g., sharedExport or webDriverExport) and have both createWebDriver() and the module export delegate to that function instead of instantiating a driver; update references to use the new sharedExport function and remove the createWebDriver().export call so no temporary driver is created.
4-7: Consider adding error handling for malformed JSON.
JSON.parsein bothloadmethods can throw if localStorage contains malformed data. WhilecreateStore.jswrapsdriver.load()in a try-catch (Lines 51-55), adding local error handling with a fallback tonullwould make the drivers more defensive and self-contained.🛡️ Optional: Add defensive JSON parsing
load: () => { const raw = localStorage.getItem(key); if (raw == null) return null; - return JSON.parse(raw); + try { + return JSON.parse(raw); + } catch { + return null; + } },Also applies to: 26-29
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/persistence/webDriver.js` around lines 4 - 7, The load function(s) in src/platform/persistence/webDriver.js currently call JSON.parse(raw) which can throw on malformed data; wrap the parse in a try/catch inside each load implementation (the load arrow functions that read localStorage via getItem(key)) and return null on parse errors, logging or silently swallowing as appropriate so the driver is defensive and self-contained (apply the same change to both load implementations referenced around the earlier and later blocks).src/game/ui/ui.js (1)
11-12: Good progress on UI decoupling via viewmodel selectors.The introduction of
selectIsMapBuilder,selectRunPhase, andselectUiMetaselectors enforces cleaner boundaries between UI and sim layers. However, there's some inconsistency: whilesync()at Line 124 usesselectIsMapBuilder(current), several methods still derive builder state directly fromstate?.sim?.runPhase:
- Line 183:
_refreshActionFeedbackViewdefault parameter- Line 210:
_syncBuilderPhaseUidefault parameter- Line 275:
_syncBuilderHoverOverlaydefault parameterConsider updating these default parameters to use
selectIsMapBuilder(state)for consistency.♻️ Optional: Consistent selector usage in default parameters
- _refreshActionFeedbackView(state = this._store?.getState?.(), isBuilder = String(state?.sim?.runPhase || "") === RUN_PHASE.MAP_BUILDER) { + _refreshActionFeedbackView(state = this._store?.getState?.(), isBuilder = selectIsMapBuilder(state)) { - _syncBuilderPhaseUi(state = this._store?.getState?.(), isBuilder = String(state?.sim?.runPhase || "") === RUN_PHASE.MAP_BUILDER) { + _syncBuilderPhaseUi(state = this._store?.getState?.(), isBuilder = selectIsMapBuilder(state)) { - _syncBuilderHoverOverlay(state = this._store?.getState?.(), isBuilder = String(state?.sim?.runPhase || "") === RUN_PHASE.MAP_BUILDER) { + _syncBuilderHoverOverlay(state = this._store?.getState?.(), isBuilder = selectIsMapBuilder(state)) {Also applies to: 124-124, 210-211, 245-245
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/game/ui/ui.js` around lines 11 - 12, The default parameter logic in several UI methods still derives builder state from state?.sim?.runPhase; update those defaults to use the selector for consistency: replace uses in _refreshActionFeedbackView, _syncBuilderPhaseUi, and _syncBuilderHoverOverlay so their default parameter calls selectIsMapBuilder(state) (matching sync() which uses selectIsMapBuilder(current)); ensure you import/select selectIsMapBuilder is used and remove any direct runPhase-derived truthiness in those default params so all builder checks use selectIsMapBuilder(state).tests/test-persistence-map-builder-reload.mjs (1)
36-36: Use a fresh driver instance for reload simulation.At Line 36, reusing the same
storageDriverobject can mask reload bugs if driver internals ever become stateful. Prefer constructing a new driver with the same key for the reload store.♻️ Suggested change
- const reloadStore = createStore(manifest, { reducer, simStep: simStepPatch }, { storageDriver }); + const reloadStorageDriver = createMetaOnlyWebDriver(storageKey); + const reloadStore = createStore(manifest, { reducer, simStep: simStepPatch }, { storageDriver: reloadStorageDriver });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test-persistence-map-builder-reload.mjs` at line 36, The test currently reuses the same storageDriver when creating reloadStore via createStore(manifest, { reducer, simStep: simStepPatch }, { storageDriver }), which can hide stateful-driver bugs; change the test to construct a new driver instance (e.g., create a fresh storage driver with the same key/config) and pass that new instance to createStore for reloadStore so reloadStore uses a fresh storageDriver while keeping manifest, reducer, and simStepPatch unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/game/ui/ui.input.js`:
- Around line 396-398: The code re-reads grid dimensions from state.meta (const
gridW/gridH) which shadows and bypasses the gridW/gridH previously obtained from
selectGridSize(state); fix by removing the direct state access and reuse the
already-destructured gridW and gridH variables in the call to
selectBuilderBrushTiles (ensure the call to selectBuilderBrushTiles(gridW,
gridH, wx, wy, brushSize) uses the existing variables rather than
Number(state?.meta?.gridW) / Number(state?.meta?.gridH)); this keeps the
selector abstraction (selectGridSize) authoritative and avoids shadowing.
---
Nitpick comments:
In `@src/game/ui/ui.js`:
- Around line 11-12: The default parameter logic in several UI methods still
derives builder state from state?.sim?.runPhase; update those defaults to use
the selector for consistency: replace uses in _refreshActionFeedbackView,
_syncBuilderPhaseUi, and _syncBuilderHoverOverlay so their default parameter
calls selectIsMapBuilder(state) (matching sync() which uses
selectIsMapBuilder(current)); ensure you import/select selectIsMapBuilder is
used and remove any direct runPhase-derived truthiness in those default params
so all builder checks use selectIsMapBuilder(state).
In `@src/platform/persistence/webDriver.js`:
- Line 53: The export property currently calls createWebDriver().export which
constructs a throwaway driver to borrow its export method; refactor by
extracting the export logic into a standalone function (e.g., sharedExport or
webDriverExport) and have both createWebDriver() and the module export delegate
to that function instead of instantiating a driver; update references to use the
new sharedExport function and remove the createWebDriver().export call so no
temporary driver is created.
- Around line 4-7: The load function(s) in src/platform/persistence/webDriver.js
currently call JSON.parse(raw) which can throw on malformed data; wrap the parse
in a try/catch inside each load implementation (the load arrow functions that
read localStorage via getItem(key)) and return null on parse errors, logging or
silently swallowing as appropriate so the driver is defensive and self-contained
(apply the same change to both load implementations referenced around the
earlier and later blocks).
In `@tests/test-persistence-map-builder-reload.mjs`:
- Line 36: The test currently reuses the same storageDriver when creating
reloadStore via createStore(manifest, { reducer, simStep: simStepPatch }, {
storageDriver }), which can hide stateful-driver bugs; change the test to
construct a new driver instance (e.g., create a fresh storage driver with the
same key/config) and pass that new instance to createStore for reloadStore so
reloadStore uses a fresh storageDriver while keeping manifest, reducer, and
simStepPatch unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7efe53a3-cda8-4485-b7e2-ae56f8ee1e39
📒 Files selected for processing (30)
devtools/demo-live-attest.mjsdocs/ARCHITECTURE.mddocs/STATUS.mdsrc/app/main.jssrc/game/runtime/infraRuntime.jssrc/game/runtime/orderNavigation.jssrc/game/runtime/processActiveOrderRuntime.jssrc/game/runtime/stateCounts.jssrc/game/sim/reducer/core.jssrc/game/sim/reducer/winConditions.jssrc/game/sim/runtime/infraRuntime.jssrc/game/sim/runtime/orderNavigation.jssrc/game/sim/runtime/processActiveOrderRuntime.jssrc/game/sim/runtime/stateCounts.jssrc/game/ui/ui.input.jssrc/game/ui/ui.jssrc/game/viewmodel/builderResources.jssrc/game/viewmodel/builderSelectors.jssrc/game/viewmodel/tileInteractionSelectors.jssrc/game/viewmodel/uiStateSelectors.jssrc/kernel/store/createStore.jssrc/kernel/store/persistence.jssrc/platform/persistence/webDriver.jstests/evidence/spec-map.mjstests/test-active-order-runtime.mjstests/test-deterministic-patch-chain.mjstests/test-persistence-drivers.mjstests/test-persistence-map-builder-reload.mjstests/test-runtime-boundaries.mjstests/test-ui-foundation-e2e.mjs
Comment on lines
+396
to
+398
| const gridW = Number(state?.meta?.gridW || 16) | 0; | ||
| const gridH = Number(state?.meta?.gridH || 16) | 0; | ||
| const tiles = selectBuilderBrushTiles(gridW, gridH, wx, wy, brushSize); |
There was a problem hiding this comment.
Inconsistent grid size access shadows selector values.
Lines 396-398 directly read state?.meta?.gridW/gridH and shadow the gridW/gridH already destructured from selectGridSize(state) on line 264. This bypasses the selector abstraction and could cause subtle inconsistencies if the selector logic diverges from raw state access.
🔧 Proposed fix: reuse existing gridW/gridH
if (isBuilderBrushMode(mode)) {
const brushSize = this._builderBrushSize || 1;
- const gridW = Number(state?.meta?.gridW || 16) | 0;
- const gridH = Number(state?.meta?.gridH || 16) | 0;
const tiles = selectBuilderBrushTiles(gridW, gridH, wx, wy, brushSize);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const gridW = Number(state?.meta?.gridW || 16) | 0; | |
| const gridH = Number(state?.meta?.gridH || 16) | 0; | |
| const tiles = selectBuilderBrushTiles(gridW, gridH, wx, wy, brushSize); | |
| const tiles = selectBuilderBrushTiles(gridW, gridH, wx, wy, brushSize); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/game/ui/ui.input.js` around lines 396 - 398, The code re-reads grid
dimensions from state.meta (const gridW/gridH) which shadows and bypasses the
gridW/gridH previously obtained from selectGridSize(state); fix by removing the
direct state access and reuse the already-destructured gridW and gridH variables
in the call to selectBuilderBrushTiles (ensure the call to
selectBuilderBrushTiles(gridW, gridH, wx, wy, brushSize) uses the existing
variables rather than Number(state?.meta?.gridW) / Number(state?.meta?.gridH));
this keeps the selector abstraction (selectGridSize) authoritative and avoids
shadowing.
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
This PR completes a deterministic architecture split so the UI can ask for intent while kernel/domain remain the write authority. It removes direct
ui -> simimports, moves simulation runtime helpers into canonicalsimruntime modules, and makes browser persistence an explicit platform adapter.Problem
The codebase had tight coupling between UI, simulation helpers, and runtime utility modules. Browser persistence behavior was implicitly tied to kernel defaults. This made boundaries harder to enforce and increased drift risk for deterministic replay and future seed-synchronized multiplayer.
Root Cause
Layer ownership had grown organically:
src/game/runtime/*while reducer logic insrc/game/sim/*imported it,What Changed
src/game/sim/runtime/*for active-order execution, navigation, infra helpers, and state counters.src/game/runtime/*as compatibility re-export facades.ui.input.jsandui.jsto consume selector functions instead of direct sim imports and scattered raw state reads.createNullDriverdefault path).src/platform/persistence/webDriver.js.tests/test-deterministic-patch-chain.mjsto enforce identical reducer/simStep patch chains for same seed + same actions.sim -> game/runtime, forbid directui -> simimports, and prevent browser globals in kernel files.User Impact
Validation
Executed and passing:
npm run llm:pipeline:checknpm run gate:blocking(truth, determinism matrix, smoke-e2e gate)node tests/test-runtime-boundaries.mjsnode tests/test-whole-repo-dispatch-truth.mjsnode tests/test-deterministic-patch-chain.mjsnode tests/test-persistence-map-builder-reload.mjsnpm run test:quickSmoke e2e note:
tests/test-ui-foundation-e2e.mjsreportsSKIPPED playwright_not_installedin this environment, and the smoke gate handles that path as designed.Commit Structure
This PR is intentionally split into atomic commits:
refactor(sim): move runtime helpers into canonical sim runtime layerrefactor(ui): route ui state reads through viewmodel selectorsrefactor(kernel): split web persistence into explicit platform adaptertest(determinism): add patch-chain guard and refresh architecture docsSummary by CodeRabbit
New Features
Refactoring
Tests