Pierre diffs update by ShpetimA · Pull Request #23 · ShpetimA/open-warden

ShpetimA · 2026-06-16T20:18:29Z

No description provided.

ShpetimA · 2026-06-16T20:18:35Z

/review

ShpetimA · 2026-06-16T20:18:58Z

/review

ShpetimA · 2026-06-16T20:36:48Z

/review

ShpetimA · 2026-06-16T20:38:15Z

/review

ShpetimA · 2026-06-16T20:41:53Z

/review

orc-review

This PR upgrades @pierre/diffs from 1.1.15 to 1.2.7 and introduces a major architectural shift from single-file diff views to multi-file CodeView diff panes for Changes, PR review, and PR files screens. It also adds git error logging, LSP multi-document support, and improved repo action error dialogs.

I found several issues worth addressing:

Concurrency risk in git error logging: lastGitCommandErrorLog is a module-level singleton. If multiple git commands fail concurrently (or if one fails while another is awaiting the log path), the stored log path can be overwritten, causing the error dialog to point to the wrong log file.
Patch version mismatch: The patch file patches/@pierre__diffs@1.2.4.patch targets version 1.2.4, but the installed version is 1.2.7. Without explicit pnpm.patchedDependencies configuration, this patch is likely not applied, meaning the forceRenderOverride fix is missing.
Performance regression from eager fetching: The new PullRequestCodeViewDiffPane and ChangesCodeViewDiffPane subscribe to file-version queries for all files simultaneously, rather than just the active file. For large PRs or snapshots with many changes, this creates numerous concurrent RTK Query subscriptions and refetches all of them on window focus/reconnect.
Security concern: Git error logs are written to a predictable world-readable temp directory (os.tmpdir()) and may contain sensitive repository paths or credentials from stderr/stdout.
Missing tests: Several large new files (695–756 line components, complex hooks) have no test coverage.
Minor: Root package.json adds electron as a production dependency, which is atypical for a monorepo workspace root.

General comments

Missing test coverage: The PR adds several large, complex new modules with zero tests:
PullRequestCodeViewDiffPane.tsx (695 lines)
ChangesCodeViewDiffPane.tsx (756 lines)
DiffScrollbarMarkers.tsx
useMultiDiffCodeViewOptions.ts
useMultiDiffDiagnostics.ts
RepoActionErrorDialog.tsx

At minimum, useMultiDiffCodeViewOptions.ts and useMultiDiffDiagnostics.ts deserve unit tests since they manage refs, caches, and DOM decoration logic that is easy to regress.

API contract note: The old useCurrentLspDocument hook was renamed to useCurrentLspDocuments and its signature changed from (repoPath, relPath, text) to ([{repoPath, relPath, text}]). Callers in GeneralFileViewer.tsx were updated, but verify there are no other consumers in the codebase that still import the old hook.

Behavioral change in diff fetching: The switch from on-demand single-file fetching to all-files eager fetching is a significant architectural change. While it may improve perceived performance for small PRs, it risks degrading performance for large PRs (100+ files) due to:

Memory pressure from holding all file versions in Redux cache
IPC/main-thread contention from concurrent git operations
Slower window-focus recovery due to mass refetching

Consider adding a threshold or virtualization strategy to limit active subscriptions to visible/nearby files.

Findings not posted inline

apps/desktop/electron/git.ts:44 (RIGHT) — line is not commentable in the GitHub diff
lastGitCommandErrorLog is a module-level singleton. If two git commands fail concurrently (or interleave during the async getLastGitCommandErrorLogPath IPC call), the second failure overwrites the first before the caller can read it. Consider scoping the error log by repoPath+timestamp or returning the logPath directly in the thrown error so callers don't need a second round-trip.
apps/desktop/electron/git.ts:42 (RIGHT) — line is not commentable in the GitHub diff
Writing error logs to os.tmpdir() with a predictable directory name can leak sensitive git output (repo paths, file names, or credentials from hooks) on multi-user systems where /tmp is world-readable. Consider using a user-private directory (e.g., inside the app's user data path) and restricting file permissions.
apps/desktop/src/features/diff-view/hooks/useMultiDiffCodeViewOptions.ts:1145 (RIGHT) — line is not commentable in the GitHub diff
queueMicrotask(() => forceUpdate()) can fire after the component unmounts. While React 19 generally handles this gracefully, it is safer to guard with a mounted ref or use a batched state update inside the existing effect cleanup.
apps/desktop/src/features/pull-requests/components/PullRequestCodeViewDiffPane.tsx:2431 (RIGHT) — line is not commentable in the GitHub diff
useEnsureBranchFileVersionQueries subscribes to all targets at once. For large PRs this creates many concurrent RTK Query subscriptions with refetchOnFocus: true, causing a burst of IPC requests on every window focus. The previous architecture fetched only the active file. Consider limiting eager fetching or paginating the target list.
apps/desktop/src/features/source-control/components/ChangesCodeViewDiffPane.tsx:3730 (RIGHT) — line is not commentable in the GitHub diff
Same eager-fetching concern: useEnsureFileVersionQueries subscribes to queries for every changed file in the snapshot. In repos with hundreds of changes, this will create excessive concurrent subscriptions and refetch them all on focus/reconnect.

orc-review · 2026-06-16T20:46:08Z

@@ -22,10 +25,18 @@
    "format:check": "pnpm fmt:check",
    "check": "pnpm lint && pnpm typecheck && pnpm test && pnpm fmt:check"
  },


