diff --git a/apps/desktop/src/main/bootstrap/os-startup-tweaks.ts b/apps/desktop/src/main/bootstrap/os-startup-tweaks.ts index f92b1007..a37fb752 100644 --- a/apps/desktop/src/main/bootstrap/os-startup-tweaks.ts +++ b/apps/desktop/src/main/bootstrap/os-startup-tweaks.ts @@ -56,10 +56,16 @@ function applyWindowsStartupTweaks(): void { * real keychain. Cost: cookie encryption degrades to a static key, but the key was already stored in * plaintext, so no real loss. Removable once there's a proper Developer ID signature. Must be before * app.whenReady(). + * - disable-features MediaSessionService/HardwareMediaKeyHandling: Chromium's macOS "Now Playing" / + * media-session integration queries the MediaPlayer framework, which makes macOS prompt for "access + * Apple Music / your media library" on launch. We play no media and expose no now-playing controls, so + * this permission is unexpected and confusing; disabling the features stops Chromium from ever touching + * the media library. Must be before app.whenReady(). * - Prepend common CLI dirs to PATH (see augmentMacPath): must be before pr-agent probing / running. */ function applyMacStartupTweaks(): void { app.commandLine.appendSwitch('use-mock-keychain'); + app.commandLine.appendSwitch('disable-features', 'MediaSessionService,HardwareMediaKeyHandling'); augmentMacPath(); } diff --git a/apps/desktop/src/main/services/notifications.ts b/apps/desktop/src/main/services/notifications.ts index 29865c7e..6f947dd4 100644 --- a/apps/desktop/src/main/services/notifications.ts +++ b/apps/desktop/src/main/services/notifications.ts @@ -67,7 +67,11 @@ function activateOnClick(e: PollNotificationEvent): () => void { anchor: // File-level comments (no line) aren't line-navigable → no anchor (clicking just opens the PR). e.comment?.anchor && e.comment.anchor.line != null - ? { path: e.comment.anchor.path, line: e.comment.anchor.line } + ? { + path: e.comment.anchor.path, + line: e.comment.anchor.line, + side: e.comment.anchor.side, + } : null, }); }; diff --git a/apps/desktop/src/renderer/src/components/features/pr/PrPanel.tsx b/apps/desktop/src/renderer/src/components/features/pr/PrPanel.tsx index 4e58b7e5..53b8d5da 100644 --- a/apps/desktop/src/renderer/src/components/features/pr/PrPanel.tsx +++ b/apps/desktop/src/renderer/src/components/features/pr/PrPanel.tsx @@ -37,13 +37,13 @@ export interface PrPanelProps { pendingDiffNav?: { runId?: string; findingId?: string; - anchor: { path: string; startLine: number; endLine: number }; + anchor: { path: string; startLine: number; endLine: number; side?: 'old' | 'new' }; } | null; onDiffNavConsumed?: () => void; onRequestDiffNav?: (target: { runId?: string; findingId?: string; - anchor: { path: string; startLine: number; endLine: number }; + anchor: { path: string; startLine: number; endLine: number; side?: 'old' | 'new' }; }) => void; /** External request to switch to a given tab (e.g. clicking a summary comment notification → 'activity'); cleared via onPendingTabConsumed after consumption. */ pendingTab?: PrTab | null; @@ -233,7 +233,7 @@ export function PrPanel({ // File-level anchors (no line) aren't line-navigable; CommentItem doesn't make them clickable, guard anyway. if (a.line == null) return; onRequestDiffNav?.({ - anchor: { path: a.path, startLine: a.line, endLine: a.line }, + anchor: { path: a.path, startLine: a.line, endLine: a.line, side: a.side }, }); }} /> @@ -252,6 +252,7 @@ export function PrPanel({ path: d.anchor.path, startLine: d.anchor.startLine, endLine: d.anchor.endLine, + side: d.anchor.side, }, }); }} @@ -281,6 +282,7 @@ export function PrPanel({ path: d.anchor.path, startLine: d.anchor.startLine, endLine: d.anchor.endLine, + side: d.anchor.side, }, }); }} diff --git a/apps/desktop/src/renderer/src/components/features/pr/tabs/comments/CommentItem.tsx b/apps/desktop/src/renderer/src/components/features/pr/tabs/comments/CommentItem.tsx index 858a436e..7251b145 100644 --- a/apps/desktop/src/renderer/src/components/features/pr/tabs/comments/CommentItem.tsx +++ b/apps/desktop/src/renderer/src/components/features/pr/tabs/comments/CommentItem.tsx @@ -22,43 +22,9 @@ const InlineCodeContext = lazy(() => import('./InlineCodeContext').then((m) => ({ default: m.InlineCodeContext })), ); -/** - * Structural equality comparison of the comment tree (by remoteId + body + version + edit/delete permissions + recursive replies). poll mostly returns - * comments with unchanged content: on equality, skip setState and keep the old reference so React bails out, avoiding pointless re-render of the whole - * comment tree (including inline Monaco) (refresh flicker). - */ -export function sameCommentList(a: readonly PrComment[], b: readonly PrComment[]): boolean { - if (a.length !== b.length) return false; - for (let i = 0; i < a.length; i++) { - const x = a[i]!; - const y = b[i]!; - if ( - x.remoteId !== y.remoteId || - x.body !== y.body || - x.version !== y.version || - x.canEdit !== y.canEdit || - x.canDelete !== y.canDelete || - !sameReactions(x.reactions, y.reactions) || - !sameCommentList(x.replies, y.replies) - ) { - return false; - } - } - return true; -} - -/** Equality comparison of the reactions array (emoji + count + mine triple matching item by item): lets reaction changes after a toggle trigger a re-render. */ -function sameReactions(a: PrComment['reactions'], b: PrComment['reactions']): boolean { - const x = a ?? []; - const y = b ?? []; - if (x.length !== y.length) return false; - for (let i = 0; i < x.length; i++) { - if (x[i]!.emoji !== y[i]!.emoji || x[i]!.count !== y[i]!.count || x[i]!.mine !== y[i]!.mine) { - return false; - } - } - return true; -} +// Structural comment-tree equality lives in the shared module so the diff view (useDiffComments) can reuse the same poll bail; re-exported here for the +// activity/comments-page importers that historically pulled it from this file. +export { sameCommentList } from '../shared/commentEquality'; /** * Maximum indent level for nested replies: past this level recursion continues but **no further indent is added** (flattened display), avoiding diff --git a/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/DiffView.tsx b/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/DiffView.tsx index 8e11a08f..85a6eda0 100644 --- a/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/DiffView.tsx +++ b/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/DiffView.tsx @@ -21,6 +21,7 @@ import { BackendErrorBanner, BackendErrorView, SyncProgress } from './DiffStatus import { BlameColumn } from './blame/BlameColumn'; import { fileKey, type PendingCommitView } from './diff-types'; import { + useActualRenderSideBySide, useBlame, useChangedFiles, useCommentZones, @@ -60,7 +61,7 @@ interface DiffViewProps { pendingNav?: { runId?: string; findingId?: string; - anchor: { path: string; startLine: number; endLine: number }; + anchor: { path: string; startLine: number; endLine: number; side?: 'old' | 'new' }; } | null; onNavConsumed?: () => void; /** @@ -107,6 +108,10 @@ export function DiffView({ const drafts = useDraftsForPr(pr.localId); // Use state, not a ref: onMount fires asynchronously, so a state change is required to re-run the subsequent useEffect decoration logic. const [diffEditor, setDiffEditor] = useState(null); + // The ACTUAL render mode: renderSideBySide is only the toolbar intent, but Monaco auto-degrades to inline when the + // pane is too narrow. Positioning (which inner editor a comment/draft zone, glyph, or "+" adder targets) must use + // this, not the intent, or old-side items land on the original editor Monaco has hidden. See useActualRenderSideBySide. + const actualSideBySide = useActualRenderSideBySide(diffEditor, renderSideBySide); const progress = useSyncProgress(pr); const { fileListWidth, startFileListResize } = useFileListWidth(); @@ -168,9 +173,16 @@ export function DiffView({ pendingNav, onNavConsumed, triggerAutoEdit, + // Intent prop: useDiffNav reads the actual mode live at reveal time to place old-side targets correctly. + renderSideBySide, }); // Capture the Diff selection → selectionStore, so ChatPane can carry the selected code as implicit context into agent/ask questions. - useSelectionCapture({ diffEditor, selected, prLocalId: pr.localId, renderSideBySide }); + useSelectionCapture({ + diffEditor, + selected, + prLocalId: pr.localId, + renderSideBySide: actualSideBySide, + }); // sidebar mode: 'tree' (file tree) / 'search' (cross-file search), defaults to the file tree. Returns to 'tree' on PR switch. const [sidebarMode, setSidebarMode] = useState<'tree' | 'search'>('tree'); @@ -237,7 +249,9 @@ export function DiffView({ attachmentBase, prLocalId: pr.localId, prWebUrl: pr.url, - renderSideBySide, + // The actual render mode, so old-side comment zones + glyph/tick markers land on the visible editor after an + // auto-degrade to unified (the reported "published comment lacks position" case). + renderSideBySide: actualSideBySide, commentHardBreaks, reactionsMode, attachmentsEnabled, @@ -254,7 +268,7 @@ export function DiffView({ selected, prLocalId: pr.localId, registerEditTrigger, - renderSideBySide, + renderSideBySide: actualSideBySide, commentHardBreaks, attachmentsEnabled, mentionCandidates, @@ -271,7 +285,8 @@ export function DiffView({ prLocalId: pr.localId, platform: pr.platform, scopeKind: scope.kind, - renderSideBySide, + // Actual mode: wire the old-side "+" adder only when the original editor is actually visible (not auto-degraded). + renderSideBySide: actualSideBySide, readOnly, triggerAutoEdit, t, diff --git a/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/DraftZoneList.tsx b/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/DraftZoneList.tsx index f60adc55..035aaead 100644 --- a/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/DraftZoneList.tsx +++ b/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/DraftZoneList.tsx @@ -7,8 +7,10 @@ import { DraftZone } from '../drafts/DraftZone'; /** * Container for multiple drafts on the same line; each is an independent DraftZone (maintaining its own read/edit), separated by hr. * onSave / onDelete call IPC drafts:update / drafts:delete here; after writing to disk the main side - * broadcasts a drafts:changed event → drafts-store refetches → DiffView's top-level useEffect rebuilds the - * zones (this component unmounts/remounts along with it). + * broadcasts a drafts:changed event → drafts-store refetches → useDraftZones' content effect calls the zone + * controller's update(), which **reconciles** this zone in place (keyed by draft id) rather than unmounting it, so a + * DraftZone whose editor is open keeps its text / focus across the refresh. Only a removed draft (deleted / published) + * unmounts. */ export function DraftZoneList({ drafts, diff --git a/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/hooks/index.ts b/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/hooks/index.ts index b5074e68..46b67b2c 100644 --- a/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/hooks/index.ts +++ b/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/hooks/index.ts @@ -11,6 +11,10 @@ export { useBlame, type BlameState } from './useBlame'; export { useDraftAutoEdit, type DraftAutoEdit } from './useDraftAutoEdit'; export { useDiffNav, type PendingNav, type PendingScroll } from './useDiffNav'; export { useCommentZones } from './useCommentZones'; +export { + useActualRenderSideBySide, + isActualSideBySide, +} from './useActualRenderSideBySide'; export { useDiffOverviewMarks } from './useDiffOverviewMarks'; export { useDraftZones } from './useDraftZones'; export { useLineCommentAdder } from './useLineCommentAdder'; diff --git a/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/hooks/useActualRenderSideBySide.ts b/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/hooks/useActualRenderSideBySide.ts new file mode 100644 index 00000000..2e42295e --- /dev/null +++ b/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/hooks/useActualRenderSideBySide.ts @@ -0,0 +1,61 @@ +import { useEffect, useState } from 'react'; +import { type editor as MonacoEditor } from 'monaco-editor'; + +/** + * `renderSideBySide` on the toolbar is the user's *intent* (persisted toggle). Monaco auto-degrades side-by-side → + * inline when the pane is too narrow (`useInlineViewWhenSpaceIsLimited`, on by default), leaving the intent `true` + * while the actual layout is unified. Monaco reflects the real mode in the `.monaco-diff-editor` root node's + * `side-by-side` class (removed when downgrading to inline). + * + * Anything that positions content by editor side — which inner editor a comment/draft zone, glyph dot, overview tick, + * or reveal targets — must key off the **actual** mode, not the intent: in the auto-degraded state the original editor + * is hidden, so an old-side item routed there by intent lands on an invisible editor ("missing position"). This + * returns the actual mode: the intent AND the class being present. + */ +export function isActualSideBySide( + diffEditor: MonacoEditor.IStandaloneDiffEditor, + renderSideBySide: boolean, +): boolean { + if (!renderSideBySide) return false; + const el = diffEditor.getContainerDomNode().querySelector('.monaco-diff-editor'); + return el ? el.classList.contains('side-by-side') : true; +} + +/** + * Reactive {@link isActualSideBySide}: recomputes when Monaco's layout crosses the side-by-side ↔ inline breakpoint + * (`onDidLayoutChange`) or the diff recomputes (`onDidUpdateDiff`). rAF-coalesced and deferred so the class is read + * after Monaco has switched it (the layout event may precede the class flip). Returns the intent prop directly until + * the editor is available. + */ +export function useActualRenderSideBySide( + diffEditor: MonacoEditor.IStandaloneDiffEditor | null, + renderSideBySide: boolean, +): boolean { + const [actual, setActual] = useState(renderSideBySide); + useEffect(() => { + if (!diffEditor) { + setActual(renderSideBySide); + return; + } + const read = (): void => setActual(isActualSideBySide(diffEditor, renderSideBySide)); + read(); + let raf = 0; + const schedule = (): void => { + if (raf) return; + raf = requestAnimationFrame(() => { + raf = 0; + read(); + }); + }; + // The original editor still emits layout changes as it collapses to / expands from hidden at the breakpoint; + // onDidUpdateDiff covers the first async diff settling after a file switch. + const layoutDisp = diffEditor.getOriginalEditor().onDidLayoutChange(schedule); + const diffDisp = diffEditor.onDidUpdateDiff(schedule); + return () => { + if (raf) cancelAnimationFrame(raf); + layoutDisp.dispose(); + diffDisp.dispose(); + }; + }, [diffEditor, renderSideBySide]); + return actual; +} diff --git a/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/hooks/useCommentZones.tsx b/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/hooks/useCommentZones.tsx index d191c8fa..6157d406 100644 --- a/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/hooks/useCommentZones.tsx +++ b/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/hooks/useCommentZones.tsx @@ -8,6 +8,7 @@ import { renderHoverMd, } from '../inline-comments/InlineCommentZone'; import { createInlineZones, type InlineZonesController } from '../zones/mountInlineZones'; +import { remapOldByLineToModified } from '../zones/line-mapping'; import type { LoadedContent } from '../diff-types'; /** @@ -158,16 +159,29 @@ export function useCommentZones(opts: { const originalEditor = diffEditor.getOriginalEditor(); const modifiedEditor = diffEditor.getModifiedEditor(); - const originalDecorations = originalEditor.createDecorationsCollection( - buildDecorations(oldByLine), - ); + // Old-side markers: in side-by-side they sit on the (visible) original editor; in unified — including + // auto-degraded-from-side-by-side, since renderSideBySide here is the ACTUAL render mode — the original editor is + // hidden, so remap them onto the modified editor at the mapped line (mirroring how the zone bodies are routed in + // computeDesired), otherwise the glyph dot + overview tick vanish with the hidden editor. + const modifiedLines = new Map(newByLine); + let originalLines: Map | null = oldByLine; + if (!renderSideBySide && oldByLine.size > 0) { + originalLines = null; + const remappedOld = remapOldByLineToModified(diffEditor.getLineChanges() ?? [], oldByLine); + for (const [line, cs] of remappedOld) { + modifiedLines.set(line, [...(modifiedLines.get(line) ?? []), ...cs]); + } + } + const originalDecorations = originalLines + ? originalEditor.createDecorationsCollection(buildDecorations(originalLines)) + : null; const modifiedDecorations = modifiedEditor.createDecorationsCollection( - buildDecorations(newByLine), + buildDecorations(modifiedLines), ); return () => { try { - originalDecorations.clear(); + originalDecorations?.clear(); modifiedDecorations.clear(); } catch { // editor already disposed diff --git a/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/hooks/useDiffComments.ts b/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/hooks/useDiffComments.ts index 46487f48..533ab094 100644 --- a/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/hooks/useDiffComments.ts +++ b/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/hooks/useDiffComments.ts @@ -2,6 +2,7 @@ import { useEffect, useState } from 'react'; import type { PrComment, StoredPullRequest } from '@meebox/shared'; import { invoke, subscribe } from '../../../../../../api'; import { formatBackendError, type FormattedError } from '../../../../../../errors'; +import { sameCommentList } from '../../shared/commentEquality'; export interface DiffCommentsState { comments: PrComment[]; @@ -37,7 +38,9 @@ export function useDiffComments( const fetchList = (force: boolean): void => { invoke('diff:listComments', { localId: pr.localId, force }) .then((cs) => { - if (!cancelled) setComments(cs); + // poll mostly returns unchanged comments: keep the old array reference on structural equality so downstream memos + // (mentionCandidates) stay identity-stable and the inline draft/comment zones aren't torn down mid-edit (see useDraftZones). + if (!cancelled) setComments((prev) => (sameCommentList(prev, cs) ? prev : cs)); }) .catch((e: unknown) => { if (!cancelled) { diff --git a/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/hooks/useDiffNav.ts b/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/hooks/useDiffNav.ts index 11d8214d..d9c6bb6b 100644 --- a/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/hooks/useDiffNav.ts +++ b/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/hooks/useDiffNav.ts @@ -3,6 +3,8 @@ import { type editor as MonacoEditor } from 'monaco-editor'; import type { ReviewDraft } from '@meebox/shared'; import type { DiffChangedFile } from '@meebox/ipc'; import { fileKey, type LoadedContent } from '../diff-types'; +import { mapOriginalLineToModified } from '../zones/line-mapping'; +import { isActualSideBySide } from './useActualRenderSideBySide'; export interface PendingScroll { line: number; @@ -13,7 +15,7 @@ export interface PendingScroll { export interface PendingNav { runId?: string; findingId?: string; - anchor: { path: string; startLine: number; endLine: number }; + anchor: { path: string; startLine: number; endLine: number; side?: 'old' | 'new' }; } /** @@ -31,6 +33,8 @@ export function useDiffNav(opts: { pendingNav: PendingNav | null | undefined; onNavConsumed: (() => void) | undefined; triggerAutoEdit: (draftId: string) => void; + /** Toolbar intent; the reveal reads the ACTUAL mode from this at reveal time (see isActualSideBySide) to place old-side targets. */ + renderSideBySide: boolean; }): { pendingScroll: PendingScroll | null; setPendingScroll: React.Dispatch>; @@ -45,6 +49,7 @@ export function useDiffNav(opts: { pendingNav, onNavConsumed, triggerAutoEdit, + renderSideBySide, } = opts; const [pendingScroll, setPendingScroll] = useState(null); @@ -71,7 +76,9 @@ export function useDiffNav(opts: { setPendingScroll({ // Take endLine to align with the draft zone / publish anchor (see zone line number comment), so the highlight line matches the draft zone line: pendingNav.anchor.endLine, - side: 'new', + // Preserve the anchor's side so an old-side (base) comment/draft reveals on the correct side; older producers that + // don't carry side fall back to 'new' (head), the common case for code-review findings. + side: pendingNav.anchor.side ?? 'new', draftId: matchingDraft?.id, }); onNavConsumed?.(); @@ -84,10 +91,6 @@ export function useDiffNav(opts: { // pendingScroll comes from the nav effect (set alongside setSelectedKey); cleared after reveal useEffect(() => { if (!pendingScroll || !diffEditor || !content || !selected) return; - const editor = - pendingScroll.side === 'old' - ? diffEditor.getOriginalEditor() - : diffEditor.getModifiedEditor(); let highlightTimer: ReturnType | undefined; let revealed = false; @@ -95,15 +98,27 @@ export function useDiffNav(opts: { // onDidUpdateDiff may fire multiple times, only jump once if (revealed) return; revealed = true; + // Resolve the target editor + line by side AND the ACTUAL render mode (read live here, after the diff has settled + // and Monaco's `.side-by-side` class is stable). An old-side target sits on the original editor only when it is + // genuinely visible; when Monaco has auto-degraded to unified the original editor is hidden, so remap the old line + // onto the modified editor (same mapping the old-side zones use) and reveal there — otherwise the reveal lands on + // an invisible editor / the wrong line. + const revealOld = + pendingScroll.side === 'old' && isActualSideBySide(diffEditor, renderSideBySide); + const editor = revealOld ? diffEditor.getOriginalEditor() : diffEditor.getModifiedEditor(); + const line = + pendingScroll.side === 'old' && !revealOld + ? mapOriginalLineToModified(diffEditor.getLineChanges() ?? [], pendingScroll.line) + : pendingScroll.line; // Center-scroll to the target line - editor.revealLineInCenter(pendingScroll.line); + editor.revealLineInCenter(line); // Brief highlight: 300ms yellow-background pulse const collection = editor.createDecorationsCollection([ { range: { - startLineNumber: pendingScroll.line, + startLineNumber: line, startColumn: 1, - endLineNumber: pendingScroll.line, + endLineNumber: line, endColumn: 1, }, options: { @@ -143,7 +158,7 @@ export function useDiffNav(opts: { }; // triggerAutoEdit not in deps — it changes reference every render, including it would make reveal rerun every frame, repeatedly locating/highlighting // eslint-disable-next-line react-hooks/exhaustive-deps - }, [pendingScroll, diffEditor, content, selected]); + }, [pendingScroll, diffEditor, content, selected, renderSideBySide]); return { pendingScroll, setPendingScroll }; } diff --git a/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/hooks/useDiffOverviewMarks.ts b/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/hooks/useDiffOverviewMarks.ts index 7c7f842f..b69086a9 100644 --- a/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/hooks/useDiffOverviewMarks.ts +++ b/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/hooks/useDiffOverviewMarks.ts @@ -2,6 +2,7 @@ import { useEffect } from 'react'; import { editor as MonacoEditorNs, type editor as MonacoEditor } from 'monaco-editor'; import type { DiffChangedFile } from '@meebox/ipc'; import type { LoadedContent } from '../diff-types'; +import { isActualSideBySide } from './useActualRenderSideBySide'; // Add/change green, delete red (same color family as GitHub diff). diff marks go on the overview ruler's Left lane, // separate from comment anchors (Right lane, see useCommentZones), so they don't overlap. @@ -38,14 +39,6 @@ export function useDiffOverviewMarks(opts: { const modCol = modifiedEditor.createDecorationsCollection([]); const origCol = originalEditor.createDecorationsCollection([]); - // Whether actually side-by-side: read Monaco's `.monaco-diff-editor.side-by-side` class that reflects the actual render mode - // (the class is removed when auto-downgrading to inline on insufficient width); fall back to the user intent prop when unavailable. - const isSideBySide = (): boolean => { - if (!renderSideBySide) return false; - const el = diffEditor.getContainerDomNode().querySelector('.monaco-diff-editor'); - return el ? el.classList.contains('side-by-side') : true; - }; - const Lane = MonacoEditorNs.OverviewRulerLane; const deco = ( startLine: number, @@ -59,7 +52,7 @@ export function useDiffOverviewMarks(opts: { const refresh = (): void => { const changes = diffEditor.getLineChanges() ?? []; - const sideBySide = isSideBySide(); + const sideBySide = isActualSideBySide(diffEditor, renderSideBySide); const modDecos: MonacoEditor.IModelDeltaDecoration[] = []; const origDecos: MonacoEditor.IModelDeltaDecoration[] = []; for (const c of changes) { diff --git a/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/hooks/useDraftZones.tsx b/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/hooks/useDraftZones.tsx index f6a5d175..87174018 100644 --- a/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/hooks/useDraftZones.tsx +++ b/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/hooks/useDraftZones.tsx @@ -1,14 +1,22 @@ -import { useEffect } from 'react'; +import { useEffect, useRef } from 'react'; import { type editor as MonacoEditor } from 'monaco-editor'; import type { PlatformKind, PlatformUser, ReviewDraft } from '@meebox/shared'; import type { DiffChangedFile } from '@meebox/ipc'; import { DraftZoneList } from '../DraftZoneList'; -import { mountInlineZones } from '../zones/mountInlineZones'; +import { createInlineZones, type InlineZonesController } from '../zones/mountInlineZones'; import type { LoadedContent } from '../diff-types'; /** - * Inline draft view zones (blue background, editable). Uses the same mountInlineZones mechanism as comment zones, bucketed by anchor.side, - * with endLine as the zone line number (aligned with the publish anchor, WYSIWYG). + * Inline draft view zones (blue background, editable). Uses the same zone mechanism as comment zones, bucketed by + * anchor.side, with endLine as the zone line number (aligned with the publish anchor, WYSIWYG). + * + * Split into two effects like {@link useCommentZones}: a **structural** effect owns the zone controller's lifecycle + * (recreated only when the editor / file / view orientation / scope changes); a **content** effect calls + * `controller.update(...)` whenever the drafts or passthrough props change, which **reconciles** zones by (side, line) + * key rather than tearing them down. This lets an in-progress inline draft edit survive a drafts/comments refresh + * (e.g. the poller pulling a new remote comment mid-typing, or another draft being added elsewhere): the anchored + * line's zone keeps its React root, so the open editor's text / focus / caret aren't discarded. A genuinely removed + * draft (deleted / published / anchor moved / file switch) still unmounts, firing its useDraftZone cancel cleanup. * * Does not render rejected (user decided not to send) / posted (the remote comment is already taken over by CommentZone; re-rendering would be visually duplicate). * The commit read-only view (scopeKind !== 'all') does not render drafts (anchored on the PR full-diff line numbers, not applicable to a single commit). @@ -48,10 +56,44 @@ export function useDraftZones(opts: { scopeKind, } = opts; + // Structural lifecycle: (re)create the zone controller only when the editor / file / view orientation / scope + // changes. A drafts or comments refresh does NOT touch these deps, so the controller (and its live zones) survives — + // the content effect below then reconciles into it instead of tearing everything down. + const controllerRef = useRef | null>(null); useEffect(() => { if (!diffEditor || !content || !selected) return; // commit read-only view: does not render local draft zones (drafts are anchored on the PR full-diff line numbers, not applicable to a single commit). if (scopeKind !== 'all') return; + const controller = createInlineZones({ + diffEditor, + renderSideBySide, + zoneClassName: 'monaco-draft-zone', + innerClassName: 'monaco-draft-zone-inner', + stopEvents: [ + 'mousedown', + 'mouseup', + 'click', + 'dblclick', + 'keydown', + 'keyup', + 'wheel', + 'contextmenu', + ], + }); + controllerRef.current = controller; + return () => { + controller.dispose(); + controllerRef.current = null; + }; + }, [diffEditor, content, selected, renderSideBySide, scopeKind]); + + // Content sync: reconcile draft zones whenever the drafts / passthrough props change. Declared after the structural + // effect so on mount the controller exists before this runs (React runs setup in order). Reconcile means a surviving + // draft re-renders in place (an open editor keeps its text / focus), only added/removed drafts mount/unmount. + useEffect(() => { + const controller = controllerRef.current; + if (!controller || !diffEditor || !content || !selected) return; + if (scopeKind !== 'all') return; const fileDrafts = (drafts ?? []).filter((d) => { if (d.status === 'rejected' || d.status === 'posted') return false; // Reply-drafts render nested under their parent comment (ReplyDraftList), not as standalone line zones — skip them here. @@ -59,7 +101,6 @@ export function useDraftZones(opts: { if (!d.anchor) return false; return d.anchor.path === selected.path || selected.oldPath === d.anchor.path; }); - if (fileDrafts.length === 0) return; const oldByLine = new Map(); const newByLine = new Map(); @@ -75,23 +116,12 @@ export function useDraftZones(opts: { target.set(anchor.endLine, arr); } - return mountInlineZones({ - diffEditor, - renderSideBySide, + // Reconcile: unchanged draft lines re-render in place (an open draft editor keeps its text / focus / caret), only + // genuinely added/removed lines mount/unmount. An empty set removes all remaining zones (no early-return, so + // deleting the last draft on a line tears its zone down through the controller). + controller.update({ oldByLine, newByLine, - zoneClassName: 'monaco-draft-zone', - innerClassName: 'monaco-draft-zone-inner', - stopEvents: [ - 'mousedown', - 'mouseup', - 'click', - 'dblclick', - 'keydown', - 'keyup', - 'wheel', - 'contextmenu', - ], initialHeight: (ds) => Math.max(ds.length * 60, 80), render: (ds) => ( ), }); - // Does not depend on zone rebuilds triggered by autoEditTokens / registerEditTrigger (registerEditTrigger is a stable - // useCallback) — avoids trigger-induced DraftZone unmount/mount, eliminating the race of re-entering edit mode after cancel. }, [ diffEditor, drafts, diff --git a/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/zones/mountInlineZones.ts b/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/zones/mountInlineZones.ts index 717ee8c9..088baf62 100644 --- a/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/zones/mountInlineZones.ts +++ b/apps/desktop/src/renderer/src/components/features/pr/tabs/diff/zones/mountInlineZones.ts @@ -10,35 +10,14 @@ import { remapOldByLineToModified } from './line-mapping'; * `removeZone` / `unmount` cleanup. The differences (which events to intercept, initial height estimation, what component * to render) are injected via options. * - * Two entry points sharing the same internals: - * - {@link createInlineZones} returns a persistent controller whose `update(content)` **reconciles** zones by - * `(side, line)` key: an unchanged key re-renders its existing React root in place (preserving the zone's React - * state, e.g. an in-progress inline reply/edit that must survive a comments refresh / poll), a new key mounts a - * fresh zone, a vanished key is removed. Use this when the zone content changes independently of the editor/file. - * - {@link mountInlineZones} is the original one-shot form (create + populate once, teardown on cleanup), kept for - * callers that rebuild their whole zone set per effect run (the draft zones). + * {@link createInlineZones} returns a persistent controller whose `update(content)` **reconciles** zones by + * `(side, line)` key: an unchanged key re-renders its existing React root in place (preserving the zone's React + * state, e.g. an in-progress inline reply/edit/draft that must survive a comments/drafts refresh / poll), a new key + * mounts a fresh zone, a vanished key is removed. Both the inline comment zones (useCommentZones) and the inline draft + * zones (useDraftZones) drive it from a split structural/content effect pair, so their editors survive a refresh. * * The comment zone's extra glyph decorations are not managed here (the caller useCommentZones creates / clears them itself). */ -export interface MountInlineZonesOptions { - diffEditor: MonacoEditor.IStandaloneDiffEditor; - renderSideBySide: boolean; - /** Old-side (deleted / base-side context lines) buckets: key = line number */ - oldByLine: Map; - /** New-side (added / head-side context lines) buckets: key = line number */ - newByLine: Map; - /** Class of the monaco wrapper dom ('monaco-comment-zone' / 'monaco-draft-zone') */ - zoneClassName: string; - /** Class of the real visual container inner ('monaco-comment-zone-inner' / 'monaco-draft-zone-inner') */ - innerClassName: string; - /** Set of events stopPropagation takes over on dom + inner (drafts include keydown/wheel etc., comments only mouse-click kinds) */ - stopEvents: readonly string[]; - /** Initial zone height (px) estimation; lineHeight is monaco's current line height */ - initialHeight: (items: T[], lineHeight: number) => number; - /** Render the zone content (React node) */ - render: (items: T[]) => ReactNode; -} - /** Structural options for {@link createInlineZones}: everything that identifies where/how zones mount, minus the content. */ export interface CreateInlineZonesOptions { diffEditor: MonacoEditor.IStandaloneDiffEditor; @@ -269,6 +248,9 @@ export function createInlineZones(opts: CreateInlineZonesOptions): InlineZone for (const [line, items] of content.newByLine) { desired.set(`new:${line}`, { editor: modifiedEditor, afterLine: line, items }); } + // renderSideBySide here is the ACTUAL render mode (callers pass useActualRenderSideBySide, not the toolbar intent): + // only mount old-side zones on the original editor when it is genuinely visible; when Monaco has auto-degraded to + // inline the original editor is hidden, so remap old lines onto the modified editor instead. if (renderSideBySide) { for (const [line, items] of content.oldByLine) { desired.set(`old:${line}`, { editor: originalEditor, afterLine: line, items }); @@ -376,25 +358,3 @@ export function createInlineZones(opts: CreateInlineZonesOptions): InlineZone return { update, dispose }; } - -/** - * One-shot form (create + populate once, teardown on cleanup). Behaviourally identical to the pre-controller - * mechanism, kept for callers (the draft zones) that rebuild their entire zone set on each effect run. Returns a - * cleanup function to call in the effect's teardown. - */ -export function mountInlineZones(opts: MountInlineZonesOptions): () => void { - const controller = createInlineZones({ - diffEditor: opts.diffEditor, - renderSideBySide: opts.renderSideBySide, - zoneClassName: opts.zoneClassName, - innerClassName: opts.innerClassName, - stopEvents: opts.stopEvents, - }); - controller.update({ - oldByLine: opts.oldByLine, - newByLine: opts.newByLine, - initialHeight: opts.initialHeight, - render: opts.render, - }); - return () => controller.dispose(); -} diff --git a/apps/desktop/src/renderer/src/components/features/pr/tabs/shared/commentEquality.ts b/apps/desktop/src/renderer/src/components/features/pr/tabs/shared/commentEquality.ts new file mode 100644 index 00000000..afec5377 --- /dev/null +++ b/apps/desktop/src/renderer/src/components/features/pr/tabs/shared/commentEquality.ts @@ -0,0 +1,39 @@ +import type { PrComment } from '@meebox/shared'; + +/** + * Structural equality comparison of the comment tree (by remoteId + body + version + edit/delete permissions + recursive replies). poll mostly returns + * comments with unchanged content: on equality, callers skip setState and keep the old reference so React bails out, avoiding pointless re-render of the + * whole comment tree (including inline Monaco) and — in the diff view — the teardown/rebuild of open inline editors driven by the comments-array churn. + */ +export function sameCommentList(a: readonly PrComment[], b: readonly PrComment[]): boolean { + if (a.length !== b.length) return false; + for (let i = 0; i < a.length; i++) { + const x = a[i]!; + const y = b[i]!; + if ( + x.remoteId !== y.remoteId || + x.body !== y.body || + x.version !== y.version || + x.canEdit !== y.canEdit || + x.canDelete !== y.canDelete || + !sameReactions(x.reactions, y.reactions) || + !sameCommentList(x.replies, y.replies) + ) { + return false; + } + } + return true; +} + +/** Equality comparison of the reactions array (emoji + count + mine triple matching item by item): lets reaction changes after a toggle trigger a re-render. */ +function sameReactions(a: PrComment['reactions'], b: PrComment['reactions']): boolean { + const x = a ?? []; + const y = b ?? []; + if (x.length !== y.length) return false; + for (let i = 0; i < x.length; i++) { + if (x[i]!.emoji !== y[i]!.emoji || x[i]!.count !== y[i]!.count || x[i]!.mine !== y[i]!.mine) { + return false; + } + } + return true; +} diff --git a/apps/desktop/src/renderer/src/hooks/usePrNavigation.ts b/apps/desktop/src/renderer/src/hooks/usePrNavigation.ts index 4c5a457c..2b65d6d7 100644 --- a/apps/desktop/src/renderer/src/hooks/usePrNavigation.ts +++ b/apps/desktop/src/renderer/src/hooks/usePrNavigation.ts @@ -10,7 +10,9 @@ import { formatBackendError } from '../errors'; export interface PendingDiffNav { runId?: string; findingId?: string; - anchor: { path: string; startLine: number; endLine: number }; + // `side` decides which diff side the reveal targets (old = base/left, new = head/right); omitted defaults to 'new'. + // Required so an old-side comment/draft reveal lands correctly (incl. after an auto-degrade to unified — see useDiffNav). + anchor: { path: string; startLine: number; endLine: number; side?: 'old' | 'new' }; } export interface PrNavigation { @@ -156,7 +158,9 @@ export function usePrNavigation({ return subscribe('notification:activate', ({ localId, kind, anchor }) => { void jumpToPr(localId); if (anchor) { - setPendingDiffNav({ anchor: { path: anchor.path, startLine: anchor.line, endLine: anchor.line } }); + setPendingDiffNav({ + anchor: { path: anchor.path, startLine: anchor.line, endLine: anchor.line, side: anchor.side }, + }); } else if (kind === 'mention' || kind === 'reply') { setPendingTab('activity'); } diff --git a/packages/ipc/src/events.ts b/packages/ipc/src/events.ts index a0ff45fa..b7ad9ada 100644 --- a/packages/ipc/src/events.ts +++ b/packages/ipc/src/events.ts @@ -84,7 +84,7 @@ export interface IpcEvents { 'notification:activate': { localId: string; kind: PollNotificationKind; - anchor: { path: string; line: number } | null; + anchor: { path: string; line: number; side: 'old' | 'new' } | null; }; }