Merge upstream pingdotgg/t3code: 11 commits (tracing, IntelliJ, pagination, bug fixes)#44
Merged
aaditagrawal merged 15 commits intomainfrom Apr 4, 2026
Merged
Conversation
Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Julius Marminge <juliusmarminge@users.noreply.github.com>
…ames (pingdotgg#1684) Co-authored-by: Julius Marminge <julius0216@outlook.com>
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
…n, bug fixes) Resolved conflicts in 9 files: - editor.ts: migrated fork's extra editors to upstream's launchStyle schema - open.ts/open.test.ts: adapted macOS app detection to new launchStyle API - ProviderRuntimeIngestion.ts: kept token usage tracking, adopted Effect.fn(name) - settings.ts: kept both GenericProviderSettings and ObservabilitySettings - Icons.tsx: kept both OpenCodeIcon and IntelliJIdeaIcon - ThreadTerminalDrawer.tsx: merged imports and terminal utilities - GitActionsControl.tsx: merged pagination with fork's branch logic - terminalStateStore.test.ts: merged localStorage mock with expanded state
github-actions
bot
added
vouch:trusted
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 4 minutes and 26 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds end-to-end observability: new trace/metric primitives, local-file trace sink and OTLP integration, RPC/span instrumentation, server config/schema updates and persisted observability settings, UI/desktop plumbing, extensive metric/span instrumentation across server subsystems, branch-list pagination, and many tests and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (WebSocket / RPC Caller)
participant Ws as WsRpcGroup / RPC Server
participant Obs as Observability Layer
participant Tracer as Tracer (LocalFile / OTLP delegate)
participant Sink as TraceSink (NDJSON file)
participant Metrics as Metrics exporter
Client->>Ws: RPC request
Ws->>Obs: observeRpcEffect / annotate span
Obs->>Tracer: start span (with attributes)
Tracer->>Tracer: execute effect, collect timing/attributes
Tracer->>Metrics: update timers/counters (with outcome)
Tracer->>Sink: push serialized TraceRecord (if sampled)
Tracer->>Obs: end span
Obs->>Client: RPC response
rect rgba(0,128,128,0.5)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (4)
apps/web/src/orchestrationRecovery.test.ts (1)
101-115: Duplicate test — consider removing or differentiating.This test is identical to the first test at lines 6-20 (same setup, same assertions). The test name suggests it should verify the diagnostic state explaining why replay is requested (i.e.,
pendingReplayBeforeResetvsobservedAhead), but sinceresolveReplayNeedAfterRecovery()is internal, those values aren't accessible.Options:
- Remove this duplicate test.
- Expose a method (e.g.,
getReplayDecisionContext()) if external visibility into the replay decision rationale is valuable.- At minimum, update the test name to avoid implying coverage that doesn't exist.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/orchestrationRecovery.test.ts` around lines 101 - 115, The test in orchestrationRecovery.test.ts is a duplicate of the earlier one and either should be removed or changed to reflect the actual behavior being verified; either delete the duplicated it(...) block that uses createOrchestrationRecoveryCoordinator() and asserts getState(), or rename the test to avoid implying it inspects the replay rationale, or add a new public accessor (e.g., getReplayDecisionContext()) on the coordinator so the test can assert pendingReplayBeforeReset vs observedAhead; update tests to call getReplayDecisionContext() if you expose it, otherwise remove the duplicate test to avoid redundant coverage.apps/server/src/terminal/Layers/Manager.ts (1)
1337-1379: Decide whether these counters mean attempts or successes.Line 1337 increments
terminalSessionsTotalbefore the PTY reaches"running", and Line 1754 incrementsterminalRestartsTotalbeforeassertValidCwd()/startSession()finish. As placed, both counters measure attempts rather than successful lifecycle transitions, which can skew dashboards. If you want success counts, move the increments onto the post-start success path.Also applies to: 1754-1754
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/terminal/Layers/Manager.ts` around lines 1337 - 1379, The counters terminalSessionsTotal and terminalRestartsTotal currently increment on attempt; change them to increment only after a successful lifecycle transition by moving the increment calls off the pre-spawn/restart path and into the post-success path(s): for new sessions, call increment(terminalSessionsTotal, { lifecycle: eventType }) after trySpawn completes and after modifyManagerState sets session.status = "running" (or right after publishEvent on success); for restarts, call increment(terminalRestartsTotal, ...) after assertValidCwd()/startSession() finish successfully. Locate the symbols terminalSessionsTotal, terminalRestartsTotal, trySpawn, modifyManagerState, startSession, and assertValidCwd to reposition the increments accordingly so they reflect successful starts rather than attempts.apps/server/src/git/Layers/GitCore.test.ts (1)
389-417: Make this pagination test order deterministic.Lines 393-395 create three branches that all still point at the same commit, so the expected page contents depend on whatever tie-breaker
listBranches()uses for equal-recency branches. Since this test is really about cursor behavior, giving each branch a distinct commit/timestamp would make it much less brittle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/git/Layers/GitCore.test.ts` around lines 389 - 417, The test for listBranches is brittle because feature-a/b/c all point to the same commit so ordering is non-deterministic; update the test (in GitCore.test.ts) to create a distinct commit on each branch after creating it so each branch has a unique timestamp/recency (e.g., createBranch for "feature-a" then switch/commit a change on that branch, repeat for "feature-b" and "feature-c"), then call listBranches and assert pages as before; this ensures listBranches, createBranch and initRepoWithCommit produce deterministic ordering for pagination.packages/contracts/src/git.ts (1)
125-132: Consider treating emptyqueryas the no-filter case.Using
TrimmedNonEmptyStringhere means callers that forward""from a search box will fail validation instead of getting the first page of branches. Normalizing empty input toundefinedwould make this RPC boundary less brittle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/contracts/src/git.ts` around lines 125 - 132, The GitListBranchesInput currently requires query to be a TrimmedNonEmptyStringSchema which rejects "" from callers; change the query field so empty strings are treated as no-filter by making it optional and accepting a trimmed string (or a schema that permits empty after trim) and normalize empty values to undefined before validation or via a transform in the schema; update the GitListBranchesInput definition that references query and any consumers of GitListBranchesInput to handle query === undefined as the no-filter case.
🤖 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/cli.ts`:
- Around line 158-166: The function loadPersistedObservabilitySettings currently
masks all read errors by converting readFileString failures to an empty string,
which hides permission/toctou errors after exists returned true; change the
error handling so that after confirming the file exists you do not swallow read
errors — for example, remove the .pipe(Effect.orElseSucceed(() => "")) on
fs.readFileString and instead let the Effect fail (or convert it to a
descriptive error via Effect.mapError) so callers can observe/report the real
failure; locate the read call to FileSystem.readFileString inside
loadPersistedObservabilitySettings and replace the silent fallback with error
propagation or a mapped error that includes the path.
- Around line 31-32: Validate otlpTracesUrl and otlpMetricsUrl at config
boundary instead of Schema.String: replace Schema.optional(Schema.String) with a
URL-aware validator (e.g., Schema.optional(Schema.Url) or
Schema.optional(Schema.String.refine(s => { try { new URL(s); return true }
catch { return false } }))) so malformed endpoints are rejected early, and add
numeric lower-bound checks for the sizing/interval knobs (use Schema.Number or
Schema.Int with .min(0) or .greaterThan(0) as appropriate) so negative or zero
rotation/interval values are rejected during config parsing; update the config
schema definitions where otlpTracesUrl and otlpMetricsUrl are declared and where
the interval/rotation knobs are defined so validation fails fast.
In `@apps/server/src/git/Layers/GitCore.ts`:
- Around line 743-759: The current tracing span created in Effect.withSpan
inside execute (wrapping executeRaw) exports the full filesystem path via the
"git.cwd" attribute; change this to avoid shipping absolute paths by replacing
"git.cwd": input.cwd with a sanitized value such as a repository identifier or
path basename (e.g., derive from input.repoId or path.basename(input.cwd)), and
add an explicit opt-in flag (env var or debug option) that, when enabled,
includes the full input.cwd for debugging; update the attribute key usage in the
Effect.withSpan call (and any metric attributes if necessary) so only the safe
identifier is emitted by default and the full path is emitted only when the
opt-in flag is true.
In `@apps/server/src/observability/Attributes.ts`:
- Around line 20-75: normalizeJsonValue currently treats any previously-seen
object as circular because `seen` never unwinds, causing shared-but-acyclic
graphs like {a: shared, b: shared} to mark the second reference as "[Circular]".
Fix by switching `seen` to track the current recursion path (or implement a
`visit(value, seen, () => { ... })` helper that pushes before recursion and pops
after) and apply that same visit pattern to the Set and plain-object branches
(and confirm Array/Map branches use the visit helper too); update calls in
normalizeJsonValue so each recursive branch (Array, Map, Set, plain object) uses
visit/markSeen that unwinds after traversal instead of a global persistent mark.
Ensure you reference and update normalizeJsonValue, the Set branch, and the
plain-object branch (and any markSeen/visit helpers) so shared non-circular
references are preserved rather than reported as "[Circular]".
In `@apps/server/src/observability/Layers/Observability.ts`:
- Around line 23-27: The code currently treats only undefined as "disabled" for
OTLP endpoints, so empty strings can accidentally enable exporters; modify the
conditional logic around OtlpTracer.make (for traces) and the analogous Otlp
metrics exporter construction (where config.otlpMetricsUrl is used) to treat
empty or all-whitespace strings as disabled—e.g., check that the URL is
non-empty (trimmed) before calling OtlpTracer.make or the metrics maker and
return undefined if the URL is missing/empty; apply the same hardened check to
both traces and metrics initialization sites.
In `@apps/server/src/observability/TraceSink.ts`:
- Around line 31-43: The flushUnsafe function currently requeues failed chunks
indefinitely (buffer and sink.write), causing unbounded memory growth on
repeated write failures; add a hard cap constant (e.g., MAX_RETRY_BUFFER_CHUNKS
or MAX_RETRY_BUFFER_BYTES) and enforce it in the catch path that requeues the
chunk in flushUnsafe (and the similar catch at the other write location) so that
when the buffer exceeds the cap you drop the oldest chunk(s) or the new chunk
and emit a single error/log event; ensure you update the requeue logic around
buffer.unshift(chunk) to check the cap and trim/drop accordingly to prevent
unbounded growth.
In `@apps/server/src/open.test.ts`:
- Around line 111-118: The test's assertion for IDEA launch is flaky because
resolveEditorLaunch(..., "darwin") may return the macOS GUI fallback if the
`idea` CLI is not on PATH; update the test to make the command-path assertion
deterministic by either calling resolveEditorLaunch with platform "linux" when
asserting a CLI command (replace the "darwin" case used for command/path
expectations) or explicitly stub the command/app availability check used by
resolveEditorLaunch so the macOS fallback path is exercised only when intended;
locate the assertions around resolveEditorLaunch in open.test.ts (the blocks
starting at the current snippet and the similar block at lines 273-289) and
change those specific tests to use "linux" or add a stub/mocking hook for the
CLI presence check before asserting the expected command.
In `@apps/server/src/open.ts`:
- Around line 300-304: The trace is currently including raw filesystem paths via
the "open.cwd": input.cwd attribute in the Effect.annotateCurrentSpan call;
remove or redact that full path and replace it with low-cardinality metadata
(for example, a boolean or sanitized value such as path basename, workspace ID,
or the literal "<redacted>") so you no longer export absolute workspace/file
paths. Update the Effect.annotateCurrentSpan invocation (the attribute key
"open.cwd" / value input.cwd) to use the sanitized value instead of the raw
input.cwd.
- Around line 329-333: The platform-specific availability check currently calls
isCommandAvailable(editorDef.command) which uses the host OS rather than the
requested platform; update the check in resolveEditorLaunch (the block using
platform, editorDef.command, MAC_APP_NAMES and isMacAppInstalled) to call
isCommandAvailable with the requested platform (e.g.,
isCommandAvailable(editorDef.command, platform) or whatever overload the helper
supports) so the fallback for darwin/win32 respects the passed-in platform
instead of the host environment.
In `@apps/server/src/server.test.ts`:
- Line 577: The test currently asserts POSIX-only paths with
assert.equal(first.config.observability.logsDirectoryPath.endsWith("/logs"),
true); — replace this with a platform-neutral check: import Node's path and
assert the directory name using
path.basename(first.config.observability.logsDirectoryPath) (or compare against
path.join(...)/path.sep) so the assertion uses path.basename(...) === 'logs' (or
assert.strictEqual(path.basename(...), 'logs')) instead of endsWith("/logs"),
referencing first.config.observability.logsDirectoryPath and the existing assert
call.
In `@apps/server/src/terminal/Layers/Manager.ts`:
- Around line 1311-1316: Replace the raw working directory being attached to
spans in the annotateCurrentSpan call: do not pass input.cwd directly. Instead
compute a non-reversible fingerprint (e.g., SHA-256 or other hash) or redact
sensitive path segments and attach that value (e.g., "cwd_hash" or sanitized
basename) in place of input.cwd when calling annotateCurrentSpan along with
session.threadId, session.terminalId and eventType; update the key name to
reflect that it is hashed/sanitized (e.g., "terminal.cwd_hash" or
"terminal.cwd_safe") so full paths/usernames are never exported.
In `@apps/web/src/components/ChatView.tsx`:
- Around line 751-752: The mountedTerminalThreadIds state is initialized to []
which causes an open terminal to be missing until the reconcile effect runs and
creates a one-frame UI jump; to fix, derive the initial value from the same
reconcile logic used later (or run the first reconciliation in a layout effect)
so the initial render matches the reconciled state — locate the
mountedTerminalThreadIds / setMountedTerminalThreadIds state declaration in
ChatView and either (a) compute its initial value by calling the reconcile
function (or its pure helper) used in lines ~855-869 and pass that as the
useState initializer, or (b) move the first reconciliation from useEffect to
useLayoutEffect so mountedTerminalThreadIds is updated before paint. Ensure you
update any related logic that assumes an empty initial array to accept the
reconciled initial set.
In `@apps/web/src/components/ThreadTerminalDrawer.tsx`:
- Around line 603-625: The subscription handler (created via
useTerminalStateStore.subscribe / unsubscribeTerminalEvents) can miss events
that arrive between the initial selectTerminalEventEntries() read and setting
terminalHydratedRef.current = true; fix by doing one more "catch-up" read+apply
after marking hydrated: after you set terminalHydratedRef.current = true, re-run
selectTerminalEventEntries(terminalEventEntriesByKey, threadId, terminalId) and
call applyPendingTerminalEvents(...) if the last entry id changed to ensure any
events that landed during hydration are replayed; apply the same change to the
other identical subscription block that uses selectTerminalEventEntries and
applyPendingTerminalEvents (the one referenced in the comment).
In `@apps/web/src/orchestrationEventEffects.ts`:
- Around line 50-65: The archive/unarchive handlers currently replace the entire
threadLifecycleEffects entry (threadLifecycleEffects.set(event.payload.threadId,
...)), dropping previously accumulated flags like clearPromotedDraft and
clearDeletedThread; change both "thread.archived" and "thread.unarchived" to
merge with any existing entry: read the existing value
(threadLifecycleEffects.get(event.payload.threadId) || {}), copy its
clearPromotedDraft and clearDeletedThread fields (defaulting to false) and only
update removeTerminalState to true/false, then call set with the merged object;
also add a regression test covering the created → archived transition to ensure
clearPromotedDraft survives the archive operation.
In `@apps/web/src/routes/__root.tsx`:
- Around line 504-508: The current handler only skips recording terminal events
when a thread exists and is archived, but doesn't ignore events when the thread
is gone; update the conditional around useStore.getState().threads.find((entry)
=> entry.id === event.threadId) so that if the lookup returns falsy (no thread)
OR thread.archivedAt !== null you return early and do not call
useTerminalStateStore.getState().recordTerminalEvent(event); reference the
thread lookup (useStore.getState().threads and event.threadId), the archivedAt
check, and the recordTerminalEvent call when making the change.
In `@docs/observability.md`:
- Around line 168-253: The shell examples assume the T3CODE_HOME env var is
exported; update each command that references T3CODE_HOME (and the monorepo
./dev example) to either use the documented default path (~/.t3/...) directly
for local instructions or use a shell fallback when referencing T3CODE_HOME so
the commands work when it is unset; apply this change to all snippets that
mention server.trace.ndjson, tail -f, and the jq filters so they reliably point
at the default ~/.t3/... userdata/logs/server.trace.ndjson or a T3CODE_HOME
fallback.
In `@packages/shared/src/serverSettings.ts`:
- Around line 5-6: ServerSettingsJson currently decodes the entire
ServerSettings payload (via fromLenientJson(ServerSettings)), which causes
unrelated schema drift to block reading observability fields; change the logic
to decode only the observability subtree (or parse raw JSON and read
observability) instead of requiring full ServerSettings validity. Specifically,
replace the use of ServerSettingsJson/fromLenientJson(ServerSettings) when you
just need observability and extract observability.otlpTracesUrl and
observability.otlpMetricsUrl from a narrow schema or plain object, so corruption
in other parts of settings.json won’t prevent restoring those two URLs.
---
Nitpick comments:
In `@apps/server/src/git/Layers/GitCore.test.ts`:
- Around line 389-417: The test for listBranches is brittle because
feature-a/b/c all point to the same commit so ordering is non-deterministic;
update the test (in GitCore.test.ts) to create a distinct commit on each branch
after creating it so each branch has a unique timestamp/recency (e.g.,
createBranch for "feature-a" then switch/commit a change on that branch, repeat
for "feature-b" and "feature-c"), then call listBranches and assert pages as
before; this ensures listBranches, createBranch and initRepoWithCommit produce
deterministic ordering for pagination.
In `@apps/server/src/terminal/Layers/Manager.ts`:
- Around line 1337-1379: The counters terminalSessionsTotal and
terminalRestartsTotal currently increment on attempt; change them to increment
only after a successful lifecycle transition by moving the increment calls off
the pre-spawn/restart path and into the post-success path(s): for new sessions,
call increment(terminalSessionsTotal, { lifecycle: eventType }) after trySpawn
completes and after modifyManagerState sets session.status = "running" (or right
after publishEvent on success); for restarts, call
increment(terminalRestartsTotal, ...) after assertValidCwd()/startSession()
finish successfully. Locate the symbols terminalSessionsTotal,
terminalRestartsTotal, trySpawn, modifyManagerState, startSession, and
assertValidCwd to reposition the increments accordingly so they reflect
successful starts rather than attempts.
In `@apps/web/src/orchestrationRecovery.test.ts`:
- Around line 101-115: The test in orchestrationRecovery.test.ts is a duplicate
of the earlier one and either should be removed or changed to reflect the actual
behavior being verified; either delete the duplicated it(...) block that uses
createOrchestrationRecoveryCoordinator() and asserts getState(), or rename the
test to avoid implying it inspects the replay rationale, or add a new public
accessor (e.g., getReplayDecisionContext()) on the coordinator so the test can
assert pendingReplayBeforeReset vs observedAhead; update tests to call
getReplayDecisionContext() if you expose it, otherwise remove the duplicate test
to avoid redundant coverage.
In `@packages/contracts/src/git.ts`:
- Around line 125-132: The GitListBranchesInput currently requires query to be a
TrimmedNonEmptyStringSchema which rejects "" from callers; change the query
field so empty strings are treated as no-filter by making it optional and
accepting a trimmed string (or a schema that permits empty after trim) and
normalize empty values to undefined before validation or via a transform in the
schema; update the GitListBranchesInput definition that references query and any
consumers of GitListBranchesInput to handle query === undefined as the no-filter
case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 599c89b6-24ad-4675-aeb0-76f5696777aa
📒 Files selected for processing (82)
README.mdapps/desktop/src/main.tsapps/server/src/cli-config.test.tsapps/server/src/cli.tsapps/server/src/config.tsapps/server/src/git/Layers/GitCore.test.tsapps/server/src/git/Layers/GitCore.tsapps/server/src/git/Layers/GitManager.test.tsapps/server/src/git/Layers/GitManager.tsapps/server/src/observability/Attributes.test.tsapps/server/src/observability/Attributes.tsapps/server/src/observability/Layers/Observability.tsapps/server/src/observability/LocalFileTracer.test.tsapps/server/src/observability/LocalFileTracer.tsapps/server/src/observability/Metrics.test.tsapps/server/src/observability/Metrics.tsapps/server/src/observability/RpcInstrumentation.test.tsapps/server/src/observability/RpcInstrumentation.tsapps/server/src/observability/TraceRecord.tsapps/server/src/observability/TraceSink.test.tsapps/server/src/observability/TraceSink.tsapps/server/src/open.test.tsapps/server/src/open.tsapps/server/src/orchestration/Layers/CheckpointReactor.tsapps/server/src/orchestration/Layers/OrchestrationEngine.test.tsapps/server/src/orchestration/Layers/OrchestrationEngine.tsapps/server/src/orchestration/Layers/ProviderCommandReactor.tsapps/server/src/orchestration/Layers/ProviderRuntimeIngestion.tsapps/server/src/persistence/Layers/Sqlite.tsapps/server/src/provider/Layers/ProviderService.test.tsapps/server/src/provider/Layers/ProviderService.tsapps/server/src/server.test.tsapps/server/src/server.tsapps/server/src/serverLogger.tsapps/server/src/serverRuntimeStartup.tsapps/server/src/serverSettings.test.tsapps/server/src/telemetry/Layers/AnalyticsService.tsapps/server/src/terminal/Layers/Manager.tsapps/server/src/ws.tsapps/web/src/components/BranchToolbar.logic.tsapps/web/src/components/BranchToolbarBranchSelector.tsxapps/web/src/components/ChatView.browser.tsxapps/web/src/components/ChatView.logic.test.tsapps/web/src/components/ChatView.logic.tsapps/web/src/components/ChatView.tsxapps/web/src/components/DiffPanel.tsxapps/web/src/components/GitActionsControl.logic.test.tsapps/web/src/components/GitActionsControl.tsxapps/web/src/components/Icons.tsxapps/web/src/components/KeybindingsToast.browser.tsxapps/web/src/components/Sidebar.tsxapps/web/src/components/ThreadTerminalDrawer.test.tsapps/web/src/components/ThreadTerminalDrawer.tsxapps/web/src/components/chat/OpenInPicker.tsxapps/web/src/components/settings/SettingsPanels.browser.tsxapps/web/src/components/settings/SettingsPanels.tsxapps/web/src/lib/gitReactQuery.test.tsapps/web/src/lib/gitReactQuery.tsapps/web/src/lib/terminalStateCleanup.test.tsapps/web/src/lib/terminalStateCleanup.tsapps/web/src/orchestrationEventEffects.test.tsapps/web/src/orchestrationEventEffects.tsapps/web/src/orchestrationRecovery.test.tsapps/web/src/orchestrationRecovery.tsapps/web/src/routes/__root.tsxapps/web/src/rpc/serverState.test.tsapps/web/src/rpc/serverState.tsapps/web/src/rpc/serverStateBootstrap.tsxapps/web/src/terminalStateStore.test.tsapps/web/src/terminalStateStore.tsapps/web/src/wsNativeApi.test.tsdocs/observability.mdpackages/contracts/src/editor.tspackages/contracts/src/git.tspackages/contracts/src/server.tspackages/contracts/src/settings.tspackages/shared/package.jsonpackages/shared/src/git.tspackages/shared/src/serverSettings.test.tspackages/shared/src/serverSettings.tsscripts/package.jsonturbo.json
💤 Files with no reviewable changes (2)
- apps/web/src/rpc/serverStateBootstrap.tsx
- scripts/package.json
Comment on lines
+31
to
+32
| otlpTracesUrl: Schema.optional(Schema.String), | ||
| otlpMetricsUrl: Schema.optional(Schema.String), |
There was a problem hiding this comment.
Validate the new observability settings at the boundary.
otlpTracesUrl / otlpMetricsUrl are accepted as arbitrary strings, and the new sizing/interval knobs have no lower-bound checks. A malformed endpoint or negative interval/rotation value now gets through config resolution and only fails later in the tracer/exporter.
Also applies to: 93-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/cli.ts` around lines 31 - 32, Validate otlpTracesUrl and
otlpMetricsUrl at config boundary instead of Schema.String: replace
Schema.optional(Schema.String) with a URL-aware validator (e.g.,
Schema.optional(Schema.Url) or Schema.optional(Schema.String.refine(s => { try {
new URL(s); return true } catch { return false } }))) so malformed endpoints are
rejected early, and add numeric lower-bound checks for the sizing/interval knobs
(use Schema.Number or Schema.Int with .min(0) or .greaterThan(0) as appropriate)
so negative or zero rotation/interval values are rejected during config parsing;
update the config schema definitions where otlpTracesUrl and otlpMetricsUrl are
declared and where the interval/rotation knobs are defined so validation fails
fast.
Comment on lines
+158
to
+166
| const loadPersistedObservabilitySettings = Effect.fn(function* (settingsPath: string) { | ||
| const fs = yield* FileSystem.FileSystem; | ||
| const exists = yield* fs.exists(settingsPath).pipe(Effect.orElseSucceed(() => false)); | ||
| if (!exists) { | ||
| return { otlpTracesUrl: undefined, otlpMetricsUrl: undefined }; | ||
| } | ||
|
|
||
| const raw = yield* fs.readFileString(settingsPath).pipe(Effect.orElseSucceed(() => "")); | ||
| return parsePersistedServerObservabilitySettings(raw); |
There was a problem hiding this comment.
Don't silently discard persisted OTLP settings on read failures.
After Line 160 confirms the file exists, Line 165 turns every readFileString failure into "". Permission problems or TOCTOU races then look like “no settings”, which breaks the restart-persistence guarantee and hides the real fault.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/cli.ts` around lines 158 - 166, The function
loadPersistedObservabilitySettings currently masks all read errors by converting
readFileString failures to an empty string, which hides permission/toctou errors
after exists returned true; change the error handling so that after confirming
the file exists you do not swallow read errors — for example, remove the
.pipe(Effect.orElseSucceed(() => "")) on fs.readFileString and instead let the
Effect fail (or convert it to a descriptive error via Effect.mapError) so
callers can observe/report the real failure; locate the read call to
FileSystem.readFileString inside loadPersistedObservabilitySettings and replace
the silent fallback with error propagation or a mapped error that includes the
path.
Comment on lines
+743
to
+759
| const execute: GitCoreShape["execute"] = (input) => | ||
| executeRaw(input).pipe( | ||
| withMetrics({ | ||
| counter: gitCommandsTotal, | ||
| timer: gitCommandDuration, | ||
| attributes: { | ||
| operation: input.operation, | ||
| }, | ||
| }), | ||
| Effect.withSpan(input.operation, { | ||
| kind: "client", | ||
| attributes: { | ||
| "git.operation": input.operation, | ||
| "git.cwd": input.cwd, | ||
| "git.args_count": input.args.length, | ||
| }, | ||
| }), |
There was a problem hiding this comment.
Don’t export absolute repo paths in every git span.
input.cwd will usually include usernames and local project names. With OTLP enabled, this ships host-specific filesystem data off-box on every git command. Prefer a repo id/basename, or gate full paths behind an explicit debug-only opt-in.
Possible fix
Effect.withSpan(input.operation, {
kind: "client",
attributes: {
"git.operation": input.operation,
- "git.cwd": input.cwd,
+ "git.repo": path.basename(input.cwd),
"git.args_count": input.args.length,
},
}),📝 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 execute: GitCoreShape["execute"] = (input) => | |
| executeRaw(input).pipe( | |
| withMetrics({ | |
| counter: gitCommandsTotal, | |
| timer: gitCommandDuration, | |
| attributes: { | |
| operation: input.operation, | |
| }, | |
| }), | |
| Effect.withSpan(input.operation, { | |
| kind: "client", | |
| attributes: { | |
| "git.operation": input.operation, | |
| "git.cwd": input.cwd, | |
| "git.args_count": input.args.length, | |
| }, | |
| }), | |
| const execute: GitCoreShape["execute"] = (input) => | |
| executeRaw(input).pipe( | |
| withMetrics({ | |
| counter: gitCommandsTotal, | |
| timer: gitCommandDuration, | |
| attributes: { | |
| operation: input.operation, | |
| }, | |
| }), | |
| Effect.withSpan(input.operation, { | |
| kind: "client", | |
| attributes: { | |
| "git.operation": input.operation, | |
| "git.repo": path.basename(input.cwd), | |
| "git.args_count": input.args.length, | |
| }, | |
| }), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/git/Layers/GitCore.ts` around lines 743 - 759, The current
tracing span created in Effect.withSpan inside execute (wrapping executeRaw)
exports the full filesystem path via the "git.cwd" attribute; change this to
avoid shipping absolute paths by replacing "git.cwd": input.cwd with a sanitized
value such as a repository identifier or path basename (e.g., derive from
input.repoId or path.basename(input.cwd)), and add an explicit opt-in flag (env
var or debug option) that, when enabled, includes the full input.cwd for
debugging; update the attribute key usage in the Effect.withSpan call (and any
metric attributes if necessary) so only the safe identifier is emitted by
default and the full path is emitted only when the opt-in flag is true.
Comment on lines
+603
to
625
| const unsubscribeTerminalEvents = useTerminalStateStore.subscribe((state, previousState) => { | ||
| if (!terminalHydratedRef.current) { | ||
| return; | ||
| } | ||
|
|
||
| const previousLastEntryId = | ||
| selectTerminalEventEntries( | ||
| previousState.terminalEventEntriesByKey, | ||
| threadId, | ||
| terminalId, | ||
| ).at(-1)?.id ?? 0; | ||
| const nextEntries = selectTerminalEventEntries( | ||
| state.terminalEventEntriesByKey, | ||
| threadId, | ||
| terminalId, | ||
| ); | ||
| const nextLastEntryId = nextEntries.at(-1)?.id ?? 0; | ||
| if (nextLastEntryId === previousLastEntryId) { | ||
| return; | ||
| } | ||
|
|
||
| applyPendingTerminalEvents(nextEntries); | ||
| }); |
There was a problem hiding this comment.
Do one more catch-up pass after hydrating the terminal.
An event that lands after the selectTerminalEventEntries(...) read but before terminalHydratedRef.current = true is ignored by the subscription and never replayed if nothing else arrives. That can drop the terminal’s final error or exited event.
Possible fix
for (const entry of replayEntries) {
applyTerminalEvent(entry.event);
}
lastAppliedTerminalEventIdRef.current = bufferedEntries.at(-1)?.id ?? 0;
terminalHydratedRef.current = true;
+ applyPendingTerminalEvents(
+ selectTerminalEventEntries(
+ useTerminalStateStore.getState().terminalEventEntriesByKey,
+ threadId,
+ terminalId,
+ ),
+ );
if (autoFocus) {
window.requestAnimationFrame(() => {
activeTerminal.focus();
});
}Also applies to: 643-656
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/ThreadTerminalDrawer.tsx` around lines 603 - 625, The
subscription handler (created via useTerminalStateStore.subscribe /
unsubscribeTerminalEvents) can miss events that arrive between the initial
selectTerminalEventEntries() read and setting terminalHydratedRef.current =
true; fix by doing one more "catch-up" read+apply after marking hydrated: after
you set terminalHydratedRef.current = true, re-run
selectTerminalEventEntries(terminalEventEntriesByKey, threadId, terminalId) and
call applyPendingTerminalEvents(...) if the last entry id changed to ensure any
events that landed during hydration are replayed; apply the same change to the
other identical subscription block that uses selectTerminalEventEntries and
applyPendingTerminalEvents (the one referenced in the comment).
Comment on lines
+50
to
+65
| case "thread.archived": { | ||
| threadLifecycleEffects.set(event.payload.threadId, { | ||
| clearPromotedDraft: false, | ||
| clearDeletedThread: false, | ||
| removeTerminalState: true, | ||
| }); | ||
| break; | ||
| } | ||
|
|
||
| case "thread.unarchived": { | ||
| threadLifecycleEffects.set(event.payload.threadId, { | ||
| clearPromotedDraft: false, | ||
| clearDeletedThread: false, | ||
| removeTerminalState: false, | ||
| }); | ||
| break; |
There was a problem hiding this comment.
Preserve earlier lifecycle flags when archive state changes.
On Line 51 and Line 60, the new set() calls replace the whole entry. A batch like thread.created → thread.archived now drops clearPromotedDraft, so the promoted draft can survive even though the thread was created. These archive transitions should only flip removeTerminalState and keep any previously accumulated flags for that thread. Please add a created → archived regression case alongside the new archive tests.
💡 Proposed fix
case "thread.archived": {
+ const current = threadLifecycleEffects.get(event.payload.threadId);
threadLifecycleEffects.set(event.payload.threadId, {
- clearPromotedDraft: false,
- clearDeletedThread: false,
+ clearPromotedDraft: current?.clearPromotedDraft ?? false,
+ clearDeletedThread: current?.clearDeletedThread ?? false,
removeTerminalState: true,
});
break;
}
case "thread.unarchived": {
+ const current = threadLifecycleEffects.get(event.payload.threadId);
threadLifecycleEffects.set(event.payload.threadId, {
- clearPromotedDraft: false,
- clearDeletedThread: false,
+ clearPromotedDraft: current?.clearPromotedDraft ?? false,
+ clearDeletedThread: current?.clearDeletedThread ?? false,
removeTerminalState: false,
});
break;
}📝 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
| case "thread.archived": { | |
| threadLifecycleEffects.set(event.payload.threadId, { | |
| clearPromotedDraft: false, | |
| clearDeletedThread: false, | |
| removeTerminalState: true, | |
| }); | |
| break; | |
| } | |
| case "thread.unarchived": { | |
| threadLifecycleEffects.set(event.payload.threadId, { | |
| clearPromotedDraft: false, | |
| clearDeletedThread: false, | |
| removeTerminalState: false, | |
| }); | |
| break; | |
| case "thread.archived": { | |
| const current = threadLifecycleEffects.get(event.payload.threadId); | |
| threadLifecycleEffects.set(event.payload.threadId, { | |
| clearPromotedDraft: current?.clearPromotedDraft ?? false, | |
| clearDeletedThread: current?.clearDeletedThread ?? false, | |
| removeTerminalState: true, | |
| }); | |
| break; | |
| } | |
| case "thread.unarchived": { | |
| const current = threadLifecycleEffects.get(event.payload.threadId); | |
| threadLifecycleEffects.set(event.payload.threadId, { | |
| clearPromotedDraft: current?.clearPromotedDraft ?? false, | |
| clearDeletedThread: current?.clearDeletedThread ?? false, | |
| removeTerminalState: false, | |
| }); | |
| break; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/orchestrationEventEffects.ts` around lines 50 - 65, The
archive/unarchive handlers currently replace the entire threadLifecycleEffects
entry (threadLifecycleEffects.set(event.payload.threadId, ...)), dropping
previously accumulated flags like clearPromotedDraft and clearDeletedThread;
change both "thread.archived" and "thread.unarchived" to merge with any existing
entry: read the existing value
(threadLifecycleEffects.get(event.payload.threadId) || {}), copy its
clearPromotedDraft and clearDeletedThread fields (defaulting to false) and only
update removeTerminalState to true/false, then call set with the merged object;
also add a regression test covering the created → archived transition to ensure
clearPromotedDraft survives the archive operation.
- Redact raw filesystem paths from trace span attributes (git, open, terminal) using path.basename() instead of full cwd to prevent PII leaks via OTLP - Cap TraceSink retry buffer at 5MB to prevent unbounded memory growth - Fix shared-reference false positives in normalizeJsonValue by unwinding the seen set after recursion (visit/unvisit pattern) - Treat empty/whitespace OTLP URLs as disabled in Observability layer - Parse only observability subtree in serverSettings.ts to avoid unrelated schema drift blocking OTLP URL persistence - Use platform-neutral path assertion in server.test.ts - Use useLayoutEffect for terminal thread reconciliation to avoid flash - Add catch-up pass after terminal hydration to prevent dropped events - Preserve accumulated lifecycle flags in orchestrationEventEffects archive - Guard against terminal events for deleted threads in __root.tsx - Use shell fallback for T3CODE_HOME in observability docs - Make IDEA launch tests deterministic by using linux platform - Pass platform parameter to isCommandAvailable for cross-platform correctness
There was a problem hiding this comment.
Actionable comments posted: 3
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/open.ts (1)
107-120:⚠️ Potential issue | 🟡 MinorConfirm intent:
traeandantigravityeditors are missing fromMAC_APP_NAMESmap.Both editors are declared in
packages/contracts/src/editor.tsbut have no entries in the macOS app-name map (lines 107-120). If this omission is unintentional, the.appdiscovery andopen -afallback will not work for these editors on macOS when their CLI shims are missing. Either add entries for these editors or confirm this exclusion is intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/open.ts` around lines 107 - 120, The MAC_APP_NAMES map is missing entries for the editors "trae" and "antigravity", which prevents macOS .app discovery and the open -a fallback for those EditorId values; update the MAC_APP_NAMES constant to include mappings for the EditorId keys "trae" and "antigravity" with their correct macOS application display names (e.g., "Trae" and "Antigravity" or the actual bundle names) so the code paths using MAC_APP_NAMES (in functions that resolve editor apps and call open -a) can locate and open those editors, or explicitly confirm in a comment above MAC_APP_NAMES that the omission is intentional if you do not want to support those editors on macOS.
🤖 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/open.ts`:
- Around line 327-333: The fallback using `open -a <app> --args ...` won't pass
args to an already-running macOS app; update the logic around MAC_APP_NAMES,
isMacAppInstalled and the branch that returns { command: "open", args: [...] }
to handle running apps: detect whether the app is currently running (e.g., via
`pgrep`/`osascript`/ps) and if it is, invoke an AppleScript/osascript payload to
tell the app to open the target file/URL (or use the app's URL scheme) so
file/line info is delivered; only use `open -a ... --args` for launching a new
instance. Ensure the code that composes the return value for the darwin fallback
returns the osascript/AppleScript invocation when the app is running and keeps
the existing `open` invocation when not.
In `@apps/web/src/components/ChatView.tsx`:
- Around line 478-481: The splitTerminal callback currently calls
storeSplitTerminal unconditionally and must reinstate the
MAX_TERMINALS_PER_GROUP guard: before calling storeSplitTerminal and
bumpFocusRequestId, check the current number of terminals for the given threadId
(use the same selector/state access used by the active-thread shortcut path) and
if it is already >= MAX_TERMINALS_PER_GROUP, return early (or show the same UI
warning) otherwise proceed to call storeSplitTerminal(threadId,
`terminal-${randomUUID()}`) and bumpFocusRequestId(); keep the dependency array
as [bumpFocusRequestId, storeSplitTerminal, threadId].
- Around line 428-452: The worktreePath currently uses a nullish fallback that
lets a draftThread's path override an authoritative serverThread when
serverThread exists but has worktreePath === null; change the logic so that if
serverThread is present you use serverThread.worktreePath (even if null), and
only fall back to draftThread.worktreePath when serverThread is absent—update
the worktreePath assignment (referencing serverThread, draftThread, and
worktreePath) accordingly and ensure the cwd useMemo continues to derive from
that corrected value.
---
Outside diff comments:
In `@apps/server/src/open.ts`:
- Around line 107-120: The MAC_APP_NAMES map is missing entries for the editors
"trae" and "antigravity", which prevents macOS .app discovery and the open -a
fallback for those EditorId values; update the MAC_APP_NAMES constant to include
mappings for the EditorId keys "trae" and "antigravity" with their correct macOS
application display names (e.g., "Trae" and "Antigravity" or the actual bundle
names) so the code paths using MAC_APP_NAMES (in functions that resolve editor
apps and call open -a) can locate and open those editors, or explicitly confirm
in a comment above MAC_APP_NAMES that the omission is intentional if you do not
want to support those editors on macOS.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 484692dd-9b52-41e5-adbd-d4bf48745fc0
📒 Files selected for processing (14)
apps/server/src/git/Layers/GitCore.tsapps/server/src/observability/Attributes.tsapps/server/src/observability/Layers/Observability.tsapps/server/src/observability/TraceSink.tsapps/server/src/open.test.tsapps/server/src/open.tsapps/server/src/server.test.tsapps/server/src/terminal/Layers/Manager.tsapps/web/src/components/ChatView.tsxapps/web/src/components/ThreadTerminalDrawer.tsxapps/web/src/orchestrationEventEffects.tsapps/web/src/routes/__root.tsxdocs/observability.mdpackages/shared/src/serverSettings.ts
✅ Files skipped from review due to trivial changes (2)
- apps/server/src/terminal/Layers/Manager.ts
- docs/observability.md
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/web/src/orchestrationEventEffects.ts
- apps/server/src/observability/Layers/Observability.ts
- packages/shared/src/serverSettings.ts
- apps/server/src/observability/Attributes.ts
- apps/server/src/open.test.ts
- apps/server/src/observability/TraceSink.ts
- apps/web/src/components/ThreadTerminalDrawer.tsx
Comment on lines
+327
to
333
| // On macOS, fall back to `open -a` when the CLI tool isn't in PATH | ||
| // but the .app bundle is installed. | ||
| if (platform === "darwin" && !isCommandAvailable(editorDef.command, { platform })) { | ||
| const macApp = MAC_APP_NAMES[editorDef.id]; | ||
| if (macApp && isMacAppInstalled(macApp)) { | ||
| return { command: "open", args: ["-a", macApp, input.cwd] }; | ||
| return { command: "open", args: ["-a", macApp, "--args", ...args] }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
On macOS, when an application is already running, does open -a "" --args ...still deliver--argsto that app, or are those arguments only used whenopenlaunches a new instance? Please cite Appleopen documentation or other primary sources.
💡 Result:
No, when an application is already running, open -a "" --args ... does not deliver the --args arguments to that app. Instead, it activates (brings to the foreground) the existing instance without passing the arguments. The --args are only passed to a new instance of the app when open launches it (i.e., when no instance is running, or with the -n flag to force a new instance). This behavior aligns with how Launch Services works: for a running app, open sends an Apple event (like 'rapp' to reopen/activate) rather than relaunching the executable with new argv. Command-line arguments via --args are only delivered to the app's main function on initial launch.
Citations:
- 1: https://www.unix.com/man-page/osx/1/open?os=osx§ion=1&query=open
- 2: https://stackoverflow.com/questions/66719941/macos-app-why-does-commandline-arguments-of-an-already-open-app-not-contain-th
- 3: https://developer.apple.com/library/archive/documentation/Carbon/Conceptual/LaunchServicesConcepts/LSCTasks/LSCTasks.html
- 4: https://stackoverflow.com/questions/15848450/how-to-grab-command-line-arguments-when-application-is-reopen-on-mac
The open -a ... --args fallback will not deliver arguments to an already-running application.
When the editor is already open, open -a "<app>" --args ... only activates the existing instance and does not pass the --args arguments to it. Command-line arguments are only delivered when open launches a new instance; for running apps, macOS Launch Services sends a reopen event without argv delivery. This makes the no-CLI fallback unreliable in the common case where the editor is already open, potentially leaving the user without the requested file/line information.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/open.ts` around lines 327 - 333, The fallback using `open -a
<app> --args ...` won't pass args to an already-running macOS app; update the
logic around MAC_APP_NAMES, isMacAppInstalled and the branch that returns {
command: "open", args: [...] } to handle running apps: detect whether the app is
currently running (e.g., via `pgrep`/`osascript`/ps) and if it is, invoke an
AppleScript/osascript payload to tell the app to open the target file/URL (or
use the app's URL scheme) so file/line info is delivered; only use `open -a ...
--args` for launching a new instance. Ensure the code that composes the return
value for the darwin fallback returns the osascript/AppleScript invocation when
the app is running and keeps the existing `open` invocation when not.
- Use explicit ternary for worktreePath/projectId to prevent stale draft state from overriding authoritative server thread values - Add MAX_TERMINALS_PER_GROUP guard to PersistentThreadTerminalDrawer's splitTerminal callback, matching the active-thread path
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/src/components/ChatView.tsx (1)
554-578: Double visibility gating is intentional but worth a brief comment.The outer
<div className={visible ? undefined : "hidden"}>hides the DOM node via CSS, while the innervisibleprop onThreadTerminalDrawershort-circuits resize/effect work. Both are useful, but readers may wonder why there are two layers. A one-liner comment clarifying "CSS hide + effect skip for performance" would aid future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/ChatView.tsx` around lines 554 - 578, Add a one-line comment above the outer <div> explaining why there are two visibility checks: the div's className (visible ? undefined : "hidden") is for CSS DOM hiding while the ThreadTerminalDrawer visible prop short-circuits internal effects/resizes for performance. Locate the return block in ChatView.tsx where ThreadTerminalDrawer is rendered and insert the comment referencing the outer div and the ThreadTerminalDrawer visible prop so future readers understand the dual gating intent.
🤖 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/web/src/components/ChatView.tsx`:
- Around line 4479-4490: The visibility check for each drawer uses the
component-level terminalState.terminalOpen instead of the per-thread state;
update the PersistentThreadTerminalDrawer prop `visible` inside the
mountedTerminalThreadIds.map to use the corresponding thread's state lookup (use
`terminalStateByThreadId[mountedThreadId]?.terminalOpen`) combined with the
existing activeThreadId check (i.e., `mountedThreadId === activeThreadId &&
terminalStateByThreadId[mountedThreadId]?.terminalOpen`) so each drawer derives
visibility from its own terminal state.
---
Nitpick comments:
In `@apps/web/src/components/ChatView.tsx`:
- Around line 554-578: Add a one-line comment above the outer <div> explaining
why there are two visibility checks: the div's className (visible ? undefined :
"hidden") is for CSS DOM hiding while the ThreadTerminalDrawer visible prop
short-circuits internal effects/resizes for performance. Locate the return block
in ChatView.tsx where ThreadTerminalDrawer is rendered and insert the comment
referencing the outer div and the ThreadTerminalDrawer visible prop so future
readers understand the dual gating intent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 08159b10-b72f-43ba-be7e-249f9eb9ff0b
📒 Files selected for processing (1)
apps/web/src/components/ChatView.tsx
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
7 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.
Upstream Sync
Merges 11 commits from
pingdotgg/t3codemain into our fork.Upstream Changes
Features:
Bug Fixes:
Conflict Resolution (9 files)
packages/contracts/src/editor.tslaunchStyleschemaapps/server/src/open.tslaunchStyleAPIapps/server/src/open.test.tsapps/server/src/orchestration/Layers/ProviderRuntimeIngestion.tsEffect.fn(name)conventionpackages/contracts/src/settings.tsGenericProviderSettings(fork) andObservabilitySettings(upstream)apps/web/src/components/Icons.tsxOpenCodeIcon(fork) andIntelliJIdeaIcon(upstream)apps/web/src/components/ThreadTerminalDrawer.tsxapps/web/src/components/GitActionsControl.tsxapps/web/src/terminalStateStore.test.tsVerification
bun fmtbun lint(0 errors)bun typecheckSummary by CodeRabbit
New Features
Improvements