Adding electron as a root dependencies entry is unusual for a monorepo. It forces every workspace package to resolve electron and can bloat non-desktop packages. It should typically remain a devDependency or live only in apps/desktop/package.json.

orc-review · 2026-06-16T20:46:08Z

@@ -0,0 +1,21 @@
+diff --git a/dist/components/VirtualizedFileDiff.js b/dist/components/VirtualizedFileDiff.js


This patch targets @pierre/diffs@1.2.4, but apps/desktop/package.json installs 1.2.7. Unless pnpm.patchedDependencies is explicitly configured (not visible in the diff), pnpm will not apply this patch to the newer version, so the forceRenderOverride fix will be missing at runtime.

ShpetimA · 2026-06-16T20:49:43Z

/review

ShpetimA · 2026-06-16T21:02:52Z

/review

ShpetimA · 2026-06-16T21:03:02Z

/review

ShpetimA · 2026-06-16T21:27:55Z

/review

ShpetimA · 2026-06-17T09:18:40Z

/review

ShpetimA · 2026-06-17T16:56:51Z

/review

orc-review

This PR upgrades @pierre/diffs from 1.1.15 to 1.2.7 and introduces a multi-diff code view for PR and changes screens, replacing the single-file DiffWorkspace approach. It adds git error logging, a RepoActionErrorDialog, multi-file LSP diagnostics, and refactors LSP document syncing to support multiple files simultaneously.

Issues found:

Git error log global singleton – lastGitCommandErrorLog is a module-level variable. Concurrent or rapid git failures from different repos/commands overwrite it, causing getLastGitCommandErrorLogPath to potentially return the wrong log (or a log from a different repo). This is a concurrency risk.
Git error log temp directory accumulation – Failed git command logs are written to os.tmpdir() with no cleanup. Logs contain full stderr/stdout which may include sensitive data. Over time, this leaks data and accumulates disk usage.
Unnecessary scroll jumps in PR diff pane – PullRequestCodeViewDiffPane includes parsedDiffs.length in its scroll useEffect deps. When new diffs load incrementally, the effect re-runs and re-scrolls to the active item, causing disruptive scroll jumps.
DiffScrollbarMarkers code duplication – DiffScrollbarMarkers and buildDiffScrollbarMarkers are duplicated in both DiffViewer.tsx and the new DiffScrollbarMarkers.tsx. They have already diverged slightly (e.g., hover:w-4 vs hover:w-3). This is a maintenance risk.
LSP documents key uses text length – useCurrentLspDocuments derives documentsKey from document.text.length. If a file is edited and the character count happens to stay the same, the key won't change. While documents is also in the effect deps today, the key is misleading and could cause missed re-syncs if the deps are refactored.
Accessibility gap in scrollbar markers – The diff scrollbar marker buttons only have onPointerDown. Keyboard users cannot activate them with Enter/Space.
Missing tests – None of the new major components (ChangesCodeViewDiffPane, PullRequestCodeViewDiffPane, RepoActionErrorDialog, DiffScrollbarMarkers, useMultiDiffDiagnostics, useMultiDiffCodeViewOptions) have tests. The PR only adds one test for LspSymbolPeek positioning.

General comments

Missing tests: The PR introduces several large new components and hooks (ChangesCodeViewDiffPane, PullRequestCodeViewDiffPane, RepoActionErrorDialog, DiffScrollbarMarkers, useMultiDiffDiagnostics, useMultiDiffCodeViewOptions, useCurrentLspDocuments) but does not include any tests for them. The only test added is for LspSymbolPeek positioning. Consider adding at least unit tests for the scrollbar marker math, the error log path extraction, and the LSP document sync logic.
Git error log cleanup: writeGitCommandErrorLog creates a new log file on every failed git command but never deletes old files. The lastGitCommandErrorLog only stores the most recent path, so previous files are orphaned. Consider adding a cleanup step that deletes the log file after a timeout or on the next successful git command.
Code duplication: DiffScrollbarMarkers and buildDiffScrollbarMarkers exist in both DiffViewer.tsx and DiffScrollbarMarkers.tsx. The old inline version should be removed and DiffViewer.tsx should import from the new shared module to avoid drift.

Findings not posted inline

