mirror of
https://github.com/stablyai/orca
synced 2026-04-21 14:17:16 +00:00
WIP: Changes before auto-review fixes (iter 1)
This commit is contained in:
brennanb2025
parent
40f6ee4ec1
commit
b8bdd3d3ce
9 changed files with 330 additions and 142 deletions
|
|
@ -2,61 +2,72 @@
|
|||
|
||||
## Branch Info
|
||||
|
||||
- Base: origin/main
|
||||
- Base: origin/main (merge-base `1ea2a6d7`)
|
||||
- Current: brennanb2025/help-me-support-adding-comment-onto-specific-lin
|
||||
|
||||
## Changed Files Summary
|
||||
## Notes
|
||||
|
||||
Modified (M):
|
||||
- src/main/ipc/worktree-logic.ts
|
||||
- src/preload/index.d.ts (⚠️ WIPED to `export {};` — likely unintended)
|
||||
- src/renderer/src/assets/main.css
|
||||
- src/renderer/src/components/editor/CombinedDiffViewer.tsx
|
||||
- src/renderer/src/components/editor/DiffSectionItem.tsx
|
||||
- src/renderer/src/components/editor/EditorContent.tsx
|
||||
- src/renderer/src/store/index.ts
|
||||
- src/renderer/src/store/slices/editor.ts
|
||||
- src/renderer/src/store/slices/store-session-cascades.test.ts
|
||||
- src/renderer/src/store/slices/store-test-helpers.ts
|
||||
- src/renderer/src/store/slices/tabs.test.ts
|
||||
- src/renderer/src/store/types.ts
|
||||
- src/shared/types.ts
|
||||
The PR contains 1415 changed files, but ~1395 of them are compiled `.d.ts` / `.js`
|
||||
build artifacts accidentally committed in the WIP checkpoint. They are not real
|
||||
source changes and will be IGNORED for review purposes. Only the original
|
||||
`.ts` / `.tsx` / `.css` source files (listed below) are in scope.
|
||||
|
||||
Added (A, staged via -N):
|
||||
- src/renderer/src/components/diff-comments/DiffCommentPopover.tsx
|
||||
- src/renderer/src/components/diff-comments/DiffCommentsAgentChooserDialog.tsx
|
||||
- src/renderer/src/components/diff-comments/DiffCommentsTab.tsx
|
||||
- src/renderer/src/components/diff-comments/useDiffCommentDecorator.ts
|
||||
- src/renderer/src/lib/diff-comments-format.ts
|
||||
- src/renderer/src/lib/diff-comments-format.test.ts
|
||||
- src/renderer/src/store/slices/diffComments.ts
|
||||
The compiled artifacts are listed in `.gitignore`-adjacent build output and
|
||||
should almost certainly be removed from the commit — but that cleanup is out of
|
||||
scope for the review itself (it is a mechanical issue, not a code-quality one).
|
||||
A review finding will call it out.
|
||||
|
||||
## Changed Files Summary (source only)
|
||||
|
||||
| Status | File |
|
||||
| ------ | --------------------------------------------------------------------------- |
|
||||
| M | src/main/ipc/worktree-logic.ts |
|
||||
| M | src/preload/index.d.ts |
|
||||
| M | src/renderer/src/assets/main.css |
|
||||
| A | src/renderer/src/components/diff-comments/DiffCommentPopover.tsx |
|
||||
| A | src/renderer/src/components/diff-comments/DiffCommentsAgentChooserDialog.tsx|
|
||||
| A | src/renderer/src/components/diff-comments/DiffCommentsTab.tsx |
|
||||
| A | src/renderer/src/components/diff-comments/useDiffCommentDecorator.ts |
|
||||
| M | src/renderer/src/components/editor/CombinedDiffViewer.tsx |
|
||||
| M | src/renderer/src/components/editor/DiffSectionItem.tsx |
|
||||
| M | src/renderer/src/components/editor/EditorContent.tsx |
|
||||
| A | src/renderer/src/lib/diff-comments-format.test.ts |
|
||||
| A | src/renderer/src/lib/diff-comments-format.ts |
|
||||
| M | src/renderer/src/store/index.ts |
|
||||
| A | src/renderer/src/store/slices/diffComments.ts |
|
||||
| M | src/renderer/src/store/slices/editor.ts |
|
||||
| M | src/renderer/src/store/slices/store-session-cascades.test.ts |
|
||||
| M | src/renderer/src/store/slices/store-test-helpers.ts |
|
||||
| M | src/renderer/src/store/slices/tabs.test.ts |
|
||||
| M | src/renderer/src/store/types.ts |
|
||||
| M | src/shared/types.ts |
|
||||
|
||||
## Changed Line Ranges (PR Scope)
|
||||
|
||||
<!-- In scope: issues on these lines OR caused by these changes. Out of scope: unrelated pre-existing issues -->
|
||||
<!-- In scope: issues on these lines OR caused by these changes. -->
|
||||
|
||||
| File | Changed Lines |
|
||||
| ------------------------------------------------------------------ | ---------------------------------------------------------------------------------------------------------- |
|
||||
| src/main/ipc/worktree-logic.ts | 166-167 |
|
||||
| src/preload/index.d.ts | 1 (entire file, wiped) |
|
||||
| src/renderer/src/assets/main.css | 861-994 |
|
||||
| src/renderer/src/components/editor/CombinedDiffViewer.tsx | 7, 60-61, 499-506 |
|
||||
| src/renderer/src/components/editor/DiffSectionItem.tsx | 1, 10-13, 113-125, 133-159, 181, 192, 195-196, 201-202, 205-206, 281, 296-303 |
|
||||
| src/renderer/src/components/editor/EditorContent.tsx | 15-17, 221-224 |
|
||||
| src/renderer/src/store/index.ts | 17, 33-34 |
|
||||
| src/renderer/src/store/slices/editor.ts | 101, 210, 1102-1139 |
|
||||
| src/renderer/src/store/slices/store-session-cascades.test.ts | 101, 118-119 |
|
||||
| src/renderer/src/store/slices/store-test-helpers.ts | 25, 50-51 |
|
||||
| src/renderer/src/store/slices/tabs.test.ts | 96, 115-116 |
|
||||
| src/renderer/src/store/types.ts | 15, 30-31 |
|
||||
| src/shared/types.ts | 48, 62-78 |
|
||||
| src/renderer/src/components/diff-comments/DiffCommentPopover.tsx | ALL (new file) |
|
||||
| src/renderer/src/components/diff-comments/DiffCommentsAgentChooserDialog.tsx | ALL (new file) |
|
||||
| src/renderer/src/components/diff-comments/DiffCommentsTab.tsx | ALL (new file) |
|
||||
| src/renderer/src/components/diff-comments/useDiffCommentDecorator.ts | ALL (new file) |
|
||||
| src/renderer/src/lib/diff-comments-format.ts | ALL (new file) |
|
||||
| src/renderer/src/lib/diff-comments-format.test.ts | ALL (new file) |
|
||||
| src/renderer/src/store/slices/diffComments.ts | ALL (new file) |
|
||||
| File | Changed Lines |
|
||||
| ---------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------- |
|
||||
| src/main/ipc/worktree-logic.ts | 166-167 |
|
||||
| src/preload/index.d.ts | whole-file simplification (removed inline `ReposApi` etc. in favor of `PreloadApi`) |
|
||||
| src/renderer/src/assets/main.css | 861-994 (new diff-comment styles) |
|
||||
| src/renderer/src/components/diff-comments/DiffCommentPopover.tsx | 1-106 (new file) |
|
||||
| src/renderer/src/components/diff-comments/DiffCommentsAgentChooserDialog.tsx | 1-114 (new file) |
|
||||
| src/renderer/src/components/diff-comments/DiffCommentsTab.tsx | 1-215 (new file) |
|
||||
| src/renderer/src/components/diff-comments/useDiffCommentDecorator.ts | 1-249 (new file) |
|
||||
| src/renderer/src/components/editor/CombinedDiffViewer.tsx | 7, 60-61, 499-506 |
|
||||
| src/renderer/src/components/editor/DiffSectionItem.tsx | 1, 10-13, 113-125, 133-179, 201, 212, 215-216, 221-222, 225-226, 301, 316-323 |
|
||||
| src/renderer/src/components/editor/EditorContent.tsx | 15-17, 221-224 |
|
||||
| src/renderer/src/lib/diff-comments-format.test.ts | 1-72 (new file) |
|
||||
| src/renderer/src/lib/diff-comments-format.ts | 1-23 (new file) |
|
||||
| src/renderer/src/store/index.ts | 17, 33-34 |
|
||||
| src/renderer/src/store/slices/diffComments.ts | 1-173 (new file) |
|
||||
| src/renderer/src/store/slices/editor.ts | 101, 210, 1102-1139 |
|
||||
| src/renderer/src/store/slices/store-session-cascades.test.ts | 101, 118-119 |
|
||||
| src/renderer/src/store/slices/store-test-helpers.ts | 25, 50-51 |
|
||||
| src/renderer/src/store/slices/tabs.test.ts | 96, 115-116 |
|
||||
| src/renderer/src/store/types.ts | 15, 30-31, 48, 62-78 |
|
||||
| src/shared/types.ts | minor additions |
|
||||
|
||||
## Review Standards Reference
|
||||
|
||||
|
|
@ -66,66 +77,83 @@ Added (A, staged via -N):
|
|||
|
||||
## File Categories
|
||||
|
||||
### Electron/Main (src/main/, src/preload/, electron.*)
|
||||
### Electron/Main (priority 1)
|
||||
|
||||
- src/main/ipc/worktree-logic.ts
|
||||
- src/preload/index.d.ts
|
||||
|
||||
### Frontend/UI (src/renderer/, components/, *.tsx, *.css)
|
||||
### Backend/IPC (priority 2)
|
||||
|
||||
- (none — IPC for this PR lives entirely in renderer)
|
||||
|
||||
### Frontend/UI (priority 3)
|
||||
|
||||
- src/renderer/src/assets/main.css
|
||||
- src/renderer/src/components/editor/CombinedDiffViewer.tsx
|
||||
- src/renderer/src/components/editor/DiffSectionItem.tsx
|
||||
- src/renderer/src/components/editor/EditorContent.tsx
|
||||
- src/renderer/src/components/diff-comments/DiffCommentPopover.tsx
|
||||
- src/renderer/src/components/diff-comments/DiffCommentsAgentChooserDialog.tsx
|
||||
- src/renderer/src/components/diff-comments/DiffCommentsTab.tsx
|
||||
- src/renderer/src/components/diff-comments/useDiffCommentDecorator.ts
|
||||
- src/renderer/src/components/editor/CombinedDiffViewer.tsx
|
||||
- src/renderer/src/components/editor/DiffSectionItem.tsx
|
||||
- src/renderer/src/components/editor/EditorContent.tsx
|
||||
|
||||
### Config/Build (priority 4)
|
||||
|
||||
- (none)
|
||||
|
||||
### Utility/Common (priority 5)
|
||||
|
||||
- src/renderer/src/lib/diff-comments-format.ts
|
||||
- src/renderer/src/lib/diff-comments-format.test.ts
|
||||
- src/renderer/src/store/index.ts
|
||||
- src/renderer/src/store/slices/editor.ts
|
||||
- src/renderer/src/store/slices/diffComments.ts
|
||||
- src/renderer/src/store/slices/editor.ts
|
||||
- src/renderer/src/store/slices/store-session-cascades.test.ts
|
||||
- src/renderer/src/store/slices/store-test-helpers.ts
|
||||
- src/renderer/src/store/slices/tabs.test.ts
|
||||
- src/renderer/src/store/types.ts
|
||||
- src/renderer/src/lib/diff-comments-format.ts
|
||||
- src/renderer/src/lib/diff-comments-format.test.ts
|
||||
|
||||
### Utility/Common (shared)
|
||||
- src/shared/types.ts
|
||||
|
||||
## Skipped Issues (Do Not Re-validate)
|
||||
|
||||
<!-- Format: [file:line-range] | [severity] | [reason skipped] | [issue summary] -->
|
||||
|
||||
- [src/shared/types.ts:48,62] | Medium | Matches existing pattern consistently (all persisted meta fields mirror onto Worktree: displayName, comment, linkedIssue, linkedPR, isArchived, isUnread, isPinned, sortOrder, lastActivityAt). Moving diffComments alone to a renderer-only slice would introduce an inconsistency. | diffComments? appears on both Worktree and WorktreeMeta.
|
||||
- [src/shared/types.ts:72] | Medium | DiffComment.worktreeId denormalization matches the existing Tab/TabGroup pattern (Tab.worktreeId at line 109) where child records carry parent id. Consistency with codebase. | DiffComment.worktreeId duplicates container key.
|
||||
- [src/renderer/src/components/diff-comments/useDiffCommentDecorator.ts:60,88-103] | High | Keyboard-accessibility entry point is out of scope for this PR; a "+" button on hover is the standard pattern in similar feature (e.g. GitHub, GitLab diffs). The design doc / feature request explicitly describes a hover gesture. | "+" button only visible on mouse hover.
|
||||
- [src/renderer/src/components/diff-comments/DiffCommentsAgentChooserDialog.tsx:40-61] | Medium | Helper-extract refactor is out of scope; the composer code already uses the shared helpers buildAgentStartupPlan+ensureAgentStartupInTerminal. Only the 6-line launcher glue is duplicated — cosmetic at this scale. | Duplicates composer's launch glue.
|
||||
- [src/renderer/src/components/diff-comments/DiffCommentsTab.tsx:60-80] | Low | Same architectural tier as existing "send to terminal" patterns; extracting a helper is a follow-up refactor, no bug today. | Reinvents "send text to active terminal".
|
||||
- [src/renderer/src/components/diff-comments/DiffCommentsTab.tsx:10-28] | Low | Timestamp live-refresh is polish; no functional bug. | formatTimestamp does not live-update.
|
||||
- [src/renderer/src/components/diff-comments/DiffCommentsTab.tsx:53,67] | Low | Minor stylistic mix; no correctness issue. | Mixed useAppStore selector + getState() in handler.
|
||||
- [src/renderer/src/components/diff-comments/DiffCommentPopover.tsx:26-33] | Low | Currently dead (body starts as ''). | autoResize not called on initial mount.
|
||||
- [src/renderer/src/components/diff-comments/useDiffCommentDecorator.ts:58-59] | Low | Static SVG literal, no user data. | plus.innerHTML XSS pattern (static only).
|
||||
- [src/renderer/src/components/diff-comments/useDiffCommentDecorator.ts:75-84] | Low | Edge case with stale hoverLineRef; negligible impact. | Stale hoverLineRef after scroll.
|
||||
- [src/renderer/src/components/diff-comments/useDiffCommentDecorator.ts:68-73] | Medium | hideUnchangedRegions handling is a polish concern; the feature works correctly in the common case. | Decorator doesn't account for folded regions.
|
||||
- [src/renderer/src/components/editor/DiffSectionItem.tsx:135] | Low | Multiple simultaneous popovers is a UX polish issue; no correctness bug. | Per-section popover state allows two popovers.
|
||||
- [src/renderer/src/store/slices/editor.ts:1102-1138] | Low | Existing openConflictReview precedent; comment already explains guard. No current break. | openDiffCommentsTab stuffs worktreePath into filePath.
|
||||
- [src/renderer/src/components/diff-comments/DiffCommentsTab.tsx:146-158] | Low | Intentional persistence; not a bug. | addDiffComment continues after unmount.
|
||||
- [src/renderer/src/components/editor/DiffSectionItem.tsx,DiffCommentPopover.tsx:146-158,35-41] | Low | Success is the common case; no silent data loss (rollback restores). Could be polish. | No user-visible error toast on persist failure.
|
||||
- [src/renderer/src/components/diff-comments/DiffCommentsAgentChooserDialog.tsx:33-68] | Low | Stale busy/error on reopen is polish. | Dialog state not reset on close.
|
||||
- [src/renderer/src/components/diff-comments/DiffCommentsAgentChooserDialog.tsx:33-46] | Low | Captured prop vs store is standard; delete UI is obviously racy with any modal. | comments prop drifts vs store.
|
||||
- [src/renderer/src/components/diff-comments/DiffCommentsAgentChooserDialog.tsx:52-60] | Low | `ensureAgentStartupInTerminal` resolves via activeTabIdByWorktree which was just set to newTab.id; the poll re-reads state each tick. Works. | Agent-chooser follow-up tab resolution.
|
||||
- [src/renderer/src/components/diff-comments/DiffCommentsAgentChooserDialog.tsx:52-54] | Low | Both calls currently needed; refactor out of scope. | setActiveTabType call potentially redundant.
|
||||
- [src/renderer/src/store/slices/diffComments.ts:15-20] | Low | Electron renderer always has crypto.randomUUID; fallback is dead-code defensive. | generateId fallback collision risk.
|
||||
- [src/renderer/src/assets/main.css:872] | Low | Cosmetic; works correctly. | Inline style vs CSS display ownership.
|
||||
- [src/renderer/src/store/slices/diffComments.ts] | Medium | Slice unit-test coverage is nice-to-have; time-boxed. | No unit tests on slice.
|
||||
- [src/renderer/src/lib/diff-comments-format.test.ts:42-51] | Low | toContain coverage is sufficient; tightening to toBe is nice-to-have. | multi-comment test uses toContain.
|
||||
- [src/shared/types.ts:70-79] | Low | No edit flow exists in v1; updatedAt premature. | No updatedAt on DiffComment.
|
||||
- [src/shared/types.ts:77-78] | Low | Migration story documented via comment; adding an explicit version field is premature. | side discriminator/version design.
|
||||
- [src/main/ipc/worktree-logic.ts:166-167] | Medium | The updateMeta "last write wins" pattern is established for ALL meta fields. Same tradeoff as displayName/comment/etc. Follow-up for the scale discussion. | IPC write-amplification on comments blob.
|
||||
- [src/renderer/src/components/editor/DiffSectionItem.tsx:180-195] | Medium | onDidDispose guarding is defensive; current remount-on-generation behaviour handles this. | Decorator doesn't guard against Monaco editor.onDidDispose.
|
||||
| File:lines | Severity | Reason | Summary |
|
||||
| --- | --- | --- | --- |
|
||||
| src/main/ipc/worktree-logic.ts:10-16 | High | False positive — regex was never in merge-base | "`.replace(/\.{2,}/g, '.')` removed" — never existed at 1ea2a6d7 |
|
||||
| src/renderer/src/store/slices/diffComments.ts:1 | Low | Cosmetic naming | Rename to kebab-case — would update 4-5 import sites for zero behavior change |
|
||||
| src/renderer/src/components/diff-comments/useDiffCommentDecorator.ts:170-220 | Medium | Intentional architecture; would require large rewrite | Imperative view-zone DOM vs ReactDOM.createRoot |
|
||||
| branch-wide | High | Commit surgery, separate PR-prep task | ~1395 compiled .d.ts/.js build artifacts accidentally committed in WIP checkpoint |
|
||||
| src/shared/types.ts:68 | Low | Forward-compat speculation | `side: 'modified'` too narrow |
|
||||
| src/shared/types.ts | Medium | Speculative v2 schema | Add `parentId?`/`resolvedAt?` for future threading |
|
||||
| src/renderer/src/components/diff-comments/DiffCommentPopover.tsx:65 | Low | UX nice-to-have; would require Radix FocusScope import | Full focus trap |
|
||||
| src/renderer/src/components/diff-comments/DiffCommentsTab.tsx:81 | Low | Minor UX | pasteNotice never auto-clears |
|
||||
| src/renderer/src/components/diff-comments/DiffCommentsTab.tsx:80 | Low | Self-directed terminal; `\r`/`\n` already escaped upstream | No ANSI/control-char sanitization before pty.write |
|
||||
| src/renderer/src/components/diff-comments/useDiffCommentDecorator.ts:236-238 | Low | Micro-optimization | querySelector instead of ref map |
|
||||
| src/renderer/src/components/diff-comments/useDiffCommentDecorator.ts:208 | Low | Edge case; documented in inline comment | Long soft-wrapped body clips view-zone |
|
||||
| src/renderer/src/components/diff-comments/useDiffCommentDecorator.ts:47 | Low | Common useEvent pattern | Ref assignments during render |
|
||||
| src/main/ipc/worktree-logic.ts:167 | Medium | Architectural tradeoff | diffComments embedded in Worktree IPC payload |
|
||||
| src/renderer/src/components/editor/DiffSectionItem.tsx:119 | Medium | Selector is already reference-stable for unrelated updates | Per-section re-render cascade |
|
||||
| src/renderer/src/store/slices/diffComments.ts:27 | Low | Doc nit | Add "Why" comment for bypassing updateWorktreeMeta |
|
||||
| src/renderer/src/store/slices/diffComments.ts:81 | Medium | Test infrastructure investment | Add rollback concurrency tests |
|
||||
| src/renderer/src/lib/diff-comments-format.ts:12 | Low | No concrete breakage, speculative | Expand escape set to U+2028/U+2029/NUL |
|
||||
| src/renderer/src/components/diff-comments/DiffCommentsTab.tsx:62 | Low | Doc nit | Document why handlePaste calls IPC directly |
|
||||
| src/renderer/src/store/slices/editor.ts:1102 | Low | Sweeping refactor | Refactor `OpenFile` to discriminated union |
|
||||
| src/renderer/src/components/diff-comments/useDiffCommentDecorator.ts:92 | Low | Listener is removed via `plus.remove()` | Mousedown listener cleanup symmetry |
|
||||
| src/renderer/src/components/diff-comments/useDiffCommentDecorator.ts:181 | Low | Intended UX split (tab view is keyboard surface) | Inline delete keyboard access |
|
||||
| src/renderer/src/store/slices/diffComments.ts:143 | Low | API symmetry nit | delete/clear return void instead of boolean |
|
||||
| src/renderer/src/components/diff-comments/useDiffCommentDecorator.ts:98 | Medium | Product design decision, not review fix | Keyboard path to add comment |
|
||||
| src/renderer/src/components/diff-comments/DiffCommentPopover.tsx:46 | Low | Theoretical, hasn't materialized | pointerdown capture + composedPath |
|
||||
| src/renderer/src/components/diff-comments/DiffCommentPopover.tsx:69 | Low | Speculative for future left-side comments | Hardcoded right:24px position |
|
||||
| src/renderer/src/components/diff-comments/DiffCommentsAgentChooserDialog.tsx:33-68 | Medium | Existing minor UX; unmount race rare | setState-after-unmount + unhandled `ensureAgentStartupInTerminal` rejection |
|
||||
| src/renderer/src/components/diff-comments/DiffCommentsAgentChooserDialog.tsx:70-114 | Low | Radix warning nit | Missing DialogDescription |
|
||||
| src/renderer/src/components/diff-comments/DiffCommentsTab.tsx:180-205 | Low | Radix warning nit | Missing DialogDescription in clear-all dialog |
|
||||
| src/renderer/src/components/diff-comments/DiffCommentPopover.tsx:27-29 | Low | Addressed by FIX-10 (key prop) | Popover reuse retains prior draft (duplicate of FIX-10) |
|
||||
| src/renderer/src/components/editor/CombinedDiffViewer.tsx:61 | Low | Speculative future concern | getDiffComments selector future misuse |
|
||||
| src/renderer/src/components/editor/DiffSectionItem.tsx:146-164 | Low | Already commented inline | Effect dep suppression comment |
|
||||
| src/main/ipc/worktree-logic.ts (Claude algo-arch duplicate of FIX-1) | High | Duplicate of FIX-1 | `repos.add`/`addRemote` return type |
|
||||
|
||||
## Iteration State
|
||||
|
||||
Current iteration: 1
|
||||
Last completed phase: Review
|
||||
Last completed phase: Validation
|
||||
Files fixed this iteration: []
|
||||
Pending fixes: 12 (FIX-1 through FIX-12)
|
||||
|
|
|
|||
|
|
@ -22,11 +22,33 @@ export function DiffCommentPopover({
|
|||
}: Props): React.JSX.Element {
|
||||
const [body, setBody] = useState('')
|
||||
const textareaRef = useRef<HTMLTextAreaElement | null>(null)
|
||||
const popoverRef = useRef<HTMLDivElement | null>(null)
|
||||
|
||||
useEffect(() => {
|
||||
textareaRef.current?.focus()
|
||||
}, [])
|
||||
|
||||
// Why: Monaco's editor area does not bubble a synthetic React click up to
|
||||
// the popover's onClick. Without a document-level mousedown listener, the
|
||||
// popover has no way to detect clicks outside its own bounds. We keep the
|
||||
// `onMouseDown={ev.stopPropagation()}` on the popover root so that this
|
||||
// listener sees outside-clicks only.
|
||||
useEffect(() => {
|
||||
const onDocumentMouseDown = (ev: MouseEvent): void => {
|
||||
if (!popoverRef.current) {
|
||||
return
|
||||
}
|
||||
if (popoverRef.current.contains(ev.target as Node)) {
|
||||
return
|
||||
}
|
||||
onCancel()
|
||||
}
|
||||
document.addEventListener('mousedown', onDocumentMouseDown)
|
||||
return () => {
|
||||
document.removeEventListener('mousedown', onDocumentMouseDown)
|
||||
}
|
||||
}, [onCancel])
|
||||
|
||||
const autoResize = (el: HTMLTextAreaElement): void => {
|
||||
el.style.height = 'auto'
|
||||
el.style.height = `${Math.min(el.scrollHeight, 240)}px`
|
||||
|
|
@ -42,6 +64,7 @@ export function DiffCommentPopover({
|
|||
|
||||
return (
|
||||
<div
|
||||
ref={popoverRef}
|
||||
className="orca-diff-comment-popover"
|
||||
style={{ top: `${top}px` }}
|
||||
onMouseDown={(ev) => ev.stopPropagation()}
|
||||
|
|
|
|||
|
|
@ -1,7 +1,8 @@
|
|||
import { useMemo, useState } from 'react'
|
||||
import { Trash2, MessageSquare, Terminal, Play, Clipboard } from 'lucide-react'
|
||||
import { Trash2, MessageSquare, FileCode, Play, Clipboard } from 'lucide-react'
|
||||
import { useAppStore } from '@/store'
|
||||
import { Button } from '@/components/ui/button'
|
||||
import { Dialog, DialogContent, DialogHeader, DialogTitle } from '@/components/ui/dialog'
|
||||
import type { OpenFile } from '@/store/slices/editor'
|
||||
import type { DiffComment } from '../../../../shared/types'
|
||||
import { formatDiffComments } from '@/lib/diff-comments-format'
|
||||
|
|
@ -41,17 +42,14 @@ function groupByFile(comments: DiffComment[]): Record<string, DiffComment[]> {
|
|||
return groups
|
||||
}
|
||||
|
||||
export function DiffCommentsTab({
|
||||
activeFile
|
||||
}: {
|
||||
activeFile: OpenFile
|
||||
}): React.JSX.Element {
|
||||
export function DiffCommentsTab({ activeFile }: { activeFile: OpenFile }): React.JSX.Element {
|
||||
const worktreeId = activeFile.worktreeId
|
||||
const comments = useAppStore((s) => s.getDiffComments(worktreeId))
|
||||
const deleteDiffComment = useAppStore((s) => s.deleteDiffComment)
|
||||
const clearDiffComments = useAppStore((s) => s.clearDiffComments)
|
||||
const getActiveTab = useAppStore((s) => s.getActiveTab)
|
||||
const [agentDialogOpen, setAgentDialogOpen] = useState(false)
|
||||
const [clearDialogOpen, setClearDialogOpen] = useState(false)
|
||||
const [pasteNotice, setPasteNotice] = useState<string | null>(null)
|
||||
|
||||
const groups = useMemo(() => groupByFile(comments), [comments])
|
||||
|
|
@ -111,9 +109,7 @@ export function DiffCommentsTab({
|
|||
if (comments.length === 0) {
|
||||
return
|
||||
}
|
||||
if (confirm(`Delete all ${comments.length} comments for this worktree?`)) {
|
||||
void clearDiffComments(worktreeId)
|
||||
}
|
||||
setClearDialogOpen(true)
|
||||
}}
|
||||
disabled={comments.length === 0}
|
||||
>
|
||||
|
|
@ -139,7 +135,7 @@ export function DiffCommentsTab({
|
|||
{fileEntries.map(([filePath, list]) => (
|
||||
<section key={filePath} className="px-3 py-2">
|
||||
<header className="mb-1.5 flex items-center gap-2 text-xs font-medium text-muted-foreground">
|
||||
<Terminal className="size-3" />
|
||||
<FileCode className="size-3" />
|
||||
<span className="truncate">{filePath}</span>
|
||||
<span className="tabular-nums">({list.length})</span>
|
||||
</header>
|
||||
|
|
@ -177,6 +173,32 @@ export function DiffCommentsTab({
|
|||
)}
|
||||
</div>
|
||||
|
||||
<Dialog open={clearDialogOpen} onOpenChange={setClearDialogOpen}>
|
||||
<DialogContent className="max-w-sm">
|
||||
<DialogHeader>
|
||||
<DialogTitle>Clear all diff comments?</DialogTitle>
|
||||
</DialogHeader>
|
||||
<div className="text-sm text-muted-foreground">
|
||||
This will permanently delete {comments.length} comment
|
||||
{comments.length === 1 ? '' : 's'} for this worktree. This cannot be undone.
|
||||
</div>
|
||||
<div className="flex justify-end gap-2">
|
||||
<Button variant="ghost" onClick={() => setClearDialogOpen(false)}>
|
||||
Cancel
|
||||
</Button>
|
||||
<Button
|
||||
variant="destructive"
|
||||
onClick={() => {
|
||||
setClearDialogOpen(false)
|
||||
void clearDiffComments(worktreeId)
|
||||
}}
|
||||
>
|
||||
Delete all
|
||||
</Button>
|
||||
</div>
|
||||
</DialogContent>
|
||||
</Dialog>
|
||||
|
||||
<DiffCommentsAgentChooserDialog
|
||||
open={agentDialogOpen}
|
||||
onOpenChange={setAgentDialogOpen}
|
||||
|
|
|
|||
|
|
@ -31,6 +31,13 @@ export function useDiffCommentDecorator({
|
|||
const hoverLineRef = useRef<number | null>(null)
|
||||
const viewZoneIdsRef = useRef<Map<string, string>>(new Map())
|
||||
const disposablesRef = useRef<IDisposable[]>([])
|
||||
// Why: cache each comment's rendered DOM node and last-applied body so the
|
||||
// view-zone effect can patch an existing card in place when only the body
|
||||
// changed, rather than removing and re-adding every zone on each render.
|
||||
// Rebuilding all zones caused visible flicker and wasted DOM work whenever
|
||||
// a single comment was added, edited, or deleted.
|
||||
const domNodesByCommentIdRef = useRef<Map<string, HTMLElement>>(new Map())
|
||||
const bodyByCommentIdRef = useRef<Map<string, string>>(new Map())
|
||||
// Why: stash the consumer callbacks in refs so the decorator effect's
|
||||
// cleanup does not run on every parent render. The parent passes inline
|
||||
// arrow functions; without this, each render would tear down and re-attach
|
||||
|
|
@ -116,6 +123,15 @@ export function useDiffCommentDecorator({
|
|||
disposablesRef.current = []
|
||||
plus.removeEventListener('click', handleClick)
|
||||
plus.remove()
|
||||
// Why: when the editor is swapped or torn down, its view zones go with
|
||||
// it. Clear the tracking Maps so a subsequent editor mount starts from
|
||||
// a known-empty state rather than trying to remove stale zone ids from
|
||||
// a dead editor. The diff effect below deliberately has no cleanup so
|
||||
// comment-only changes don't cause a full zone rebuild; this cleanup
|
||||
// is the single place we reset zone tracking.
|
||||
viewZoneIdsRef.current.clear()
|
||||
domNodesByCommentIdRef.current.clear()
|
||||
bodyByCommentIdRef.current.clear()
|
||||
}
|
||||
}, [editor])
|
||||
|
||||
|
|
@ -124,18 +140,31 @@ export function useDiffCommentDecorator({
|
|||
return
|
||||
}
|
||||
|
||||
const relevant = comments.filter(
|
||||
(c) => c.filePath === filePath && c.worktreeId === worktreeId
|
||||
)
|
||||
const relevant = comments.filter((c) => c.filePath === filePath && c.worktreeId === worktreeId)
|
||||
const relevantMap = new Map(relevant.map((c) => [c.id, c] as const))
|
||||
|
||||
const currentIds = viewZoneIdsRef.current
|
||||
editor.changeViewZones((accessor) => {
|
||||
for (const id of currentIds.values()) {
|
||||
accessor.removeZone(id)
|
||||
}
|
||||
currentIds.clear()
|
||||
const domNodesByCommentId = domNodesByCommentIdRef.current
|
||||
const bodyByCommentId = bodyByCommentIdRef.current
|
||||
|
||||
editor.changeViewZones((accessor) => {
|
||||
// Why: remove only the zones whose comments are gone. Rebuilding all
|
||||
// zones on every change caused flicker and dropped focus/selection in
|
||||
// adjacent UI; a diff-based pass keeps the untouched cards stable.
|
||||
for (const [commentId, zoneId] of currentIds) {
|
||||
if (!relevantMap.has(commentId)) {
|
||||
accessor.removeZone(zoneId)
|
||||
currentIds.delete(commentId)
|
||||
domNodesByCommentId.delete(commentId)
|
||||
bodyByCommentId.delete(commentId)
|
||||
}
|
||||
}
|
||||
|
||||
// Add zones for newly-added comments.
|
||||
for (const c of relevant) {
|
||||
if (currentIds.has(c.id)) {
|
||||
continue
|
||||
}
|
||||
const dom = document.createElement('div')
|
||||
dom.className = 'orca-diff-comment-inline'
|
||||
|
||||
|
|
@ -169,23 +198,50 @@ export function useDiffCommentDecorator({
|
|||
card.appendChild(body)
|
||||
dom.appendChild(card)
|
||||
|
||||
// Why: estimate height from line count so the zone is close to the
|
||||
// right size on first paint. Monaco sets heightInPx authoritatively at
|
||||
// insertion and does not re-measure the DOM node, so a fixed 72 clipped
|
||||
// multi-line bodies. The per-line estimate handles typical review
|
||||
// notes without needing a post-attach measurement pass.
|
||||
const lineCount = c.body.split('\n').length
|
||||
const heightInPx = Math.max(56, 28 + lineCount * 18)
|
||||
|
||||
const id = accessor.addZone({
|
||||
afterLineNumber: c.lineNumber,
|
||||
heightInPx: 72,
|
||||
heightInPx,
|
||||
domNode: dom,
|
||||
suppressMouseDown: true
|
||||
})
|
||||
currentIds.set(c.id, id)
|
||||
domNodesByCommentId.set(c.id, dom)
|
||||
bodyByCommentId.set(c.id, c.body)
|
||||
}
|
||||
|
||||
// Patch existing zones whose body text changed in place — avoids the
|
||||
// full rebuild that would otherwise flicker the card.
|
||||
for (const c of relevant) {
|
||||
if (!currentIds.has(c.id)) {
|
||||
continue
|
||||
}
|
||||
const previousBody = bodyByCommentId.get(c.id)
|
||||
if (previousBody === c.body) {
|
||||
continue
|
||||
}
|
||||
const dom = domNodesByCommentId.get(c.id)
|
||||
if (!dom) {
|
||||
continue
|
||||
}
|
||||
const bodyEl = dom.querySelector('.orca-diff-comment-body')
|
||||
if (bodyEl) {
|
||||
bodyEl.textContent = c.body
|
||||
}
|
||||
bodyByCommentId.set(c.id, c.body)
|
||||
}
|
||||
})
|
||||
|
||||
return () => {
|
||||
editor.changeViewZones((accessor) => {
|
||||
for (const id of currentIds.values()) {
|
||||
accessor.removeZone(id)
|
||||
}
|
||||
})
|
||||
currentIds.clear()
|
||||
}
|
||||
// Why: intentionally no cleanup. React would run cleanup BEFORE the next
|
||||
// effect body on every `comments` identity change, wiping all zones and
|
||||
// forcing a full rebuild — exactly the flicker this diff-based pass is
|
||||
// meant to avoid. Zone teardown lives in the editor-scoped effect above,
|
||||
// which only fires when the editor itself is replaced/unmounted.
|
||||
}, [editor, filePath, worktreeId, comments])
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1,4 +1,4 @@
|
|||
import React, { lazy, useMemo, useState, type MutableRefObject } from 'react'
|
||||
import React, { lazy, useEffect, useMemo, useState, type MutableRefObject } from 'react'
|
||||
import { LazySection } from './LazySection'
|
||||
import { ChevronDown, ChevronRight, ExternalLink } from 'lucide-react'
|
||||
import { DiffEditor, type DiffOnMount } from '@monaco-editor/react'
|
||||
|
|
@ -130,8 +130,7 @@ export function DiffSectionItem({
|
|||
editorFontZoomLevel
|
||||
)
|
||||
|
||||
const [modifiedEditor, setModifiedEditor] =
|
||||
useState<monacoEditor.ICodeEditor | null>(null)
|
||||
const [modifiedEditor, setModifiedEditor] = useState<monacoEditor.ICodeEditor | null>(null)
|
||||
const [popover, setPopover] = useState<{ lineNumber: number; top: number } | null>(null)
|
||||
|
||||
useDiffCommentDecorator({
|
||||
|
|
@ -143,6 +142,27 @@ export function DiffSectionItem({
|
|||
onDeleteComment: (id) => void deleteDiffComment(worktreeId, id)
|
||||
})
|
||||
|
||||
useEffect(() => {
|
||||
if (!modifiedEditor || !popover) {
|
||||
return
|
||||
}
|
||||
const update = (): void => {
|
||||
const top =
|
||||
modifiedEditor.getTopForLineNumber(popover.lineNumber) - modifiedEditor.getScrollTop()
|
||||
setPopover((prev) => (prev ? { ...prev, top } : prev))
|
||||
}
|
||||
const scrollSub = modifiedEditor.onDidScrollChange(update)
|
||||
const contentSub = modifiedEditor.onDidContentSizeChange(update)
|
||||
return () => {
|
||||
scrollSub.dispose()
|
||||
contentSub.dispose()
|
||||
}
|
||||
// Why: depend on popover.lineNumber (not the whole popover object) so the
|
||||
// effect doesn't re-subscribe on every top update it dispatches. The guard
|
||||
// on `popover` above handles the popover-closed case.
|
||||
// eslint-disable-next-line react-hooks/exhaustive-deps
|
||||
}, [modifiedEditor, popover?.lineNumber])
|
||||
|
||||
const handleSubmitComment = (body: string): void => {
|
||||
if (!popover) {
|
||||
return
|
||||
|
|
|
|||
|
|
@ -3,14 +3,18 @@
|
|||
// because the body is surfaced inside literal quotes. Escape backslashes
|
||||
// first so that `\"` in user input does not decay into an unescaped quote.
|
||||
export function formatDiffComment(c) {
|
||||
const escaped = c.body.replace(/\\/g, '\\\\').replace(/"/g, '\\"');
|
||||
return [
|
||||
`File: ${c.filePath}`,
|
||||
`Line: ${c.lineNumber}`,
|
||||
`User comment: "${escaped}"`,
|
||||
`Comment metadata: This comment was left on the modified branch.`
|
||||
].join('\n');
|
||||
const escaped = c.body
|
||||
.replace(/\\/g, '\\\\')
|
||||
.replace(/"/g, '\\"')
|
||||
.replace(/\r/g, '\\r')
|
||||
.replace(/\n/g, '\\n')
|
||||
return [
|
||||
`File: ${c.filePath}`,
|
||||
`Line: ${c.lineNumber}`,
|
||||
`User comment: "${escaped}"`,
|
||||
`Comment metadata: This comment was left on the modified branch.`
|
||||
].join('\n')
|
||||
}
|
||||
export function formatDiffComments(comments) {
|
||||
return comments.map(formatDiffComment).join('\n\n');
|
||||
return comments.map(formatDiffComment).join('\n\n')
|
||||
}
|
||||
|
|
|
|||
|
|
@ -37,6 +37,12 @@ describe('formatDiffComment', () => {
|
|||
const out = formatDiffComment(makeComment({ body: 'path\\to\\"thing"' }))
|
||||
expect(out).toContain('User comment: "path\\\\to\\\\\\"thing\\""')
|
||||
})
|
||||
|
||||
it('escapes newlines so the body cannot break out of the fixed 4-line structure', () => {
|
||||
const out = formatDiffComment(makeComment({ body: 'first\nsecond' }))
|
||||
expect(out).toContain('User comment: "first\\nsecond"')
|
||||
expect(out.split('\n')).toHaveLength(4)
|
||||
})
|
||||
})
|
||||
|
||||
describe('formatDiffComments', () => {
|
||||
|
|
@ -45,9 +51,19 @@ describe('formatDiffComments', () => {
|
|||
makeComment({ id: 'a', lineNumber: 1, body: 'first' }),
|
||||
makeComment({ id: 'b', lineNumber: 2, body: 'second' })
|
||||
])
|
||||
expect(out).toContain('Line: 1')
|
||||
expect(out).toContain('Line: 2')
|
||||
expect(out.split('\n\n')).toHaveLength(2)
|
||||
expect(out).toBe(
|
||||
[
|
||||
'File: src/app.ts',
|
||||
'Line: 1',
|
||||
'User comment: "first"',
|
||||
'Comment metadata: This comment was left on the modified branch.',
|
||||
'',
|
||||
'File: src/app.ts',
|
||||
'Line: 2',
|
||||
'User comment: "second"',
|
||||
'Comment metadata: This comment was left on the modified branch.'
|
||||
].join('\n')
|
||||
)
|
||||
})
|
||||
|
||||
it('returns an empty string for an empty input', () => {
|
||||
|
|
|
|||
|
|
@ -5,7 +5,11 @@ import type { DiffComment } from '../../../shared/types'
|
|||
// because the body is surfaced inside literal quotes. Escape backslashes
|
||||
// first so that `\"` in user input does not decay into an unescaped quote.
|
||||
export function formatDiffComment(c: DiffComment): string {
|
||||
const escaped = c.body.replace(/\\/g, '\\\\').replace(/"/g, '\\"')
|
||||
const escaped = c.body
|
||||
.replace(/\\/g, '\\\\')
|
||||
.replace(/"/g, '\\"')
|
||||
.replace(/\r/g, '\\r')
|
||||
.replace(/\n/g, '\\n')
|
||||
return [
|
||||
`File: ${c.filePath}`,
|
||||
`Line: ${c.lineNumber}`,
|
||||
|
|
|
|||
|
|
@ -5,9 +5,7 @@ import { findWorktreeById, getRepoIdFromWorktreeId } from './worktree-helpers'
|
|||
|
||||
export type DiffCommentsSlice = {
|
||||
getDiffComments: (worktreeId: string, filePath?: string) => DiffComment[]
|
||||
addDiffComment: (
|
||||
input: Omit<DiffComment, 'id' | 'createdAt'>
|
||||
) => Promise<DiffComment | null>
|
||||
addDiffComment: (input: Omit<DiffComment, 'id' | 'createdAt'>) => Promise<DiffComment | null>
|
||||
deleteDiffComment: (worktreeId: string, commentId: string) => Promise<void>
|
||||
clearDiffComments: (worktreeId: string) => Promise<void>
|
||||
}
|
||||
|
|
@ -19,6 +17,11 @@ function generateId(): string {
|
|||
return `${Date.now().toString(36)}-${Math.random().toString(36).slice(2, 10)}`
|
||||
}
|
||||
|
||||
// Why: return a stable reference when no comments exist so selectors don't
|
||||
// produce a fresh `[]` on every store update. A new array identity would
|
||||
// trigger re-renders in any consumer using referential equality.
|
||||
const EMPTY_COMMENTS: DiffComment[] = []
|
||||
|
||||
async function persist(worktreeId: string, diffComments: DiffComment[]): Promise<void> {
|
||||
await window.api.worktrees.updateMeta({
|
||||
worktreeId,
|
||||
|
|
@ -66,10 +69,18 @@ function mutateComments(
|
|||
// Why: if the IPC write fails, the optimistic renderer state drifts from
|
||||
// disk. Roll back so what the user sees always matches what will survive a
|
||||
// reload.
|
||||
//
|
||||
// Identity guard: we only revert when the current diffComments array is
|
||||
// strictly identical (===) to the `next` array this mutation produced. If
|
||||
// another mutation has already landed (e.g. Add B succeeded while Add A was
|
||||
// still in flight), it will have replaced the array with a different
|
||||
// identity. In that case we must leave the newer state alone — rolling back
|
||||
// to our stale `previous` would erase B along with the failed A.
|
||||
function rollback(
|
||||
set: Parameters<StateCreator<AppState, [], [], DiffCommentsSlice>>[0],
|
||||
worktreeId: string,
|
||||
previous: DiffComment[] | undefined
|
||||
previous: DiffComment[] | undefined,
|
||||
expectedCurrent: DiffComment[]
|
||||
): void {
|
||||
const repoId = getRepoIdFromWorktreeId(worktreeId)
|
||||
set((s) => {
|
||||
|
|
@ -77,6 +88,13 @@ function rollback(
|
|||
if (!repoList) {
|
||||
return {}
|
||||
}
|
||||
const target = repoList.find((w) => w.id === worktreeId)
|
||||
// Why: only roll back if no other mutation landed since this one. If a
|
||||
// later write already replaced the comments array with a different
|
||||
// identity, our stale `previous` would erase that newer state.
|
||||
if (target?.diffComments !== expectedCurrent) {
|
||||
return {}
|
||||
}
|
||||
const nextList: Worktree[] = repoList.map((w) =>
|
||||
w.id === worktreeId ? { ...w, diffComments: previous } : w
|
||||
)
|
||||
|
|
@ -90,7 +108,7 @@ export const createDiffCommentsSlice: StateCreator<AppState, [], [], DiffComment
|
|||
) => ({
|
||||
getDiffComments: (worktreeId, filePath) => {
|
||||
const worktree = findWorktreeById(get().worktreesByRepo, worktreeId)
|
||||
const all = worktree?.diffComments ?? []
|
||||
const all = worktree?.diffComments ?? EMPTY_COMMENTS
|
||||
if (!filePath) {
|
||||
return all
|
||||
}
|
||||
|
|
@ -103,10 +121,7 @@ export const createDiffCommentsSlice: StateCreator<AppState, [], [], DiffComment
|
|||
id: generateId(),
|
||||
createdAt: Date.now()
|
||||
}
|
||||
const result = mutateComments(set, input.worktreeId, (existing) => [
|
||||
...existing,
|
||||
comment
|
||||
])
|
||||
const result = mutateComments(set, input.worktreeId, (existing) => [...existing, comment])
|
||||
if (!result) {
|
||||
return null
|
||||
}
|
||||
|
|
@ -115,7 +130,7 @@ export const createDiffCommentsSlice: StateCreator<AppState, [], [], DiffComment
|
|||
return comment
|
||||
} catch (err) {
|
||||
console.error('Failed to persist diff comments:', err)
|
||||
rollback(set, input.worktreeId, result.previous)
|
||||
rollback(set, input.worktreeId, result.previous, result.next)
|
||||
return null
|
||||
}
|
||||
},
|
||||
|
|
@ -132,7 +147,7 @@ export const createDiffCommentsSlice: StateCreator<AppState, [], [], DiffComment
|
|||
await persist(worktreeId, result.next)
|
||||
} catch (err) {
|
||||
console.error('Failed to persist diff comments:', err)
|
||||
rollback(set, worktreeId, result.previous)
|
||||
rollback(set, worktreeId, result.previous, result.next)
|
||||
}
|
||||
},
|
||||
|
||||
|
|
@ -147,7 +162,7 @@ export const createDiffCommentsSlice: StateCreator<AppState, [], [], DiffComment
|
|||
await persist(worktreeId, result.next)
|
||||
} catch (err) {
|
||||
console.error('Failed to persist diff comments:', err)
|
||||
rollback(set, worktreeId, result.previous)
|
||||
rollback(set, worktreeId, result.previous, result.next)
|
||||
}
|
||||
}
|
||||
})
|
||||
|
|
|
|||
Loading…
Reference in a new issue