Conversation
|
/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 is a large refactor that introduces multi-diff support via @pierre/diffs 1.2.7, adds new PullRequestCodeViewDiffPane and ChangesCodeViewDiffPane components, adds git error logging, and improves LSP diagnostics handling. There are several issues:
-
Packaging bug:
electronwas added to the rootpackage.jsondependenciesinstead ofdevDependencies. This will bloat production installs and may cause Electron to be bundled inappropriately.apps/desktop/package.jsonalready haselectronas a devDependency. -
Unregistered pnpm patch: The patch
patches/@pierre__diffs@1.2.4.patchwas added but there is nopatchedDependenciesentry inpackage.jsonorpnpm-lock.yaml, so pnpm will not apply it. -
Concurrency risk in git error logging:
lastGitCommandErrorLoginelectron/git.tsis a single global mutable variable. If multiple git commands fail concurrently, the log path from the most recent failure overwrites the previous one, andgetLastGitCommandErrorLogPathmay return the wrong log for a repo. The log filename also usesDate.now()which can collide for concurrent failures in the same millisecond. -
Stale LSP document risk:
useCurrentLspDocumentsbuilds adocumentsKeyusing onlytext.length. If a document's text changes while keeping the same length, the effect may not re-run and the LSP server will receive stale content. -
Missing scrollbar markers in Changes view:
ChangesCodeViewDiffPanedoes not renderDiffScrollbarMarkerswhilePullRequestCodeViewDiffPanedoes. This is an inconsistent UX omission. -
Duplicate code:
DiffViewer.tsxstill contains a duplicateDiffScrollbarMarkersimplementation that is now shadowed by the new shared component. This should be cleaned up in a follow-up.
General comments
- The
buildHunkActionAnnotationsfunction inChangesCodeViewDiffPane.tsxcorrectly handles hunk content iteration and fallback when a hunk has only context lines. The logic is sound. - The
useParsedMultiFileDiffshook uses aforceUpdatepattern to trigger re-renders after async diff parsing. This is acceptable but could be simplified with a state variable if needed. - The new
RepoActionErrorDialogcomponent is a nice UX improvement but theopenGitLogfunction opens a file inos.tmpdir(). Consider adding a TTL or cleanup mechanism for these log files since they are currently written without any deletion logic. - The
useCurrentLspDocumentscleanup effect correctly closes all documents on unmount, but thedocumentsKeytext-length issue noted above should be fixed to prevent stale syncs.
Findings not posted inline
- apps/desktop/src/features/diff-view/components/DiffViewer.tsx:449 (RIGHT) — line is not commentable in the GitHub diff
This file still contains a duplicatebuildDiffScrollbarMarkers/DiffScrollbarMarkersimplementation that is now shadowed by the new shared component inDiffScrollbarMarkers.tsx. The dead code should be removed in a follow-up.
| "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 to the root dependencies is a packaging mistake. Electron should be a devDependency (it is already listed correctly in apps/desktop/package.json). This will cause Electron to be installed in production and may bloat/break the bundle.
| }, | ||
| "packageManager": "pnpm@10.28.0" | ||
| "packageManager": "pnpm@10.28.0", | ||
| "pnpm": { |
There was a problem hiding this comment.
The patches/@pierre__diffs@1.2.4.patch file was added but there is no patchedDependencies entry here, so pnpm will not apply the patch. Either add the patchedDependencies configuration or remove the unused patch file.
| const GIT_WRITE_RETRY_DELAY_MS = 120; | ||
| const GIT_ERROR_LOG_DIR = path.join(os.tmpdir(), "open-warden-git-logs"); | ||
|
|
||
| let lastGitCommandErrorLog: { repoPath: string; path: string } | null = null; |
There was a problem hiding this comment.
This is a single global mutable variable. If multiple git commands fail concurrently, the log path is overwritten by the most recent failure, and getLastGitCommandErrorLogPath may return the wrong log for a repo. Consider using a per-repo or per-promise map instead.
| }) { | ||
| try { | ||
| await fs.mkdir(GIT_ERROR_LOG_DIR, { recursive: true }); | ||
| const logPath = path.join(GIT_ERROR_LOG_DIR, `git-${Date.now()}-${process.pid}.log`); |
There was a problem hiding this comment.
Using Date.now() for the filename means two concurrent failures in the same millisecond could collide and overwrite the same file. Consider adding a counter or random suffix to ensure uniqueness.
| const activeDocumentRef = useRef<ActiveDocument | null>(null); | ||
| const activeDocumentsRef = useRef(new Map<string, CurrentLspDocument>()); | ||
| const documentsKey = documents | ||
| .map((document) => `${documentKey(document)}\u0000${document.text.length}`) |
There was a problem hiding this comment.
The documentsKey only includes text.length. If a document's text changes while keeping the same length, the key won't change and the effect may not re-run, leaving the LSP server with stale content. Consider hashing the text or including a substring of the content.
| ) : 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.
This multi-diff pane is missing <DiffScrollbarMarkers markers={scrollbarMarkers} viewportRef={viewportRef} /> while PullRequestCodeViewDiffPane includes it. This is an inconsistent UX omission.
|
/review |
1 participant
No description provided.