apps/desktop/electron/git.ts:44 (RIGHT) — line is not commentable in the GitHub diff
lastGitCommandErrorLog is a global module-level variable. If two git commands fail concurrently (or in quick succession from different repos), the last one wins. This means getLastGitCommandErrorLogPath can return a log from a different repo or a more recent unrelated command. Consider making the log path part of the error object and avoiding the global singleton.
apps/desktop/electron/git.ts:42 (RIGHT) — line is not commentable in the GitHub diff
Logs are written to os.tmpdir() with no cleanup. On shared systems, this directory may be readable by other users. The logs contain full git stderr/stdout which may include sensitive data (e.g., file paths, error messages). Consider using a user-specific temp directory and periodically cleaning up old logs.
apps/desktop/src/features/pull-requests/components/PullRequestCodeViewDiffPane.tsx:2639 (RIGHT) — line is not commentable in the GitHub diff
Including parsedDiffs.length in this useEffect dependency array causes the effect to re-run whenever new diffs are loaded. If the user is already viewing the active file, this will force a re-scroll to activeItemId, creating disruptive scroll jumps while incremental diffs are still loading. Remove parsedDiffs.length from the deps; activeItemId and focusedLineNumber are sufficient for the intended behavior.
apps/desktop/src/features/diff-view/components/DiffViewer.tsx:449 (RIGHT) — line is not commentable in the GitHub diff
This function is duplicated in the new DiffScrollbarMarkers.tsx file. The same duplication applies to DiffScrollbarMarker type, clampPercent, getDiffTotalLines, and the DiffScrollbarMarkers component. Maintenance risk: fixes to one will not apply to the other. Consider importing from the shared module instead.
apps/desktop/src/features/diff-view/components/DiffScrollbarMarkers.tsx:623 (RIGHT) — line is not commentable in the GitHub diff
These marker buttons only have onPointerDown. Keyboard users cannot activate them with Enter/Space. Add onClick (or onKeyDown handling Enter/Space) to satisfy accessibility requirements.

orc-review · 2026-06-17T17:00:40Z

-  const activeDocumentRef = useRef<ActiveDocument | null>(null);
+  const activeDocumentsRef = useRef(new Map<string, CurrentLspDocument>());
+  const documentsKey = documents
+    .map((document) => `${documentKey(document)}\u0000${document.text.length}`)


documentsKey is derived from document.text.length rather than the actual text content. If a file is edited and the character count happens to stay the same, the key won't change. While documents is also in the deps array today, this key is misleading and would miss changes if the effect deps were ever refactored to rely on it. Use a hash of the text or the text itself instead.

ShpetimA · 2026-06-17T17:42:43Z

/review

orc-review

This PR upgrades the @pierre/diffs library from 1.1.15 to 1.2.7 and introduces multi-diff code view support via new CodeView-based panes for Changes and Pull Request screens. It also adds git error logging, a repo action error dialog, and improves LSP diagnostics handling across multiple diff items.

Critical / High issues found:

Concurrency risk in git error logging (apps/desktop/electron/git.ts): lastGitCommandErrorLog is a global mutable variable. If multiple git commands fail concurrently (e.g., across different repos or rapid operations), the stored log path can be overwritten by a later error before getLastGitCommandErrorLogPath is called. This means the error dialog may show or open the wrong log file.
Dead patch file (patches/@pierre__diffs@1.2.4.patch): The patch is for version 1.2.4 but apps/desktop/package.json installs 1.2.7. There is no patchedDependencies configuration in package.json, so pnpm will not apply this patch. The patch fixes virtualized diff force-render behavior; without it, the new multi-diff views may exhibit stale rendering or scroll-jump issues.
Code duplication (DiffScrollbarMarkers): The new DiffScrollbarMarkers.tsx file exports buildDiffScrollbarMarkers and DiffScrollbarMarkers, but DiffViewer.tsx (lines 418–566) still contains an identical copy. The old DiffViewer.tsx should be updated to import from the new shared file to avoid drift.
Missing tests: Several new, large components and hooks have no test coverage:
- ChangesCodeViewDiffPane.tsx (756 lines)
- PullRequestCodeViewDiffPane.tsx (695 lines)
- useMultiDiffDiagnostics.ts
- useMultiDiffCodeViewOptions.ts
- RepoActionErrorDialog.tsx
- setRepoActionError reducer in sourceControlSlice
Electron version mismatch: The root package.json adds electron: ^42.3.0 as a dependency, while apps/desktop/package.json pins electron: "41.0.2" as a devDependency. This creates a version mismatch and may cause runtime or build issues in the monorepo.
Missing scrollbar markers in changes view: PullRequestCodeViewDiffPane.tsx renders <DiffScrollbarMarkers markers={scrollbarMarkers} viewportRef={viewportRef} />, but ChangesCodeViewDiffPane.tsx does not. This is an inconsistent UX omission.
Potential memory leak in useMultiDiffDiagnostics: renderedRootNodesRef stores DOM node references per item. If a file is removed from the view (e.g., staged and then committed), its entry is never deleted because onPostRender is not called for removed items, and the cleanup effect only runs when diagnosticsByLineByItem changes. Over time, detached DOM nodes can accumulate.
Placeholder diff injection risk: createPlaceholderDiff in useMultiDiffCodeViewOptions.ts builds a git patch string with diff --git a/${fileName} b/${fileName}. If fileName contains spaces or special characters, the generated patch is malformed. Although this is display-only, it could cause processFile to throw or produce garbled output.
Pre-commit hook change: The pre-commit hook was changed from pnpm lint && pnpm fmt:check to pnpm precommit. The new precommit script is pnpm lint && pnpm fmt:check. This is functionally equivalent, but the change itself is harmless.

