Polish GUI chat and persistent settings - #12
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughDisplays structured parsed agent output in chat (preferring parsed messages), adds a web-only configurable split terminal persisted in settings, enables Enter-to-send on web, and implements daemon CORS preflight handling with tests. ChangesParsed Agent Output & Split Terminal Feature
Sequence Diagram(s)sequenceDiagram
participant Relay as Relay Polling
participant ChatScreen
participant Transcript as Parsed Transcript Lib
participant MessageList as Message Renderer
Relay->>ChatScreen: outputResult.parsed
ChatScreen->>Transcript: messagesFromParsedAgentOutput(parsed)
Transcript->>ChatScreen: ChatMessage[] (filtered)
ChatScreen->>MessageList: render allMessages (parsed or merged)
Note over ChatScreen: Chat terminal split toggles pane visibility
sequenceDiagram
participant Browser
participant Daemon
Browser->>Daemon: OPTIONS /projects (preflight)
Daemon->>Daemon: handle() detects OPTIONS
Daemon->>Daemon: setCorsHeaders(res)
Daemon->>Browser: 204 + CORS headers
Browser->>Daemon: GET /projects (actual request)
Daemon->>Daemon: setCorsHeaders(res)
Daemon->>Browser: 200 + JSON + CORS headers
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 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
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
app/lib/parsed-transcript.test.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. 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: 4
🧹 Nitpick comments (1)
app/lib/parsed-transcript.test.ts (1)
9-43: ⚡ Quick winAdd a regression case for
kind-based parsed blocks.These tests only exercise
type, butmessagesFromParsedAgentOutputalso acceptskind. Add one case usingkind: "prompt" | "response"to lock compatibility behavior.Suggested test addition
+ it("accepts legacy kind-based prompt/response blocks", () => { + const messages = messagesFromParsedAgentOutput({ + blocks: [ + { kind: "prompt", text: "hi" }, + { kind: "response", text: "hello" }, + ], + }); + + expect(messages).toEqual([ + { id: "parsed-0-prompt", role: "user", parts: [{ type: "text", text: "hi" }] }, + { id: "parsed-1-response", role: "assistant", parts: [{ type: "text", text: "hello" }] }, + ]); + });🤖 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 `@app/lib/parsed-transcript.test.ts` around lines 9 - 43, The test currently covers parsed blocks using the "type" property but misses blocks using the alternative "kind" property; add a regression case in the same test file that calls messagesFromParsedAgentOutput with blocks using kind: "prompt" and kind: "response" (instead of type) and assert the returned messages match the same ordered user/assistant message shape and ids as the existing assertions; ensure you reference messagesFromParsedAgentOutput and include mixed cases (e.g., some blocks using kind and some using type) if you want to lock compatibility behavior.
🤖 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 `@app/app/`(main)/agent/[sessionId]/chat.tsx:
- Around line 246-259: The split-toggle UI (the View containing the Pressable
that uses setShowTerminalSplit, canShowTerminalSplit and showSplit) is being
rendered on native where it is always disabled; wrap or conditionally render
that entire control so it only appears on web (e.g., check Platform.OS === 'web'
or use a provided isWeb helper) and hide it on non-web so the inert header
action is not shown.
In `@app/stores/settings.test.ts`:
- Around line 25-32: The test setup stubs the global window in beforeAll via
vi.stubGlobal("window", globalThis) but never restores it; add a teardown to
call vi.unstubAllGlobals() (use afterAll to match repo patterns) so subsequent
tests do not inherit the browser-like window; place the afterAll alongside
beforeAll in the same test file (settings.test.ts) to ensure cleanup after
tests, referencing the existing beforeAll and vi.stubGlobal usage.
- Around line 54-66: The test uses a fixed sleep (setTimeout) which is flaky;
replace the sleep in the "persists focused atom writes to aimux-settings" test
by polling until persistence appears: after calling createStore() and
store.set(settingsModule.themePreferenceAtom, "light") /
store.set(settingsModule.chatTerminalSplitAtom, true), use a testing utility
like Vitest's waitFor to repeatedly call AsyncStorage.getItem("aimux-settings")
and assert that JSON.parse(...) equals the expected object, instead of awaiting
setTimeout(50); this ensures the assertion waits for the persistence to complete
reliably.
In `@src/daemon.ts`:
- Around line 163-167: Replace the wildcard CORS in setCorsHeaders with an
explicit allowlist check: read the incoming Origin header, compare it against a
whitelist (e.g., http://localhost, http://127.0.0.1 and any required dev
origins), and only set Access-Control-Allow-Origin and related headers when
Origin is allowed; for disallowed Origins do not set the CORS headers and ensure
preflight OPTIONS requests are rejected with a 403 (and actual requests receive
a 403/unauthorized response). Update any other CORS helper near the daemon
request handling (the other block referenced in the diff) to use the same Origin
allowlist check so both preflight and normal requests consistently reject
non-allowed origins.
---
Nitpick comments:
In `@app/lib/parsed-transcript.test.ts`:
- Around line 9-43: The test currently covers parsed blocks using the "type"
property but misses blocks using the alternative "kind" property; add a
regression case in the same test file that calls messagesFromParsedAgentOutput
with blocks using kind: "prompt" and kind: "response" (instead of type) and
assert the returned messages match the same ordered user/assistant message shape
and ids as the existing assertions; ensure you reference
messagesFromParsedAgentOutput and include mixed cases (e.g., some blocks using
kind and some using type) if you want to lock compatibility behavior.
🪄 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 Plus
Run ID: b72aca72-5402-4e1a-ae08-4fc79e40b37a
📒 Files selected for processing (16)
AGENTS.mdCLAUDE.mdapp/app/(main)/agent/[sessionId]/chat.tsxapp/app/(main)/settings.tsxapp/components/ChatComposer.tsxapp/lib/events.tsapp/lib/jotai-storage.tsapp/lib/parsed-transcript.test.tsapp/lib/parsed-transcript.tsapp/lib/theme-effect.tsapp/stores/chat.tsapp/stores/settings.test.tsapp/stores/settings.tsapp/stores/ui.tssrc/daemon.test.tssrc/daemon.ts
💤 Files with no reviewable changes (1)
- app/stores/ui.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/daemon.test.ts (1)
427-428:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid fixed daemon ports in integration tests to prevent flaky failures.
Line 427 and Line 458 hard-code ports (
49191,49192). If either port is occupied, these tests fail intermittently.💡 Suggested change
+import { createServer } from "node:net"; + +async function getFreeLoopbackPort(): Promise<number> { + return await new Promise<number>((resolve, reject) => { + const s = createServer(); + s.once("error", reject); + s.listen(0, "127.0.0.1", () => { + const addr = s.address(); + if (!addr || typeof addr === "string") { + s.close(); + reject(new Error("failed to allocate loopback port")); + return; + } + const port = addr.port; + s.close((err) => (err ? reject(err) : resolve(port))); + }); + }); +} ... - const port = "49191"; + const port = String(await getFreeLoopbackPort()); ... - const port = "49192"; + const port = String(await getFreeLoopbackPort());Also applies to: 458-459
🤖 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/daemon.test.ts` around lines 427 - 428, Replace the hard-coded port assignments in the test (the local variable named port and the env var process.env.AIMUX_DAEMON_PORT used in src/daemon.test.ts) with a dynamic free port allocation (for example: obtain an available port via the OS by binding to port 0 or using a helper like get-port) and set process.env.AIMUX_DAEMON_PORT to that value; do the same for the second occurrence around the other test (the block that currently uses "49192") so both tests use unique ephemeral ports and avoid collisions, and ensure any helper used returns the numeric port as a string if needed and is cleaned up after the test.
🤖 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.
Outside diff comments:
In `@src/daemon.test.ts`:
- Around line 427-428: Replace the hard-coded port assignments in the test (the
local variable named port and the env var process.env.AIMUX_DAEMON_PORT used in
src/daemon.test.ts) with a dynamic free port allocation (for example: obtain an
available port via the OS by binding to port 0 or using a helper like get-port)
and set process.env.AIMUX_DAEMON_PORT to that value; do the same for the second
occurrence around the other test (the block that currently uses "49192") so both
tests use unique ephemeral ports and avoid collisions, and ensure any helper
used returns the numeric port as a string if needed and is cleaned up after the
test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: df52ae6e-3706-4fb4-b2d6-1f46cc6ef192
📒 Files selected for processing (4)
app/app/(main)/agent/[sessionId]/chat.tsxapp/stores/settings.test.tssrc/daemon.test.tssrc/daemon.ts
This was referenced May 23, 2026
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
Verification
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests