Couple of small UI/UX improvements#13
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR introduces rate-limit visibility across the platform, enhances OpenCode agent capabilities with model/collaboration mode selection, refactors sidebar preferences to centralized storage utilities, integrates PWA support with offline capabilities, improves CodexAgent IPC frame buffering per thread, and updates supporting schemas and type definitions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/src/agents/adapters/codex-agent.ts (1)
161-175:⚠️ Potential issue | 🟠 MajorToken-usage broadcasts are now mixed into live-state reduction input.
These events are stored in
streamEventsByThreadId, butreadLiveState()reduces that store as stream-state events. This can trigger false validation warnings and noisy pruning on normal token updates.Suggested fix
for (const event of rawEvents) { + if (event.type !== "broadcast" || event.method !== "thread-stream-state-changed") { + continue; + } try { events.push(parseThreadStreamStateChangedBroadcast(event)); validRawEvents.push(event);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/agents/adapters/codex-agent.ts` around lines 161 - 175, The reducer is mistakenly treating "thread/tokenUsage/updated" as a stream-state event; update the handling in the codex-agent branch (the block around the frame.method checks and the conversationId extraction) so token-usage broadcasts are ignored for live-state reduction: only process "thread-stream-state-changed" into streamEventsByThreadId (skip/return early for "thread/tokenUsage/updated"), and ensure the conversationId is derived from params only for the stream-state branch (i.e., do not map token updates into the streamEventsByThreadId store used by readLiveState()). This keeps readLiveState() reductions free of token-usage noise.
🧹 Nitpick comments (2)
apps/server/src/agents/adapters/opencode-agent.ts (1)
88-99: Avoid silent per-directory failures in session listing.The empty
catchdrops directory-specific errors without visibility, which makes partial data loss hard to diagnose in production.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/agents/adapters/opencode-agent.ts` around lines 88 - 99, The current per-directory loop around this.service.listSessions silently swallows errors; update the catch to capture the error (e.g., catch (err)) and log a clear, contextual message including the directory and the error details so failures are visible when listing sessions for each directory (refer to the for (const directory of directories) loop, this.service.listSessions, the sessions map and this.threadDirectoryById); keep the continue behavior after logging so other directories are still processed.apps/web/test/app.test.tsx (1)
390-396: Make the rate-limit mock configurable per test.This branch hardcodes
/api/account/rate-limitsto an error path for all tests, which blocks success-path coverage (including usage badge rendering). A fixture toggle here would let tests exercise both 200 and 400 behaviors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/test/app.test.tsx` around lines 390 - 396, The current mock that returns a 400 for pathname === "/api/account/rate-limits" is hardcoded and prevents tests from exercising the success path; change the mock to consult a test-configurable flag (e.g., a global or exported fixture like __mockRateLimitsResponse or setMockRateLimitResponse) instead of returning a fixed error Response, defaulting to the existing 400 behavior when the flag is unset, and allow tests to toggle it to a 200 JSON payload so tests can cover both error and success (badge) rendering paths; update the mock branch that creates the Response for "/api/account/rate-limits" to branch based on that configurable value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/http-schemas.ts`:
- Around line 79-84: The parseQuery function currently uses the forbidden
unknown type for its second parameter and should instead use Zod's precise
input/output types; update the signature of parseQuery(schema: Schema, value:
unknown): z.infer<Schema> to use z.input<Schema> for the value parameter and
z.output<Schema> for the return type, keeping the function body as
schema.parse(value) and ensuring the function name parseQuery and the generic
Schema extends z.ZodTypeAny remain unchanged.
In `@apps/server/src/index.ts`:
- Around line 744-747: The route is letting parseQuery(AgentScopedQuerySchema,
{...}) throw and bubble up as a 500; wrap the parseQuery call in a try/catch (or
use a validation guard) in the handlers that call parseQuery (the `/api/models`
handler around parseQuery + resolveCapabilityAdapter("canListModels",
query.agentId) and likewise the `/api/collaboration-modes` handler) and return a
400 response when validation fails instead of letting the error propagate;
ensure you only call resolveCapabilityAdapter with a validated query.agentId and
send a clear 400 error payload when parseQuery throws/returns validation errors.
In `@apps/web/package.json`:
- Line 37: Move "vite-plugin-pwa" out of the runtime dependencies into
devDependencies in apps/web/package.json: remove the "vite-plugin-pwa" entry
from the top-level "dependencies" object and add the same version entry under
"devDependencies" so the plugin remains available for build time used by
apps/web/vite.config.ts (which generates virtual:pwa-register) but is not
shipped to production; after editing, reinstall/update the lockfile
(npm/yarn/pnpm) to ensure the lockfile reflects the change.
In `@apps/web/src/App.tsx`:
- Around line 967-969: The current update only sets rateLimits when a successful
read returns (if (nrl) setRateLimits(nrl)), which leaves stale values after
failures; change the logic in the update path(s) that call setRateLimits (the
block with if (nrl) setRateLimits(nrl) and the analogous block at 1368-1370) so
that when the read fails or nrl is falsy you explicitly clear the state (call
setRateLimits(null) or setRateLimits(undefined) / an empty object) instead of
doing nothing, ensuring quota badges don't show stale data.
- Around line 1834-1845: The color swatch buttons are unlabeled and need
accessible names so screen readers know what they do; update the button in the
swatch map (the element that calls setProjectColors with [group.key] and color
as GroupColor) to include a clear accessible label (e.g., an aria-label like
"Set {group.name} color to {color}" or add visually-hidden text inside the
button) that references the group (group.key or group.name) and the color value
so assistive tech users understand the action.
In `@apps/web/src/lib/sidebar-prefs.ts`:
- Around line 18-25: The readStorage function currently swallows JSON parse and
Zod validation errors and returns defaults; change it to fail hard and surface
clear errors instead: remove the broad silent catch and, when JSON.parse or
schema.parse throws, throw a new Error (or rethrow) that includes the storage
key (STORAGE_PREFIX + '.' + key) and the underlying error message so callers
know which stored value is corrupted; keep using schema.parse for validation but
do not fall back to defaultValue—fail fast in readStorage (function name:
readStorage, symbol: schema.parse, STORAGE_PREFIX) so persisted-data mismatches
are reported.
In `@packages/opencode-api/src/service.ts`:
- Around line 132-147: listAgents (and similar methods listSessions,
listProjectDirectories, listProviders) currently cast API responses with "as
Agent[]" without runtime validation; add a Zod schema for the expected Agent
shape, import z from 'zod', validate the response (use schema.parse or
schema.safeParse) against result.data returned by client.app.agents, and throw
or surface a clear error if validation fails; update each method to return the
parsed value instead of the raw assertion and reuse/declare shared Zod schemas
for other list* methods matching their expected types.
- Around line 119-129: The response is being blindly cast to a TypeScript type
(payload) instead of validated at runtime; add a Zod schema (e.g.,
ListProvidersResponseSchema) that matches { all?: array of
OpenCodeProviderSummary, default?: record(string,string), connected?: array of
string } and use schema.parse() or schema.safeParse(result.data) inside
listProviders (or the function returning payload) to validate result.data,
throwing or logging a clear error on failure; then map the validated value to
the returned shape (all -> all, default -> defaults, connected -> connected).
---
Outside diff comments:
In `@apps/server/src/agents/adapters/codex-agent.ts`:
- Around line 161-175: The reducer is mistakenly treating
"thread/tokenUsage/updated" as a stream-state event; update the handling in the
codex-agent branch (the block around the frame.method checks and the
conversationId extraction) so token-usage broadcasts are ignored for live-state
reduction: only process "thread-stream-state-changed" into
streamEventsByThreadId (skip/return early for "thread/tokenUsage/updated"), and
ensure the conversationId is derived from params only for the stream-state
branch (i.e., do not map token updates into the streamEventsByThreadId store
used by readLiveState()). This keeps readLiveState() reductions free of
token-usage noise.
---
Nitpick comments:
In `@apps/server/src/agents/adapters/opencode-agent.ts`:
- Around line 88-99: The current per-directory loop around
this.service.listSessions silently swallows errors; update the catch to capture
the error (e.g., catch (err)) and log a clear, contextual message including the
directory and the error details so failures are visible when listing sessions
for each directory (refer to the for (const directory of directories) loop,
this.service.listSessions, the sessions map and this.threadDirectoryById); keep
the continue behavior after logging so other directories are still processed.
In `@apps/web/test/app.test.tsx`:
- Around line 390-396: The current mock that returns a 400 for pathname ===
"/api/account/rate-limits" is hardcoded and prevents tests from exercising the
success path; change the mock to consult a test-configurable flag (e.g., a
global or exported fixture like __mockRateLimitsResponse or
setMockRateLimitResponse) instead of returning a fixed error Response,
defaulting to the existing 400 behavior when the flag is unset, and allow tests
to toggle it to a 200 JSON payload so tests can cover both error and success
(badge) rendering paths; update the mock branch that creates the Response for
"/api/account/rate-limits" to branch based on that configurable value.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
bun.lockis excluded by!**/*.lockpackages/codex-protocol/src/generated/app-server/GetAccountRateLimitsResponseSchema.tsis excluded by!**/generated/**packages/codex-protocol/src/generated/app-server/index.tsis excluded by!**/generated/**
📒 Files selected for processing (17)
apps/server/src/agents/adapters/codex-agent.tsapps/server/src/agents/adapters/opencode-agent.tsapps/server/src/agents/types.tsapps/server/src/http-schemas.tsapps/server/src/index.tsapps/web/package.jsonapps/web/src/App.tsxapps/web/src/components/ui/tooltip.tsxapps/web/src/lib/api.tsapps/web/src/lib/sidebar-prefs.tsapps/web/src/main.tsxapps/web/src/pwa.d.tsapps/web/test/app.test.tsxapps/web/vite.config.tspackages/codex-api/src/app-server-client.tspackages/codex-protocol/src/app-server.tspackages/opencode-api/src/service.ts
Owner
|
Very cool! Does it work on mobile as well? I also made some sidebar UX improvements so I think we conflict a bit, would you mind merging the latest? |
Contributor
Author
Hi 👋 |
Contributor
Author
|
@achimala merged the new code into this branch, added mobile preview screenshot Also removed some of my model selection logic customization since this feature doesn't work for me at all at the moment
If you could help with the testing of this branch - would be great. I don't know all the internals and frankly I don't want to change them, I want this PR to be purely the UI/UX updates. If I broke something on the backend - would be bad |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
apps/server/src/http-schemas.ts (1)
28-33:⚠️ Potential issue | 🟠 MajorReplace forbidden
unknowninparseQuerywith strict Zod input/output types.Line 30 uses
unknown, which violates the repo rule and weakens the type boundary for parsed query input.Proposed fix
export function parseQuery<Schema extends z.ZodTypeAny>( schema: Schema, - value: unknown -): z.infer<Schema> { + value: z.input<Schema> +): z.output<Schema> { return schema.parse(value); }#!/bin/bash # Verify parseQuery signature no longer uses forbidden unknown and uses strict Zod boundaries. rg -nP 'export function parseQuery|value:\s*unknown|z\.infer<Schema>|z\.input<Schema>|z\.output<Schema>' apps/server/src/http-schemas.tsAs per coding guidelines: “You CANNOT use
as anyorunknownin this codebase, they are FORBIDDEN.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/http-schemas.ts` around lines 28 - 33, The parseQuery function currently accepts value: unknown which violates the forbidden-unknown rule; update the signature to use strict Zod input/output types by changing the parameter type to z.input<Schema> (and the return type to z.output<Schema> or keep z.infer<Schema> if preferred) so the function becomes parseQuery<Schema extends z.ZodTypeAny>(schema: Schema, value: z.input<Schema>): z.output<Schema>; keep the body as schema.parse(value) and ensure imports/types still resolve for z.input/z.output usage.
🧹 Nitpick comments (2)
apps/web/src/lib/command-action-ui.ts (1)
27-33: Minor redundancy indisplayPathusage.The
pathvariable is already trimmed on line 37 (action.path?.trim() ?? ""), and line 55 checks!pathbefore callingdisplayPath(path). InsidedisplayPath, the path is trimmed again and checked for emptiness—both operations are redundant at that point. You could simplify by usingpathdirectly.♻️ Optional simplification
function displayPath(path: string): string { - const cleaned = path.trim(); - if (!cleaned) { - return "(path)"; - } - return cleaned; + return path || "(path)"; }Or inline in the
listFilesbranch:if (actionType === "listFiles") { if (!path) { return { iconKey: "listFiles", text: "list files" }; } return { iconKey: "listFiles", - text: truncate(displayPath(path)), - tooltip: displayPath(path) + text: truncate(path), + tooltip: path }; }Also applies to: 54-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/command-action-ui.ts` around lines 27 - 33, displayPath currently re-trims and re-checks emptiness even though callers already pass a trimmed non-empty path (see action.path?.trim() and the preceding !path guard); update displayPath (and call sites in the listFiles branch) to assume the input is already trimmed and non-empty by removing the extra trim()/emptiness logic from the displayPath function and simply return the passed string, and adjust any callers that relied on its fallback "(path)" to handle empty-path formatting instead.apps/web/src/App.tsx (1)
2030-2034: Consider adding a comment to clarify the codex-specific loading logic.The conditional combines multiple concerns (debug tab, codex provider, stream events flag). A brief comment would help future maintainers understand why stream events are always loaded for codex threads.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/App.tsx` around lines 2030 - 2034, Add a brief inline comment above the shouldLoadStreamEvents conditional explaining the codex-specific behavior: that stream events are always loaded for threads where threadAgentId === "codex" (even if includeStreamEvents is false) to support live codex-provider updates, and that the rest of the condition gates loading to the debug tab and read-permissions (canReadStreamEvents) and to the currently selected thread (selectedThreadIdRef/current threadId). Place this comment right above the declaration of shouldLoadStreamEvents and reference the variables used (shouldLoadStreamEvents, canReadStreamEvents, activeTabRef, threadAgentId, selectedThreadIdRef, includeStreamEvents) so future maintainers understand why codex threads bypass includeStreamEvents.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/agents/adapters/opencode-agent.ts`:
- Around line 183-186: The code currently accepts and stores raw model strings
in setCollaborationMode and lets parseModelSelection return null later in
sendMessage, silently downgrading invalid values; change setCollaborationMode to
validate the incoming model string immediately (use parseModelSelection or a
dedicated validator) and throw a clear error if it returns null/invalid, and
only store a validated model into threadModeById; update any callers/tests that
expect silent nulls accordingly so invalid model formats fail fast instead of
being converted during sendMessage.
In `@apps/server/src/index.ts`:
- Around line 968-971: The response currently uses jsonResponse(res, 400, { ok:
false, error: "No agent supports rate limit reading" }) which misclassifies a
backend capability/state issue as a bad client request; update the status code
to a server-capability code (e.g., jsonResponse(res, 503, ...) or 501) so
clients and monitoring see a server-side failure. Locate the call to
jsonResponse in index.ts where the error string "No agent supports rate limit
reading" is returned and replace the 400 status with 503 (or 501) while keeping
the payload unchanged.
In `@apps/web/vite.config.ts`:
- Around line 13-20: The Web manifest configured in manifest (vite.config.ts) is
missing an icons array which prevents PWA installability; add an icons field to
the manifest with entries for 192x192 and 512x512 PNGs (e.g. { src:
"/icons/icon-192.png", sizes: "192x192", type: "image/png" } and { src:
"/icons/icon-512.png", sizes: "512x512", type: "image/png" }), ensure those PNG
files exist in the app’s public assets (e.g. apps/web/public/icons/) and that
Vite is serving the public directory (confirm publicDir or static asset config
so /icons/* is reachable) before committing the manifest change.
In `@packages/codex-protocol/src/app-server.ts`:
- Around line 28-29: The local schema wrapper
AppServerGetAccountRateLimitsResponseBaseSchema currently calls .passthrough()
on GeneratedGetAccountRateLimitsResponseSchema which is redundant and relaxes
validation; remove the .passthrough() call so
AppServerGetAccountRateLimitsResponseBaseSchema simply references
GeneratedGetAccountRateLimitsResponseSchema directly, and audit other local
wrappers in this file that wrap generated schemas (e.g., other
AppServer*...BaseSchema declarations) to remove any additional .passthrough()
uses to enforce strict validation.
---
Duplicate comments:
In `@apps/server/src/http-schemas.ts`:
- Around line 28-33: The parseQuery function currently accepts value: unknown
which violates the forbidden-unknown rule; update the signature to use strict
Zod input/output types by changing the parameter type to z.input<Schema> (and
the return type to z.output<Schema> or keep z.infer<Schema> if preferred) so the
function becomes parseQuery<Schema extends z.ZodTypeAny>(schema: Schema, value:
z.input<Schema>): z.output<Schema>; keep the body as schema.parse(value) and
ensure imports/types still resolve for z.input/z.output usage.
---
Nitpick comments:
In `@apps/web/src/App.tsx`:
- Around line 2030-2034: Add a brief inline comment above the
shouldLoadStreamEvents conditional explaining the codex-specific behavior: that
stream events are always loaded for threads where threadAgentId === "codex"
(even if includeStreamEvents is false) to support live codex-provider updates,
and that the rest of the condition gates loading to the debug tab and
read-permissions (canReadStreamEvents) and to the currently selected thread
(selectedThreadIdRef/current threadId). Place this comment right above the
declaration of shouldLoadStreamEvents and reference the variables used
(shouldLoadStreamEvents, canReadStreamEvents, activeTabRef, threadAgentId,
selectedThreadIdRef, includeStreamEvents) so future maintainers understand why
codex threads bypass includeStreamEvents.
In `@apps/web/src/lib/command-action-ui.ts`:
- Around line 27-33: displayPath currently re-trims and re-checks emptiness even
though callers already pass a trimmed non-empty path (see action.path?.trim()
and the preceding !path guard); update displayPath (and call sites in the
listFiles branch) to assume the input is already trimmed and non-empty by
removing the extra trim()/emptiness logic from the displayPath function and
simply return the passed string, and adjust any callers that relied on its
fallback "(path)" to handle empty-path formatting instead.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockpackages/codex-protocol/src/generated/app-server/index.tsis excluded by!**/generated/**
📒 Files selected for processing (17)
apps/server/src/agents/adapters/codex-agent.tsapps/server/src/agents/adapters/opencode-agent.tsapps/server/src/agents/types.tsapps/server/src/http-schemas.tsapps/server/src/index.tsapps/server/src/unified/adapter.tsapps/web/package.jsonapps/web/src/App.tsxapps/web/src/components/ChatTimeline.tsxapps/web/src/components/ui/tooltip.tsxapps/web/src/lib/api.tsapps/web/src/lib/code-language.tsapps/web/src/lib/command-action-ui.tsapps/web/src/main.tsxapps/web/vite.config.tspackages/codex-api/src/app-server-client.tspackages/codex-protocol/src/app-server.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/web/src/main.tsx
- apps/web/src/lib/api.ts
- packages/codex-api/src/app-server-client.ts
- apps/server/src/agents/adapters/codex-agent.ts
- apps/server/src/agents/types.ts
Owner
Sorry about that, the breakage should be fixed now. Some files got dropped from the merge yesterday :( Yep I'm testing out the branch now |
# Conflicts: # apps/web/src/lib/code-language.ts # apps/web/src/lib/command-action-ui.ts
Owner
|
I tested it out and fixed a couple new merge conflicts, and improved the mobile layout a little to avoid the new stuff taking up so much space. This works great, thanks for the contribution! |
achimala
approved these changes
Mar 1, 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
2 participants
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.
Preview
Mobile preview
Summary by CodeRabbit
New Features
Improvements