Overall: The PR is a large refactor with good architectural direction (shared multi-diff hooks, centralized error handling), but it has critical correctness and maintenance issues that should be addressed before merging.

General comments

The DiffScrollbarMarkers component was extracted to a new file but DiffViewer.tsx still contains its own copy. Consider a follow-up PR to deduplicate by having DiffViewer.tsx import from the shared file.
The useMultiDiffDiagnostics hook uses useEffect to iterate over renderedRootNodesRef.current and deletes entries while iterating. While Map iteration handles this safely in JavaScript, it is worth documenting or adding a cleanup function that is called when items are explicitly removed.
The ChangesCodeViewDiffPane and PullRequestCodeViewDiffPane both use useMemo(() => getDiffTheme(), []) which creates a new theme object on every render (because [] means it only runs once, but the function is called inside useMemo). Wait, actually useMemo(() => getDiffTheme(), []) is correct - it only runs once. But the getDiffTheme() function might return a new object each time. This is fine as long as the theme is memoized.

Findings not posted inline

apps/desktop/src/features/diff-view/components/DiffViewer.tsx:449 (RIGHT) — line is not commentable in the GitHub diff
Code duplication: buildDiffScrollbarMarkers is already defined in DiffScrollbarMarkers.tsx. The DiffViewer.tsx should import it from the shared file instead of maintaining its own copy.
apps/desktop/src/features/diff-view/components/DiffViewer.tsx:503 (RIGHT) — line is not commentable in the GitHub diff
Code duplication: DiffScrollbarMarkers component is already defined in DiffScrollbarMarkers.tsx. The DiffViewer.tsx should import and use the shared component to avoid maintenance drift.

orc-review · 2026-06-17T17:44:48Z

 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;


Concurrency risk: lastGitCommandErrorLog is a global mutable variable. If two git commands fail concurrently, the second error overwrites the first before getLastGitCommandErrorLogPath can retrieve it. Consider storing the log path in a per-repo or per-promise map, or returning it directly with the error instead of using shared global state.

orc-review · 2026-06-17T17:44:48Z

+    const stderr = rawStderr.trim() || error.message;
    const code = typeof rawCode === "number" ? rawCode : null;
-    const commandError = new GitCommandError(args, stderr, code);
+    const logPath = await writeGitCommandErrorLog({ repoPath, args, stderr, stdout, code });


The logPath is computed asynchronously and stored globally. If another runGit fails in between, the log path for this error will be lost. This makes the error dialog potentially show the wrong log or no log at all.

orc-review · 2026-06-17T17:44:48Z

@@ -0,0 +1,21 @@
+diff --git a/dist/components/VirtualizedFileDiff.js b/dist/components/VirtualizedFileDiff.js


Dead patch: this patch targets @pierre/diffs@1.2.4, but apps/desktop/package.json installs 1.2.7. There is no patchedDependencies configuration, so pnpm will not apply this patch. The patch should be renamed to match the installed version or pnpm.patchedDependencies should be configured in package.json.

orc-review · 2026-06-17T17:44:48Z

    "@hookform/resolvers": "^5.2.2",
    "@m2d/react-markdown": "^1.0.0",
-    "@pierre/diffs": "1.1.15",
+    "@pierre/diffs": "1.2.7",


Version mismatch with patch: package is 1.2.7 but patch file is 1.2.4. Ensure the patch is applied or the fix is already included in 1.2.7.

orc-review · 2026-06-17T17:44:48Z

