Conversation
|
/review |
1 similar comment
|
/review |
![orc-review[bot]](https://avatars.githubusercontent.com/u/32248437?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR upgrades @pierre/diffs from 1.1.15 to 1.2.7 and introduces a major refactor: multi-file virtualized diff panes for both Pull Requests (PullRequestCodeViewDiffPane) and Changes (ChangesCodeViewDiffPane), replacing the old single-file DiffWorkspace. It also adds git error logging with a RepoActionErrorDialog, refactors LSP document handling to support multiple files (useCurrentLspDocuments), adds line-level diagnostic highlighting, and introduces diff scrollbar markers.
The architecture is sound and the changes are well-structured. Tests pass. However, there are a few critical and high issues that need attention before merge:
-
Critical – Orphaned patch file:
patches/@pierre__diffs@1.2.4.patchis added but there is nopnpm.patchedDependenciesconfiguration inpackage.json(orpnpm-workspace.yaml), so pnpm will not apply it. Additionally, the patch targets version 1.2.4 while the installed package is 1.2.7. The patch contains aforceRenderOverridefix that is essential for hunk expansion in the virtualized diff; without it, the UI may fail to re-render after expanding hunks. -
High – Missing scrollbar markers in Changes pane:
ChangesCodeViewDiffPanedoes not include<DiffScrollbarMarkers>whilePullRequestCodeViewDiffPanedoes. This is a UX inconsistency. -
High – Git error log accumulation:
writeGitCommandErrorLogwrites toos.tmpdir()/open-warden-git-logs/but never cleans up old files. Over time this directory will grow indefinitely. -
Medium – Incomplete git log integration: Only
commitStagedusestoGitErrorResultwhich captures thelogPath. Other git endpoints (stageFile,unstageFile,discardFile,updateIndexFileContents, etc.) usetoErrorResultwithoutlogPath, so the "Open Git Log" button only appears for commit failures. -
Medium – Root-level
electrondependency:electronis added as a rootdependencyinstead ofdevDependency. Since Electron is a build/runtime tool for the desktop app, this is unusual and could cause unwanted package resolution in non-desktop workspaces. -
Low – Potential scroll jump:
PullRequestCodeViewDiffPaneincludesparsedDiffs.lengthin the scrolluseEffectdependency array, which may cause the view to re-scroll when background diffs finish loading. -
Low – No tests for new components: The PR adds ~700-line components (
PullRequestCodeViewDiffPane,ChangesCodeViewDiffPane) and a new dialog (RepoActionErrorDialog) without unit tests. The existing tests pass but do not cover the new multi-diff logic.
General comments
- The
DiffWorkspaceandDiffViewercomponents are still present and used for single-file diff views (e.g.,GeneralFileViewerusesDiffWorkspaceindirectly?). The duplication of[data-line][data-lsp-diagnostic-line]CSS betweenDiffViewer.tsxanduseMultiDiffCodeViewOptions.tsis intentional but should be consolidated in a follow-up to avoid drift. - The
useParsedMultiFileDiffshook usesqueueMicrotaskto force a re-render after cache population. This is safe but consider whetherstartTransitionorrequestAnimationFramewould be more appropriate for React 19 concurrent features. - The
buildRepoActionErrorfunction inactions.tshas a hardcodedmessageforcommitactions. This is user-facing copy and is fine, but consider extracting it to a constants file for easier localization in the future.
Findings not posted inline
- apps/desktop/src/features/diff-view/components/DiffViewer.tsx:503 (RIGHT) — line is not commentable in the GitHub diff
This localDiffScrollbarMarkersfunction has the same name as the exported component inDiffScrollbarMarkers.tsx, which is confusing. Consider renaming the local one to avoid collisions and clarify intent (e.g.,DiffViewerScrollbarMarkers).
| "format:check": "pnpm fmt:check", | ||
| "check": "pnpm lint && pnpm typecheck && pnpm test && pnpm fmt:check" | ||
| }, | ||
| "dependencies": { |
There was a problem hiding this comment.
Adding electron as a root-level dependency is unusual. Electron is a build/runtime tool for the desktop app and should typically be a devDependency. This may cause unwanted package resolution in non-desktop workspaces.
| @@ -0,0 +1,21 @@ | |||
| diff --git a/dist/components/VirtualizedFileDiff.js b/dist/components/VirtualizedFileDiff.js | |||
There was a problem hiding this comment.
This patch is orphaned: there is no pnpm.patchedDependencies configuration in package.json or pnpm-workspace.yaml, so pnpm will not apply it. Additionally, the patch targets @pierre/diffs@1.2.4 while the workspace installs 1.2.7. The forceRenderOverride fix inside is critical for hunk expansion in the virtualized diff. Without it, expanding hunks may fail to re-render.
| ) : parsedDiffs.length === 0 ? ( | ||
| <div className="text-muted-foreground p-3 text-sm">No renderable diff content.</div> | ||
| ) : null} | ||
| <div className="relative min-h-0 min-w-0 flex-1"> |
There was a problem hiding this comment.
ChangesCodeViewDiffPane is missing <DiffScrollbarMarkers markers={scrollbarMarkers} viewportRef={viewportRef} /> inside the scroll container. The PR diff pane (PullRequestCodeViewDiffPane) includes it, so this is a UX inconsistency.
| const GIT_TIMEOUT_MS = 30_000; | ||
| const GIT_WRITE_RETRY_COUNT = 3; | ||
| const GIT_WRITE_RETRY_DELAY_MS = 120; | ||
| const GIT_ERROR_LOG_DIR = path.join(os.tmpdir(), "open-warden-git-logs"); |
There was a problem hiding this comment.
GIT_ERROR_LOG_DIR is created under os.tmpdir() but old log files are never cleaned up. Over time this directory will accumulate indefinitely. Consider adding a cleanup mechanism (e.g., deleting files older than N days or on app startup).
| return { data: await commitStaged(repoPath, message) }; | ||
| } catch (error) { | ||
| return { error: toErrorResult(error) }; | ||
| return { error: await toGitErrorResult(error, repoPath) }; |
There was a problem hiding this comment.
Only commitStaged uses toGitErrorResult which captures the logPath. All other git endpoints (stageFile, unstageFile, discardFile, updateIndexFileContents, etc.) use toErrorResult without a logPath, so the "Open Git Log" button only works for commit failures. Consider using toGitErrorResult for all git mutation endpoints so users can inspect logs for any git failure.
| align: "start", | ||
| behavior: "instant", | ||
| }); | ||
| }, [activeItemId, codeViewKey, focusedLineNumber, parsedDiffs.length]); |
There was a problem hiding this comment.
Including parsedDiffs.length in the scroll useEffect dependency array means that when a background diff finishes loading and the array grows, the effect re-runs and scrolls back to the active item. This could cause unwanted scroll jumps if the user has already scrolled elsewhere.
| markers: DiffScrollbarMarker[]; | ||
| viewportRef: RefObject<HTMLElement | null>; | ||
| }) { | ||
| const scrollToPercent = useCallback( |
There was a problem hiding this comment.
The scrollToPercent callback computes maxScrollTop = scrollHeight - clientHeight. If scrollHeight is less than or equal to clientHeight, maxScrollTop becomes 0 or negative and scrollTop is set to a negative value. While browsers clamp this, explicitly guarding with Math.max(0, ...) would be safer.
1 participant
No description provided.