Sync fork with upstream/main (post-v0.0.15, 17 commits)#40
Merged
aaditagrawal merged 21 commits intomainfrom Mar 30, 2026
Merged
Sync fork with upstream/main (post-v0.0.15, 17 commits)#40aaditagrawal merged 21 commits intomainfrom
aaditagrawal merged 21 commits intomainfrom
Conversation
Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: cursor[bot] <206951365+cursor[bot]@users.noreply.github.com>
Co-authored-by: Ewan <ewanjordaan07@gmail.com>
Co-authored-by: Julius Marminge <julius0216@outlook.com>
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: Julius Marminge <julius0216@outlook.com>
Co-authored-by: Julius Marminge <julius0216@outlook.com>
Integrates upstream changes from 7f3387a..59f23d7 including: - Effect.fn annotations for Git core, orchestration reactors, and DrainableWorker - Provider model lists now dynamic based on auth context - Thread archiving, hidden thread status, and sidebar thread features - UI fixes: composer menu scroll/highlight, changed-files scroll preservation, debounced thread jump hints, desktop confirm dialog defaults - Inline sqlite error classification, projection pipeline side effect refactors - Fix model settings getting stuck, broken Codex model selection Conflict resolution strategy: accept upstream's structural changes (Effect.fn wrappers, STM-based DrainableWorker, auth context refactor) while preserving fork's multi-provider extensions across all 14 conflicting files.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors many Effect generators into named Changes
Sequence Diagram(s)sequenceDiagram
participant TestHarness as Test Harness
participant Reactor as OrchestrationReactor
participant EffectRuntime as Effect Runtime
participant ScopeMgr as Scope Manager
TestHarness->>Reactor: invoke reactor.start()
Reactor->>EffectRuntime: produce Effect<void, never, Scope>
TestHarness->>EffectRuntime: pipe Scope.provide(scope)
EffectRuntime->>ScopeMgr: acquire scope & fork scoped streams
ScopeMgr-->>TestHarness: scoped reactor running
sequenceDiagram
participant Caller as Server/Caller
participant Provider as Provider Layer (Codex/Claude)
participant Probe as CLI/SDK Probe
participant AccountMod as CodexAccount Module
Caller->>Provider: checkProviderStatus(resolveAccount?)
Provider->>Probe: run CLI/SDK probe
Probe->>AccountMod: parse account/auth output
AccountMod->>Provider: return CodexAccountSnapshot
Provider->>Provider: adjustModelsForAccount/Subscription()
Provider->>Caller: return probe + adjusted models
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
vouch:trusted
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/server/src/git/Layers/CodexTextGeneration.ts (1)
160-166:⚠️ Potential issue | 🟠 MajorUse normalized reasoning effort in the Codex command config.
The code computes capability-aware options, but
reasoningEffortis still taken from raw input. That bypasses validation/defaulting and can send invalid effort values to CLI.🔧 Proposed fix
const normalizedOptions = normalizeCodexModelOptionsWithCapabilities( getCodexModelCapabilities(modelSelection.model), modelSelection.options, ); const reasoningEffort = - modelSelection.options?.reasoningEffort ?? CODEX_GIT_TEXT_GENERATION_REASONING_EFFORT; + normalizedOptions?.reasoningEffort ?? CODEX_GIT_TEXT_GENERATION_REASONING_EFFORT;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/git/Layers/CodexTextGeneration.ts` around lines 160 - 166, The reasoningEffort is being read from raw modelSelection.options instead of the capability-normalized object; change the code to derive reasoningEffort from normalizedOptions (the result of normalizeCodexModelOptionsWithCapabilities) and fall back to CODEX_GIT_TEXT_GENERATION_REASONING_EFFORT if missing so the value sent to ChildProcess.make / the Codex command config is validated/defaulted. Locate the normalizeCodexModelOptionsWithCapabilities/getCodexModelCapabilities call and replace uses of modelSelection.options?.reasoningEffort with normalizedOptions.reasoningEffort (or the fallback constant) before building the ChildProcess.make command config.apps/server/src/git/Layers/ClaudeTextGeneration.ts (1)
88-115:⚠️ Potential issue | 🟠 MajorUse
normalizedOptionswhen resolving the Claude API model id.
normalizeClaudeModelOptionsWithCapabilities(...)resolvescontextWindow, butresolveApiModelIdstill reads rawmodelSelection.options. This can silently drop the normalized context-window mapping.🔧 Proposed fix
const normalizedOptions = normalizeClaudeModelOptionsWithCapabilities( getClaudeModelCapabilities(modelSelection.model), modelSelection.options, ); + const normalizedSelection: ClaudeModelSelection = { + ...modelSelection, + options: normalizedOptions, + }; const settings = { ...(typeof normalizedOptions?.thinking === "boolean" ? { alwaysThinkingEnabled: normalizedOptions.thinking } : {}), ...(normalizedOptions?.fastMode ? { fastMode: true } : {}), @@ "--json-schema", jsonSchemaStr, "--model", - resolveApiModelId(modelSelection), + resolveApiModelId(normalizedSelection), ...(normalizedOptions?.effort ? ["--effort", normalizedOptions.effort] : []), ...(Object.keys(settings).length > 0 ? ["--settings", JSON.stringify(settings)] : []),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/git/Layers/ClaudeTextGeneration.ts` around lines 88 - 115, The code normalizes Claude options into normalizedOptions but still calls resolveApiModelId with the original modelSelection, which can ignore normalized contextWindow; update the call to resolveApiModelId to pass the normalized options (e.g., resolveApiModelId({ ...modelSelection, options: normalizedOptions })) so the resolved API model id uses the normalized contextWindow and other adjusted fields; edit the call site in the runClaudeCommand generator where resolveApiModelId(modelSelection) is used and ensure any downstream uses expect the updated selection shape.apps/web/src/components/chat/ProviderModelPicker.browser.tsx (1)
126-155:⚠️ Potential issue | 🟠 Major
providersnever influences the rendered model list.
mountPicker()acceptsproviders, but the helper still buildsmodelOptionsByProviderfrom hard-coded empty custom-model arrays and never passesprops.providersanywhere. The new Spark assertions later in this file therefore do not actually vary account-scoped model availability, so they can pass or fail for the wrong reason.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/chat/ProviderModelPicker.browser.tsx` around lines 126 - 155, mountPicker currently ignores its providers argument and always calls buildModelOptionsByProvider with hard-coded empty custom model arrays, so the rendered model list never reflects the test's providers; update mountPicker to incorporate props.providers into the modelOptionsByProvider creation (either pass props.providers into buildModelOptionsByProvider if it supports it, or merge props.providers into the custom* model arrays before calling buildModelOptionsByProvider) and ensure the resulting modelOptionsByProvider is passed into the ProviderModelPicker component so assertions about provider-scoped models (in tests referencing ProviderModelPicker and modelOptionsByProvider) reflect the supplied providers.apps/server/src/orchestration/Layers/OrchestrationReactor.test.ts (1)
57-65:⚠️ Potential issue | 🟡 MinorClose the manual scope in a
finally.If startup or the expectation fails, the explicit scope never closes and its fibers outlive this test.
runtime.dispose()does not clean up the separate scope created here.Proposed fix
const reactor = await runtime.runPromise(Effect.service(OrchestrationReactor)); const scope = await Effect.runPromise(Scope.make("sequential")); - await Effect.runPromise(reactor.start().pipe(Scope.provide(scope))); - - expect(started).toEqual([ - "provider-runtime-ingestion", - "provider-command-reactor", - "checkpoint-reactor", - ]); - - await Effect.runPromise(Scope.close(scope, Exit.void)); + try { + await Effect.runPromise(reactor.start().pipe(Scope.provide(scope))); + + expect(started).toEqual([ + "provider-runtime-ingestion", + "provider-command-reactor", + "checkpoint-reactor", + ]); + } finally { + await Effect.runPromise(Scope.close(scope, Exit.void)); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/orchestration/Layers/OrchestrationReactor.test.ts` around lines 57 - 65, The test currently starts the reactor with a manual scope and closes it only after assertions, which can leak fibers if startup or the expect throws; wrap the shutdown call in a finally block so Scope.close(scope, Exit.void) is always run (i.e., call Effect.runPromise(Scope.close(scope, Exit.void)) from a finally after Effect.runPromise(reactor.start().pipe(Scope.provide(scope))) and the expect), ensuring the created scope is reliably closed even on failures.
🧹 Nitpick comments (4)
packages/shared/src/DrainableWorker.ts (1)
61-65: Return type mismatch between interface and implementation.The interface at line 21 declares
enqueueas returningEffect.Effect<void>, but this implementation explicitly annotates the return type asEffect.Effect<boolean, never, never>(fromTxQueue.offer). While TypeScript accepts this assignment, the explicit annotation creates a discrepancy. Consider discarding the boolean to align with the interface contract.♻️ Proposed fix to align return type
- const enqueue = (element: A): Effect.Effect<boolean, never, never> => + const enqueue = (element: A): Effect.Effect<void, never, never> => TxQueue.offer(queue, element).pipe( Effect.tap(() => TxRef.update(outstanding, (n) => n + 1)), Effect.tx, + Effect.asVoid, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/DrainableWorker.ts` around lines 61 - 65, The method enqueue currently declares a return type of Effect.Effect<boolean, never, never> because it returns TxQueue.offer; change the implementation to match the interface by discarding the boolean result and returning an Effect of void: pipe the TxQueue.offer(queue, element) through a map/asUnit to convert the boolean to void before calling Effect.tap(…) and Effect.tx so enqueue's observable type aligns with the interface; update references to TxQueue.offer, enqueue, TxRef.update(outstanding, …) and queue accordingly.docs/effect-fn-checklist.md (1)
71-99: Use repository-relative links instead of local filesystem paths.These
/Users/julius/...links won’t resolve for collaborators or on GitHub. Please switch them to repo-relative links so the checklist is navigable by everyone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/effect-fn-checklist.md` around lines 71 - 99, The checklist uses absolute local paths (e.g., links pointing to /Users/julius/Development/Work/codething-mvp/...) which won’t resolve for others; update each link in docs/effect-fn-checklist.md to repo-relative paths by removing the local prefix so they point to files under apps/server/... and keep the symbol anchors (for example change links for makeGitCore, handleTraceLine, emitCompleteLines, commit, pushCurrentBranch, pullCurrentBranch, checkoutBranch in GitCore.ts and configurePullRequestHeadUpstream, materializePullRequestHeadBranch, findOpenPr, findLatestPr, runCommitStep, runPrStep, runFeatureBranchStep in GitManager.ts and runProjectorForEvent, applyProjectsProjection, applyThreadsProjection in ProjectionPipeline.ts) so the anchors like `#L513` remain but the path is repo-relative.apps/server/src/provider/codexAppServer.ts (1)
86-90: Minor: AbortSignal listener is never removed.The abort listener added at Line 90 is not removed when the probe completes or fails normally. If the
AbortSignalis long-lived (e.g., reused across multiple operations), this could lead to a minor memory leak with accumulated stale listeners.♻️ Suggested fix to clean up the abort listener
+ const abortHandler = () => fail(new Error("Codex account probe aborted.")); + if (input.signal?.aborted) { - fail(new Error("Codex account probe aborted.")); + fail(new Error("Codex account probe aborted.")); return; } - input.signal?.addEventListener("abort", () => fail(new Error("Codex account probe aborted."))); + input.signal?.addEventListener("abort", abortHandler); const cleanup = () => { output.removeAllListeners(); output.close(); child.removeAllListeners(); + input.signal?.removeEventListener("abort", abortHandler); if (!child.killed) { killCodexChildProcess(child); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/codexAppServer.ts` around lines 86 - 90, The abort listener added to input.signal (via input.signal?.addEventListener("abort", ...)) isn't removed on normal completion or failure; create a named handler (e.g., const onAbort = () => fail(new Error("Codex account probe aborted."))) and register it with addEventListener, then call input.signal?.removeEventListener("abort", onAbort) whenever the probe finishes or fails (including after the early input.signal?.aborted check and in any finally/cleanup path) so the listener is always cleaned up to avoid leaking handlers.apps/server/src/provider/codexAccount.ts (1)
48-54: Potential type safety gap:planTypecast without validation.The
planTypefromaccount?.planTypeis cast directly toCodexPlanType | nullwithout validating that it actually matches one of the allowed values. If the API returns an unexpected string (e.g.,"premium"), it will be stored as-is and pass through to consumers, potentially causing issues downstream.Consider validating the value against the known plan types:
♻️ Suggested validation approach
+const CODEX_KNOWN_PLAN_TYPES = new Set<CodexPlanType>([ + "free", "go", "plus", "pro", "team", "business", "enterprise", "edu", "unknown" +]); + +function toCodexPlanType(value: unknown): CodexPlanType { + return typeof value === "string" && CODEX_KNOWN_PLAN_TYPES.has(value as CodexPlanType) + ? (value as CodexPlanType) + : "unknown"; +} if (accountType === "chatgpt") { - const planType = (account?.planType as CodexPlanType | null) ?? "unknown"; + const planType = toCodexPlanType(account?.planType); return { type: "chatgpt", planType, sparkEnabled: CODEX_SPARK_ENABLED_PLAN_TYPES.has(planType), }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/codexAccount.ts` around lines 48 - 54, The code casts account?.planType to CodexPlanType without validation (in the block where accountType === "chatgpt"), which can allow unexpected strings through; update the logic that computes planType to check account?.planType against the set of allowed CodexPlanType values (or against CODEX_SPARK_ENABLED_PLAN_TYPES keys) and only accept it if it matches, otherwise fall back to "unknown" (so consumers of planType, CODEX_SPARK_ENABLED_PLAN_TYPES.has(planType), and related code see a safe, validated 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/git/Layers/GitManager.ts`:
- Around line 1081-1118: findLocalHeadBranch may return a branch object that
comes from pullRequest.headBranch while the code always returns
localPullRequestBranch as the branch name, causing metadata to point to a
non-existent branch in the reused worktree; change the return to use the actual
branch backing the reused worktree (existingBranchBeforeFetch.name) when
existingBranchBeforeFetch is non-null and its name differs from
localPullRequestBranch (keep using localPullRequestBranch only when no
existingBranchBeforeFetch or when it's the same), and apply the same fix in the
later identical block (lines referenced around existingBranchBeforeFetch /
ensureExistingWorktreeUpstream) so branch and worktreePath consistently reflect
the real branch object returned by findLocalHeadBranch.
- Around line 699-723: In resolveBaseBranch, replace the call to
gitCore.readConfigValue(cwd, `branch.${branch}.gh-merge-base`) with the nullable
helper gitCore.readConfigValueNullable so the optional config key is treated as
absent rather than failing; keep the subsequent logic (checking
upstreamRef/headContext, calling gitHubCli.getDefaultBranch, and returning
"main") unchanged and ensure the variable name remains `configured`.
In `@apps/server/src/orchestration/Layers/OrchestrationReactor.test.ts`:
- Around line 27-30: The test stubs are performing side-effects
(started.push(...)) before returning the Effect, so the test would pass even if
makeOrchestrationReactor ignores the returned Effect; change each stubbed
start/stop lambda to perform the push inside the returned Effect (e.g., return
Effect.sync(() => started.push("provider-runtime-ingestion"))) so the push only
runs when the Effect is executed; update the same pattern for the other
occurrences (the start/stop lambdas referenced in this test and the ones at the
other noted locations).
In `@apps/server/src/provider/Layers/ClaudeProvider.ts`:
- Around line 549-629: The code currently calls resolveSubscriptionType(...)
even when the auth probe timed out or returned an unauthenticated result,
allowing stale cached SDK metadata to be used; change the logic so
resolveSubscriptionType is only invoked when the auth probe succeeded and
indicates an authenticated status. Concretely: after Result.isSuccess(authProbe)
&& Option.isSome(authProbe.success) parse the probe with
parseClaudeAuthStatusFromOutput(authProbe.success.value) and only call
resolveSubscriptionType(claudeSettings.binaryPath) if that parsed result shows
an authenticated status (e.g., parsed.status !== 'unauthenticated' or
parsed.auth.status === 'authenticated'); keep
extractSubscriptionTypeFromOutput(...) and
extractClaudeAuthMethodFromOutput(...) as before and then compute resolvedModels
= adjustModelsForSubscription(models, subscriptionType).
In `@apps/server/src/provider/Layers/CodexProvider.ts`:
- Around line 467-535: The code currently calls resolveAccount() and
adjustCodexModelsForAccount(...) before handling the result of
runCodexCommand(["login","status"]), causing stale account info to be used for
timeout/error branches; change the flow so you only call resolveAccount(...) and
then adjustCodexModelsForAccount(models, account) after confirming authProbe is
a success (Option.isSome) and parsing the auth with
parseAuthStatusFromOutput(authProbe.success.value); keep
parseAuthStatusFromOutput, codexAuthSubType, codexAuthSubLabel and the final
buildServerProvider usage but in the timeout/failure branches return providers
that do not use the previously resolved account or adjusted models (use the
original models variable instead) so account lookup only affects the
authenticated branch.
In `@apps/web/src/components/chat/MessagesTimeline.tsx`:
- Around line 269-278: The current override for
rowVirtualizer.shouldAdjustScrollPositionOnItemSizeChange allows adjustments for
any non-visible item; change it so only items fully above the viewport can
trigger adjustment. Specifically, in the
shouldAdjustScrollPositionOnItemSizeChange handler
(rowVirtualizer.shouldAdjustScrollPositionOnItemSizeChange), keep the early
return false for itemIntersectsViewport, then add a check to return false if the
item is below the viewport (e.g., item.start >= scrollOffset + viewportHeight),
and only for items fully above the viewport (e.g., item.end <= scrollOffset)
compute remainingDistance = instance.getTotalSize() - (scrollOffset +
viewportHeight) and return remainingDistance > AUTO_SCROLL_BOTTOM_THRESHOLD_PX.
This prevents size changes for below-viewport rows from shifting the current
view.
In `@apps/web/src/components/Sidebar.tsx`:
- Around line 1865-1869: threadMetaClassName currently forces
"pointer-events-none" on the metadata wrapper, preventing the TooltipTrigger
around ProviderLogo from receiving hover/focus; update the class logic so the
wrapper no longer disables pointer events for the ProviderLogo trigger (for
example: remove "pointer-events-none" from threadMetaClassName or conditionally
add a class like "pointer-events-auto" to the TooltipTrigger/ProviderLogo
element), locate usages of threadMetaClassName and the ProviderLogo
TooltipTrigger and ensure only non-interactive parts remain pointer-events-none
while the TooltipTrigger element itself can receive pointer events.
- Around line 1638-1705: The bug is that projectThreads is built from
visibleThreads (excluding archived threads) which makes a project appear empty
while archived threads still block deletion; fix by deriving two lists:
allProjectThreads = threads.filter(t => t.projectId === project.id) and
projectThreadsVisible = visibleThreads.filter(t => t.projectId === project.id),
then use projectThreadsVisible for rendering/sorting (pass to
sortThreadsForSidebar and to getVisibleThreadsForProject) but use
allProjectThreads for emptiness/deletion checks and for
orderedProjectThreadIds/hiddenThreadStatus computation that must consider
archived threads; update variable names (projectThreads -> projectThreadsVisible
or projectAllThreads) inside renderedProjects so showEmptyThreadState and
orderedProjectThreadIds use allProjectThreads while renderedThreads/use of
pinnedCollapsedThread continue to use projectThreadsVisible.
In `@docs/effect-fn-checklist.md`:
- Around line 195-197: Remove the trailing empty fenced code block that starts
at the end of docs/effect-fn-checklist.md (the untyped ``` fence around line
196) which triggers markdownlint MD040; replace it with the intended checklist
line (e.g., the checklist entry referencing
apps/server/src/provider/makeManagedServerProvider.ts) or delete the empty fence
entirely so the document ends without an empty code block.
In `@packages/shared/src/DrainableWorker.ts`:
- Line 36: Update the JSDoc for DrainableWorker to correctly describe the
returned properties: change the `@returns` text to reference "enqueue and drain"
instead of "queue and drain" so it matches the actual returned object (symbols:
DrainableWorker, enqueue, drain).
---
Outside diff comments:
In `@apps/server/src/git/Layers/ClaudeTextGeneration.ts`:
- Around line 88-115: The code normalizes Claude options into normalizedOptions
but still calls resolveApiModelId with the original modelSelection, which can
ignore normalized contextWindow; update the call to resolveApiModelId to pass
the normalized options (e.g., resolveApiModelId({ ...modelSelection, options:
normalizedOptions })) so the resolved API model id uses the normalized
contextWindow and other adjusted fields; edit the call site in the
runClaudeCommand generator where resolveApiModelId(modelSelection) is used and
ensure any downstream uses expect the updated selection shape.
In `@apps/server/src/git/Layers/CodexTextGeneration.ts`:
- Around line 160-166: The reasoningEffort is being read from raw
modelSelection.options instead of the capability-normalized object; change the
code to derive reasoningEffort from normalizedOptions (the result of
normalizeCodexModelOptionsWithCapabilities) and fall back to
CODEX_GIT_TEXT_GENERATION_REASONING_EFFORT if missing so the value sent to
ChildProcess.make / the Codex command config is validated/defaulted. Locate the
normalizeCodexModelOptionsWithCapabilities/getCodexModelCapabilities call and
replace uses of modelSelection.options?.reasoningEffort with
normalizedOptions.reasoningEffort (or the fallback constant) before building the
ChildProcess.make command config.
In `@apps/server/src/orchestration/Layers/OrchestrationReactor.test.ts`:
- Around line 57-65: The test currently starts the reactor with a manual scope
and closes it only after assertions, which can leak fibers if startup or the
expect throws; wrap the shutdown call in a finally block so Scope.close(scope,
Exit.void) is always run (i.e., call Effect.runPromise(Scope.close(scope,
Exit.void)) from a finally after
Effect.runPromise(reactor.start().pipe(Scope.provide(scope))) and the expect),
ensuring the created scope is reliably closed even on failures.
In `@apps/web/src/components/chat/ProviderModelPicker.browser.tsx`:
- Around line 126-155: mountPicker currently ignores its providers argument and
always calls buildModelOptionsByProvider with hard-coded empty custom model
arrays, so the rendered model list never reflects the test's providers; update
mountPicker to incorporate props.providers into the modelOptionsByProvider
creation (either pass props.providers into buildModelOptionsByProvider if it
supports it, or merge props.providers into the custom* model arrays before
calling buildModelOptionsByProvider) and ensure the resulting
modelOptionsByProvider is passed into the ProviderModelPicker component so
assertions about provider-scoped models (in tests referencing
ProviderModelPicker and modelOptionsByProvider) reflect the supplied providers.
---
Nitpick comments:
In `@apps/server/src/provider/codexAccount.ts`:
- Around line 48-54: The code casts account?.planType to CodexPlanType without
validation (in the block where accountType === "chatgpt"), which can allow
unexpected strings through; update the logic that computes planType to check
account?.planType against the set of allowed CodexPlanType values (or against
CODEX_SPARK_ENABLED_PLAN_TYPES keys) and only accept it if it matches, otherwise
fall back to "unknown" (so consumers of planType,
CODEX_SPARK_ENABLED_PLAN_TYPES.has(planType), and related code see a safe,
validated value).
In `@apps/server/src/provider/codexAppServer.ts`:
- Around line 86-90: The abort listener added to input.signal (via
input.signal?.addEventListener("abort", ...)) isn't removed on normal completion
or failure; create a named handler (e.g., const onAbort = () => fail(new
Error("Codex account probe aborted."))) and register it with addEventListener,
then call input.signal?.removeEventListener("abort", onAbort) whenever the probe
finishes or fails (including after the early input.signal?.aborted check and in
any finally/cleanup path) so the listener is always cleaned up to avoid leaking
handlers.
In `@docs/effect-fn-checklist.md`:
- Around line 71-99: The checklist uses absolute local paths (e.g., links
pointing to /Users/julius/Development/Work/codething-mvp/...) which won’t
resolve for others; update each link in docs/effect-fn-checklist.md to
repo-relative paths by removing the local prefix so they point to files under
apps/server/... and keep the symbol anchors (for example change links for
makeGitCore, handleTraceLine, emitCompleteLines, commit, pushCurrentBranch,
pullCurrentBranch, checkoutBranch in GitCore.ts and
configurePullRequestHeadUpstream, materializePullRequestHeadBranch, findOpenPr,
findLatestPr, runCommitStep, runPrStep, runFeatureBranchStep in GitManager.ts
and runProjectorForEvent, applyProjectsProjection, applyThreadsProjection in
ProjectionPipeline.ts) so the anchors like `#L513` remain but the path is
repo-relative.
In `@packages/shared/src/DrainableWorker.ts`:
- Around line 61-65: The method enqueue currently declares a return type of
Effect.Effect<boolean, never, never> because it returns TxQueue.offer; change
the implementation to match the interface by discarding the boolean result and
returning an Effect of void: pipe the TxQueue.offer(queue, element) through a
map/asUnit to convert the boolean to void before calling Effect.tap(…) and
Effect.tx so enqueue's observable type aligns with the interface; update
references to TxQueue.offer, enqueue, TxRef.update(outstanding, …) and queue
accordingly.
🪄 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: 8587f395-462c-4510-b4b6-5d580d74eb96
📒 Files selected for processing (59)
apps/desktop/src/confirmDialog.tsapps/server/integration/OrchestrationEngineHarness.integration.tsapps/server/src/codexAppServerManager.test.tsapps/server/src/codexAppServerManager.tsapps/server/src/git/Layers/ClaudeTextGeneration.tsapps/server/src/git/Layers/CodexTextGeneration.tsapps/server/src/git/Layers/GitCore.tsapps/server/src/git/Layers/GitManager.test.tsapps/server/src/git/Layers/GitManager.tsapps/server/src/orchestration/Layers/CheckpointReactor.test.tsapps/server/src/orchestration/Layers/CheckpointReactor.tsapps/server/src/orchestration/Layers/OrchestrationReactor.test.tsapps/server/src/orchestration/Layers/OrchestrationReactor.tsapps/server/src/orchestration/Layers/ProjectionPipeline.tsapps/server/src/orchestration/Layers/ProviderCommandReactor.test.tsapps/server/src/orchestration/Layers/ProviderCommandReactor.tsapps/server/src/orchestration/Layers/ProviderRuntimeIngestion.test.tsapps/server/src/orchestration/Layers/ProviderRuntimeIngestion.tsapps/server/src/orchestration/Services/CheckpointReactor.tsapps/server/src/orchestration/Services/OrchestrationReactor.tsapps/server/src/orchestration/Services/ProviderCommandReactor.tsapps/server/src/orchestration/Services/ProviderRuntimeIngestion.tsapps/server/src/persistence/NodeSqliteClient.tsapps/server/src/provider/Layers/ClaudeProvider.tsapps/server/src/provider/Layers/CodexProvider.tsapps/server/src/provider/Layers/ProviderRegistry.test.tsapps/server/src/provider/Layers/ProviderRegistry.tsapps/server/src/provider/codexAccount.tsapps/server/src/provider/codexAppServer.tsapps/server/src/provider/providerSnapshot.tsapps/server/src/serverSettings.test.tsapps/server/src/serverSettings.tsapps/server/src/wsServer.test.tsapps/server/src/wsServer.tsapps/web/src/components/BranchToolbarBranchSelector.tsxapps/web/src/components/ChatView.browser.tsxapps/web/src/components/KeybindingsToast.browser.tsxapps/web/src/components/Sidebar.logic.test.tsapps/web/src/components/Sidebar.logic.tsapps/web/src/components/Sidebar.tsxapps/web/src/components/chat/ChangedFilesTree.tsxapps/web/src/components/chat/ComposerCommandMenu.tsxapps/web/src/components/chat/MessagesTimeline.tsxapps/web/src/components/chat/ProviderModelPicker.browser.tsxapps/web/src/components/chat/TraitsPicker.browser.tsxapps/web/src/components/chat/composerProviderRegistry.test.tsxapps/web/src/components/chat/composerProviderRegistry.tsxapps/web/src/components/settings/SettingsPanels.tsxapps/web/src/components/ui/autocomplete.tsxapps/web/src/components/ui/combobox.tsxapps/web/src/providerModels.tsapps/web/src/wsNativeApi.test.tsdocs/effect-fn-checklist.mdpackages/contracts/src/server.tspackages/contracts/src/settings.test.tspackages/contracts/src/settings.tspackages/shared/src/DrainableWorker.tspackages/shared/src/model.test.tspackages/shared/src/model.ts
💤 Files with no reviewable changes (2)
- apps/web/src/components/BranchToolbarBranchSelector.tsx
- packages/contracts/src/settings.test.ts
apps/server/src/orchestration/Layers/OrchestrationReactor.test.ts
Outdated
Show resolved
Hide resolved
Comment on lines
+1638
to
+1705
| const visibleThreads = useMemo( | ||
| () => threads.filter((thread) => thread.archivedAt === null), | ||
| [threads], | ||
| ); | ||
| const sortedProjects = useMemo( | ||
| () => sortProjectsForSidebar(projects, threads, appSettings.sidebarProjectSortOrder), | ||
| [appSettings.sidebarProjectSortOrder, projects, threads], | ||
| () => sortProjectsForSidebar(projects, visibleThreads, appSettings.sidebarProjectSortOrder), | ||
| [appSettings.sidebarProjectSortOrder, projects, visibleThreads], | ||
| ); | ||
| const isManualProjectSorting = appSettings.sidebarProjectSortOrder === "manual"; | ||
| const renderedProjects = useMemo( | ||
| () => | ||
| sortedProjects.map((project) => { | ||
| const projectThreads = sortThreadsForSidebar( | ||
| visibleThreads.filter((thread) => thread.projectId === project.id), | ||
| appSettings.sidebarThreadSortOrder, | ||
| ); | ||
| const threadStatuses = new Map( | ||
| projectThreads.map((thread) => [ | ||
| thread.id, | ||
| resolveThreadStatusPill({ | ||
| thread, | ||
| hasPendingApprovals: derivePendingApprovals(thread.activities).length > 0, | ||
| hasPendingUserInput: derivePendingUserInputs(thread.activities).length > 0, | ||
| }), | ||
| ]), | ||
| ); | ||
| const projectStatus = resolveProjectStatusIndicator( | ||
| projectThreads.map((thread) => threadStatuses.get(thread.id) ?? null), | ||
| ); | ||
| const activeThreadId = routeThreadId ?? undefined; | ||
| const isThreadListExpanded = expandedThreadListsByProject.has(project.id); | ||
| const pinnedCollapsedThread = | ||
| !project.expanded && activeThreadId | ||
| ? (projectThreads.find((thread) => thread.id === activeThreadId) ?? null) | ||
| : null; | ||
| const shouldShowThreadPanel = project.expanded || pinnedCollapsedThread !== null; | ||
| const { | ||
| hasHiddenThreads, | ||
| hiddenThreads, | ||
| visibleThreads: visibleProjectThreads, | ||
| } = getVisibleThreadsForProject({ | ||
| threads: projectThreads, | ||
| activeThreadId, | ||
| isThreadListExpanded, | ||
| previewLimit: THREAD_PREVIEW_LIMIT, | ||
| }); | ||
| const hiddenThreadStatus = resolveProjectStatusIndicator( | ||
| hiddenThreads.map((thread) => threadStatuses.get(thread.id) ?? null), | ||
| ); | ||
| const orderedProjectThreadIds = projectThreads.map((thread) => thread.id); | ||
| const renderedThreads = pinnedCollapsedThread | ||
| ? [pinnedCollapsedThread] | ||
| : visibleProjectThreads; | ||
| const showEmptyThreadState = project.expanded && projectThreads.length === 0; | ||
|
|
||
| return { | ||
| hasHiddenThreads, | ||
| hiddenThreadStatus, | ||
| orderedProjectThreadIds, | ||
| project, | ||
| projectStatus, | ||
| projectThreads, | ||
| threadStatuses, | ||
| renderedThreads, | ||
| showEmptyThreadState, | ||
| shouldShowThreadPanel, | ||
| isThreadListExpanded, | ||
| }; |
There was a problem hiding this comment.
Archived-only projects now look empty but still behave non-empty.
projectThreads is derived from visibleThreads, so a project whose threads are all archived now renders showEmptyThreadState = true and exposes no rows at all. Later in this same file, project removal still checks the full threads list, so the user can end up with “No threads yet” while the project remains undeletable because hidden archived threads still exist.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/Sidebar.tsx` around lines 1638 - 1705, The bug is
that projectThreads is built from visibleThreads (excluding archived threads)
which makes a project appear empty while archived threads still block deletion;
fix by deriving two lists: allProjectThreads = threads.filter(t => t.projectId
=== project.id) and projectThreadsVisible = visibleThreads.filter(t =>
t.projectId === project.id), then use projectThreadsVisible for
rendering/sorting (pass to sortThreadsForSidebar and to
getVisibleThreadsForProject) but use allProjectThreads for emptiness/deletion
checks and for orderedProjectThreadIds/hiddenThreadStatus computation that must
consider archived threads; update variable names (projectThreads ->
projectThreadsVisible or projectAllThreads) inside renderedProjects so
showEmptyThreadState and orderedProjectThreadIds use allProjectThreads while
renderedThreads/use of pinnedCollapsedThread continue to use
projectThreadsVisible.
- Skip 2 browser tests incompatible with fork's static model picker: "codex spark" (tests upstream's dynamic model lists) and "dispatches canonical slug" (expects menuitemradio, fork uses sub-menus) - Use readConfigValueNullable for optional gh-merge-base git config - Return actual branch name from reused worktree (not localPullRequestBranch) - Fix OrchestrationReactor test stubs to run inside returned Effect - Gate ClaudeProvider subscription type resolution on successful auth - Gate CodexProvider account probe on successful auth - Fix Sidebar: distinguish "no threads" from "all archived", restore pointer-events on provider tooltip trigger - Fix MessagesTimeline scroll adjustment to only apply for items above viewport - Fix DrainableWorker JSDoc: "queue" -> "enqueue" - Remove trailing empty fenced code blocks in effect-fn-checklist.md
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/shared/src/DrainableWorker.ts (1)
61-65: Return type annotation contradicts interface contract.The explicit
Effect.Effect<boolean, never, never>annotation conflicts with the interface declaration at line 21 which specifiesEffect.Effect<void>. While TypeScript'svoidhandling allows this to type-check, the explicit boolean annotation creates confusion about intent.Additionally, the relevant code snippet from
CheckpointReactor.tsshows callers mixingEffect.voidreturns withworker.enqueue(...)returns in the same callback, which relies on type compatibility.For an unbounded queue,
TxQueue.offeralways succeeds, so the boolean return value provides no useful information here.♻️ Proposed fix: align implementation with interface
- const enqueue = (element: A): Effect.Effect<boolean, never, never> => + const enqueue = (element: A): Effect.Effect<void, never, never> => TxQueue.offer(queue, element).pipe( Effect.tap(() => TxRef.update(outstanding, (n) => n + 1)), Effect.tx, + Effect.asVoid, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/DrainableWorker.ts` around lines 61 - 65, The enqueue function's explicit return type Effect.Effect<boolean, never, never> conflicts with the declared interface type (Effect.Effect<void>); change the signature to return Effect.Effect<void> (or the project's void alias) and convert the boolean result from TxQueue.offer(queue, element) into void before returning (e.g., map/asUnit) so callers get a void Effect; update the implementation around enqueue, TxQueue.offer, TxRef.update, outstanding and queue to ensure the pipeline still updates outstanding and runs Effect.tx while yielding a void result.apps/web/src/components/Sidebar.tsx (2)
866-866: Unused destructured variabledeleteThreadAction.
deleteThreadActionis destructured fromuseThreadActions()but never used in this file. The localdeleteThreadcallback (line 1193) is used instead for its worktree removal logic. Consider removing the unused destructuring to reduce confusion.- const { archiveThread, deleteThread: deleteThreadAction } = useThreadActions(); + const { archiveThread } = useThreadActions();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/Sidebar.tsx` at line 866, Remove the unused destructured variable to avoid confusion: delete the `deleteThreadAction` destructuring from the call to `useThreadActions()` in Sidebar (where `const { archiveThread, deleteThread: deleteThreadAction } = useThreadActions();` is declared) since the local `deleteThread` callback is used for worktree removal logic; leave `archiveThread` intact and ensure no other references to `deleteThreadAction` remain in the file.
2048-2055: Potential race condition withrequestAnimationFramefocus.The
requestAnimationFrameis scheduled immediately aftersetConfirmingArchiveThreadId, but React's state update is asynchronous. The confirm button might not be rendered yet when the callback fires, causing the focus to silently fail.A more robust approach would be to use
useEffectto focus whenconfirmingArchiveThreadIdchanges:useEffect(() => { if (confirmingArchiveThreadId) { confirmArchiveButtonRefs.current.get(confirmingArchiveThreadId)?.focus(); } }, [confirmingArchiveThreadId]);Then simplify the onClick:
onClick={(event) => { event.preventDefault(); event.stopPropagation(); setConfirmingArchiveThreadId(thread.id); - requestAnimationFrame(() => { - confirmArchiveButtonRefs.current.get(thread.id)?.focus(); - }); }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/Sidebar.tsx` around lines 2048 - 2055, The onClick currently calls setConfirmingArchiveThreadId(...) then schedules focus with requestAnimationFrame which can race with React's render; replace that pattern by moving the focus logic into a useEffect that watches confirmingArchiveThreadId and, when non-null, calls confirmArchiveButtonRefs.current.get(confirmingArchiveThreadId)?.focus(); update the onClick (the handler that calls setConfirmingArchiveThreadId) to just set the id and remove the requestAnimationFrame focus call so focus happens reliably from the useEffect.
🤖 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/git/Layers/GitManager.ts`:
- Around line 526-543: The nullable helper readConfigValueNullable currently
converts any gitCore.readConfigValue failure to null; change it so it only
suppresses the "config key not found" case and rethrows all other errors.
Specifically, update readConfigValueNullable to call gitCore.readConfigValue,
catch errors, detect the missing-key/NotFound error (by error code/type/message
used by gitCore.readConfigValue), return null only for that case, and rethrow
for malformed config, permission issues, etc.; then callers such as
resolveRemoteRepositoryContext should keep using readConfigValueNullable
unchanged but will now see real errors propagated from gitCore.readConfigValue.
Include the same fix for the other call sites mentioned (the other
readConfigValueNullable usages).
- Around line 1195-1199: The branch name used to create the Git branch may come
directly from suggestion.branch and can contain invalid ref characters; sanitize
the model-provided branch before using it. Change the logic so preferredBranch
is built by applying sanitizeFeatureBranchName to suggestion.branch (not just
the subject), then pass that sanitized preferredBranch into
resolveAutoFeatureBranchName, and finally ensure the resolvedBranch is also
sanitized before calling gitCore.createBranch({ cwd, branch: resolvedBranch });
reference: suggestion.branch, sanitizeFeatureBranchName,
resolveAutoFeatureBranchName, gitCore.createBranch.
- Around line 969-986: The code calls findOpenPr(cwd, headContext.headSelectors)
after creating a PR and currently lets any error there bubble up; change that to
catch errors from the second findOpenPr call and, on failure, return the same
partial-success payload as the !created branch (status: "created", baseBranch,
headBranch: headContext.headBranch, title: generated.title) instead of throwing;
implement this by wrapping the second findOpenPr invocation and the block that
uses created.url/number in a try/catch and returning the fallback object on
catch so a successful createPullRequest is not treated as a failure.
---
Nitpick comments:
In `@apps/web/src/components/Sidebar.tsx`:
- Line 866: Remove the unused destructured variable to avoid confusion: delete
the `deleteThreadAction` destructuring from the call to `useThreadActions()` in
Sidebar (where `const { archiveThread, deleteThread: deleteThreadAction } =
useThreadActions();` is declared) since the local `deleteThread` callback is
used for worktree removal logic; leave `archiveThread` intact and ensure no
other references to `deleteThreadAction` remain in the file.
- Around line 2048-2055: The onClick currently calls
setConfirmingArchiveThreadId(...) then schedules focus with
requestAnimationFrame which can race with React's render; replace that pattern
by moving the focus logic into a useEffect that watches
confirmingArchiveThreadId and, when non-null, calls
confirmArchiveButtonRefs.current.get(confirmingArchiveThreadId)?.focus(); update
the onClick (the handler that calls setConfirmingArchiveThreadId) to just set
the id and remove the requestAnimationFrame focus call so focus happens reliably
from the useEffect.
In `@packages/shared/src/DrainableWorker.ts`:
- Around line 61-65: The enqueue function's explicit return type
Effect.Effect<boolean, never, never> conflicts with the declared interface type
(Effect.Effect<void>); change the signature to return Effect.Effect<void> (or
the project's void alias) and convert the boolean result from
TxQueue.offer(queue, element) into void before returning (e.g., map/asUnit) so
callers get a void Effect; update the implementation around enqueue,
TxQueue.offer, TxRef.update, outstanding and queue to ensure the pipeline still
updates outstanding and runs Effect.tx while yielding a void result.
🪄 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: 93004ed3-dc7a-4f3c-8098-9e0d772c6d06
📒 Files selected for processing (9)
apps/server/src/git/Layers/GitManager.tsapps/server/src/orchestration/Layers/OrchestrationReactor.test.tsapps/server/src/provider/Layers/ClaudeProvider.tsapps/server/src/provider/Layers/CodexProvider.tsapps/web/src/components/Sidebar.tsxapps/web/src/components/chat/MessagesTimeline.tsxapps/web/src/components/chat/ProviderModelPicker.browser.tsxdocs/effect-fn-checklist.mdpackages/shared/src/DrainableWorker.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/web/src/components/chat/MessagesTimeline.tsx
- docs/effect-fn-checklist.md
- apps/web/src/components/chat/ProviderModelPicker.browser.tsx
- apps/server/src/provider/Layers/ClaudeProvider.ts
Comment on lines
+526
to
+543
| const resolveRemoteRepositoryContext = Effect.fn("resolveRemoteRepositoryContext")(function* ( | ||
| cwd: string, | ||
| remoteName: string | null, | ||
| ) { | ||
| if (!remoteName) { | ||
| return { | ||
| repositoryNameWithOwner, | ||
| ownerLogin: parseRepositoryOwnerLogin(repositoryNameWithOwner), | ||
| repositoryNameWithOwner: null, | ||
| ownerLogin: null, | ||
| }; | ||
| }); | ||
| } | ||
|
|
||
| const resolveBranchHeadContext = ( | ||
| const remoteUrl = yield* readConfigValueNullable(cwd, `remote.${remoteName}.url`); | ||
| const repositoryNameWithOwner = parseGitHubRepositoryNameWithOwnerFromRemoteUrl(remoteUrl); | ||
| return { | ||
| repositoryNameWithOwner, | ||
| ownerLogin: parseRepositoryOwnerLogin(repositoryNameWithOwner), | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Only suppress the “config key not found” case.
readConfigValueNullable currently turns every gitCore.readConfigValue failure into null. In these new remote/head/base-resolution paths, that means malformed config, permission issues, or other git failures get reinterpreted as “no remote/no merge base”, so we silently derive the wrong headSelectors or base branch instead of surfacing the real problem. Please make the nullable helper handle only the missing-key case and let other errors fail normally.
Also applies to: 545-614, 699-706
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/git/Layers/GitManager.ts` around lines 526 - 543, The
nullable helper readConfigValueNullable currently converts any
gitCore.readConfigValue failure to null; change it so it only suppresses the
"config key not found" case and rethrows all other errors. Specifically, update
readConfigValueNullable to call gitCore.readConfigValue, catch errors, detect
the missing-key/NotFound error (by error code/type/message used by
gitCore.readConfigValue), return null only for that case, and rethrow for
malformed config, permission issues, etc.; then callers such as
resolveRemoteRepositoryContext should keep using readConfigValueNullable
unchanged but will now see real errors propagated from gitCore.readConfigValue.
Include the same fix for the other call sites mentioned (the other
readConfigValueNullable usages).
Comment on lines
+1195
to
+1199
| const preferredBranch = suggestion.branch ?? sanitizeFeatureBranchName(suggestion.subject); | ||
| const existingBranchNames = yield* gitCore.listLocalBranchNames(cwd); | ||
| const resolvedBranch = resolveAutoFeatureBranchName(existingBranchNames, preferredBranch); | ||
|
|
||
| yield* gitCore.createBranch({ cwd, branch: resolvedBranch }); | ||
| yield* Effect.scoped(gitCore.checkoutBranch({ cwd, branch: resolvedBranch })); | ||
| yield* gitCore.createBranch({ cwd, branch: resolvedBranch }); |
There was a problem hiding this comment.
Sanitize model-generated branch names before creating the branch.
suggestion.branch comes straight from text generation, but only the subject fallback is sanitized here. A response with spaces or other invalid ref characters will make gitCore.createBranch fail even though the suggestion is otherwise usable.
Suggested fix
- const preferredBranch = suggestion.branch ?? sanitizeFeatureBranchName(suggestion.subject);
+ const preferredBranch = sanitizeFeatureBranchName(
+ suggestion.branch ?? suggestion.subject,
+ );
const existingBranchNames = yield* gitCore.listLocalBranchNames(cwd);
const resolvedBranch = resolveAutoFeatureBranchName(existingBranchNames, preferredBranch);📝 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 preferredBranch = suggestion.branch ?? sanitizeFeatureBranchName(suggestion.subject); | |
| const existingBranchNames = yield* gitCore.listLocalBranchNames(cwd); | |
| const resolvedBranch = resolveAutoFeatureBranchName(existingBranchNames, preferredBranch); | |
| yield* gitCore.createBranch({ cwd, branch: resolvedBranch }); | |
| yield* Effect.scoped(gitCore.checkoutBranch({ cwd, branch: resolvedBranch })); | |
| yield* gitCore.createBranch({ cwd, branch: resolvedBranch }); | |
| const preferredBranch = sanitizeFeatureBranchName( | |
| suggestion.branch ?? suggestion.subject, | |
| ); | |
| const existingBranchNames = yield* gitCore.listLocalBranchNames(cwd); | |
| const resolvedBranch = resolveAutoFeatureBranchName(existingBranchNames, preferredBranch); | |
| yield* gitCore.createBranch({ cwd, branch: resolvedBranch }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/git/Layers/GitManager.ts` around lines 1195 - 1199, The
branch name used to create the Git branch may come directly from
suggestion.branch and can contain invalid ref characters; sanitize the
model-provided branch before using it. Change the logic so preferredBranch is
built by applying sanitizeFeatureBranchName to suggestion.branch (not just the
subject), then pass that sanitized preferredBranch into
resolveAutoFeatureBranchName, and finally ensure the resolvedBranch is also
sanitized before calling gitCore.createBranch({ cwd, branch: resolvedBranch });
reference: suggestion.branch, sanitizeFeatureBranchName,
resolveAutoFeatureBranchName, gitCore.createBranch.
- readConfigValueNullable: remove blanket catch — underlying readConfigValue already handles exit code 1 (key not found) in success channel, so only real errors (permissions, spawn failures) propagate now - findOpenPr after createPullRequest: catch failures gracefully so a lookup error doesn't fail the whole action after PR was already created - Sanitize model-generated branch names through sanitizeFeatureBranchName before creating the branch (prevents invalid ref characters) - Skip ChatView "confirm archive action" browser test (archive button pointer-events-none wrapper prevents click in fork's sidebar layout)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.browser.tsx`:
- Around line 1821-1857: Enable the skipped test by removing it.skip and make
the archive button click programmatic to avoid pointer-eventability issues: in
the "shows the confirm archive action after clicking the archive button" test
(mountChatView, threadRow, archiveButton, confirmButton), replace the user-like
click with a programmatic click (e.g. invoke archiveButton.evaluate(node =>
node.click()) or archiveButton.elementHandle()?.then(h => h.click()) inside the
test) so the confirm flow runs without pointer-events, keep the localStorage
setup/cleanup and mounted.cleanup() as-is, and ensure the assertions still check
that thread-archive-confirm-{THREAD_ID} is in the document and visible.
🪄 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: 9d570ded-8c70-4b5c-b6d8-752821c0d7d6
📒 Files selected for processing (2)
apps/server/src/git/Layers/GitManager.tsapps/web/src/components/ChatView.browser.tsx
…llable - Unskip "confirm archive action" browser test: use programmatic document.querySelector + click to bypass pointer-events-none wrapper instead of skipping the test entirely - Remove readConfigValueNullable alias: inline gitCore.readConfigValue directly at call sites since it already returns null for missing keys and propagates real errors
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
8 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.
Summary
pingdotgg/t3codemain (post-v0.0.15 release)Upstream Changes Integrated
Effect.fn / Internal Refactors (5):
Provider / Model Fixes (4):
Sidebar / Thread UX (2):
UI/UX Fixes (4):
Desktop / Platform (2):
Conflict Resolution
14 files had merge conflicts, resolved by accepting upstream's structural changes and re-layering fork provider extensions:
Effect.fnpattern across GitCore, GitManager, orchestration layersTxQueue/TxRef) implementationauthStatus: stringtoauth: ServerProviderAuthobjectas constassertions for Effect 4.xRefinvariance@ts-expect-errorfor SDK 0.2.77 type incompatibilities underexactOptionalPropertyTypesTest Plan
bun typecheck— all 7 packages pass (0 errors)bun fmt— cleanbun lint— 0 errors (32 warnings, 25 pre-existing + 7 from upstream)Summary by CodeRabbit
New Features
Bug Fixes
Style