Fix sidebar collapse and Codex project labels#8
Closed
davej wants to merge 2 commits into
Closed
Conversation
📝 WalkthroughWalkthroughThe PR introduces workspace labeling support for agents by adding a Changes
Sequence DiagramsequenceDiagram
participant Codex as Codex Global State
participant Server as Server (index.ts)
participant Descriptor as Agent Descriptor
participant API as /api/agents Response
participant Web as Web Client (App.tsx)
participant UI as Project Groups
Codex->>Server: electron-workspace-root-labels
Server->>Server: readCodexWorkspaceRootLabels()
Server->>Descriptor: buildAgentDescriptor(..., projectLabels)
Descriptor->>Descriptor: Include projectLabels field
Descriptor->>API: Return AgentDescriptor
API->>Web: Send agents with projectLabels
Web->>Web: Aggregate projectLabelsByPath from descriptors
Web->>Web: Resolve group labels from projectLabelsByPath
Web->>UI: Render project groups with workspace labels
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
apps/server/src/index.ts (1)
117-136: Consider caching or async reads for workspace labels.
readFileSyncin the/api/agentshot path can block the event loop every refresh; a small cache or async read would reduce tail latency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/index.ts` around lines 117 - 136, readCodexWorkspaceRootLabels currently uses synchronous fs.readFileSync which blocks the event loop; change it to a non-blocking, cached implementation: create an in-memory cache (e.g., module-level variable like workspaceLabelsCache with a TTL or last-loaded timestamp) and replace readCodexWorkspaceRootLabels with an async variant (e.g., readCodexWorkspaceRootLabelsAsync) that uses fs.promises.readFile(CODEX_GLOBAL_STATE_PATH, "utf8") and CodexGlobalStateSchema.parse(JSON.parse(...)); on read errors return {} and log with toErrorMessage as before; update any callers (such as the /api/agents hot path) to await the async function or read from the cache synchronously if fresh to avoid blocking.
🤖 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/index.ts`:
- Around line 42-46: The schema CodexGlobalStateSchema currently uses
.passthrough(), allowing unknown keys and bypassing strict validation; replace
.passthrough() with .strict() on the z.object(...) so the schema enforces exact
keys (e.g., "electron-workspace-root-labels") and rejects extra properties,
keeping the symbol CodexGlobalStateSchema and the object definition otherwise
unchanged.
In `@apps/web/src/App.tsx`:
- Around line 589-599: projectLabelsByPath currently assigns labels even if they
are empty/whitespace which leads to blank group names; update the logic that
builds the Map (inside the useMemo that calculates projectLabelsByPath) to trim
the label and only set it when label.trim() is non-empty, otherwise skip setting
so the UI falls back to the basename; apply the same guard wherever
descriptor.projectLabels values are consumed (the other occurrences handling
project labels) to ensure empty or whitespace-only labels are ignored.
---
Nitpick comments:
In `@apps/server/src/index.ts`:
- Around line 117-136: readCodexWorkspaceRootLabels currently uses synchronous
fs.readFileSync which blocks the event loop; change it to a non-blocking, cached
implementation: create an in-memory cache (e.g., module-level variable like
workspaceLabelsCache with a TTL or last-loaded timestamp) and replace
readCodexWorkspaceRootLabels with an async variant (e.g.,
readCodexWorkspaceRootLabelsAsync) that uses
fs.promises.readFile(CODEX_GLOBAL_STATE_PATH, "utf8") and
CodexGlobalStateSchema.parse(JSON.parse(...)); on read errors return {} and log
with toErrorMessage as before; update any callers (such as the /api/agents hot
path) to await the async function or read from the cache synchronously if fresh
to avoid blocking.
Comment on lines
+42
to
+46
| const CodexGlobalStateSchema = z | ||
| .object({ | ||
| "electron-workspace-root-labels": z.record(z.string()).default({}) | ||
| }) | ||
| .passthrough(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && cat -n apps/server/src/index.ts 2>/dev/null | head -60 || echo "File not found or different path"Repository: achimala/farfield
Length of output: 43
🌐 Web query:
Zod .strict() method vs .passthrough() difference schema validation
💡 Result:
In Zod object schemas, these methods control what happens to unknown (extra) keys during parsing:
.strict(): rejects unknown keys →parse()throws aZodError(issue code likeunrecognized_keys). Use when you want an exact data contract. [1].passthrough(): keeps unknown keys → they are preserved in the parsed output. Use when you want to accept/forward extra data (e.g., metadata, logging). [1]- (For context) default is “strip”: unknown keys are removed from the output unless you change the behavior. [1]
Example:
import { z } from "zod";
const Base = z.object({ name: z.string() });
Base.parse({ name: "A", extra: 1 }); // => { name: "A" } (default: strip)
Base.passthrough().parse({ name: "A", extra: 1 }); // => { name: "A", extra: 1 }
Base.strict().parse({ name: "A", extra: 1 }); // => throws ZodErrorIf you want unknown keys allowed but validated, use .catchall(schema) instead of passthrough/strict (it makes “unknown” keys no longer “unknown”—they must match the catchall schema). [1]
Sources:
[1] Zod docs on npm (“Unrecognized keys”, .passthrough, .strict, default stripping, .catchall). https://www.npmjs.com/package/zod/v/3.7.0
🏁 Script executed:
fd -e ts -e tsx | grep -E "(index\.ts|schema)" | head -20Repository: achimala/farfield
Length of output: 1719
🏁 Script executed:
cat -n apps/server/src/index.ts | head -80Repository: achimala/farfield
Length of output: 2962
🏁 Script executed:
cat -n apps/server/src/index.ts | sed -n '30,60p'Repository: achimala/farfield
Length of output: 1171
🏁 Script executed:
rg -A 5 -B 2 "CodexGlobalStateSchema" apps/server/src/index.tsRepository: achimala/farfield
Length of output: 589
🏁 Script executed:
rg -n "readCodexWorkspaceRootLabels\|CodexGlobalState" apps/server/src/index.ts | head -20Repository: achimala/farfield
Length of output: 43
Use a strict Zod schema for Codex global state.
The schema uses .passthrough() which allows unknown keys, violating the strict type validation requirement. Replace with .strict() to enforce exact schema compliance.
Proposed fix
const CodexGlobalStateSchema = z
.object({
"electron-workspace-root-labels": z.record(z.string()).default({})
})
- .passthrough();
+ .strict();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const CodexGlobalStateSchema = z | |
| .object({ | |
| "electron-workspace-root-labels": z.record(z.string()).default({}) | |
| }) | |
| .passthrough(); | |
| const CodexGlobalStateSchema = z | |
| .object({ | |
| "electron-workspace-root-labels": z.record(z.string()).default({}) | |
| }) | |
| .strict(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/index.ts` around lines 42 - 46, The schema
CodexGlobalStateSchema currently uses .passthrough(), allowing unknown keys and
bypassing strict validation; replace .passthrough() with .strict() on the
z.object(...) so the schema enforces exact keys (e.g.,
"electron-workspace-root-labels") and rejects extra properties, keeping the
symbol CodexGlobalStateSchema and the object definition otherwise unchanged.
Comment on lines
+589
to
+599
| const projectLabelsByPath = useMemo(() => { | ||
| const labels = new Map<string, string>(); | ||
| for (const descriptor of agentDescriptors) { | ||
| for (const [projectPath, label] of Object.entries(descriptor.projectLabels)) { | ||
| if (!labels.has(projectPath)) { | ||
| labels.set(projectPath, label); | ||
| } | ||
| } | ||
| } | ||
| return labels; | ||
| }, [agentDescriptors]); |
There was a problem hiding this comment.
Guard against empty project labels to avoid blank group names.
If a label is an empty string or whitespace, the UI will render an empty group label instead of the basename.
🛠️ Suggested hardening
- const projectLabelsByPath = useMemo(() => {
- const labels = new Map<string, string>();
- for (const descriptor of agentDescriptors) {
- for (const [projectPath, label] of Object.entries(descriptor.projectLabels)) {
- if (!labels.has(projectPath)) {
- labels.set(projectPath, label);
- }
- }
- }
- return labels;
- }, [agentDescriptors]);
+ const projectLabelsByPath = useMemo(() => {
+ const labels = new Map<string, string>();
+ for (const descriptor of agentDescriptors) {
+ for (const [projectPath, label] of Object.entries(descriptor.projectLabels)) {
+ const normalizedPath = projectPath.trim();
+ const normalizedLabel = label.trim();
+ if (!normalizedPath || !normalizedLabel) continue;
+ if (!labels.has(normalizedPath)) {
+ labels.set(normalizedPath, normalizedLabel);
+ }
+ }
+ }
+ return labels;
+ }, [agentDescriptors]);- const label = projectPath
- ? (projectLabelsByPath.get(projectPath) ?? basenameFromPath(projectPath))
- : "Unknown";
+ const label = projectPath
+ ? (projectLabelsByPath.get(projectPath) ?? basenameFromPath(projectPath))
+ : "Unknown";- label: projectLabelsByPath.get(normalized) ?? basenameFromPath(normalized),
+ label: projectLabelsByPath.get(normalized) ?? basenameFromPath(normalized),Also applies to: 616-618, 655-656
🤖 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 589 - 599, projectLabelsByPath currently
assigns labels even if they are empty/whitespace which leads to blank group
names; update the logic that builds the Map (inside the useMemo that calculates
projectLabelsByPath) to trim the label and only set it when label.trim() is
non-empty, otherwise skip setting so the UI falls back to the basename; apply
the same guard wherever descriptor.projectLabels values are consumed (the other
occurrences handling project labels) to ensure empty or whitespace-only labels
are ignored.
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
Testing
Summary by CodeRabbit
New Features
Tests