Repository navigation
Fix rapid typing and save sequencing regressions - #198
Conversation
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (12)
WalkthroughThis PR adds a per-session workspace text-operation sequencer, threads it through edit/checkpoint/export flow, changes pending-save handling, updates history/settings key behavior, and introduces a new default title scene with orbit-based lighting. Tests and harnesses were expanded to cover the new sequencing, rendering, and scene-loading behavior. ChangesWorkspace cutover
Title scene lighting
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Abort queued saves after an edit obstruction
When Ctrl-S/:w is now accepted while a predicted edit is still pending, this export command is queued behind the edit operations. If any earlier edit returns Obstructed, applyTextEditObstruction marks the authority obstructed, but the queued export still runs and is accepted because its request id is now the latest; a successful export then clears dirty/obstruction state and can write a snapshot after a failed edit. The queued save needs to be canceled or rejected when a preceding edit obstruction is recorded.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@spec/workspace-app-echo-cutover.spec.mjs`:
- Around line 120-130: The receipt generation in the insertText mock is using
calls.insert.length after awaiting pending.promise, which can cause multiple
queued inserts to resolve with the same receiptId. Update the insertText helper
in the spec so it captures the sequence number before awaiting and uses that
saved value when building the receiptId, and apply the same fix to the other
matching mock block referenced by the spec so per-edit history sequencing
remains unique.
- Around line 643-656: The observed-window helper is computing offsets with
text.length, which is UTF-16 code units instead of bytes, so non-ASCII cases can
produce invalid adapter expectations. Update observedDocumentWindow (and the
related lines 694-699 logic) to derive startByte/endByte from byte-accurate text
encoding rather than string length, keeping byteOffsetAtLine as the source of
truth and using the encoded byte size for each visible line’s end offset.
In `@spec/workspace-reading-cache-guard.spec.mjs`:
- Around line 40-42: The current guard in workspace-reading-cache-guard.spec.mjs
only checks that workspaceModelWithTextAuthorityEditor appears somewhere in
runtimeSource, which is too weak to validate the intended rebuild path. Tighten
the assertion by matching a more specific pattern tied to the actual runtime
flow under test, such as the exact call site or surrounding identifiers used
when the cache read/rebuild logic executes, so unrelated imports or dead code
cannot satisfy it. Use the existing runtimeSource check in this spec and anchor
it to the concrete behavior exercised by the cache guard test.
In `@src/app/workspace/workspace-text-operation-sequencer.ts`:
- Around line 3-20: Replace the module-scoped textOperationQueues state in
sequenceWorkspaceTextOperation with an injected sequencer owned by the workspace
wiring. Move the WeakMap into a factory-created sequencer or dependency class,
then pass that sequencer into the command path that currently calls
sequenceWorkspaceTextOperation so lifecycle is managed by DI instead of ambient
globals. Keep the sequencing behavior the same, but make the session queue
storage live on the injected dependency rather than at module scope.
🪄 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: 2193fea8-4ac8-4473-9174-7511e8654d7e
📒 Files selected for processing (12)
spec/workspace-app-echo-cutover.spec.mjsspec/workspace-command-line.spec.mjsspec/workspace-key-bindings.spec.mjsspec/workspace-reading-cache-guard.spec.mjsspec/workspace-text-cutover.spec.mjssrc/app/workspace/drawer.tssrc/app/workspace/workspace-save-key.tssrc/app/workspace/workspace-text-commands.tssrc/app/workspace/workspace-text-operation-sequencer.tssrc/app/workspace/workspace-text-reading-cache.tssrc/app/workspace/workspace-text-result-guards.tssrc/app/workspace/workspace-text-runtime-state.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: release-gate / jedit-echo
🧰 Additional context used
📓 Path-based instructions (3)
**
⚙️ CodeRabbit configuration file
**: # jedit Agent ContractGit Safety
- Never amend commits. Make a new commit.
- Never rebase unless the user explicitly approves it after a concrete explanation.
- Never force git operations.
- Draft pull requests are allowed only for design-cycle kickoff or active cycle
work. Convert them to ready before merge. If draft PRs are unavailable, use a
normal PR with thework-in-progresslabel.Quality Doctrine
These are hard repo rules, not suggestions:
- No
any.- No
unknown.- Hexagonal architecture, strict.
- Encoding and decoding happen only at the boundary, inside a port/adapter.
- One file equals one runtime truth.
- Runtime truth beats compile-time theater.
- No ad hoc string comparison in core logic. Prefer
instanceof, numeric tags, symbols, or explicit runtime objects.- No magic strings.
- No magic numbers.
- SOLID.
- DRY.
- DI.
- Red to green.
- Tests are spec.
- Agent-first.
- No file over 500 lines of code.
Structure
ARCHITECTURE.mdis the canonical repo architecture doctrine.docs/BEARING.mdrecords the current execution gravity.docs/method/process.mdis the canonical cycle workflow.docs/method/roadmap-planning.mddefines release-gate, roadmap, slice, and
proof policy for multi-cycle planning.docs/design/TEMPLATE.mdis the required template for full cycle designs.docs/design/0034-design-cycle-template-and-lifecycle.mdrecords the policy
decision that made executable design docs official.docs/design/0024-jedit-powered-by-echo-release-gate.mdis the completed
baseline plan for the first thirty slices proving jedit has an installed
package/evidence path while Echo remains generic.docs/design/0025-echo-application-hosting-pattern.mdis the completed
post-release-pressure ten-slice plan for proving the reusable Echo
application-hosting pattern before applying it to Graft, Think, or other apps.docs/design/0026-echo-hosting-hardening-first-twenty.mdis th...
Files:
spec/workspace-key-bindings.spec.mjssrc/app/workspace/workspace-text-result-guards.tsspec/workspace-reading-cache-guard.spec.mjssrc/app/workspace/workspace-text-operation-sequencer.tsspec/workspace-command-line.spec.mjssrc/app/workspace/drawer.tssrc/app/workspace/workspace-text-reading-cache.tssrc/app/workspace/workspace-text-commands.tssrc/app/workspace/workspace-save-key.tsspec/workspace-app-echo-cutover.spec.mjsspec/workspace-text-cutover.spec.mjssrc/app/workspace/workspace-text-runtime-state.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Noanytype usage in TypeScript code
Nounknowntype usage in TypeScript code
No TypeScript file over 500 lines of code
Make invalid states unrepresentable at runtime where possible
Prefer constructor or factory injection over ambient singletons for new code
Files:
src/app/workspace/workspace-text-result-guards.tssrc/app/workspace/workspace-text-operation-sequencer.tssrc/app/workspace/drawer.tssrc/app/workspace/workspace-text-reading-cache.tssrc/app/workspace/workspace-text-commands.tssrc/app/workspace/workspace-save-key.tssrc/app/workspace/workspace-text-runtime-state.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js}: No ad hoc string comparison in core logic; preferinstanceof, numeric tags, symbols, or explicit runtime objects
No magic strings in code
No magic numbers in code
Files:
src/app/workspace/workspace-text-result-guards.tssrc/app/workspace/workspace-text-operation-sequencer.tssrc/app/workspace/drawer.tssrc/app/workspace/workspace-text-reading-cache.tssrc/app/workspace/workspace-text-commands.tssrc/app/workspace/workspace-save-key.tssrc/app/workspace/workspace-text-runtime-state.ts
🔇 Additional comments (9)
src/app/workspace/drawer.ts (1)
9-9: LGTM!Also applies to: 51-55, 91-93
src/app/workspace/workspace-text-reading-cache.ts (1)
3-4: LGTM!Also applies to: 145-155, 193-210
src/app/workspace/workspace-text-result-guards.ts (1)
36-44: LGTM!src/app/workspace/workspace-text-runtime-state.ts (1)
32-43: LGTM!Also applies to: 178-186, 188-206, 332-334, 452-452
src/app/workspace/workspace-text-commands.ts (1)
5-8: LGTM!Also applies to: 34-34, 164-206
src/app/workspace/workspace-save-key.ts (1)
1-1: LGTM!Also applies to: 53-79
spec/workspace-text-cutover.spec.mjs (1)
1021-1021: LGTM!Also applies to: 1063-1070
spec/workspace-command-line.spec.mjs (1)
729-729: LGTM!Also applies to: 767-779
spec/workspace-key-bindings.spec.mjs (1)
130-179: LGTM!
Code Lawyer Activity SummaryAll actionable review threads have been addressed and resolved. Local full check is green on head
Verification:
Current gate state:
|
Code Lawyer Self-Audit FindingCc @codex for second opinion.
|
Code Lawyer Recheck Activity SummaryRecheck completed on head
Verification:
Current gate state:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da6d394e58
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Code Lawyer Activity Summary
Verification:
|
|
Code Lawyer Re-Audit Summary No new actionable PR review threads or self-audit issues were found on head
No commits were made in this re-audit pass. |
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/workspace/workspace-text-runtime-state.ts (1)
430-435: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore graft refresh after applied text edits.
Line 434 now only re-runs source highlighting. That removes the previous post-edit graft/projection refresh path, so an open or current-file Graft view will stay stale after text edits until some unrelated action refreshes it. Reintroduce the conditional graft refresh here instead of downgrading this path to highlight-only.
🤖 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 `@src/app/workspace/workspace-text-runtime-state.ts` around lines 430 - 435, The refreshAfterEdit path currently only calls beginEditorSourceHighlightRefresh, which drops the post-edit graft/projection refresh and leaves open Graft views stale. Update refreshAfterEdit in workspace-text-runtime-state to restore the conditional graft refresh logic alongside the existing source highlight refresh, using the same WorkspaceRuntimeDependencies/WorkspaceModel flow so current-file or open graft state is refreshed after applied text edits.
♻️ Duplicate comments (1)
spec/workspace-reading-cache-guard.spec.mjs (1)
42-46: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThese regex guards are still not function-local.
[\s\S]*workspaceModelWithTextAuthorityEditor\(can still match a later occurrence elsewhere inruntimeSource, soeditorAfterTextEditorwithTextAuthoritycan stop calling it and this test will still pass. Match the concrete call/return site inside each function body instead.As per coding guidelines, "Tests are spec."
🤖 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 `@spec/workspace-reading-cache-guard.spec.mjs` around lines 42 - 46, The regex guards in the spec are too broad and can still match a later workspaceModelWithTextAuthorityEditor call outside the target function, so tighten each assertion to the concrete call/return site within editorAfterTextEdit and withTextAuthority. Update the patterns in workspace-reading-cache-guard.spec.mjs so they anchor to the relevant function bodies and only pass when those specific functions directly invoke or return workspaceModelWithTextAuthorityEditor, rather than any later occurrence in runtimeSource.Source: Coding guidelines
🤖 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 `@spec/settings-drawer.spec.mjs`:
- Around line 50-56: The settings drawer spec now only checks rendered text, so
keep at least one cell-level style assertion to cover the focused row’s
selected-marker/row styling. In spec/settings-drawer.spec.mjs, retain or
reintroduce a surface-cell assertion alongside the text checks in the drawer
render test so the token/wiring regression can’t pass on glyph text alone.
In `@spec/settings-session.spec.mjs`:
- Around line 60-65: Update the settings focus movement test around
moveSettingsFocusIndex to also cover the rowCount <= 0 guard, not just wrapping
behavior. Add an assertion in the existing settings focus movement test (or a
nearby new test in the same spec) that exercises the zero-row case and verifies
the function returns the expected fallback index when rowCount is 0, so the
escape hatch stays covered.
In `@spec/workspace-app-echo-cutover.spec.mjs`:
- Around line 293-367: This spec file is growing past the 500-line limit, so
split the newly added queue/UTF-8 helper logic and/or the cutover cases into a
dedicated spec/helper module instead of keeping them in
workspace-app-echo-cutover.spec.mjs. Move the related test blocks around the
real workspace app path and any shared setup utilities into a separate file, and
update the existing spec to import or reference those helpers so the current
file stays focused and under the size cap.
- Around line 320-323: The `multiRangeEdit` stub is returning a nested
obstruction object instead of the obstruction payload itself, so it no longer
matches the `insertText` shape. Update `multiRangeEdit` in
`workspace-app-echo-cutover.spec.mjs` to return the direct result of
`productionTextObstruction('multi-range unsupported')` using the same payload
structure as the existing `insertText` stub, so the spec reflects the intended
obstruction response.
- Around line 353-366: The queued save result is being ignored in the spec, so
the follow-up model/commands from harness.runtime.update(...) are never
asserted. Update the workspace-app echo cutover test to capture and apply the
result of runtime.update(...) from saveMessage, then assert on the returned
model and any emitted commands after pendingInsert.resolve() and
applyWorkspaceMessage(...), using harness.runtime.update and harness.model so
the test verifies the queued save is not dropped.
In `@spec/workspace-text-cutover.spec.mjs`:
- Around line 1431-1438: The first coalesced save path is not being validated
for duplicate checkpoint emission. In the test around runtime.update, capture
the command list returned from the first update using firstMessage and
secondSave, then assert it is empty before proceeding to the second
runtime.update call. Keep the existing second assertion that checkpointCommands
has length 1 so the test covers both the no-op first export and the single
checkpoint on the second export.
In `@spec/workspace-text-operation-sequencer-guard.spec.mjs`:
- Around line 8-11: The current spec only blocks a top-level WeakMap
declaration, so it can miss module-scoped shared sequencer state created through
an object or helper. Update the test around
createWorkspaceTextOperationSequencer to assert that any WeakMap-backed state is
instantiated inside the factory body, not at module scope, by checking the
source for factory-local creation patterns rather than just a single top-level
spelling.
In `@src/adapters/workspace-production-text-session.ts`:
- Line 5: The adapter currently re-exports
createWorkspaceTextOperationSequencer, which makes
src/adapters/workspace-production-text-session.ts expose an unrelated runtime
factory. Remove that export from the adapter and update the call sites to import
createWorkspaceTextOperationSequencer directly from its own module, keeping the
adapter focused on the production session factory only.
In `@src/app/settings-session.ts`:
- Line 91: `KEY_Q` in `settings-session.ts` duplicates the close-key truth
already defined in `WorkspaceKeys.Q` from
`app/workspace/settings-key-bindings.ts`, which can drift from the drawer
hint/specs and tests. Remove the local `KEY_Q` constant and import/reuse the
shared close binding from the centralized key registry instead, updating any
references in `settings-session` to use that shared symbol so there is only one
runtime source of truth.
In `@src/app/workspace/workspace-text-operation-sequencer.ts`:
- Around line 10-20: The sequencer is using raw filePath/bufferId strings as its
identity, which leads to ad hoc string comparisons in the core logic. Update the
ActiveTextExportOperation and BlockedTextOperation types, plus the related logic
in workspace-text-operation-sequencer and the export-reuse check around the
matching/lookup path, to carry a dedicated runtime identity object or nominal id
instead of comparing strings directly. Move the string-based boundary data into
that runtime object immediately at the edge, and use that object consistently
throughout the sequencer.
In `@src/app/workspace/workspace-text-result-guards.ts`:
- Around line 69-79: The dependent edit blocker message in
dependentEditBlockedIssue should not synthesize a fake request id when
blockedByClientSeq is undefined. Update the message construction so it uses an
explicit unknown marker or omits the request suffix entirely when the sequence
is absent, instead of defaulting to request:0. Keep the change localized to
dependentEditBlockedIssue and preserve the existing prefix, filePath, and
RuntimeIssue shape.
In `@src/ui/settings-drawer.ts`:
- Around line 74-90: The fallback in settings-drawer rendering returns the
already-shrunk body from renderSettingsDrawerBody, so when drawer() yields no
overlay.surface the caller gets a smaller, misaligned surface. Update the logic
in the settings-drawer function so the fallback path produces a full-size
surface matching options.width and options.height, while still using the
shrunken content only for the inner drawer body. Use the
renderSettingsDrawerBody and drawer symbols to locate the return path and keep
the fallback consistent with the outer overlay dimensions.
---
Outside diff comments:
In `@src/app/workspace/workspace-text-runtime-state.ts`:
- Around line 430-435: The refreshAfterEdit path currently only calls
beginEditorSourceHighlightRefresh, which drops the post-edit graft/projection
refresh and leaves open Graft views stale. Update refreshAfterEdit in
workspace-text-runtime-state to restore the conditional graft refresh logic
alongside the existing source highlight refresh, using the same
WorkspaceRuntimeDependencies/WorkspaceModel flow so current-file or open graft
state is refreshed after applied text edits.
---
Duplicate comments:
In `@spec/workspace-reading-cache-guard.spec.mjs`:
- Around line 42-46: The regex guards in the spec are too broad and can still
match a later workspaceModelWithTextAuthorityEditor call outside the target
function, so tighten each assertion to the concrete call/return site within
editorAfterTextEdit and withTextAuthority. Update the patterns in
workspace-reading-cache-guard.spec.mjs so they anchor to the relevant function
bodies and only pass when those specific functions directly invoke or return
workspaceModelWithTextAuthorityEditor, rather than any later occurrence in
runtimeSource.
🪄 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: a9a70d08-62f4-4f4b-bfa7-29a3f141c50d
📒 Files selected for processing (30)
spec/settings-drawer.spec.mjsspec/settings-session.spec.mjsspec/workspace-app-echo-cutover.spec.mjsspec/workspace-echo-app-harness.mjsspec/workspace-helpers.mjsspec/workspace-lifecycle.spec.mjsspec/workspace-reading-cache-guard.spec.mjsspec/workspace-text-boundaries.spec.mjsspec/workspace-text-cutover.spec.mjsspec/workspace-text-operation-sequencer-guard.spec.mjsspec/workspace-text-operation-sequencer.spec.mjsspec/workspace-title-screen.spec.mjssrc/adapters/workspace-app.tssrc/adapters/workspace-production-text-session.tssrc/app/settings-session.tssrc/app/workspace/editor-session.tssrc/app/workspace/editor-trust-preflight-runtime-fixtures.tssrc/app/workspace/focused-pane-key-bindings.tssrc/app/workspace/key-binding-context.tssrc/app/workspace/runtime.tssrc/app/workspace/settings-key-bindings.tssrc/app/workspace/viewer-key.tssrc/app/workspace/viewer.tssrc/app/workspace/workspace-runtime-dependencies.tssrc/app/workspace/workspace-save-key.tssrc/app/workspace/workspace-text-commands.tssrc/app/workspace/workspace-text-operation-sequencer.tssrc/app/workspace/workspace-text-result-guards.tssrc/app/workspace/workspace-text-runtime-state.tssrc/ui/settings-drawer.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: test / echo-authority
- GitHub Check: test / cycle-proofs
- GitHub Check: test / contract-api
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Noanytype usage in TypeScript code
Nounknowntype usage in TypeScript code
No TypeScript file over 500 lines of code
Make invalid states unrepresentable at runtime where possible
Prefer constructor or factory injection over ambient singletons for new code
Files:
src/app/workspace/workspace-runtime-dependencies.tssrc/app/workspace/runtime.tssrc/app/workspace/settings-key-bindings.tssrc/adapters/workspace-production-text-session.tssrc/app/workspace/key-binding-context.tssrc/app/workspace/focused-pane-key-bindings.tssrc/app/workspace/editor-session.tssrc/app/workspace/editor-trust-preflight-runtime-fixtures.tssrc/adapters/workspace-app.tssrc/app/workspace/viewer.tssrc/ui/settings-drawer.tssrc/app/workspace/workspace-text-commands.tssrc/app/workspace/workspace-text-operation-sequencer.tssrc/app/workspace/workspace-save-key.tssrc/app/settings-session.tssrc/app/workspace/workspace-text-result-guards.tssrc/app/workspace/viewer-key.tssrc/app/workspace/workspace-text-runtime-state.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js}: No ad hoc string comparison in core logic; preferinstanceof, numeric tags, symbols, or explicit runtime objects
No magic strings in code
No magic numbers in code
Files:
src/app/workspace/workspace-runtime-dependencies.tssrc/app/workspace/runtime.tssrc/app/workspace/settings-key-bindings.tssrc/adapters/workspace-production-text-session.tssrc/app/workspace/key-binding-context.tssrc/app/workspace/focused-pane-key-bindings.tssrc/app/workspace/editor-session.tssrc/app/workspace/editor-trust-preflight-runtime-fixtures.tssrc/adapters/workspace-app.tssrc/app/workspace/viewer.tssrc/ui/settings-drawer.tssrc/app/workspace/workspace-text-commands.tssrc/app/workspace/workspace-text-operation-sequencer.tssrc/app/workspace/workspace-save-key.tssrc/app/settings-session.tssrc/app/workspace/workspace-text-result-guards.tssrc/app/workspace/viewer-key.tssrc/app/workspace/workspace-text-runtime-state.ts
**
⚙️ CodeRabbit configuration file
**: # jedit Agent ContractGit Safety
- Never amend commits. Make a new commit.
- Never rebase unless the user explicitly approves it after a concrete explanation.
- Never force git operations.
- Draft pull requests are allowed only for design-cycle kickoff or active cycle
work. Convert them to ready before merge. If draft PRs are unavailable, use a
normal PR with thework-in-progresslabel.Quality Doctrine
These are hard repo rules, not suggestions:
- No
any.- No
unknown.- Hexagonal architecture, strict.
- Encoding and decoding happen only at the boundary, inside a port/adapter.
- One file equals one runtime truth.
- Runtime truth beats compile-time theater.
- No ad hoc string comparison in core logic. Prefer
instanceof, numeric tags, symbols, or explicit runtime objects.- No magic strings.
- No magic numbers.
- SOLID.
- DRY.
- DI.
- Red to green.
- Tests are spec.
- Agent-first.
- No file over 500 lines of code.
Structure
ARCHITECTURE.mdis the canonical repo architecture doctrine.docs/BEARING.mdrecords the current execution gravity.docs/method/process.mdis the canonical cycle workflow.docs/method/roadmap-planning.mddefines release-gate, roadmap, slice, and
proof policy for multi-cycle planning.docs/design/TEMPLATE.mdis the required template for full cycle designs.docs/design/0034-design-cycle-template-and-lifecycle.mdrecords the policy
decision that made executable design docs official.docs/design/0024-jedit-powered-by-echo-release-gate.mdis the completed
baseline plan for the first thirty slices proving jedit has an installed
package/evidence path while Echo remains generic.docs/design/0025-echo-application-hosting-pattern.mdis the completed
post-release-pressure ten-slice plan for proving the reusable Echo
application-hosting pattern before applying it to Graft, Think, or other apps.docs/design/0026-echo-hosting-hardening-first-twenty.mdis th...
Files:
src/app/workspace/workspace-runtime-dependencies.tssrc/app/workspace/runtime.tsspec/workspace-text-operation-sequencer-guard.spec.mjssrc/app/workspace/settings-key-bindings.tssrc/adapters/workspace-production-text-session.tsspec/settings-drawer.spec.mjsspec/workspace-title-screen.spec.mjssrc/app/workspace/key-binding-context.tssrc/app/workspace/focused-pane-key-bindings.tsspec/workspace-reading-cache-guard.spec.mjsspec/workspace-lifecycle.spec.mjssrc/app/workspace/editor-session.tssrc/app/workspace/editor-trust-preflight-runtime-fixtures.tssrc/adapters/workspace-app.tsspec/workspace-text-boundaries.spec.mjsspec/workspace-text-operation-sequencer.spec.mjssrc/app/workspace/viewer.tsspec/workspace-echo-app-harness.mjsspec/settings-session.spec.mjssrc/ui/settings-drawer.tssrc/app/workspace/workspace-text-commands.tssrc/app/workspace/workspace-text-operation-sequencer.tsspec/workspace-helpers.mjssrc/app/workspace/workspace-save-key.tssrc/app/settings-session.tssrc/app/workspace/workspace-text-result-guards.tssrc/app/workspace/viewer-key.tssrc/app/workspace/workspace-text-runtime-state.tsspec/workspace-app-echo-cutover.spec.mjsspec/workspace-text-cutover.spec.mjs
🔇 Additional comments (16)
src/app/workspace/workspace-text-operation-sequencer.ts (1)
56-68: LGTM!src/app/workspace/workspace-text-commands.ts (1)
34-34: LGTM!Also applies to: 73-73, 108-108, 120-120, 168-173, 186-191, 204-209
src/app/workspace/workspace-runtime-dependencies.ts (1)
15-15: LGTM!Also applies to: 34-34
src/app/workspace/editor-trust-preflight-runtime-fixtures.ts (1)
39-39: LGTM!Also applies to: 139-151
src/app/workspace/key-binding-context.ts (1)
11-11: LGTM!Also applies to: 26-26
src/app/workspace/runtime.ts (1)
412-428: LGTM!src/app/workspace/focused-pane-key-bindings.ts (1)
37-43: LGTM!src/app/workspace/viewer-key.ts (1)
4-4: LGTM!Also applies to: 50-107, 127-149, 235-243, 262-289, 346-420
src/app/workspace/editor-session.ts (1)
161-172: LGTM!src/app/workspace/workspace-save-key.ts (1)
53-79: LGTM!spec/workspace-echo-app-harness.mjs (1)
10-18: LGTM!Also applies to: 57-57
spec/workspace-text-operation-sequencer.spec.mjs (1)
5-163: LGTM!spec/workspace-helpers.mjs (1)
106-118: LGTM!Also applies to: 285-319
spec/workspace-text-boundaries.spec.mjs (1)
4-8: LGTM!Also applies to: 193-193, 245-245
spec/workspace-text-cutover.spec.mjs (1)
5-5: LGTM!Also applies to: 344-344, 419-419, 452-452, 514-521, 583-590, 645-645, 709-709, 850-942, 1234-1234
spec/workspace-title-screen.spec.mjs (1)
632-664: LGTM!
Code Lawyer Activity Summary\n\n| # | Severity | Source | File | Commit | Outcome |\n| --- | --- | --- | --- | --- | --- |\n| 1 | P2 | PR |
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b7f334c5b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Code Lawyer Activity Summary Current head:
Verification:
Self-audit note: the full diff was captured locally at Cc: @codex |
Summary
Testing
██████████ 100% (11/11 slices)
🙈 OFF-ROADMAP ⎇ work +?/-? |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@scenes/bunny.jedit-scene`:
- Around line 3-4: Update the bunny scene camera setup so the X component of
camera.target in bunny.jedit-scene is aligned with the bunny mesh footprint
center instead of staying at -1.05, or increase
MAX_FACE_TARGET_CENTERLINE_OFFSET in title-scene-neon-dispersion.spec.mjs if the
wider offset is intentional. Use the camera.target values in the scene file and
the MAX_FACE_TARGET_CENTERLINE_OFFSET constant in the spec to keep the target
within the accepted centerline tolerance.
In `@spec/workspace-app-echo-typing-sequencing.spec.mjs`:
- Around line 79-82: The sequencing specs are coupled to a fixed number of
microtasks by the double Promise.resolve() waits, which makes them brittle;
update the assertions in the echo typing tests to wait for the observable state
instead. In the affected tests around pendingInserts and calls.insert, replace
the hardcoded microtask hops with a condition-based wait until the expected
in-flight mutation count is reached, using the existing sequencer/echo test
helpers so the assertions stay stable if internal await depth changes.
In `@spec/workspace-echo-test-utils.mjs`:
- Around line 46-48: The echoTextDocument.insert helper is still treating
startByte as a JavaScript string index, so multibyte content will insert at the
wrong location. Update the insert method in echoTextDocument to convert the byte
offset to a code-unit position before slicing, or change the backing
representation to bytes throughout the helper. Keep the fix localized to the
insert implementation and any related position handling so the Echo test utility
matches real runtime behavior.
- Around line 104-107: `applyWorkspaceMessage` is discarding the command queue
returned by `runtime.update`, which hides follow-up behavior from tests. Update
this helper in `spec/workspace-echo-test-utils.mjs` to preserve the commands
from `harness.runtime.update(message, harness.model)` and either return them to
the caller or immediately execute them with `harness.runAll(commands)`, while
still calling `harness.setModel(nextModel)` with the updated model.
In `@spec/workspace-text-cutover.spec.mjs`:
- Around line 943-1003: The test only applies
WorkspaceMessageTypes.TextEditResult and never starts the save it is meant to
race against, so it does not verify highlight ordering. Update the spec in
workspace-text-cutover.spec.mjs to first queue the save/export path on the
runtime, then dispatch TextEditResult and assert sourceHighlighter.highlight in
createWorkspaceRuntime runs before the queued save settles. Use the existing
symbols WorkspaceMessageTypes.TextEditResult, createWorkspaceRuntime, and
sourceHighlighter.highlight to keep the scenario tied to the real async
ordering.
- Around line 943-1003: The new TextEditResult sequencing/highlighting test
belongs in its own spec module instead of extending
spec/workspace-text-cutover.spec.mjs. Move the test case that exercises
createWorkspaceRuntime, TextEditResult, and workspaceTextAuthority into a
dedicated spec file with related coverage so this file stops growing beyond the
repo limit. Keep the existing behavior and assertions intact, but relocate the
new case and any similar save/highlighting sequencing coverage to a separate
runtime-focused spec.
🪄 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: e2c1745b-35fb-440c-b3db-9eaa61777a81
📒 Files selected for processing (23)
scenes/bunny.jedit-scenespec/settings-drawer.spec.mjsspec/settings-session.spec.mjsspec/title-scene-neon-dispersion.spec.mjsspec/workspace-app-echo-cutover.spec.mjsspec/workspace-app-echo-typing-sequencing.spec.mjsspec/workspace-echo-app-harness.mjsspec/workspace-echo-test-utils.mjsspec/workspace-helpers.mjsspec/workspace-text-cutover.spec.mjsspec/workspace-text-operation-sequencer-guard.spec.mjsspec/workspace-text-operation-sequencer.spec.mjsspec/workspace-text-result-guards.spec.mjssrc/adapters/workspace-app.tssrc/adapters/workspace-production-text-dependencies.tssrc/app/keybindings.tssrc/app/settings-session.tssrc/app/workspace/settings-key-bindings.tssrc/app/workspace/workspace-text-commands.tssrc/app/workspace/workspace-text-operation-sequencer.tssrc/app/workspace/workspace-text-result-guards.tssrc/app/workspace/workspace-text-runtime-state.tssrc/ui/settings-drawer.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: test / contract-api
- GitHub Check: test / echo-authority
- GitHub Check: test / cycle-proofs
- GitHub Check: release-gate / jedit-echo
🧰 Additional context used
📓 Path-based instructions (3)
**
⚙️ CodeRabbit configuration file
**: # jedit Agent ContractGit Safety
- Never amend commits. Make a new commit.
- Never rebase unless the user explicitly approves it after a concrete explanation.
- Never force git operations.
- Draft pull requests are allowed only for design-cycle kickoff or active cycle
work. Convert them to ready before merge. If draft PRs are unavailable, use a
normal PR with thework-in-progresslabel.Quality Doctrine
These are hard repo rules, not suggestions:
- No
any.- No
unknown.- Hexagonal architecture, strict.
- Encoding and decoding happen only at the boundary, inside a port/adapter.
- One file equals one runtime truth.
- Runtime truth beats compile-time theater.
- No ad hoc string comparison in core logic. Prefer
instanceof, numeric tags, symbols, or explicit runtime objects.- No magic strings.
- No magic numbers.
- SOLID.
- DRY.
- DI.
- Red to green.
- Tests are spec.
- Agent-first.
- No file over 500 lines of code.
Structure
ARCHITECTURE.mdis the canonical repo architecture doctrine.docs/BEARING.mdrecords the current execution gravity.docs/method/process.mdis the canonical cycle workflow.docs/method/roadmap-planning.mddefines release-gate, roadmap, slice, and
proof policy for multi-cycle planning.docs/design/TEMPLATE.mdis the required template for full cycle designs.docs/design/0034-design-cycle-template-and-lifecycle.mdrecords the policy
decision that made executable design docs official.docs/design/0024-jedit-powered-by-echo-release-gate.mdis the completed
baseline plan for the first thirty slices proving jedit has an installed
package/evidence path while Echo remains generic.docs/design/0025-echo-application-hosting-pattern.mdis the completed
post-release-pressure ten-slice plan for proving the reusable Echo
application-hosting pattern before applying it to Graft, Think, or other apps.docs/design/0026-echo-hosting-hardening-first-twenty.mdis th...
Files:
spec/workspace-text-result-guards.spec.mjsscenes/bunny.jedit-scenesrc/adapters/workspace-production-text-dependencies.tsspec/workspace-text-operation-sequencer.spec.mjsspec/workspace-text-operation-sequencer-guard.spec.mjssrc/app/workspace/settings-key-bindings.tsspec/workspace-app-echo-typing-sequencing.spec.mjssrc/app/keybindings.tsspec/workspace-helpers.mjssrc/adapters/workspace-app.tsspec/settings-drawer.spec.mjsspec/workspace-echo-test-utils.mjsspec/workspace-echo-app-harness.mjsspec/title-scene-neon-dispersion.spec.mjssrc/app/workspace/workspace-text-operation-sequencer.tssrc/app/settings-session.tsspec/settings-session.spec.mjssrc/app/workspace/workspace-text-commands.tssrc/ui/settings-drawer.tssrc/app/workspace/workspace-text-result-guards.tsspec/workspace-app-echo-cutover.spec.mjsspec/workspace-text-cutover.spec.mjssrc/app/workspace/workspace-text-runtime-state.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Noanytype usage in TypeScript code
Nounknowntype usage in TypeScript code
No TypeScript file over 500 lines of code
Make invalid states unrepresentable at runtime where possible
Prefer constructor or factory injection over ambient singletons for new code
Files:
src/adapters/workspace-production-text-dependencies.tssrc/app/workspace/settings-key-bindings.tssrc/app/keybindings.tssrc/adapters/workspace-app.tssrc/app/workspace/workspace-text-operation-sequencer.tssrc/app/settings-session.tssrc/app/workspace/workspace-text-commands.tssrc/ui/settings-drawer.tssrc/app/workspace/workspace-text-result-guards.tssrc/app/workspace/workspace-text-runtime-state.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js}: No ad hoc string comparison in core logic; preferinstanceof, numeric tags, symbols, or explicit runtime objects
No magic strings in code
No magic numbers in code
Files:
src/adapters/workspace-production-text-dependencies.tssrc/app/workspace/settings-key-bindings.tssrc/app/keybindings.tssrc/adapters/workspace-app.tssrc/app/workspace/workspace-text-operation-sequencer.tssrc/app/settings-session.tssrc/app/workspace/workspace-text-commands.tssrc/ui/settings-drawer.tssrc/app/workspace/workspace-text-result-guards.tssrc/app/workspace/workspace-text-runtime-state.ts
🔇 Additional comments (22)
src/ui/settings-drawer.ts (2)
91-99: 📐 Maintainability & Code QualityFallback now returns a full-size surface.
Correctly addresses the prior finding:
renderSettingsDrawerFallbackbuilds aoptions.width×options.heightsurface, fills it with the drawer background, and blits the shrunk content at an offset instead of returning the undersizedcontentdirectly.
73-90: LGTM!src/app/keybindings.ts (2)
26-26: 📐 Maintainability & Code QualityGood: single source of truth for the close key.
Centralizing
'q'behindJEDIT_SETTINGS_CLOSE_KEY/_LABELand having bothsettings-session.tsandsettings-key-bindings.tsimport from here resolves the earlier duplicate-constant finding.Also applies to: 36-37
36-37: 📐 Maintainability & Code QualityKeep settings-close on a single source of truth
Route every settings-close consumer throughJEDIT_SETTINGS_CLOSE_KEY; do not leave a separateqpath anywhere else.src/app/settings-session.ts (2)
4-4: 📐 Maintainability & Code QualityDuplicate-truth issue resolved.
Importing
JEDIT_SETTINGS_CLOSE_KEYfromkeybindings.jsinstead of a localKEY_Qfixes the previously flagged runtime-truth duplication.Also applies to: 131-131
116-118: LGTM!spec/settings-drawer.spec.mjs (2)
50-60: 📐 Maintainability & Code QualityCell-level assertion restored.
Keeps the
selectedMarkerchar/fg/bg checks alongside the text assertions, addressing the earlier "text-only assertions could mask a styling regression" concern.
87-111: LGTM! Good regression coverage for the fallback surface-size fix, including tiny dimensions.spec/settings-session.spec.mjs (2)
69-71: 📐 Maintainability & Code QualityZero-row guard now covered.
Adds the
rowCount <= 0boundary assertions formoveSettingsFocusIndex, resolving the earlier coverage gap.
8-20: LGTM!Also applies to: 73-121
src/app/workspace/settings-key-bindings.ts (1)
44-58: LGTM!spec/title-scene-neon-dispersion.spec.mjs (5)
237-249: Chrome-sphere check is well scoped; minor stray blank line.Line 243 has a stray blank line inside the function body — purely cosmetic, not blocking.
23-32: LGTM!
58-65: 🎯 Functional CorrectnessTarget-centerline assertion depends on unverified footprint-center semantics.
This assertion is the counterpart to the camera-target concern raised in
scenes/bunny.jedit-scene(lines 3-4): iftitleSceneObjectFootprintCenterdoesn't return an x near-1.05for the unpositioned bunny, this check fails. Flagging here since it's the actual assertion site.
87-87: LGTM!
136-136: LGTM!Also applies to: 151-151
scenes/bunny.jedit-scene (1)
68-103: LGTM!src/app/workspace/workspace-text-operation-sequencer.ts (1)
51-54: This still reduces target identity to raw string equality.
WorkspaceTextOperationTargetis still just a{ filePath, bufferId }bag, andsameWorkspaceTextOperationTarget()still drives reuse offleft.bufferId === right.bufferId. That keeps the core matching logic stringly in exactly the spot previously called out. As per coding guidelines, "No ad hoc string comparison in core logic. Preferinstanceof, numeric tags, symbols, or explicit runtime objects" and "Move stringly external data into runtime objects at the boundary immediately."Also applies to: 224-229
Source: Coding guidelines
spec/workspace-text-operation-sequencer-guard.spec.mjs (1)
16-23: The negative scan still misses module-owned state after the factory.
moduleScopeBeforeFactoryonly checks text beforecreateWorkspaceTextOperationSequencer(). A later top-levelnew WeakMap()or helper-created state would still satisfy this test, so it still doesn't prove the sequencer is factory-owned across the whole module.spec/workspace-echo-app-harness.mjs (1)
10-19: LGTM!Also applies to: 57-57, 161-161
spec/workspace-helpers.mjs (1)
112-117: LGTM!spec/workspace-app-echo-cutover.spec.mjs (1)
4-11: LGTM!Also applies to: 408-413
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/adapters/title-scene-environment-decoder.ts (1)
129-138: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDecoder silently allows both
directionandorbiton the same light with no invariant enforced.
decodeLightmerges bothdecodeLightDirectionanddecodeLightOrbitunconditionally. If a scene author sets bothdirectionandorbit, the decoder accepts it silently, and onlytitleSceneLightDirectionAt(in a different file) resolves the ambiguity by giving orbit priority. Nothing here makes that precedence discoverable or rejects the ambiguous authored state at the boundary where validation is supposed to happen.Consider rejecting (or at least warning/asserting) the combination at decode time so the invalid/ambiguous state can't be represented past the boundary.
As per coding guidelines, "Make invalid states unrepresentable at runtime where possible."
🤖 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 `@src/adapters/title-scene-environment-decoder.ts` around lines 129 - 138, decodeLight currently merges both decodeLightDirection and decodeLightOrbit without enforcing the invariant that a light cannot author both direction and orbit, so the ambiguous state slips through the decoder boundary. Update decodeLight in title-scene-environment-decoder.ts to validate the decoded light object before returning it: detect when both direction and orbit are present and either reject the input with a decode error or emit a clear warning/assertion and prevent both from coexisting. Keep the rule centralized here so downstream logic like titleSceneLightDirectionAt does not need to resolve invalid authored state.Source: Coding guidelines
🤖 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 `@spec/title-scene-render.spec.mjs`:
- Around line 264-295: The title scene orbit-light test only verifies diffuse
passthrough, leaving ambient, specularStrength, rimStrength, and direction
untested. Update the test around titleSceneDayNightEnvironmentAtHour and
titleSceneLightDirectionAt to assert those orbit light fields are preserved or
correctly derived for orbit-enabled environments, so the coverage matches the
behavior in titleSceneDayNightLight and dayNightLightAmbient.
In `@src/app/workspace/title-mesh-materials.ts`:
- Around line 15-18: The title mesh matching logic has regressed to ad hoc label
string comparison, so update `isTitleMeshMaterialObject` and
`applyTitleMeshMaterial` to use a dedicated runtime tag/boolean or object-type
check instead of raw `object.label` strings. Restore a non-string guard for
mesh-kind or decode an explicit field like `isMaterialTarget` at the adapter
boundary, and keep `TITLE_MESH_MATERIAL_OBJECT_LABELS` out of core matching
logic.
In `@src/ui/title-scene-environment.ts`:
- Around line 257-264: The `dayNightLightAmbient` helper has an inconsistent
orbit fallback compared with `dayNightLightDiffuse`, `dayNightLightSpecular`,
and `dayNightLightRim`. Update `dayNightLightAmbient` so that when `light.orbit`
is present but `light.ambient` is missing, it uses the same day-side default
pattern as the sibling helpers rather than `NIGHT_AMBIENT`. Keep the change
localized to the `dayNightLightAmbient` function in `title-scene-environment`
and preserve the existing non-orbit interpolation behavior.
- Around line 257-295: The four helpers dayNightLightAmbient,
dayNightLightDiffuse, dayNightLightSpecular, and dayNightLightRim duplicate the
same orbit-vs-mix pattern with only the property, default, and mix behavior
changing. Refactor them in src/ui/title-scene-environment.ts by introducing one
shared parameterized helper that handles the light?.orbit null check and returns
either the authored value or mixScalar result, then have each of the four
functions delegate to it with the appropriate SceneLight field and constants.
Ensure the helper preserves the existing ambient, diffuse, specularStrength, and
rimStrength fallback behavior.
- Around line 247-255: The SceneLight builder in titleSceneEnvironment is still
assigning a static direction even when light.orbit is present, which leaves the
returned object internally inconsistent. Update the direction handling in the
same orbit-aware pattern used by ambient, diffuse, specularStrength, and
rimStrength: in the function that returns the merged light object, keep
direction undefined when orbit is driving the light, and only fall back to
titleSceneSunDirection(hour, factors) for non-orbit lights. This should be
applied in the logic around titleSceneEnvironment so any direct readers of
.light.direction do not see a misleading value.
---
Outside diff comments:
In `@src/adapters/title-scene-environment-decoder.ts`:
- Around line 129-138: decodeLight currently merges both decodeLightDirection
and decodeLightOrbit without enforcing the invariant that a light cannot author
both direction and orbit, so the ambiguous state slips through the decoder
boundary. Update decodeLight in title-scene-environment-decoder.ts to validate
the decoded light object before returning it: detect when both direction and
orbit are present and either reject the input with a decode error or emit a
clear warning/assertion and prevent both from coexisting. Keep the rule
centralized here so downstream logic like titleSceneLightDirectionAt does not
need to resolve invalid authored state.
🪄 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: 8d6dd497-7fa3-44e5-9d22-acd7f4a0720c
📒 Files selected for processing (17)
scenes/bunny.jedit-scenescenes/continuum-gate.jedit-scenespec/title-scene-loader.spec.mjsspec/title-scene-neon-dispersion.spec.mjsspec/title-scene-profile-cli.spec.mjsspec/title-scene-render.spec.mjsspec/workspace-app-echo-typing-sequencing.spec.mjsspec/workspace-echo-test-utils.mjsspec/workspace-runtime.spec.mjsspec/workspace-text-cutover.spec.mjsspec/workspace-text-settlement-refresh.spec.mjsspec/workspace-title-screen.spec.mjssrc/adapters/title-scene-environment-decoder.tssrc/app/workspace/title-mesh-materials.tssrc/ports/title-scene-loader.tssrc/ui/title-scene-environment.tssrc/ui/title-screen.ts
💤 Files with no reviewable changes (1)
- spec/workspace-text-cutover.spec.mjs
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: test / echo-authority
- GitHub Check: test / contract-api
- GitHub Check: test / cycle-proofs
- GitHub Check: release-gate / jedit-echo
🧰 Additional context used
📓 Path-based instructions (3)
**
⚙️ CodeRabbit configuration file
**: # jedit Agent ContractGit Safety
- Never amend commits. Make a new commit.
- Never rebase unless the user explicitly approves it after a concrete explanation.
- Never force git operations.
- Draft pull requests are allowed only for design-cycle kickoff or active cycle
work. Convert them to ready before merge. If draft PRs are unavailable, use a
normal PR with thework-in-progresslabel.Quality Doctrine
These are hard repo rules, not suggestions:
- No
any.- No
unknown.- Hexagonal architecture, strict.
- Encoding and decoding happen only at the boundary, inside a port/adapter.
- One file equals one runtime truth.
- Runtime truth beats compile-time theater.
- No ad hoc string comparison in core logic. Prefer
instanceof, numeric tags, symbols, or explicit runtime objects.- No magic strings.
- No magic numbers.
- SOLID.
- DRY.
- DI.
- Red to green.
- Tests are spec.
- Agent-first.
- No file over 500 lines of code.
Structure
ARCHITECTURE.mdis the canonical repo architecture doctrine.docs/BEARING.mdrecords the current execution gravity.docs/method/process.mdis the canonical cycle workflow.docs/method/roadmap-planning.mddefines release-gate, roadmap, slice, and
proof policy for multi-cycle planning.docs/design/TEMPLATE.mdis the required template for full cycle designs.docs/design/0034-design-cycle-template-and-lifecycle.mdrecords the policy
decision that made executable design docs official.docs/design/0024-jedit-powered-by-echo-release-gate.mdis the completed
baseline plan for the first thirty slices proving jedit has an installed
package/evidence path while Echo remains generic.docs/design/0025-echo-application-hosting-pattern.mdis the completed
post-release-pressure ten-slice plan for proving the reusable Echo
application-hosting pattern before applying it to Graft, Think, or other apps.docs/design/0026-echo-hosting-hardening-first-twenty.mdis th...
Files:
scenes/bunny.jedit-scenescenes/continuum-gate.jedit-scenespec/workspace-runtime.spec.mjsspec/title-scene-loader.spec.mjssrc/ports/title-scene-loader.tsspec/title-scene-render.spec.mjsspec/title-scene-profile-cli.spec.mjssrc/ui/title-screen.tsspec/workspace-text-settlement-refresh.spec.mjssrc/adapters/title-scene-environment-decoder.tsspec/workspace-app-echo-typing-sequencing.spec.mjssrc/app/workspace/title-mesh-materials.tsspec/workspace-title-screen.spec.mjssrc/ui/title-scene-environment.tsspec/workspace-echo-test-utils.mjsspec/title-scene-neon-dispersion.spec.mjs
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Noanytype usage in TypeScript code
Nounknowntype usage in TypeScript code
No TypeScript file over 500 lines of code
Make invalid states unrepresentable at runtime where possible
Prefer constructor or factory injection over ambient singletons for new code
Files:
src/ports/title-scene-loader.tssrc/ui/title-screen.tssrc/adapters/title-scene-environment-decoder.tssrc/app/workspace/title-mesh-materials.tssrc/ui/title-scene-environment.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js}: No ad hoc string comparison in core logic; preferinstanceof, numeric tags, symbols, or explicit runtime objects
No magic strings in code
No magic numbers in code
Files:
src/ports/title-scene-loader.tssrc/ui/title-screen.tssrc/adapters/title-scene-environment-decoder.tssrc/app/workspace/title-mesh-materials.tssrc/ui/title-scene-environment.ts
🔇 Additional comments (19)
scenes/bunny.jedit-scene (2)
3-4: 🎯 Functional CorrectnessRe-verify camera target alignment now that it's an explicit vector.
Past review flagged
camera.target[0] = -1.05as1.03 units off from the bunny mesh footprint center (-0.017), exceedingMAX_FACE_TARGET_CENTERLINE_OFFSET = 0.25in the neon-dispersion spec, and it was marked addressed in earlier commits. This diff still emitstarget: [-1.05, 1.2, -0.24]with the same x-coordinate that was originally flagged. Please confirmspec/title-scene-neon-dispersion.spec.mjsstill passes with this exact vector (e.g., re-run it) rather than assuming the prior fix covers this restated value.
68-103: LGTM!spec/title-scene-loader.spec.mjs (1)
138-193: LGTM!src/ports/title-scene-loader.ts (1)
11-11: 🩺 Stability & AvailabilityConfirm the new default scene decodes cleanly.
This makes
continuum-gate.jedit-scenethe app's default startup scene. Since that file isn't in this review batch, please confirm it passes the newdecodeLightOrbitObjectzero-vector guard (radius/height not both 0) — a decode failure here would break app startup for every user on first load.spec/title-scene-render.spec.mjs (1)
326-329: LGTM!spec/title-scene-profile-cli.spec.mjs (1)
128-128: LGTM!src/ui/title-screen.ts (1)
26-31: LGTM!Also applies to: 330-334
src/adapters/title-scene-environment-decoder.ts (1)
154-188: LGTM!src/ui/title-scene-environment.ts (1)
27-42: LGTM!Also applies to: 121-139
spec/title-scene-neon-dispersion.spec.mjs (2)
39-94: 🎯 Functional CorrectnessDownstream of the scene-data issue flagged in
scenes/continuum-gate.jedit-scene.This test's camera/target-to-
primaryCenteralignment assertions (lines 59-64) depend onstanford-bunnyhaving an authoredposition; see the root-cause comment on the scene file. No separate fix needed here — flagging only so the two are read together.
1-38: LGTM!Constant renames, the new
loadDefaultSceneModules/loadDefaultScenehelpers, and the remaining migrated tests are consistent with the switch from the bunny scene tocontinuum-gate.jedit-scene.Also applies to: 96-227
spec/workspace-runtime.spec.mjs (1)
305-305: LGTM!spec/workspace-text-settlement-refresh.spec.mjs (2)
21-253: LGTM!Rest of the sequencing/highlighting assertions (Test 1 lines 21-112, Test 2 flow lines 114-253) are internally consistent and correctly exercise the intermediate-refresh-before-export ordering.
174-185: 🗄️ Data Integrity & IntegrationNo issue: the sequencer uses
bufferIdequality, so this inline target is a valid input and object identity does not affect queueing.> Likely an incorrect or invalid review comment.spec/workspace-app-echo-typing-sequencing.spec.mjs (1)
14-20: LGTM!The UTF-8 byte-coordinate test checks out by hand-trace, and the prior "fixed double
await Promise.resolve()" flakiness concern is now resolved viawaitForItemCount.Also applies to: 88-88, 177-177, 258-258, 328-328
src/app/workspace/title-mesh-materials.ts (1)
89-89: LGTM!Rename to
titleObjectWithMaterialand the broadenedTitleSceneObjectsignature are mechanically consistent with the callers.Also applies to: 111-122
spec/workspace-title-screen.spec.mjs (1)
140-151: LGTM!Replacing the hardcoded scene name with
port.DEFAULT_BUILT_IN_TITLE_SCENE_NAMEremoves a magic string from the test.spec/workspace-echo-test-utils.mjs (1)
44-58: LGTM!Both previously-flagged issues are resolved:
insertnow converts byte offsets viastringIndexFromUtf8ByteOffset(verified correct by hand-trace, including surrogate-pair code points), andapplyWorkspaceMessagenow drains follow-up commands/messages in a loop instead of discarding them.waitForItemCount's bounded poll also addresses the earlier "don't key assertions off a fixed tick count" feedback.Also applies to: 96-111, 113-130, 140-159
scenes/continuum-gate.jedit-scene (1)
2-5: 🎯 Functional Correctness
positionis not a valid mesh placement field here.Mesh scene objects intentionally have no
position; the loader ignores it and usesoffsetfor mesh placement instead. Addingpositionhere would be a no-op.> Likely an incorrect or invalid review comment.
| const TITLE_MESH_MATERIAL_OBJECT_LABELS = new Set([ | ||
| "stanford-bunny", | ||
| "title-primary-mesh", | ||
| ]); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Label-based matching reintroduces the ad hoc string comparison the doctrine forbids.
isTitleMeshMaterialObject now drops the mesh-kind check entirely and matches purely by testing object.label against a Set of raw strings ("stanford-bunny", "title-primary-mesh"). This is exactly the "ad hoc string comparison in core logic" / "magic strings" pattern the repo's hard doctrine calls out (prefer instanceof, numeric tags, symbols, or explicit runtime objects). It's also a step back from the prior kind+label guard: any future object (mesh or primitive) that happens to reuse one of these two label strings will now silently receive mesh-material treatment, since kind is no longer checked.
Consider decoding a dedicated boolean/tag field (e.g. isMaterialTarget) onto the runtime scene object at the adapter boundary instead of comparing labels in applyTitleMeshMaterial, keeping the string→tag translation confined to the decoder per "Encoding and decoding happen only at the boundary."
As per coding guidelines, "No ad hoc string comparison in core logic; prefer instanceof, numeric tags, symbols, or explicit runtime objects" and "No magic strings in code."
Also applies to: 103-109
🤖 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 `@src/app/workspace/title-mesh-materials.ts` around lines 15 - 18, The title
mesh matching logic has regressed to ad hoc label string comparison, so update
`isTitleMeshMaterialObject` and `applyTitleMeshMaterial` to use a dedicated
runtime tag/boolean or object-type check instead of raw `object.label` strings.
Restore a non-string guard for mesh-kind or decode an explicit field like
`isMaterialTarget` at the adapter boundary, and keep
`TITLE_MESH_MATERIAL_OBJECT_LABELS` out of core matching logic.
Source: Coding guidelines
| return { | ||
| ...light, | ||
| direction: titleSceneSunDirection(hour, factors), | ||
| ambient: mixScalar(NIGHT_AMBIENT, DAY_AMBIENT, factors.daylight), | ||
| diffuse: mixScalar(NIGHT_DIFFUSE, DAY_DIFFUSE, Math.max(factors.daylight, factors.twilight * 0.72)), | ||
| specularStrength: mixScalar(NIGHT_SPECULAR, DAY_SPECULAR, factors.daylight), | ||
| rimStrength: mixScalar(NIGHT_RIM, DAY_RIM, factors.daylight), | ||
| direction: light?.direction ?? titleSceneSunDirection(hour, factors), | ||
| ambient: dayNightLightAmbient(light, factors), | ||
| diffuse: dayNightLightDiffuse(light, factors), | ||
| specularStrength: dayNightLightSpecular(light, factors), | ||
| rimStrength: dayNightLightRim(light, factors), | ||
| }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
direction field isn't orbit-aware, unlike its three sibling fields.
ambient/diffuse/specularStrength/rimStrength were all refactored to check light?.orbit != null before falling back to day/night mixing (lines 257-295), but direction at line 249 still unconditionally computes titleSceneSunDirection(hour, factors) whenever light.direction is unset — ignoring light.orbit entirely. The returned SceneLight ends up carrying both an orbit and a semantically bogus static direction.
Currently masked because titleSceneLightDirectionAt checks orbit first and ignores direction when orbit is present, but any other code path reading .light.direction directly on this day/night-processed result gets a misleading value. Making the field orbit-aware (e.g. leaving it undefined when orbit drives the light) keeps the object internally consistent.
As per coding guidelines, "Make invalid states unrepresentable at runtime where possible."
🐛 Proposed fix
return {
...light,
- direction: light?.direction ?? titleSceneSunDirection(hour, factors),
+ direction:
+ light?.orbit != null
+ ? light.direction
+ : light?.direction ?? titleSceneSunDirection(hour, factors),
ambient: dayNightLightAmbient(light, factors),📝 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.
| return { | |
| ...light, | |
| direction: titleSceneSunDirection(hour, factors), | |
| ambient: mixScalar(NIGHT_AMBIENT, DAY_AMBIENT, factors.daylight), | |
| diffuse: mixScalar(NIGHT_DIFFUSE, DAY_DIFFUSE, Math.max(factors.daylight, factors.twilight * 0.72)), | |
| specularStrength: mixScalar(NIGHT_SPECULAR, DAY_SPECULAR, factors.daylight), | |
| rimStrength: mixScalar(NIGHT_RIM, DAY_RIM, factors.daylight), | |
| direction: light?.direction ?? titleSceneSunDirection(hour, factors), | |
| ambient: dayNightLightAmbient(light, factors), | |
| diffuse: dayNightLightDiffuse(light, factors), | |
| specularStrength: dayNightLightSpecular(light, factors), | |
| rimStrength: dayNightLightRim(light, factors), | |
| }; | |
| } | |
| return { | |
| ...light, | |
| direction: | |
| light?.orbit != null | |
| ? light.direction | |
| : light?.direction ?? titleSceneSunDirection(hour, factors), | |
| ambient: dayNightLightAmbient(light, factors), | |
| diffuse: dayNightLightDiffuse(light, factors), | |
| specularStrength: dayNightLightSpecular(light, factors), | |
| rimStrength: dayNightLightRim(light, factors), | |
| }; | |
| } |
🤖 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 `@src/ui/title-scene-environment.ts` around lines 247 - 255, The SceneLight
builder in titleSceneEnvironment is still assigning a static direction even when
light.orbit is present, which leaves the returned object internally
inconsistent. Update the direction handling in the same orbit-aware pattern used
by ambient, diffuse, specularStrength, and rimStrength: in the function that
returns the merged light object, keep direction undefined when orbit is driving
the light, and only fall back to titleSceneSunDirection(hour, factors) for
non-orbit lights. This should be applied in the logic around
titleSceneEnvironment so any direct readers of .light.direction do not see a
misleading value.
Source: Coding guidelines
1 participant
Summary
This fixes the high-priority typing/history regression where rapid insert-mode typing could submit independent production text mutations concurrently and corrupt the Echo-backed text session.
Changes:
ProductionTextSessionso rapid keypresses admit in planned order.:wqqueue save/export behind pending edits instead of exporting a partial frontier.:wqcautious: it queues the save but does not arm quit confirmation while an unresolved edit chain exists.Root Cause
Fast typing created multiple asynchronous edit commands from the optimistic local editor. Those commands could reach the production text session concurrently, so later characters could be applied before earlier characters or against stale session state.
The first visible symptom was dropped/scrambled text during normal-speed typing. The adjacent save risk was that Ctrl-S could otherwise materialize an older Echo frontier while the local optimistic editor already displayed the full text.
Validation
npm run checkgit diff --checkFocused coverage added/updated:
exportSnapshot()until queued edits settle, then saves the full sentence and checkpoints.:wqqueues save without arming quit confirmation during unresolved edit chains.Follow-Up
Queued obstruction semantics still deserve a dedicated follow-up regression for
aapplied,bobstructed,cdefalready queued. Existing dependent-obstruction tests still pass, but that exact queue-level case should be made explicit before treating the queue model as complete under obstruction.