Integrate T3Code roadmap slices#98
Open
Jay1 wants to merge 27 commits into
Open
Conversation
📝 WalkthroughWalkthroughThis PR adds backend WSL path/discovery support, managed worktree cleanup classification, OpenCode session resume changes, provider update-check settings and a Copilot spike, chat markdown wrap settings, provider usage and VCS status UI, composer prompt traits, chat timeline details/minimap, and a cockpit design document. ChangesBackend WSL Support
Managed Worktree Cleanup
Provider, Session, and Settings
Provider Usage UI
Composer Prompt Traits
Chat Timeline Enhancements
Design Documentation
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.1)apps/web/src/index.cssFile contains syntax errors that prevent linting: Line 3: Tailwind-specific syntax is disabled.; Line 5: Tailwind-specific syntax is disabled.; Line 51: Tailwind-specific syntax is disabled.; Line 54: Tailwind-specific syntax is disabled.; Line 61: Tailwind-specific syntax is disabled.; Line 466: Tailwind-specific syntax is disabled. 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
the
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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/web/src/routes/_chat.settings.tsx (1)
540-543: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate thread-linking logic diverges from
classifyManagedWorktreeCleanupChoices.
normalizeManagedWorktreePathand thelinkedThreadsFromSnapshotfilter here re-implement the same worktree/thread path-matching logic already defined inworktreeCleanup.ts'snormalizeWorktreePathandclassifyManagedWorktreeCleanupChoices. Since this copy runs against a freshly-fetched snapshot rather than the memoizedmanagedWorktreeChoices, the two implementations can silently diverge (e.g., a future change to the matching rule in one file won't be reflected in the other), producing inconsistent confirmation-dialog messaging vs. actual cleanup-safety classification.♻️ Suggested consolidation
-function normalizeManagedWorktreePath(value: string | null | undefined): string | null { - const trimmed = value?.trim(); - return trimmed && trimmed.length > 0 ? trimmed : null; -} +// Reuse the shared helper from worktreeCleanup.ts instead of duplicating it here.Then export
normalizeWorktreePath(and ideally a smalllinkThreadsToWorktreePathhelper) fromworktreeCleanup.tsand call it from both the memoized classification and the fresh-snapshot lookup indeleteManagedWorktree.Also applies to: 1306-1312
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/routes/_chat.settings.tsx` around lines 540 - 543, The thread-linking and worktree path matching logic in `deleteManagedWorktree`/`linkedThreadsFromSnapshot` is duplicated from `worktreeCleanup.ts`, which risks the two paths diverging over time. Move the canonical matching into `worktreeCleanup.ts` by exporting `normalizeWorktreePath` and, ideally, a shared `linkThreadsToWorktreePath` helper, then have both `classifyManagedWorktreeCleanupChoices` and the fresh-snapshot lookup in `deleteManagedWorktree` call that shared logic instead of `normalizeManagedWorktreePath` and the local filter.apps/web/src/components/BranchToolbar.tsx (1)
34-50: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winProvider usage resolution diverges from ChatView, causing inconsistent chip state for draft threads.
activeProviderhere only readsserverThread, so for any thread that hasn't started yet (serverThreadisundefined), it resolves tonullregardless of the composer's selected/default provider.usageSummaryis then computed from thatnullprovider, and bothBranchToolbarEnvironmentPicker's rate-limit panel and theRuntimeUsageControlsrendered here will show "no provider" data.Meanwhile,
ChatView.tsxcomputes its ownactiveProviderUsageSummarywith a full fallback chain (session.provider ?? modelSelection.provider ?? selectedProvider) and spreads it intobranchToolbarProps— butBranchToolbarPropsdoesn't declareprovider/providerRateLimits/providerUsageLines/providerUsageIsLoading/providerUsageLearnMoreHref, so those computed values are silently dropped when passed to<BranchToolbar {...branchToolbarProps} />. Worse,ChatViewrenders the sameRuntimeUsageControlswidget directly (with its own, more complete provider) wheneverisGitRepois false, so the exact same UI element shows different provider/usage data depending on whether the project is a git repo.🔧 Suggested fix: accept the already-computed provider/usage props instead of recomputing locally
interface BranchToolbarProps { threadId: ThreadId; className?: string; onEnvModeChange: (mode: EnvMode) => void; envLocked: boolean; runtimeMode?: RuntimeMode; onRuntimeModeChange?: (mode: RuntimeMode) => void; + activeProvider?: ProviderKind | null; onHandoffToWorktree?: () => void; onHandoffToLocal?: () => void; handoffBusy?: boolean; onCheckoutPullRequestRequest?: (reference: string) => void; onComposerFocusRequest?: () => void; contextWindow?: ContextWindowSnapshot | null; cumulativeCostUsd?: number | null; activeContextWindowLabel?: string | null; pendingContextWindowLabel?: string | null; }- const activeProvider = - serverThread?.session?.provider ?? serverThread?.modelSelection.provider ?? null; + const activeProvider = + activeProviderProp ?? + serverThread?.session?.provider ?? + serverThread?.modelSelection.provider ?? + null;Then thread
activeProvider(renamed prop) fromChatView's already-computed value instead of relying solely onserverThread.Also applies to: 83-84, 194-198, 244-259
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/BranchToolbar.tsx` around lines 34 - 50, BranchToolbar is recomputing provider usage from serverThread and ignores the already-computed provider data coming from ChatView, which causes draft threads to show inconsistent “no provider” state. Update BranchToolbarProps and BranchToolbar to accept and use the provider/usage props that ChatView already prepares (activeProvider, providerRateLimits, providerUsageLines, providerUsageIsLoading, providerUsageLearnMoreHref) instead of deriving usage locally, and thread the renamed activeProvider through the BranchToolbar/RuntimeUsageControls/BranchToolbarEnvironmentPicker path so both git and non-git flows render the same provider data.apps/web/src/session-logic.ts (1)
49-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the redundant
DetailedWorkLogEntryalias
WorkLogEntryalready includesoutput,stdout,stderr,exitCode,durationMs, andpatch, so this local intersection can be simplified toWorkLogEntryunless thereadonlymodifiers are intentional.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/session-logic.ts` around lines 49 - 72, Simplify the local work-log type by removing the redundant DetailedWorkLogEntry intersection and using WorkLogEntry directly in session-logic.ts. The relevant symbols are WorkLogEntry and the alias currently defined around it; if any readonly intent was meant for subagent-related fields, preserve that explicitly instead of duplicating the same output/stdout/stderr/exitCode/durationMs/patch properties.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/server/src/git/Layers/GitCore.ts`:
- Around line 458-460: The `listManagedWorktrees` parsing in `GitCore` only
matches an exact `prunable` line, so it misses `git worktree list --porcelain`
entries that include a reason suffix. Update the `prunable` handling in the
worktree entry parser to recognize lines that start with `prunable` and still
set `currentPrunable`, preserving the reason text for any existing parse flow.
Keep the fix localized around the `listManagedWorktrees` logic and the
`prunable` branch in `GitCore`.
In `@apps/server/src/provider/Layers/OpenCodeAdapter.test.ts`:
- Around line 1484-1531: The OpenCodeAdapter resume test only checks session
reuse, so it can still pass even if the resumed event changes or session.update
drops permission data. Strengthen the existing test in OpenCodeAdapter.test by
asserting the resumed turn emits the expected resumed event and that the
sessionUpdate payload includes the runtime-mode permissions for the resumed
session. Use the existing session and turn assertions as anchors, and add
focused expectations around the mocked runtime event and sessionUpdate call
details.
In `@apps/server/src/provider/Layers/ProviderHealth.ts`:
- Around line 2044-2055: `enrichStatuses` is refetching settings instead of
using the snapshot already loaded by `loadProviderStatuses`, which can cause
inconsistent enrichment behavior. Update `enrichStatuses` to accept the upstream
`settings` (or at least `enableProviderUpdateChecks`) as an argument and pass it
through from the `loadProviderStatuses` call site. Keep the existing
`Effect.forEach`/`enrichProviderStatusWithVersionAdvisory` flow unchanged, but
remove the internal `serverSettings.getSettings` read so the same settings
snapshot is reused end to end.
In `@apps/web/src/components/chat/MessagesTimeline.tsx`:
- Around line 1895-1896: The expand/collapse state in SimpleWorkEntryRow is
shared across every changed-file button, so multi-file entries toggle together
and hide the per-file diff-open behavior. Update the per-file rendering in
MessagesTimeline to scope expansion by changed file path (or move the patch
toggle out of the file buttons), and make the
FileChangeActivityDetails/ActivityEntryDetails flow target the clicked file
instead of one shared expanded boolean. Preserve the existing
onOpenTurnDiff(turnId, changedFilePath) behavior for entries with patch data so
a file click can still open that specific diff when applicable.
In `@apps/web/src/components/chat/MessagesTimelineActivityDetails.tsx`:
- Around line 40-46: The command-activity check is duplicated in
isCommandActivity and can drift from the existing isCommandEntry logic in
session-logic.ts. Move or export the shared predicate from toDerivedWorkLogEntry
in session-logic.ts, then import and reuse it in MessagesTimelineActivityDetails
so both places rely on the same command-classification rule.
- Around line 175-184: CommandOutputBlock truncates long command output without
telling the user, so add a visible truncation note when visibleLines is shorter
than lines. Update CommandOutputBlock in MessagesTimelineActivityDetails to
render a small “showing last 40 lines” style indicator alongside the
ActivityDetailBlock content whenever COMMAND_OUTPUT_TAIL_LINES is applied, using
the existing props.title, getRenderableCommandOutputLines, and visibleLines
logic.
- Around line 1-204: Extract the pure predicate/formatting helpers from
MessagesTimelineActivityDetails into a separate
MessagesTimelineActivityDetails.logic.ts module and add matching tests in a
.logic.test.ts file. Move functions like hasExpandableActivityDetails,
hasCommandActivityDetails, isCommandActivity, hasFileChangeActivityDetails,
isFileChangeActivity, formatWorkspaceRelativePath,
getRenderableCommandOutputLines, and related output-detection helpers so the
React component only renders JSX and imports the logic. Keep the component names
ActivityEntryDetails, CommandActivityDetails, and FileChangeActivityDetails as
the UI entry points, and cover the extracted logic with unit tests.
In `@apps/web/src/components/chat/MessagesTimelineMinimap.tsx`:
- Around line 65-76: Memoize the timeline item derivation in TimelineMinimap so
it does not rescan rows on every parent re-render. Update
MessagesTimelineMinimap.tsx by using useMemo around
deriveTimelineMinimapItems(rows), and consider wrapping TimelineMinimap with
memo if it still re-renders with unchanged props. Keep the existing
TIMELINE_MINIMAP_MIN_ITEMS guard and onJump props behavior unchanged.
In `@apps/web/src/components/ChatView.structure.test.ts`:
- Around line 6-11: The structural test for runtimeUsageControlsProps is too
dependent on the exact ChatView.tsx text layout and adjacent branchToolbarProps
ordering, so it can fail on harmless reformatting. Update
ChatView.structure.test.ts to avoid the brittle regex in
extractRuntimeUsageControlsPropsSource() and instead verify the wiring through a
render-based assertion or AST-based inspection using a stable symbol from
ChatView.tsx such as runtimeUsageControlsProps (and, if needed, the ChatView
component) so the test remains valid across formatting changes.
In `@apps/web/src/index.css`:
- Around line 823-828: Remove the deprecated word-break: break-word declaration
from the wrapped chat markdown code block styles in index.css, since
overflow-wrap: anywhere already covers the wrapping behavior. Update both
affected rule blocks that target .chat-markdown
.chat-markdown-codeblock--wrapped pre and .chat-markdown
.chat-markdown-codeblock--wrapped pre code so only the non-deprecated
white-space and overflow-wrap declarations remain.
- Around line 1143-1156: The wrap toggle button in the chat table scroll
container is positioned in a way that lets it move out of view during horizontal
scrolling. Update the `.chat-markdown .chat-markdown-table-wrap-button` behavior
so it stays visible inside `.chat-markdown-table-scroll` when the table
overflows horizontally, using the existing table scroll/wrap styles as the
anchor for the fix. Keep the hover/focus visibility logic for
`.chat-markdown-table-scroll:hover .chat-markdown-table-wrap-button` and
`.chat-markdown-table-scroll:focus-within .chat-markdown-table-wrap-button`
intact while changing the positioning approach so the button remains accessible
in the unwrapped state.
In `@apps/web/src/session-logic.ts`:
- Around line 737-739: The command classification logic is duplicated between
session-logic extraction and MessagesTimelineActivityDetails.tsx rendering,
which risks future drift. Extract the predicate from session-logic.ts into a
shared helper such as isCommandWorkLogEntry and use it both in the command-entry
handling path around extractCommandResult/extractChangedFiles/extractToolPatch
and in the existing isCommandActivity logic. Keep the helper colocated with the
current classification symbols so both modules import the same source of truth.
In `@docs/adr/README.md`:
- Line 28: Update the metadata table in the README to keep it current after
adding the new ADR row. Specifically, adjust the “Last reviewed” value in the
document’s metadata table so it reflects this change, and make sure the entry in
the ADR index that includes 0009 is accompanied by the updated review date. Use
the README’s metadata table and ADR list entry as the reference points when
making the change.
In `@packages/contracts/src/backend.ts`:
- Around line 30-37: The Backend schema currently allows invalid combinations
because Backend.kind and Backend.connection.kind are modeled as separate
independent fields. Update Backend in backend.ts so kind is derived from
connection (or refactor the schema into a single discriminated union) and make
sure the Backend and BackendConnection definitions enforce only valid local/wsl
combinations. Keep the fix centered on the Backend and BackendConnection symbols
so the type no longer represents domain-invalid states.
---
Outside diff comments:
In `@apps/web/src/components/BranchToolbar.tsx`:
- Around line 34-50: BranchToolbar is recomputing provider usage from
serverThread and ignores the already-computed provider data coming from
ChatView, which causes draft threads to show inconsistent “no provider” state.
Update BranchToolbarProps and BranchToolbar to accept and use the provider/usage
props that ChatView already prepares (activeProvider, providerRateLimits,
providerUsageLines, providerUsageIsLoading, providerUsageLearnMoreHref) instead
of deriving usage locally, and thread the renamed activeProvider through the
BranchToolbar/RuntimeUsageControls/BranchToolbarEnvironmentPicker path so both
git and non-git flows render the same provider data.
In `@apps/web/src/routes/_chat.settings.tsx`:
- Around line 540-543: The thread-linking and worktree path matching logic in
`deleteManagedWorktree`/`linkedThreadsFromSnapshot` is duplicated from
`worktreeCleanup.ts`, which risks the two paths diverging over time. Move the
canonical matching into `worktreeCleanup.ts` by exporting
`normalizeWorktreePath` and, ideally, a shared `linkThreadsToWorktreePath`
helper, then have both `classifyManagedWorktreeCleanupChoices` and the
fresh-snapshot lookup in `deleteManagedWorktree` call that shared logic instead
of `normalizeManagedWorktreePath` and the local filter.
In `@apps/web/src/session-logic.ts`:
- Around line 49-72: Simplify the local work-log type by removing the redundant
DetailedWorkLogEntry intersection and using WorkLogEntry directly in
session-logic.ts. The relevant symbols are WorkLogEntry and the alias currently
defined around it; if any readonly intent was meant for subagent-related fields,
preserve that explicitly instead of duplicating the same
output/stdout/stderr/exitCode/durationMs/patch properties.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 004b6917-a9b6-4219-b98b-97bd28619f3c
📒 Files selected for processing (59)
DESIGN.mdapps/server/src/backend/backendPathResolver.tsapps/server/src/backend/backendRegistry.test.tsapps/server/src/backend/backendRegistry.tsapps/server/src/git/Layers/GitCore.test.tsapps/server/src/git/Layers/GitCore.tsapps/server/src/git/Services/GitCore.tsapps/server/src/provider/Layers/OpenCodeAdapter.test.tsapps/server/src/provider/Layers/OpenCodeAdapter.tsapps/server/src/provider/Layers/ProviderHealth.tsapps/server/src/provider/copilotProviderSpike.test.tsapps/server/src/provider/copilotProviderSpike.tsapps/server/src/provider/providerMaintenance.test.tsapps/server/src/provider/providerMaintenance.tsapps/server/src/serverSettings.test.tsapps/server/src/wsRpc.tsapps/web/src/appSettings.test.tsapps/web/src/appSettings.tsapps/web/src/chat-scroll.test.tsapps/web/src/chat-scroll.tsapps/web/src/components/BranchToolbar.structure.test.tsapps/web/src/components/BranchToolbar.tsxapps/web/src/components/BranchToolbarEnvironmentPicker.tsxapps/web/src/components/ChatMarkdown.test.tsxapps/web/src/components/ChatMarkdown.tsxapps/web/src/components/ChatView.structure.test.tsapps/web/src/components/ChatView.tsxapps/web/src/components/GitActionsControl.tsxapps/web/src/components/ProviderUsageStatusChip.logic.test.tsapps/web/src/components/ProviderUsageStatusChip.logic.tsapps/web/src/components/ProviderUsageStatusChip.tsxapps/web/src/components/RuntimeUsageControls.tsxapps/web/src/components/VcsCommandCenterStatusPanel.logic.tsapps/web/src/components/VcsCommandCenterStatusPanel.test.tsxapps/web/src/components/VcsCommandCenterStatusPanel.tsxapps/web/src/components/chat/MessagesTimeline.browser.tsxapps/web/src/components/chat/MessagesTimeline.test.tsxapps/web/src/components/chat/MessagesTimeline.tsxapps/web/src/components/chat/MessagesTimelineActivityDetails.tsxapps/web/src/components/chat/MessagesTimelineMinimap.test.tsxapps/web/src/components/chat/MessagesTimelineMinimap.tsxapps/web/src/components/chat/composerProviderRegistry.test.tsxapps/web/src/components/chat/composerProviderRegistry.tsxapps/web/src/components/chat/composerTraits.tsapps/web/src/index.cssapps/web/src/routes/_chat.settings.tsxapps/web/src/session-logic.test.tsapps/web/src/session-logic.tsapps/web/src/worktreeCleanup.test.tsapps/web/src/worktreeCleanup.tsdocs/adr/0009-copilot-provider-entry-path.mddocs/adr/README.mdpackages/contracts/src/backend.test.tspackages/contracts/src/backend.tspackages/contracts/src/index.tspackages/contracts/src/server.test.tspackages/contracts/src/server.tspackages/contracts/src/settings.test.tspackages/contracts/src/settings.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/server/src/provider/Layers/ProviderHealth.ts (1)
2044-2055: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThread the same
settingssnapshot into capability resolution too.This still mixes snapshots:
loadProviderStatusesprobes providers with the outersettings, butgetProviderMaintenanceCapabilities(status.provider)re-reads settings during enrichment. A concurrent settings edit can therefore return provider statuses from one snapshot and version-advisory/update capability data from another in the same refresh. Pass the already-loadedsettingsthrough this path as well.Proposed fix
- const getProviderMaintenanceCapabilities = Effect.fn("getProviderMaintenanceCapabilities")( - function* (provider: ProviderKind) { - const settings = yield* serverSettings.getSettings; + const getProviderMaintenanceCapabilities = Effect.fn("getProviderMaintenanceCapabilities")( + function* (provider: ProviderKind, settings?: ServerSettings) { + const resolvedSettings = settings ?? (yield* serverSettings.getSettings); if (provider === "cursor") { return makeProviderMaintenanceCapabilities({ provider, packageName: null, - updateExecutable: getProviderBinaryPath(provider, settings) || "agent", + updateExecutable: getProviderBinaryPath(provider, resolvedSettings) || "agent", updateArgs: ["update"], updateLockKey: "cursor-agent", }); } - if (provider === "opencode" && isExternalOpenCodeRuntimeActive(settings)) { + if (provider === "opencode" && isExternalOpenCodeRuntimeActive(resolvedSettings)) { return makeProviderMaintenanceCapabilities({ provider, packageName: null, @@ - const binaryPath = getProviderBinaryPath(provider, settings) || null; + const binaryPath = getProviderBinaryPath(provider, resolvedSettings) || null; return yield* resolveProviderMaintenanceCapabilitiesEffect(definition, { binaryPath, env: process.env, platform: process.platform, }).pipe(Effect.provideService(FileSystem.FileSystem, fileSystem)); }, ); @@ - getProviderMaintenanceCapabilities(status.provider).pipe( + getProviderMaintenanceCapabilities(status.provider, settings).pipe(Also applies to: 2079-2109
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/provider/Layers/ProviderHealth.ts` around lines 2044 - 2055, The enrichment path in enrichProviderStatuses is re-reading settings during capability lookup, which can mix snapshots from different refreshes. Update getProviderMaintenanceCapabilities usage in enrichProviderStatuses (and the related flow covered by loadProviderStatuses/enrichProviderStatusWithVersionAdvisory) to accept the already-loaded settings snapshot, so provider status enrichment and capability resolution both use the same ServerSettings instance. Ensure the settings object passed into enrichProviderStatuses is threaded through the capability resolution call instead of fetching settings again.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/session-logic.ts`:
- Around line 81-85: The command work-log detection in isCommandWorkLogEntry is
using nullish coalescing, which skips a valid raw command when command is an
empty string. Update the boolean check to fall back with logical OR on
workEntry.command and workEntry.rawCommand so empty command values still count
as command entries. This should preserve the derived entry handling in
toDerivedWorkLogEntry so command rows keep output, stdout, stderr, exitCode, and
durationMs.
---
Duplicate comments:
In `@apps/server/src/provider/Layers/ProviderHealth.ts`:
- Around line 2044-2055: The enrichment path in enrichProviderStatuses is
re-reading settings during capability lookup, which can mix snapshots from
different refreshes. Update getProviderMaintenanceCapabilities usage in
enrichProviderStatuses (and the related flow covered by
loadProviderStatuses/enrichProviderStatusWithVersionAdvisory) to accept the
already-loaded settings snapshot, so provider status enrichment and capability
resolution both use the same ServerSettings instance. Ensure the settings object
passed into enrichProviderStatuses is threaded through the capability resolution
call instead of fetching settings again.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dc619534-6ef7-4016-b9d8-739c7f8026fd
📒 Files selected for processing (18)
apps/server/src/backend/backendPathResolver.tsapps/server/src/backend/backendRegistry.test.tsapps/server/src/git/Layers/GitCore.test.tsapps/server/src/git/Layers/GitCore.tsapps/server/src/provider/Layers/OpenCodeAdapter.test.tsapps/server/src/provider/Layers/ProviderHealth.tsapps/web/src/components/ChatView.structure.test.tsapps/web/src/components/chat/MessagesTimeline.test.tsxapps/web/src/components/chat/MessagesTimeline.tsxapps/web/src/components/chat/MessagesTimelineActivityDetails.logic.test.tsapps/web/src/components/chat/MessagesTimelineActivityDetails.logic.tsapps/web/src/components/chat/MessagesTimelineActivityDetails.tsxapps/web/src/components/chat/MessagesTimelineMinimap.tsxapps/web/src/index.cssapps/web/src/session-logic.tsdocs/adr/README.mdpackages/contracts/src/backend.test.tspackages/contracts/src/backend.ts
Comment on lines
+81
to
+85
| export function isCommandWorkLogEntry(workEntry: CommandWorkLogEntryInput): boolean { | ||
| return ( | ||
| workEntry.itemType === "command_execution" || | ||
| workEntry.requestKind === "command" || | ||
| Boolean(workEntry.command ?? workEntry.rawCommand) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use || here instead of ??.
Boolean(workEntry.command ?? workEntry.rawCommand) misclassifies entries when command === "" but rawCommand is present. In that case toDerivedWorkLogEntry skips output, stdout, stderr, exitCode, and durationMs, so command rows lose their activity details. Fall back on any non-empty raw command here.
Proposed fix
export function isCommandWorkLogEntry(workEntry: CommandWorkLogEntryInput): boolean {
return (
workEntry.itemType === "command_execution" ||
workEntry.requestKind === "command" ||
- Boolean(workEntry.command ?? workEntry.rawCommand)
+ Boolean(workEntry.command || workEntry.rawCommand)
);
}📝 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
| export function isCommandWorkLogEntry(workEntry: CommandWorkLogEntryInput): boolean { | |
| return ( | |
| workEntry.itemType === "command_execution" || | |
| workEntry.requestKind === "command" || | |
| Boolean(workEntry.command ?? workEntry.rawCommand) | |
| export function isCommandWorkLogEntry(workEntry: CommandWorkLogEntryInput): boolean { | |
| return ( | |
| workEntry.itemType === "command_execution" || | |
| workEntry.requestKind === "command" || | |
| Boolean(workEntry.command || workEntry.rawCommand) | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/src/session-logic.ts` around lines 81 - 85, The command work-log
detection in isCommandWorkLogEntry is using nullish coalescing, which skips a
valid raw command when command is an empty string. Update the boolean check to
fall back with logical OR on workEntry.command and workEntry.rawCommand so empty
command values still count as command entries. This should preserve the derived
entry handling in toDerivedWorkLogEntry so command rows keep output, stdout,
stderr, exitCode, and durationMs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
Notes
Summary by CodeRabbit
New Features
Bug Fixes