Redesign chat cards and make proof chronological and recoverable - #933
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughThe PR adds lane-aware computer-use proof ingestion and lifecycle maintenance, exposes proof management through CLI, RPC, IPC, and preload APIs, synchronizes cleanup across storage and project operations, and updates desktop, TUI, and iOS card and proof rendering. ChangesProof artifacts and ADE cards
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
arul28
changed the title
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 591065be66
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48e8282893
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d3e4bc5b6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 277be76fa8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Cards - One resizable --chat-content-width token replaces the fixed 832px column and seven disagreeing per-row clamps; cards no longer stop 26% short of prose. - Shared chatCardPrimitives on a 16px/1fr/auto grid so titles, durations and counts align in columns down the transcript. - CI cards render an honest degraded state instead of a false green when the GitHub fetch fails, and a degraded re-emit no longer blanks a rich card. - Subagent identity reads as role + intent; the raw Codex agent path moves to a tooltip. Placeholder filler no longer stands in for a result summary. - Real durations with an hour unit, formatted schedule times, wired card actions. Proof - Artifact paths resolve against the calling agent's lane worktree, and a missing file now throws instead of silently persisting an unrenderable record. - Imports are gated on file type: the widened roots put .env.local, *.db and key material beside legitimate captures, and the store syncs to paired phones. - Delete exists end to end: broker, IPC, action registry, ade proof rm/prune, drawer, and Settings storage. Artifacts carry lane_id and are removed with their lane; archive stays non-destructive. - Proof renders inline where it was captured rather than pinned to the thread footer, with a per-turn chip into the drawer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Removing the pinned footer left computer-use proof with no inline representation: the only proof_artifact card emitter is the Cursor Cloud pull, so screenshots taken through capture/attach appeared nowhere in the transcript. The turn divider already grouped proof by capture time for its count chip; it now carries the artifacts themselves and renders ChatProofFilmstrip beneath the rule. The chip toggles it and starts collapsed, so a long capture run never buries the reply, and "open all" still reaches the drawer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ent drops The proof delete surface shipped unreachable. ade/actions/call dispatches off READ_ONLY_TOOLS/MUTATION_TOOLS, and three registered tools were in neither, so ade proof rm, prune --broken and recover all returned methodNotFound. Added them, plus a test that fails without the fix. Inline proof rendered no images: the filmstrip was mounted without a resolveThumbnailSrc, so every tile showed its kind label. Local projects now resolve previews through the ade-artifact:// project handler. Security: - Lane delete's unlink jail was a lexical prefix check. uri is CRR-replicated, so a peer can write it, and a directory symlink under the artifacts dir would let the unlink walk out. Now realpath-jailed like the broker's own resolver. - The secrets deny-list compared lexically while the allow-list beside it realpaths, so a symlink into .ade/secrets passed the barrier it exists to be. Correctness: - artifact-deleted was emitted with owner: null, which every listener drops, so deletes made outside the drawer never refreshed it. Now emitted per owner. - Batch ingest half-committed when one input was rejected; inputs resolve before any row is written. - Restored heif/tif/tiff/m4v/ogv, which the CLI kind table and the preview MIME table already supported. - browserMock lacked the four new methods, so the web renderer threw on delete. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
277be76 to
978f568
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 978f568a69
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4bcf580a96
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/desktop/src/renderer/components/chat/ChatComputerUsePanel.tsx (1)
138-147: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winBroken timeline proof cards render nothing —
isBrokenArtifactcheck is missing fromChatProofArtifactCard's ternary.
useVisibleArtifactPreviewnow early-returns for broken artifacts before ever settingloaded=true(Line 145:|| isBrokenArtifact(artifact)). But the amber "broken" branch inChatProofArtifactCard(Line 309) only triggers onmediaFailed || (loaded && !preview && (image || video)). Sinceloadednever becomestruefor a broken artifact (no fetch is attempted) and no<img>/<video>is ever rendered to fireonError/setmediaFailed, none of the ternary branches match for a broken artifact — the card falls through tonulland renders an empty body (just the header), for both image/video and generic-file broken artifacts alike.This defeats the feature's own stated purpose ("We know exactly why a tile is blank, so say it" — Line 75) for the primary chat-timeline card; only
DrawerProofTile, which checksisBrokenArtifactdirectly (Line 454), gets this right.🐛 Proposed fix
- ) : mediaFailed || (loaded && !preview && (image || video)) ? ( + ) : mediaFailed || isBrokenArtifact(artifact) || (loaded && !preview && (image || video)) ? (Also applies to: 305-319
🤖 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/desktop/src/renderer/components/chat/ChatComputerUsePanel.tsx` around lines 138 - 147, Update the broken-artifact branch in ChatProofArtifactCard’s rendering ternary to check isBrokenArtifact(artifact) directly, alongside the existing mediaFailed and preview failure conditions. Ensure broken image, video, and generic-file artifacts render the amber broken state instead of falling through to null, while preserving the existing rendering behavior for non-broken artifacts.apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx (1)
7013-7024: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNon-virtualized rows never get
resolveProofThumbnailSrc.The virtualized branch passes
turnProof,resolveProofThumbnailSrc, andonOpenProofDrawer(Lines 6970-6972), but this branch omitsresolveProofThumbnailSrc. Short transcripts (belowVIRTUALIZATION_THRESHOLD) therefore render every proof tile as a bare kind label instead of a thumbnail — which is exactly the common case for a fresh chat.🐛 Proposed fix
turnProof={turnProof} + resolveProofThumbnailSrc={resolveProofThumbnailSrc} onOpenProofDrawer={onOpenProofDrawer}🤖 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/desktop/src/renderer/components/chat/AgentChatMessageList.tsx` around lines 7013 - 7024, Update the non-virtualized EventRow render path in AgentChatMessageList to pass the existing resolveProofThumbnailSrc callback, matching the virtualized branch alongside turnProof and onOpenProofDrawer. Preserve all other row props and behavior.apps/desktop/src/renderer/components/chat/codex/CodexPlanCard.tsx (1)
116-131: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEnter/Space on the markdown toggle also fires the card's
onOpenInfo.The toggle stops click propagation, but
keydownstill bubbles to therole="button"wrapper (Lines 59-65), which has notarget !== currentTargetguard. Activating the toggle by keyboard therefore both expands the markdown and navigates to chat info.AdeCard.tsxguards this exact case at Lines 319-328 — mirror it here.🐛 Proposed fix (outside the selected range, at Lines 59-65)
onKeyDown={(keyboardEvent) => { if (!onOpenInfo) return; + // Nested controls handle their own keyboard activation; letting their + // Enter/Space bubble here would open info on top of the action. + if (keyboardEvent.target !== keyboardEvent.currentTarget) return; if (keyboardEvent.key === "Enter" || keyboardEvent.key === " ") { keyboardEvent.preventDefault(); onOpenInfo(); } }}🤖 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/desktop/src/renderer/components/chat/codex/CodexPlanCard.tsx` around lines 116 - 131, Update the CodexPlanCard role="button" wrapper’s keyboard handler near the card onOpenInfo logic to ignore events whose target differs from currentTarget, matching AdeCard.tsx. Preserve keyboard activation for the card itself while preventing Enter/Space on the markdown toggle from also opening chat info.
🧹 Nitpick comments (8)
apps/desktop/src/main/services/computerUse/computerUseArtifactBrokerService.ts (2)
1200-1231: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLane-owned-artifact SQL is duplicated in
laneService.ts.This predicate is byte-for-byte the
LANE_OWNED_ARTIFACT_IDS_SQLconstant inapps/desktop/src/main/services/lanes/laneService.ts(lines 180-200). Two copies of the same "which artifacts does a lane own" rule will drift; export it from one place and import it here.🤖 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/desktop/src/main/services/computerUse/computerUseArtifactBrokerService.ts` around lines 1200 - 1231, Replace the duplicated lane-owned-artifact SQL inside deleteArtifactsForLane with the exported LANE_OWNED_ARTIFACT_IDS_SQL from laneService.ts, importing that constant through the existing service module conventions. Preserve the current projectId and laneId parameter ordering and delete behavior.
132-159: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo stacked doc blocks on one function. The first block ("Paths an agent may read…") is orphaned above the second; merge them so the rationale reads as one comment.
🤖 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/desktop/src/main/services/computerUse/computerUseArtifactBrokerService.ts` around lines 132 - 159, Merge the two consecutive documentation blocks above isDeniedArtifactSource into a single comment, preserving both the security description and the realpath rationale as one coherent explanation attached to that function. Keep realpathOrSelf’s separate documentation unchanged.apps/desktop/src/main/services/computerUse/computerUseArtifactBrokerService.test.ts (1)
432-437: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
rmSyncmock throws when the candidate no longer exists.fs.realpathSync(String(candidate))raises ENOENT for any path already gone, so the mock surfaces an unrelated error instead of delegating. Guard the realpath call (or comparepath.resolve) so the spy stays inert for non-target paths.🤖 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/desktop/src/main/services/computerUse/computerUseArtifactBrokerService.test.ts` around lines 432 - 437, Update the rmSync spy setup to avoid calling fs.realpathSync on candidates that may already be absent; compare normalized paths safely, or guard realpathSync failures, so only canonicalFilePath triggers the simulated EACCES error and all other candidates delegate to originalRmSync.apps/desktop/src/renderer/components/chat/chatTranscriptRows.ts (1)
1058-1095: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftThe degraded/stale merge contract is implemented twice. Both copies are byte-for-byte the same algorithm, and both files already import from
apps/desktop/src/shared/adeCard.ts— the contract's own module. Any future tweak (e.g. treatingrowsTruncatedas detail, or clearingactionsdifferently) has to be made in two places or the two surfaces silently diverge.
apps/desktop/src/renderer/components/chat/chatTranscriptRows.ts#L1058-L1095: replace the body ofmergeAdeCardEventwith a call to a sharedmergeAdeCardPayloadexported fromapps/desktop/src/shared/adeCard.ts, keeping the localcardIdoverride.apps/ade-cli/src/tuiClient/format.ts#L50-L79: delete the localmergeAdeCardPayloadand import the shared one instead.🤖 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/desktop/src/renderer/components/chat/chatTranscriptRows.ts` around lines 1058 - 1095, The degraded/stale merge algorithm is duplicated and must be centralized. In apps/desktop/src/renderer/components/chat/chatTranscriptRows.ts:1058-1095, replace mergeAdeCardEvent’s merge logic with the shared mergeAdeCardPayload from apps/desktop/src/shared/adeCard.ts while preserving the local cardId override. In apps/ade-cli/src/tuiClient/format.ts:50-79, remove the local mergeAdeCardPayload implementation and import the shared function instead.apps/desktop/src/renderer/components/chat/ChatComputerUsePanel.tsx (1)
539-547: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDestructive delete/prune actions have no confirmation step.
The per-tile delete button (Line 539-547) and the "Remove them" batch prune action (Line 673-679) both permanently remove artifact records with a single click and no confirmation — including for artifacts that are currently
"available"(not broken). Given these are proof/evidence artifacts, an accidental click loses evidence with no undo path (unlike the broken-item "recover" flow, which is explicitly recoverable).♻️ Suggested guard
const handleDelete = useCallback( (artifact: ComputerUseArtifactView) => { + if (!window.confirm(`Delete "${artifact.title}"? This cannot be undone.`)) return; void withBusy([artifact.id], () => window.ade.computerUse.deleteArtifacts({ artifactId: artifact.id }), ); }, [withBusy], );Apply an analogous guard to
handlePruneBrokenfor the batch case.Also applies to: 668-681
🤖 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/desktop/src/renderer/components/chat/ChatComputerUsePanel.tsx` around lines 539 - 547, 3Add confirmation guards to both destructive artifact-removal paths: wrap the per-tile delete handler around the button invoking onDelete(artifact), and add an analogous confirmation to handlePruneBroken before batch removal. Require explicit confirmation for all artifacts, including available ones, while preserving the existing removal behavior after confirmation.apps/desktop/src/renderer/components/chat/ChatWorkLogBlock.tsx (1)
685-695: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExpansion is one-way and
aria-expandedis hardcoded.Once
showHistoryflips true the button unmounts (hiddenCountbecomes 0), so there is no way back to the folded view. Consider rendering the control wheneveractivityEntries.length > 1and toggling both the state andaria-expanded.♻️ Toggle instead of one-way expand
- {hiddenCount > 0 ? ( + {hiddenCount > 0 || showHistory ? ( <button type="button" - onClick={() => setShowHistory(true)} - aria-expanded={false} + onClick={() => setShowHistory((open) => !open)} + aria-expanded={showHistory} className="flex items-center gap-1.5 px-1 py-0.5 font-sans text-[length:calc(var(--chat-font-size)*10.5/14)] text-fg/40 transition-colors hover:text-fg/70" > <CaretDown size={10} weight="bold" aria-hidden /> - +{hiddenCount} previous tool call{hiddenCount === 1 ? "" : "s"} + {showHistory ? "Hide previous tool calls" : `+${hiddenCount} previous tool call${hiddenCount === 1 ? "" : "s"}`} </button> ) : null}🤖 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/desktop/src/renderer/components/chat/ChatWorkLogBlock.tsx` around lines 685 - 695, Update the history control in ChatWorkLogBlock so it renders whenever activityEntries.length is greater than one, including when history is expanded. Change its click handler to toggle showHistory and bind aria-expanded to the current showHistory value, while preserving the hidden-count label and folded/expanded content behavior.apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx (1)
6182-6212: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueBucketing rescans the full artifact list per
donerow.
stampedis sorted, so each window could advance a cursor instead of re-filtering all artifacts for every turn (O(dones × artifacts)). Fine at today's sizes; worth tightening if proof counts grow.🤖 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/desktop/src/renderer/components/chat/AgentChatMessageList.tsx` around lines 6182 - 6212, Optimize turnProofByRowKey by replacing the per-`done` row `stamped.filter` scan with a forward-only cursor over the sorted artifacts. Advance the cursor as artifacts fall before each window start, collect entries through `endMs`, and preserve the existing inclusive first-window and exclusive subsequent-window boundaries.apps/desktop/src/renderer/components/chat/chatCardPrimitives.tsx (1)
419-447: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueThumbnail has no error fallback.
resolveThumbnailSrccan return a URL for an artifact whose bytes are gone (deleted file,unimportedavailability), and the tile then renders the browser's broken-image glyph instead of the kind label thenullbranch already provides. AnonErrorthat clears the src recovers the honest fallback.🤖 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/desktop/src/renderer/components/chat/chatCardPrimitives.tsx` around lines 419 - 447, The artifact thumbnail image rendered in the artifacts map lacks an error fallback. Update the img element in the open artifacts block to track and clear its source when loading fails, so failed URLs render the existing kind-label fallback instead of a broken-image glyph; preserve the current null-source behavior and styling.
🤖 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/ade-cli/src/adeRpcServer.ts`:
- Around line 4398-4404: In the callerRoot resolution flow, derive the
authorized root from the server-authorized session lane and reject explicit
callerRoot or laneId values unless their canonical worktree path matches that
root; do not accept arbitrary absolute directories. Update
apps/ade-cli/src/adeRpcServer.ts lines 4398-4404 accordingly. In
apps/ade-cli/src/adeRpcServer.test.ts lines 1796-1828, replace the arbitrary
absolute-root success case with an authorized-lane case and add coverage
rejecting a different absolute root.
In `@apps/ade-cli/src/tuiClient/adeCardFormat.ts`:
- Around line 119-128: The row truncation display in the card formatting flow
must include rows removed by both the emitter and the local five-row cap. Update
the logic around rows and rowsTruncated in the card formatter to calculate the
residual rows beyond the displayed slice, then show +N more using rowsTruncated
plus that residual, matching the iOS card behavior.
In `@apps/desktop/src/main/services/adeActions/registry.ts`:
- Around line 803-814: Remove deleteArtifacts, recoverArtifact,
pruneBrokenArtifacts, and listBrokenArtifacts from the computer_use_artifacts
registry in apps/desktop/src/main/services/adeActions/registry.ts (lines
803-814) until the action boundary provides an authenticated owner scope. In
apps/ade-cli/src/adeRpcServer.ts (lines 4442-4470), derive the owner/principal
from the server authorization context and pass it into broker operations,
enforce that scope in broker queries, and permit project-wide pruning only for
an explicitly authorized maintenance role.
In
`@apps/desktop/src/main/services/computerUse/computerUseArtifactBrokerService.ts`:
- Around line 889-922: Normalize artifact URIs to jailed filesystem paths before
determining whether another row shares the file. In
apps/desktop/src/main/services/computerUse/computerUseArtifactBrokerService.ts:889-922,
update the sharedReference logic around resolveArtifactFilePath to resolve
candidate rows and compare their paths with filePath instead of relying on raw
uri equality. In apps/desktop/src/main/services/lanes/laneService.ts:3070-3077,
replace the survivor.uri equality guard with the same resolved-path comparison,
using resolveRendererArtifactPath and resolvePathWithinRoot.
In `@apps/desktop/src/main/services/lanes/laneService.ts`:
- Around line 6299-6302: Update the lane artifact deletion flow around
removeLaneArtifactFiles so the helper returns the number of files successfully
removed, while preserving warning handling for failures. Use that success count
in the returned detail message, and return undefined when none were removed so
progress reports only completed deletions.
In `@apps/desktop/src/main/services/projects/adeProjectService.ts`:
- Around line 464-480: The packs cleanup in the options.packs block deletes all
project artifact records while removing only artifactsDir, including
attachment-backed records. Restrict the deletes in the artifact row cleanup to
artifacts owned by repair.paths.artifactsDir, or also remove the attachments
root before deleting those records; preserve attachment-backed records when
their files remain available.
In `@apps/desktop/src/renderer/browserMock.ts`:
- Around line 3756-3761: Update the deleteArtifacts and pruneBrokenArtifacts
mocks to include the required freedBytes field, and make recoverArtifact resolve
a contract-valid artifact view rather than an empty object. Remove the as any
casts and construct results using the production return types or established
valid mock values, preserving the existing list fields and resolvedArg behavior.
In `@apps/desktop/src/renderer/components/settings/StorageSection.tsx`:
- Around line 1507-1511: The blocked-state message in the cleanable storage
section should state the next action: direct users to the chat proof drawer to
delete individual proofs. Update the text within the cleanable.length === 0
branch while preserving its existing styling and conditional rendering.
- Around line 1496-1499: Update the title constructed in the onRequestCleanup
handler to preserve entry.item.label’s acronym casing; remove the full label
lowercasing or only lowercase its initial character if sentence case is
required, while leaving the rest of the cleanup dialog unchanged.
In `@apps/desktop/src/shared/types/computerUseArtifacts.ts`:
- Around line 174-186: The computer-use event fanout does not consume
broker-emitted artifact-deleted events, leaving stale drawer rows. Update the
existing computer-use event subscribers and renderer/TUI/iOS handlers to handle
artifact-deleted, refresh the proof drawer, and remove the corresponding
artifact row using the existing event payload and removal flow.
In `@apps/ios/ADE/Views/Work/WorkChatRichCardViews.swift`:
- Around line 4133-4137: Update hiddenRowCount for the card.variant == "pr_ci"
branch to include failure rows omitted by visibleRows after the existing
prefix(4) limit, while retaining card.rowsTruncated and excluding filtered-out
passing rows. Use the same failure-filtered candidate collection that
visibleRows derives from so the count reports all locally hidden failing rows.
In `@apps/ios/ADE/Views/Work/WorkEventMapping.swift`:
- Line 769: Update the payload mapping around degradedReason to trim whitespace
and convert blank values to nil before passing them into merging(_:), matching
the direct-transcript parser’s optionalString behavior while preserving
non-blank reasons.
---
Outside diff comments:
In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx`:
- Around line 7013-7024: Update the non-virtualized EventRow render path in
AgentChatMessageList to pass the existing resolveProofThumbnailSrc callback,
matching the virtualized branch alongside turnProof and onOpenProofDrawer.
Preserve all other row props and behavior.
In `@apps/desktop/src/renderer/components/chat/ChatComputerUsePanel.tsx`:
- Around line 138-147: Update the broken-artifact branch in
ChatProofArtifactCard’s rendering ternary to check isBrokenArtifact(artifact)
directly, alongside the existing mediaFailed and preview failure conditions.
Ensure broken image, video, and generic-file artifacts render the amber broken
state instead of falling through to null, while preserving the existing
rendering behavior for non-broken artifacts.
In `@apps/desktop/src/renderer/components/chat/codex/CodexPlanCard.tsx`:
- Around line 116-131: Update the CodexPlanCard role="button" wrapper’s keyboard
handler near the card onOpenInfo logic to ignore events whose target differs
from currentTarget, matching AdeCard.tsx. Preserve keyboard activation for the
card itself while preventing Enter/Space on the markdown toggle from also
opening chat info.
---
Nitpick comments:
In
`@apps/desktop/src/main/services/computerUse/computerUseArtifactBrokerService.test.ts`:
- Around line 432-437: Update the rmSync spy setup to avoid calling
fs.realpathSync on candidates that may already be absent; compare normalized
paths safely, or guard realpathSync failures, so only canonicalFilePath triggers
the simulated EACCES error and all other candidates delegate to originalRmSync.
In
`@apps/desktop/src/main/services/computerUse/computerUseArtifactBrokerService.ts`:
- Around line 1200-1231: Replace the duplicated lane-owned-artifact SQL inside
deleteArtifactsForLane with the exported LANE_OWNED_ARTIFACT_IDS_SQL from
laneService.ts, importing that constant through the existing service module
conventions. Preserve the current projectId and laneId parameter ordering and
delete behavior.
- Around line 132-159: Merge the two consecutive documentation blocks above
isDeniedArtifactSource into a single comment, preserving both the security
description and the realpath rationale as one coherent explanation attached to
that function. Keep realpathOrSelf’s separate documentation unchanged.
In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx`:
- Around line 6182-6212: Optimize turnProofByRowKey by replacing the per-`done`
row `stamped.filter` scan with a forward-only cursor over the sorted artifacts.
Advance the cursor as artifacts fall before each window start, collect entries
through `endMs`, and preserve the existing inclusive first-window and exclusive
subsequent-window boundaries.
In `@apps/desktop/src/renderer/components/chat/chatCardPrimitives.tsx`:
- Around line 419-447: The artifact thumbnail image rendered in the artifacts
map lacks an error fallback. Update the img element in the open artifacts block
to track and clear its source when loading fails, so failed URLs render the
existing kind-label fallback instead of a broken-image glyph; preserve the
current null-source behavior and styling.
In `@apps/desktop/src/renderer/components/chat/ChatComputerUsePanel.tsx`:
- Around line 539-547: 3Add confirmation guards to both destructive
artifact-removal paths: wrap the per-tile delete handler around the button
invoking onDelete(artifact), and add an analogous confirmation to
handlePruneBroken before batch removal. Require explicit confirmation for all
artifacts, including available ones, while preserving the existing removal
behavior after confirmation.
In `@apps/desktop/src/renderer/components/chat/chatTranscriptRows.ts`:
- Around line 1058-1095: The degraded/stale merge algorithm is duplicated and
must be centralized. In
apps/desktop/src/renderer/components/chat/chatTranscriptRows.ts:1058-1095,
replace mergeAdeCardEvent’s merge logic with the shared mergeAdeCardPayload from
apps/desktop/src/shared/adeCard.ts while preserving the local cardId override.
In apps/ade-cli/src/tuiClient/format.ts:50-79, remove the local
mergeAdeCardPayload implementation and import the shared function instead.
In `@apps/desktop/src/renderer/components/chat/ChatWorkLogBlock.tsx`:
- Around line 685-695: Update the history control in ChatWorkLogBlock so it
renders whenever activityEntries.length is greater than one, including when
history is expanded. Change its click handler to toggle showHistory and bind
aria-expanded to the current showHistory value, while preserving the
hidden-count label and folded/expanded content behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0390fb86-0e3d-45b4-855a-9f5c912be033
⛔ Files ignored due to path filters (15)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/chat/README.mdis excluded by!docs/**docs/features/chat/composer-and-ui.mdis excluded by!docs/**docs/features/chat/tool-system.mdis excluded by!docs/**docs/features/chat/transcript-and-turns.mdis excluded by!docs/**docs/features/computer-use/README.mdis excluded by!docs/**docs/features/computer-use/artifact-broker.mdis excluded by!docs/**docs/features/computer-use/backends.mdis excluded by!docs/**docs/features/lanes/README.mdis excluded by!docs/**docs/features/lanes/worktree-isolation.mdis excluded by!docs/**docs/features/onboarding-and-settings/README.mdis excluded by!docs/**docs/features/proof.mdis excluded by!docs/**docs/features/pull-requests/README.mdis excluded by!docs/**docs/features/storage-and-recovery/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/ios-companion.mdis excluded by!docs/**
📒 Files selected for processing (66)
.gitleaksignoreapps/ade-cli/README.mdapps/ade-cli/src/adeRpcServer.test.tsapps/ade-cli/src/adeRpcServer.tsapps/ade-cli/src/bootstrap.tsapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/services/sync/syncHostService.test.tsapps/ade-cli/src/services/sync/syncHostService.tsapps/ade-cli/src/tuiClient/__tests__/format.test.tsapps/ade-cli/src/tuiClient/adeCardFormat.tsapps/ade-cli/src/tuiClient/format.tsapps/desktop/src/main/main.tsapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/computerUse/computerUseArtifactBrokerService.test.tsapps/desktop/src/main/services/computerUse/computerUseArtifactBrokerService.tsapps/desktop/src/main/services/computerUse/controlPlane.test.tsapps/desktop/src/main/services/computerUse/controlPlane.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/lanes/laneService.test.tsapps/desktop/src/main/services/lanes/laneService.tsapps/desktop/src/main/services/projects/adeProjectService.tsapps/desktop/src/main/services/proof/agentBrowserArtifactAdapter.test.tsapps/desktop/src/main/services/proof/agentBrowserArtifactAdapter.tsapps/desktop/src/main/services/prs/prChatCards.test.tsapps/desktop/src/main/services/prs/prChatCards.tsapps/desktop/src/main/services/state/kvDb.tsapps/desktop/src/main/services/storage/storageInsightsService.test.tsapps/desktop/src/main/services/storage/storageInsightsService.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/chat/AdeCard.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/ChatComputerUsePanel.test.tsxapps/desktop/src/renderer/components/chat/ChatComputerUsePanel.tsxapps/desktop/src/renderer/components/chat/ChatFileChangesPanel.tsxapps/desktop/src/renderer/components/chat/ChatWorkLogBlock.tsxapps/desktop/src/renderer/components/chat/ContextCompactDivider.tsxapps/desktop/src/renderer/components/chat/SubagentActivityCards.tsxapps/desktop/src/renderer/components/chat/chatAppearance.tsapps/desktop/src/renderer/components/chat/chatCardPrimitives.test.tsapps/desktop/src/renderer/components/chat/chatCardPrimitives.tsxapps/desktop/src/renderer/components/chat/chatTranscriptRows.test.tsapps/desktop/src/renderer/components/chat/chatTranscriptRows.tsapps/desktop/src/renderer/components/chat/codex/CodexPlanCard.tsxapps/desktop/src/renderer/components/settings/StorageSection.tsxapps/desktop/src/renderer/components/settings/storage/storageView.tsapps/desktop/src/renderer/lib/format.test.tsapps/desktop/src/renderer/lib/format.tsapps/desktop/src/shared/adeCard.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/computerUseArtifacts.tsapps/desktop/src/shared/types/storage.tsapps/ios/ADE/Models/RemoteModels.swiftapps/ios/ADE/Resources/DatabaseBootstrap.sqlapps/ios/ADE/Services/Database.swiftapps/ios/ADE/Views/Work/WorkArtifactTerminalViews.swiftapps/ios/ADE/Views/Work/WorkChatRichCardViews.swiftapps/ios/ADE/Views/Work/WorkErrorAndMessageHelpers.swiftapps/ios/ADE/Views/Work/WorkEventMapping.swiftapps/ios/ADE/Views/Work/WorkModels.swiftapps/ios/ADE/Views/Work/WorkTranscriptParser.swiftapps/ios/ADETests/ADETests.swift
💤 Files with no reviewable changes (2)
- apps/desktop/src/main/services/proof/agentBrowserArtifactAdapter.ts
- apps/desktop/src/main/services/proof/agentBrowserArtifactAdapter.test.ts
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ae00206e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/desktop/src/renderer/components/settings/StorageSection.test.tsx (1)
373-390: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBrittle DOM traversal and two scenarios in one test.
parentElement!.parentElement!.parentElement!breaks on any wrapper change — preferclosest(...)on a stable selector or a role-based container query. Splitting the pre-cleanup()casing assertion and the blocked-cleanup assertion into twoitblocks would also isolate failures.🤖 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/desktop/src/renderer/components/settings/StorageSection.test.tsx` around lines 373 - 390, Refactor the test around the proof cleanup flow into two independent it blocks: one covering ADE casing and proof-drawer navigation, and another covering blocked cleanup. In the first block, replace the chained parentElement traversal used to locate proofRow with a stable closest selector or role-based container query, while preserving the existing assertions and cleanup behavior.apps/ade-cli/src/adeRpcServer.ts (1)
4625-4664: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPer-entry ownership lookups are O(N) broker queries.
Both
list_broken_computer_use_artifactsandprune_broken_computer_use_artifactscalllistArtifacts({ artifactId })once per broken entry with a 2000-row ceiling. Fetch the caller's owned artifacts once (vialistArtifactsper authorized owner) and match against that set instead.🤖 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/ade-cli/src/adeRpcServer.ts` around lines 4625 - 4664, Replace the per-entry listArtifacts calls in both list_broken_computer_use_artifacts and prune_broken_computer_use_artifacts with one fetched ownership set: call listArtifacts once for each authorized owner from resolveAuthorizedProofOwners, then match broken entries against those results by artifact ID. Preserve the existing authorization checks, requestedLimit behavior, and prune deletion results while avoiding O(N) broker queries.apps/ade-cli/src/adeRpcServer.test.ts (1)
1310-1318: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
recoverArtifact/pruneBrokenArtifacts/listBrokenArtifactsscoping is mocked but never exercised.The test name promises coverage of "proof lifecycle tools" scoping, but only
ingest/list/deleteare actually invoked and asserted; the recover/prune/broken-listing mocks are set up and never called. Consider adding assertions (or dropping the unused mocks) so the test title matches its actual coverage.🤖 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/ade-cli/src/adeRpcServer.test.ts` around lines 1310 - 1318, Update the proof lifecycle tools test around the computerUseArtifactBrokerService mocks to either invoke and assert recoverArtifact, pruneBrokenArtifacts, and listBrokenArtifacts with the expected scoping, or remove their unused mock setup. Keep the test title aligned with the lifecycle operations it actually covers.apps/desktop/src/main/services/lanes/laneService.test.ts (1)
3373-3379: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSymlink mismatch could make the
rmSyncmock never fire on macOS.The spy compares
path.resolve(String(candidate))againstfs.realpathSync(failedFile). SincerepoRootis derived fromos.mkdtempSync(path.join(os.tmpdir(), ...)), andos.tmpdir()resolves to a symlink on macOS (/var/folders/...vs. real/private/var/folders/...),candidate(built off the non-realpath’drepoRoot) may never equalfs.realpathSync(failedFile)on such machines, so the mocked failure never triggers and the "partial failure" assertion is unreliable outside of symlink-free environments.🐛 Proposed fix: normalize both sides the same way
- if (path.resolve(String(candidate)) === fs.realpathSync(failedFile)) { + if (fs.realpathSync(path.resolve(String(candidate))) === fs.realpathSync(failedFile)) { throw Object.assign(new Error("permission denied"), { code: "EACCES" }); }🤖 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/desktop/src/main/services/lanes/laneService.test.ts` around lines 3373 - 3379, Update the rmSync spy in the partial-failure test to normalize both the candidate path and failedFile using the same path representation before comparing them, such as resolving each consistently without mixing symlink-preserving and real paths. Keep the EACCES throw behavior and originalRmSync fallback unchanged so the failure reliably triggers on macOS and other symlinked temporary-directory environments.
🤖 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/ade-cli/src/adeRpcServer.ts`:
- Around line 4571-4589: The scoped artifact-listing path in the
!projectWideAuthorized branch must sort the merged artifacts by createdAt
descending before applying the limit. Update the artifacts collection handling
around runtime.computerUseArtifactBrokerService.listArtifacts so deduplication
is preserved, then sort [...artifacts.values()] consistently with the
project-wide ordering and only afterward call slice(0, limit).
---
Nitpick comments:
In `@apps/ade-cli/src/adeRpcServer.test.ts`:
- Around line 1310-1318: Update the proof lifecycle tools test around the
computerUseArtifactBrokerService mocks to either invoke and assert
recoverArtifact, pruneBrokenArtifacts, and listBrokenArtifacts with the expected
scoping, or remove their unused mock setup. Keep the test title aligned with the
lifecycle operations it actually covers.
In `@apps/ade-cli/src/adeRpcServer.ts`:
- Around line 4625-4664: Replace the per-entry listArtifacts calls in both
list_broken_computer_use_artifacts and prune_broken_computer_use_artifacts with
one fetched ownership set: call listArtifacts once for each authorized owner
from resolveAuthorizedProofOwners, then match broken entries against those
results by artifact ID. Preserve the existing authorization checks,
requestedLimit behavior, and prune deletion results while avoiding O(N) broker
queries.
In `@apps/desktop/src/main/services/lanes/laneService.test.ts`:
- Around line 3373-3379: Update the rmSync spy in the partial-failure test to
normalize both the candidate path and failedFile using the same path
representation before comparing them, such as resolving each consistently
without mixing symlink-preserving and real paths. Keep the EACCES throw behavior
and originalRmSync fallback unchanged so the failure reliably triggers on macOS
and other symlinked temporary-directory environments.
In `@apps/desktop/src/renderer/components/settings/StorageSection.test.tsx`:
- Around line 373-390: Refactor the test around the proof cleanup flow into two
independent it blocks: one covering ADE casing and proof-drawer navigation, and
another covering blocked cleanup. In the first block, replace the chained
parentElement traversal used to locate proofRow with a stable closest selector
or role-based container query, while preserving the existing assertions and
cleanup behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f53dfe9c-9a73-4eb0-9d9a-9af9575f2f70
📒 Files selected for processing (30)
apps/ade-cli/src/adeRpcServer.test.tsapps/ade-cli/src/adeRpcServer.tsapps/ade-cli/src/tuiClient/__tests__/format.test.tsapps/ade-cli/src/tuiClient/adeCardFormat.tsapps/desktop/src/main/services/adeActions/registry.test.tsapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/computerUse/computerUseArtifactBrokerService.test.tsapps/desktop/src/main/services/computerUse/computerUseArtifactBrokerService.tsapps/desktop/src/main/services/lanes/laneService.test.tsapps/desktop/src/main/services/lanes/laneService.tsapps/desktop/src/main/services/projects/adeProjectService.tsapps/desktop/src/main/services/projects/projectLifecycle.test.tsapps/desktop/src/main/services/prs/prChatCards.test.tsapps/desktop/src/main/services/prs/prChatCards.tsapps/desktop/src/main/services/storage/storageInsightsService.test.tsapps/desktop/src/main/services/storage/storageInsightsService.tsapps/desktop/src/renderer/browserMock.test.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.test.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/chatCardPrimitives.tsxapps/desktop/src/renderer/components/chat/codex/CodexPlanCard.test.tsxapps/desktop/src/renderer/components/chat/codex/CodexPlanCard.tsxapps/desktop/src/renderer/components/settings/StorageSection.test.tsxapps/desktop/src/renderer/components/settings/StorageSection.tsxapps/ios/ADE/Views/Work/WorkChatRichCardViews.swiftapps/ios/ADE/Views/Work/WorkEventMapping.swiftapps/ios/ADETests/ADETests.swift
🚧 Files skipped from review as they are similar to previous changes (17)
- apps/desktop/src/renderer/browserMock.ts
- apps/ios/ADE/Views/Work/WorkEventMapping.swift
- apps/desktop/src/renderer/components/chat/codex/CodexPlanCard.tsx
- apps/desktop/src/renderer/components/settings/StorageSection.tsx
- apps/desktop/src/renderer/components/chat/AgentChatPane.tsx
- apps/ade-cli/src/tuiClient/tests/format.test.ts
- apps/ios/ADETests/ADETests.swift
- apps/desktop/src/main/services/projects/adeProjectService.ts
- apps/desktop/src/main/services/storage/storageInsightsService.ts
- apps/desktop/src/main/services/lanes/laneService.ts
- apps/desktop/src/main/services/prs/prChatCards.ts
- apps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsx
- apps/desktop/src/main/services/computerUse/computerUseArtifactBrokerService.test.ts
- apps/ade-cli/src/tuiClient/adeCardFormat.ts
- apps/desktop/src/renderer/components/chat/chatCardPrimitives.tsx
- apps/desktop/src/main/services/computerUse/computerUseArtifactBrokerService.ts
- apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx
|
Addressed the current-head review batch in
I did not change the lane deletion test: production passes Validation: 183 focused tests pass; desktop and ADE CLI typechecks pass on Node 22. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f17794c6e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Addressed the two current-head Codex P2 findings in
Validation: 12 focused drawer tests and desktop typecheck pass on Node 22. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c21168c50a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Addressed the drawer-specific preview-state finding in
Validation: 12 focused drawer tests and desktop typecheck pass on Node 22. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca6a80326d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/main/services/computerUse/computerUseArtifactBrokerService.test.ts (1)
344-345: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert copied proof bytes in both attached-lane tests.
Availability and file existence also pass for a truncated or incorrect copy. Assert that the stored file equals the fixture buffer.
apps/desktop/src/main/services/computerUse/computerUseArtifactBrokerService.test.ts#L344-L345: readingested.artifacts[0]!.urifromprojectRootand compare it to the PNG buffer.apps/desktop/src/main/services/computerUse/computerUseArtifactBrokerService.test.ts#L747-L749: compare the recovered file’s contents to the PNG buffer.🤖 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/desktop/src/main/services/computerUse/computerUseArtifactBrokerService.test.ts` around lines 344 - 345, Strengthen both attached-lane tests by reading the stored artifact file and asserting its contents equal the fixture PNG buffer. At apps/desktop/src/main/services/computerUse/computerUseArtifactBrokerService.test.ts:344-345, resolve ingested.artifacts[0]!.uri from projectRoot and compare the bytes; apply the same content comparison to the recovered file at :747-749.
🤖 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.
Nitpick comments:
In
`@apps/desktop/src/main/services/computerUse/computerUseArtifactBrokerService.test.ts`:
- Around line 344-345: Strengthen both attached-lane tests by reading the stored
artifact file and asserting its contents equal the fixture PNG buffer. At
apps/desktop/src/main/services/computerUse/computerUseArtifactBrokerService.test.ts:344-345,
resolve ingested.artifacts[0]!.uri from projectRoot and compare the bytes; apply
the same content comparison to the recovered file at :747-749.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b7353e42-dece-49dc-bf5d-1e66fe97214e
📒 Files selected for processing (7)
apps/ade-cli/src/adeRpcServer.test.tsapps/ade-cli/src/adeRpcServer.tsapps/desktop/src/main/services/computerUse/computerUseArtifactBrokerService.test.tsapps/desktop/src/main/services/computerUse/computerUseArtifactBrokerService.tsapps/desktop/src/renderer/components/chat/ChatComputerUsePanel.test.tsxapps/desktop/src/renderer/components/chat/ChatComputerUsePanel.tsxapps/desktop/src/renderer/components/settings/StorageSection.test.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/ade-cli/src/adeRpcServer.ts
- apps/desktop/src/main/services/computerUse/computerUseArtifactBrokerService.ts
- apps/desktop/src/renderer/components/chat/ChatComputerUsePanel.tsx
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1da24f8176
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8483c7c433
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5971a1e130
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8f2fc2f8a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| throw error; | ||
| } | ||
| const artifacts = resolved.map(({ input, kind, title, stored }) => { |
There was a problem hiding this comment.
If a later record or owner-link insert fails—for example because SQLite runs out of space or a sync trigger rejects a write—the earlier iterations of this map have already committed records and emitted events, while every staged file remains on disk. The RPC then reports failure, so retrying can duplicate the successfully persisted prefix and a link failure can leave proof without its required ownership. Wrap all record/link inserts in one transaction, remove newly staged files on rollback, and emit events only after commit.
AGENTS.md reference: AGENTS.md:L36-L36
Useful? React with 👍 / 👎.
| if (existingHadDetail && (!incomingHasDetail || incoming.degradedReason)) { | ||
| if (!incomingRows.length && existing.rows?.length) merged.rows = existing.rows; | ||
| if (!incomingMetrics.length && existing.metrics?.length) merged.metrics = existing.metrics; | ||
| if (incomingProgressTotal === 0 && existing.progress) merged.progress = existing.progress; |
There was a problem hiding this comment.
When a healthy full refresh explicitly reports no rows, metrics, or progress—for example after a PR's checks are removed—incomingHasDetail is false, so this branch preserves the previous job list and marks it stale instead of clearing it. It also prevents a recovery from clearing an old degraded reason and Retry action. Detect whether the detail fields were supplied, as the desktop merge does, rather than whether their totals are nonzero; the same value-based predicate in apps/ios/ADE/Views/Work/WorkModels.swift has the same stale-card behavior.
Useful? React with 👍 / 👎.
| if (!projectWideAuthorized && requestedLaneId && requestedLaneId !== sessionLaneId) { | ||
| throw new JsonRpcError( | ||
| JsonRpcErrorCode.invalidParams, | ||
| "laneId must match the caller's authorized chat-session lane", |
There was a problem hiding this comment.
A direct ade proof ingest --input-json call is initialized as an unbound ade-cli:<pid> agent, so it has no sessionLaneId; supplying the tool's documented laneId therefore always trips this check, even when callerRoot is inside that exact lane. The later inference path explicitly trusts these standalone CLI callers, but it is skipped whenever laneId is present, making explicit lane-scoped ingestion unusable through the desktop socket-backed RPC path. Permit an unbound ADE CLI caller here and validate the requested lane/root through laneService.
AGENTS.md reference: AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
1 participant
Summary
Validation
/qualitydual review completed; blocker/high findings fixed/testcompleted with docs, CLI, TUI, and mobile paritySummary by CodeRabbit
ade proofwith ingest/attach, remove, broken listing, prune-broken, and recover flows; added support forade-artifact://project/...URIs.