+export function useMultiDiffDiagnostics(diagnosticsByItem: Map<string, LspDiagnostic[]>) {
+  const diagnosticCloseTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
+  const isDiagnosticPopoverHoveredRef = useRef(false);
+  const renderedRootNodesRef = useRef(new Map<string, HTMLElement>());


Potential memory leak: renderedRootNodesRef stores DOM nodes. If an item is removed from the view (e.g., file is staged/committed), its entry is never deleted because onPostRender is not called for removed items, and the cleanup effect only runs when diagnosticsByLineByItem changes. Consider pruning disconnected nodes in the effect or on item removal.

orc-review · 2026-06-17T17:44:48Z

@@ -0,0 +1,133 @@
+import { useCallback, useEffect, useMemo, useRef, useState } from "react";


Missing tests: this hook manages popover state, timers, and DOM decorations across multiple items. It should have unit tests for timer cleanup, popover open/close logic, and decoration application.

orc-review · 2026-06-17T17:44:48Z

@@ -0,0 +1,429 @@
+import { useEffect, useMemo, useReducer, useRef } from "react";


Missing tests: this hook builds CodeViewOptions with many callbacks via refs. It should have unit tests for createPlaceholderDiff, getAnnotationsKey, useParsedMultiFileDiffs, and updateSelection behavior.

orc-review · 2026-06-17T17:44:48Z

@@ -0,0 +1,104 @@
+import { useState } from "react";


Missing tests: this dialog component handles error display, log opening, and output toggling. It should have tests for rendering, closing, and opening the log file.

orc-review · 2026-06-17T17:44:48Z

@@ -159,6 +162,9 @@ const sourceControlSlice = createSlice({
        state.runningAction = action.payload;


Missing tests: setRepoActionError and resetRepoViewState (which now clears repoActionError) are not covered in sourceControlSlice.test.ts. Add tests to verify the reducer correctly sets and clears the error state.

orc-review · 2026-06-17T17:44:48Z

    "check": "pnpm lint && pnpm typecheck && pnpm test && pnpm fmt:check"
  },
+  "dependencies": {
+    "electron": "^42.3.0"


Version mismatch: root package.json adds electron: ^42.3.0 as a dependency, but apps/desktop/package.json has electron: "41.0.2" as a devDependency. This mismatch can cause build/runtime issues. Either remove the root dependency or align the versions.

ShpetimA · 2026-06-17T18:23:32Z

/review

orc-review

This PR updates the Pierre diffs library to 1.2.7 and introduces a multi-diff CodeView architecture across the Changes, Pull Request, and Review screens. It also adds git error logging, LSP document multi-sync, diagnostic multi-diff support, scrollbar change markers, and a repo action error dialog. The code is generally well-structured and follows the project patterns, but there are several correctness, robustness, and testing gaps.

Correctness / Security:

useCurrentLspDocuments uses document.text.length as part of its effect key. If the text changes but the length stays the same and the array identity is preserved, the effect will not re-run and the LSP document will not be re-synced. This is a latent correctness bug.
createPlaceholderDiff constructs a raw git diff patch string using unsanitized path and message inputs. If these contain newlines or special diff characters, the generated patch could be malformed, potentially causing downstream parsing errors or producing invalid diff metadata.
lastGitCommandErrorLog is a module-level singleton. If two async runGit calls fail concurrently, the second failure can overwrite the log path before the first error's consumer reads it, causing the wrong log to be associated with an error. This is a latent concurrency risk.

Memory / Lifecycle:
4. useMultiDiffDiagnostics stores rendered DOM nodes in a useRef Map (renderedRootNodesRef). It only removes disconnected nodes during the diagnostics update effect. If the component unmounts without a diagnostics update, the HTMLElements are leaked. The onTokenLeave callback also sets a setTimeout without cleanup on unmount. While React 18 handles post-unmount state setters safely, this is an anti-pattern.

Accessibility:
5. DiffScrollbarMarkers uses role="scrollbar" on the track container without the required ARIA attributes (aria-valuenow, aria-valuemax, aria-valuemin, aria-controls). This is misleading to assistive technologies and may not convey the component's purpose correctly.

Missing Tests:
6. All new complex units—useMultiDiffDiagnostics, useMultiDiffCodeViewOptions, DiffScrollbarMarkers, ChangesCodeViewDiffPane, PullRequestCodeViewDiffPane, and RepoActionErrorDialog—are added without tests. The ChangesCodeViewDiffPane and PullRequestCodeViewDiffPane components are large, stateful, and imperative (mutating CodeView items via refs), making them high-risk for regressions without automated coverage.

General comments

Missing tests: All new complex units (useMultiDiffDiagnostics, useMultiDiffCodeViewOptions, DiffScrollbarMarkers, ChangesCodeViewDiffPane, PullRequestCodeViewDiffPane, RepoActionErrorDialog) are added without any tests. The two *CodeViewDiffPane components are large (~700–800 lines), use imperative CodeView ref mutations, and coordinate multiple async data sources. They should have at least basic mount/update/unmount tests and coverage for the item update effect logic.
Patch dependency: The PR adds a patches/@pierre__diffs@1.2.4.patch file but the package.json references @pierre/diffs 1.2.7. Verify that the patch is still compatible with the 1.2.7 release and that the patched paths in the dist/ tree match the installed version. If the library was updated after the patch was authored, the patch may fail to apply or may silently patch the wrong lines.

orc-review · 2026-06-17T18:25:48Z

  const dispatch = useAppDispatch();
-  const activeDocumentRef = useRef<ActiveDocument | null>(null);
+  const activeDocumentsRef = useRef(new Map<string, CurrentLspDocument>());
+  const documentsKey = documents


Correctness risk: The effect key uses document.text.length instead of the text content. If the text changes but the length stays the same and the documents array identity is preserved, the effect will not re-run and the LSP document will not be re-synced. Consider hashing the text or using the text itself in the key.

orc-review · 2026-06-17T18:25:48Z

+  return typeof item.version === "number" ? item.version + 1 : 1;
+}
+
+export function createPlaceholderDiff(path: string, message: string): FileDiffMetadata {


Robustness issue: createPlaceholderDiff constructs a raw git diff patch string with unsanitized path and message. If either contains newlines or special characters (e.g. \n, \t, backslash), the generated patch could be malformed. Consider escaping or sanitizing these inputs before interpolating them.

orc-review · 2026-06-17T18:25:48Z

+export function useMultiDiffDiagnostics(diagnosticsByItem: Map<string, LspDiagnostic[]>) {
+  const diagnosticCloseTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
+  const isDiagnosticPopoverHoveredRef = useRef(false);
+  const renderedRootNodesRef = useRef(new Map<string, HTMLElement>());


Memory leak: renderedRootNodesRef stores DOM nodes in a Map. Disconnected nodes are only cleaned up in the diagnostics update effect. If the component unmounts without a diagnostics update, the HTMLElements are retained. Add a cleanup effect that clears the ref on unmount.

orc-review · 2026-06-17T18:25:48Z

+    [closePopover, diagnosticsByLineByItem],
+  );
+
+  const onTokenLeave = useCallback(() => {


Anti-pattern: onTokenLeave sets a setTimeout that calls closePopover (and thus setState) without a cleanup function. Add a cleanup effect that clears the timer on unmount to avoid post-unmount state updates.

orc-review · 2026-06-17T18:25:48Z

+  if (markers.length === 0) return null;
+
+  return (
+    <div


Accessibility: The container uses role="scrollbar" without required ARIA attributes (aria-valuenow, aria-valuemax, aria-valuemin, aria-controls). A scrollbar role requires these to be meaningful. Consider using a different role (e.g. region) or adding the required ARIA properties.

orc-review · 2026-06-17T18:25:48Z

 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;


Concurrency risk: lastGitCommandErrorLog is a module-level singleton. If two runGit calls fail concurrently, the second writeGitCommandErrorLog can overwrite the log before the first error's consumer reads it, causing the wrong log to be associated with the first error.

ShpetimA · 2026-06-18T15:58:38Z

/review

ShpetimA · 2026-06-18T16:30:04Z

/review

ShpetimA · 2026-06-18T16:35:06Z

/review

ShpetimA · 2026-06-18T16:37:10Z

/review

ShpetimA · 2026-06-18T19:46:37Z

/review

ShpetimA · 2026-06-18T19:46:56Z

/review

orc-review

Large refactor that migrates the Changes and Pull-Request diff panes from the single-file DiffWorkspace to a multi-file @pierre/diffs CodeView (with a 1.1.15 -> 1.2.7 bump), plus a new git error-log pipeline (temp log files + getLastGitCommandErrorLogPath) surfaced through a RepoActionErrorDialog, and assorted LSP/diagnostics/symbol-peek/scrollbar-marker improvements.

Overall the migration looks coherent and the existing flows are preserved, but there are a few real issues worth addressing before merge:

The new patches/@pierre__diffs@1.2.4.patch is orphaned — there is no pnpm.patchedDependencies entry anywhere (package.json/pnpm-workspace.yaml) and the filename version (1.2.4) does not match the installed version (1.2.7), so pnpm will never apply it. Either register the patch against 1.2.7 or remove the file.
The git error-log path is retrieved via a process-global singleton (lastGitCommandErrorLog) plus a separate IPC round-trip, which is racy against any other concurrent git failure in the same repo (notably the Snapshot refetch that commitStaged invalidates). The commit error dialog can therefore surface the wrong command's log. Prefer threading the log path through the IPC error result directly.
useCurrentLspDocuments keys its sync effect on document.text.length, which is inconsistent with the exact text === text comparison inside the effect; equal-length content edits are only caught because the documents array identity happens to change. That coupling is fragile and should be hardened.
Git error logs are written under the OS tmpdir but never cleaned up, so they accumulate indefinitely.
The bulk of the new logic (scrollbar-marker math, error handling/cleaning, multi-file diff parsing, multi-diff diagnostics) ships without unit tests; only minor test stubs were added.

Smaller notes: electron was added as a root production dependency (unusual — normally a devDependency in apps/desktop), commitStaged is the only mutation wired to the new log-path flow (other repo-action errors won't get the "Open Git Log" button even though logs are written for them), and there are now two DiffScrollbarMarkers components (one local in DiffViewer.tsx, one new exported module).

General comments

Test coverage: this PR adds ~2400 LOC of new logic (PullRequestCodeViewDiffPane, ChangesCodeViewDiffPane, useMultiDiffCodeViewOptions, useMultiDiffDiagnostics, the git error-log pipeline, RepoActionErrorDialog, useCurrentLspDocuments) but ships essentially no tests for it — only a mock stub in actions.test.ts and a scroll-position case in LspSymbolPeek.test.tsx. The scrollbar-marker builder, ANSI/error-message cleaning (stripAnsi/cleanDesktopErrorMessage/firstErrorLine), buildRepoActionError, and createPlaceholderDiff are all pure functions and are good unit-test candidates. Please add at least baseline coverage before merge.
RepoActionError lifecycle: runRepoAction sets repoActionError on failure but never clears it on success. If a user dismisses the dialog and the same action later succeeds, that's fine (dialog controls clearing), but if a different action fails while the dialog is already open the error is silently swapped. Consider clearing repoActionError at the start of runRepoAction (or on success in the finally) to avoid showing stale/overwritten content.
The new multi-file CodeView relies on a useRef mutated during render (interactionRef.current = {...} in useMultiDiffCodeViewOptions) to keep callbacks fresh without re-creating the memoized options. That works and is idempotent under StrictMode, but it sidesteps the dependency list — any future handler that closes over props/state won't update unless also funneled through the ref. Worth a brief code comment so the invariant is preserved.
useMultiDiffDiagnostics repaints ALL rendered root nodes (renderedRootNodesRef) on any change to diagnosticsByLineByItem (its useEffect dep). For large multi-file reviews this is O(total tokens across every file) on each diagnostic update; if diagnostics stream in per-file this will do a lot of redundant querySelectorAll work. Consider scoping the repaint to the changed item ids.

Findings not posted inline

apps/desktop/src/features/diff-view/components/DiffViewer.tsx:503 (RIGHT) — line is not commentable in the GitHub diff
There's now a second DiffScrollbarMarkers here in DiffViewer.tsx alongside the newly extracted/exported one in components/DiffScrollbarMarkers.tsx. Both were updated in lockstep (right-6, hover:w-3, .matches('.diff-viewport-scroll') fallback) — this duplication will drift. Consider having DiffViewer.tsx import the shared component, or note why the single-diff path needs its own copy.

orc-review · 2026-06-18T19:52:01Z

 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;


This module-level singleton is the root of a concurrency hazard. runGit overwrites lastGitCommandErrorLog on every git failure (status/diff/log reads included, not just writes), and getLastGitCommandErrorLogPath(repoPath) only filters by repo path. Between a commit failing and the renderer's separate IPC call to fetch the log path (see toGitErrorResult in api.ts), commitStaged invalidates the Snapshot tag which triggers a getGitSnapshot refetch; if that refetch also fails in the same repo it silently overwrites this global, so the commit error dialog can display the wrong command's log.

The GitCommandError already carries the correct logPath for the failing command — the robust fix is to return it through the IPC error result (e.g. via toGitErrorResult reading error.logPath instead of re-querying the global) rather than relying on a shared mutable singleton.

orc-review · 2026-06-18T19:52:01Z

          return { data: await commitStaged(repoPath, message) };
        } catch (error) {
-          return { error: toErrorResult(error) };
+          return { error: await toGitErrorResult(error, repoPath) };


toGitErrorResult ignores the logPath already attached to the thrown GitCommandError and instead re-queries the process-global lastGitCommandErrorLog via another IPC hop. That round-trip is the window in which a concurrent git failure in the same repo can swap the log (see git.ts). Since the goal here is just to recover a value that doesn't survive Electron IPC serialization, prefer returning the log path as structured data from the commitStaged service (or embedding it in the error payload) and read it directly here.

orc-review · 2026-06-18T19:52:01Z

+  return { message, details: message, logPath: options?.logPath ?? null };
+}
+
+async function toGitErrorResult(error: unknown, repoPath: string): Promise<ErrorResult> {


Only commitStaged is routed through toGitErrorResult, so only commit errors get a logPath. runGit writes a log file for every git failure (stage/unstage/discard/etc.), but those errors still go through toErrorResult(error) with logPath: null, so their RepoActionErrorDialog never shows the "Open Git Log" button. If logs are only meant for commits, writeGitCommandErrorLog should be scoped to write commands; otherwise route the other write mutations through toGitErrorResult too for consistency.

orc-review · 2026-06-18T19:52:01Z

+}) {
+  try {
+    await fs.mkdir(GIT_ERROR_LOG_DIR, { recursive: true });
+    const logPath = path.join(GIT_ERROR_LOG_DIR, `git-${Date.now()}-${process.pid}.log`);


Log files (git-<ts>-<pid>.log) are written for every git failure but never deleted. Over time these accumulate in $TMPDIR/open-warden-git-logs. Consider a bounded rotation (e.g. keep last N, or clean on app quit), and note the content includes the full command line + repo path so a cleanup story also limits that exposure.

orc-review · 2026-06-18T19:52:01Z

-  const activeDocumentRef = useRef<ActiveDocument | null>(null);
+  const activeDocumentsRef = useRef(new Map<string, CurrentLspDocument>());
+  const documentsKey = documents
+    .map((document) => `${documentKey(document)}\u0000${document.text.length}`)


documentsKey incorporates only document.text.length, while the effect body compares full text === text. Today this happens to work only because the documents array identity changes whenever text changes (callers memoize on targetResults). If a caller ever passes a stable documents array, an equal-length content edit (e.g. replacing a character) won't change documentsKey and the LSP document won't be re-synced, leaving diagnostics/hovers stale. The previous single-document hook keyed on the full text; consider hashing or including a content version here for robustness.

orc-review · 2026-06-18T19:52:01Z

@@ -0,0 +1,21 @@
+diff --git a/dist/components/VirtualizedFileDiff.js b/dist/components/VirtualizedFileDiff.js


This patch is orphaned. There is no pnpm.patchedDependencies entry in either package.json or pnpm-workspace.yaml, so pnpm won't apply it at all; and even if registered, the filename targets 1.2.4 while apps/desktop/package.json now pins @pierre/diffs to 1.2.7, so the version would not match. The patch tweaks VirtualizedFileDiff force-render behavior — if it's still needed, register it against 1.2.7 (via pnpm patch @pierre/diffs@1.2.7); if not, delete it so it doesn't read as a silently-applied dependency.

orc-review · 2026-06-18T19:52:01Z

    "check": "pnpm lint && pnpm typecheck && pnpm test && pnpm fmt:check"
  },
+  "dependencies": {
+    "electron": "^42.3.0"


electron is now a root-level production dependency (and added to pnpm.onlyBuiltDependencies). Electron is a build/runtime tool for the desktop app, not a dependency of the root workspace package — it's almost certainly already declared (as a devDependency) in apps/desktop. Promoting it to a root prod dep drags a large platform binary into the root install and changes the dependency graph. Move it to apps/desktop (devDependency) unless there's a specific root-level reason.

orc-review · 2026-06-18T19:52:01Z

+  fileDiff: FileDiffMetadata,
+  diffStyle: DiffStyle,
+): DiffScrollbarMarker[] {
+  const totalLines = getDiffTotalLines(fileDiff, diffStyle);


The marker math reads fileDiff.splitLineCount/unifiedLineCount, hunk.splitLineStart/unifiedLineStart, and content.additions/deletions/lines directly off @pierre/diffs' FileDiffMetadata. These are untested and any change in their semantics between 1.1.15 and 1.2.7 would silently skew the scrollbar markers (or, if undefined, produce NaN% positions). Worth a small unit test covering a known hunk (split + unified) to lock in the contract now that the version was bumped.

ShpetimA · 2026-06-18T20:01:33Z

/review

ShpetimA added 3 commits May 31, 2026 15:50

update to use code viewer component instead

d55ffbd

update pierre libary and add better git errors

38d81f5

add precommit fix script

7ea565b

orc-review Bot reviewed Jun 16, 2026

View reviewed changes

orc-review Bot reviewed Jun 17, 2026

View reviewed changes

orc-review Bot reviewed Jun 18, 2026

View reviewed changes

ShpetimA closed this Jun 18, 2026

		@@ -0,0 +1,21 @@
		diff --git a/dist/components/VirtualizedFileDiff.js b/dist/components/VirtualizedFileDiff.js

		@@ -0,0 +1,133 @@
		import { useCallback, useEffect, useMemo, useRef, useState } from "react";

		@@ -0,0 +1,429 @@
		import { useEffect, useMemo, useReducer, useRef } from "react";

		@@ -159,6 +162,9 @@ const sourceControlSlice = createSlice({
		state.runningAction = action.payload;

Conversation

ShpetimA commented Jun 16, 2026

Uh oh!

ShpetimA commented Jun 16, 2026

Uh oh!

ShpetimA commented Jun 16, 2026

Uh oh!

ShpetimA commented Jun 16, 2026

Uh oh!

ShpetimA commented Jun 16, 2026

Uh oh!

ShpetimA commented Jun 16, 2026

Uh oh!

orc-review Bot left a comment

Choose a reason for hiding this comment

General comments

Findings not posted inline

Uh oh!

orc-review Bot Jun 16, 2026

Choose a reason for hiding this comment

Uh oh!

orc-review Bot Jun 16, 2026

Choose a reason for hiding this comment

Uh oh!

ShpetimA commented Jun 16, 2026

Uh oh!

ShpetimA commented Jun 16, 2026

Uh oh!

ShpetimA commented Jun 16, 2026

Uh oh!

ShpetimA commented Jun 16, 2026

Uh oh!

ShpetimA commented Jun 17, 2026

Uh oh!

ShpetimA commented Jun 17, 2026

Uh oh!

orc-review Bot left a comment

Choose a reason for hiding this comment

General comments

Findings not posted inline

Uh oh!

orc-review Bot Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

ShpetimA commented Jun 17, 2026

Uh oh!

orc-review Bot left a comment

Choose a reason for hiding this comment

General comments

Findings not posted inline

Uh oh!

orc-review Bot Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

orc-review Bot Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

orc-review Bot Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

orc-review Bot Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

orc-review Bot Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

orc-review Bot Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

orc-review Bot Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

orc-review Bot Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

orc-review Bot Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

orc-review Bot Jun 17, 2026

Choose a reason for hiding this comment