Skip to content

Commit 46361cf

Browse files
adamdotdevinadamdotdevin
committed
fix(app): session review re-rendering too aggressively
1 parent c09d3dd commit 46361cf

1 file changed

Lines changed: 67 additions & 47 deletions

File tree

‎packages/ui/src/components/session-review.tsx‎

Lines changed: 67 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -189,8 +189,10 @@ export const SessionReview = (props: SessionReviewProps) => {
189189
const [opened, setOpened] = createSignal<SessionReviewFocus | null>(null)
190190

191191
const open = () => props.open ?? store.open
192+
const files = createMemo(() => props.diffs.map((d) => d.file))
193+
const diffs = createMemo(() => new Map(props.diffs.map((d) => [d.file, d] as const)))
192194
const diffStyle = () => props.diffStyle ?? (props.split ? "split" : "unified")
193-
const hasDiffs = () => props.diffs.length > 0
195+
const hasDiffs = () => files().length > 0
194196

195197
const handleChange = (open: string[]) => {
196198
props.onOpenChange?.(open)
@@ -199,7 +201,7 @@ export const SessionReview = (props: SessionReviewProps) => {
199201
}
200202

201203
const handleExpandOrCollapseAll = () => {
202-
const next = open().length > 0 ? [] : props.diffs.map((d) => d.file)
204+
const next = open().length > 0 ? [] : files()
203205
handleChange(next)
204206
}
205207

@@ -322,51 +324,54 @@ export const SessionReview = (props: SessionReviewProps) => {
322324
<div data-slot="session-review-container" class={props.classes?.container}>
323325
<Show when={hasDiffs()} fallback={props.empty}>
324326
<Accordion multiple value={open()} onChange={handleChange}>
325-
<For each={props.diffs}>
326-
{(diff) => {
327+
<For each={files()}>
328+
{(file) => {
327329
let wrapper: HTMLDivElement | undefined
328330

329-
const expanded = createMemo(() => open().includes(diff.file))
331+
const diff = createMemo(() => diffs().get(file))
332+
const item = () => diff()!
333+
334+
const expanded = createMemo(() => open().includes(file))
330335
const [force, setForce] = createSignal(false)
331336

332-
const comments = createMemo(() => (props.comments ?? []).filter((c) => c.file === diff.file))
337+
const comments = createMemo(() => (props.comments ?? []).filter((c) => c.file === file))
333338
const commentedLines = createMemo(() => comments().map((c) => c.selection))
334339

335-
const beforeText = () => (typeof diff.before === "string" ? diff.before : "")
336-
const afterText = () => (typeof diff.after === "string" ? diff.after : "")
337-
const changedLines = () => diff.additions + diff.deletions
340+
const beforeText = () => (typeof item().before === "string" ? item().before : "")
341+
const afterText = () => (typeof item().after === "string" ? item().after : "")
342+
const changedLines = () => item().additions + item().deletions
338343

339344
const tooLarge = createMemo(() => {
340345
if (!expanded()) return false
341346
if (force()) return false
342-
if (isImageFile(diff.file)) return false
347+
if (isImageFile(file)) return false
343348
return changedLines() > MAX_DIFF_CHANGED_LINES
344349
})
345350

346-
const isAdded = () => diff.status === "added" || (beforeText().length === 0 && afterText().length > 0)
351+
const isAdded = () => item().status === "added" || (beforeText().length === 0 && afterText().length > 0)
347352
const isDeleted = () =>
348-
diff.status === "deleted" || (afterText().length === 0 && beforeText().length > 0)
349-
const isImage = () => isImageFile(diff.file)
350-
const isAudio = () => isAudioFile(diff.file)
353+
item().status === "deleted" || (afterText().length === 0 && beforeText().length > 0)
354+
const isImage = () => isImageFile(file)
355+
const isAudio = () => isAudioFile(file)
351356

352-
const diffImageSrc = dataUrlFromValue(diff.after) ?? dataUrlFromValue(diff.before)
353-
const [imageSrc, setImageSrc] = createSignal<string | undefined>(diffImageSrc)
357+
const diffImageSrc = createMemo(() => dataUrlFromValue(item().after) ?? dataUrlFromValue(item().before))
358+
const [imageSrc, setImageSrc] = createSignal<string | undefined>(diffImageSrc())
354359
const [imageStatus, setImageStatus] = createSignal<"idle" | "loading" | "error">("idle")
355360

356-
const diffAudioSrc = dataUrlFromValue(diff.after) ?? dataUrlFromValue(diff.before)
357-
const [audioSrc, setAudioSrc] = createSignal<string | undefined>(diffAudioSrc)
361+
const diffAudioSrc = createMemo(() => dataUrlFromValue(item().after) ?? dataUrlFromValue(item().before))
362+
const [audioSrc, setAudioSrc] = createSignal<string | undefined>(diffAudioSrc())
358363
const [audioStatus, setAudioStatus] = createSignal<"idle" | "loading" | "error">("idle")
359364
const [audioMime, setAudioMime] = createSignal<string | undefined>(undefined)
360365

361366
const selectedLines = createMemo(() => {
362367
const current = selection()
363-
if (!current || current.file !== diff.file) return null
368+
if (!current || current.file !== file) return null
364369
return current.range
365370
})
366371

367372
const draftRange = createMemo(() => {
368373
const current = commenting()
369-
if (!current || current.file !== diff.file) return null
374+
if (!current || current.file !== file) return null
370375
return current.range
371376
})
372377

@@ -417,6 +422,21 @@ export const SessionReview = (props: SessionReviewProps) => {
417422
requestAnimationFrame(updateAnchors)
418423
}
419424

425+
createEffect(() => {
426+
if (!isImage()) return
427+
const src = diffImageSrc()
428+
setImageSrc(src)
429+
setImageStatus("idle")
430+
})
431+
432+
createEffect(() => {
433+
if (!isAudio()) return
434+
const src = diffAudioSrc()
435+
setAudioSrc(src)
436+
setAudioStatus("idle")
437+
setAudioMime(undefined)
438+
})
439+
420440
createEffect(() => {
421441
comments()
422442
scheduleAnchors()
@@ -430,7 +450,7 @@ export const SessionReview = (props: SessionReviewProps) => {
430450
})
431451

432452
createEffect(() => {
433-
if (!open().includes(diff.file)) return
453+
if (!open().includes(file)) return
434454
if (!isImage()) return
435455
if (imageSrc()) return
436456
if (imageStatus() !== "idle") return
@@ -440,7 +460,7 @@ export const SessionReview = (props: SessionReviewProps) => {
440460
if (!reader) return
441461

442462
setImageStatus("loading")
443-
reader(diff.file)
463+
reader(file)
444464
.then((result) => {
445465
const src = dataUrl(result)
446466
if (!src) {
@@ -456,7 +476,7 @@ export const SessionReview = (props: SessionReviewProps) => {
456476
})
457477

458478
createEffect(() => {
459-
if (!open().includes(diff.file)) return
479+
if (!open().includes(file)) return
460480
if (!isAudio()) return
461481
if (audioSrc()) return
462482
if (audioStatus() !== "idle") return
@@ -465,7 +485,7 @@ export const SessionReview = (props: SessionReviewProps) => {
465485
if (!reader) return
466486

467487
setAudioStatus("loading")
468-
reader(diff.file)
488+
reader(file)
469489
.then((result) => {
470490
const src = dataUrl(result)
471491
if (!src) {
@@ -489,7 +509,7 @@ export const SessionReview = (props: SessionReviewProps) => {
489509
return
490510
}
491511

492-
setSelection({ file: diff.file, range })
512+
setSelection({ file, range })
493513
}
494514

495515
const handleLineSelectionEnd = (range: SelectedLineRange | null) => {
@@ -500,8 +520,8 @@ export const SessionReview = (props: SessionReviewProps) => {
500520
return
501521
}
502522

503-
setSelection({ file: diff.file, range })
504-
setCommenting({ file: diff.file, range })
523+
setSelection({ file, range })
524+
setCommenting({ file, range })
505525
}
506526

507527
const openComment = (comment: SessionReviewComment) => {
@@ -517,22 +537,22 @@ export const SessionReview = (props: SessionReviewProps) => {
517537

518538
return (
519539
<Accordion.Item
520-
value={diff.file}
521-
id={diffId(diff.file)}
522-
data-file={diff.file}
540+
value={file}
541+
id={diffId(file)}
542+
data-file={file}
523543
data-slot="session-review-accordion-item"
524-
data-selected={props.focusedFile === diff.file ? "" : undefined}
544+
data-selected={props.focusedFile === file ? "" : undefined}
525545
>
526546
<StickyAccordionHeader>
527547
<Accordion.Trigger>
528548
<div data-slot="session-review-trigger-content">
529549
<div data-slot="session-review-file-info">
530-
<FileIcon node={{ path: diff.file, type: "file" }} />
550+
<FileIcon node={{ path: file, type: "file" }} />
531551
<div data-slot="session-review-file-name-container">
532-
<Show when={diff.file.includes("/")}>
533-
<span data-slot="session-review-directory">{`\u202A${getDirectory(diff.file)}\u202C`}</span>
552+
<Show when={file.includes("/")}>
553+
<span data-slot="session-review-directory">{`\u202A${getDirectory(file)}\u202C`}</span>
534554
</Show>
535-
<span data-slot="session-review-filename">{getFilename(diff.file)}</span>
555+
<span data-slot="session-review-filename">{getFilename(file)}</span>
536556
<Show when={props.onViewFile}>
537557
<Tooltip value="Open file" placement="top" gutter={4}>
538558
<button
@@ -541,7 +561,7 @@ export const SessionReview = (props: SessionReviewProps) => {
541561
aria-label="Open file"
542562
onClick={(e) => {
543563
e.stopPropagation()
544-
props.onViewFile?.(diff.file)
564+
props.onViewFile?.(file)
545565
}}
546566
>
547567
<Icon name="open-file" size="small" />
@@ -557,7 +577,7 @@ export const SessionReview = (props: SessionReviewProps) => {
557577
<span data-slot="session-review-change" data-type="added">
558578
{i18n.t("ui.sessionReview.change.added")}
559579
</span>
560-
<DiffChanges changes={diff} />
580+
<DiffChanges changes={item()} />
561581
</div>
562582
</Match>
563583
<Match when={isDeleted()}>
@@ -571,7 +591,7 @@ export const SessionReview = (props: SessionReviewProps) => {
571591
</span>
572592
</Match>
573593
<Match when={true}>
574-
<DiffChanges changes={diff} />
594+
<DiffChanges changes={item()} />
575595
</Match>
576596
</Switch>
577597
<span data-slot="session-review-diff-chevron">
@@ -586,15 +606,15 @@ export const SessionReview = (props: SessionReviewProps) => {
586606
data-slot="session-review-diff-wrapper"
587607
ref={(el) => {
588608
wrapper = el
589-
anchors.set(diff.file, el)
609+
anchors.set(file, el)
590610
scheduleAnchors()
591611
}}
592612
>
593613
<Show when={expanded()}>
594614
<Switch>
595615
<Match when={isImage() && imageSrc()}>
596616
<div data-slot="session-review-image-container">
597-
<img data-slot="session-review-image" src={imageSrc()} alt={diff.file} />
617+
<img data-slot="session-review-image" src={imageSrc()} alt={file} />
598618
</div>
599619
</Match>
600620
<Match when={isImage() && isDeleted()}>
@@ -634,7 +654,7 @@ export const SessionReview = (props: SessionReviewProps) => {
634654
<Match when={!isImage()}>
635655
<Dynamic
636656
component={diffComponent}
637-
preloadedDiff={diff.preloaded}
657+
preloadedDiff={item().preloaded}
638658
diffStyle={diffStyle()}
639659
onRendered={() => {
640660
props.onDiffRendered?.()
@@ -646,12 +666,12 @@ export const SessionReview = (props: SessionReviewProps) => {
646666
selectedLines={selectedLines()}
647667
commentedLines={commentedLines()}
648668
before={{
649-
name: diff.file!,
650-
contents: typeof diff.before === "string" ? diff.before : "",
669+
name: file,
670+
contents: typeof item().before === "string" ? item().before : "",
651671
}}
652672
after={{
653-
name: diff.file!,
654-
contents: typeof diff.after === "string" ? diff.after : "",
673+
name: file,
674+
contents: typeof item().after === "string" ? item().after : "",
655675
}}
656676
/>
657677
</Match>
@@ -689,10 +709,10 @@ export const SessionReview = (props: SessionReviewProps) => {
689709
onCancel={() => setCommenting(null)}
690710
onSubmit={(comment) => {
691711
props.onLineComment?.({
692-
file: diff.file,
712+
file,
693713
selection: range(),
694714
comment,
695-
preview: selectionPreview(diff, range()),
715+
preview: selectionPreview(item(), range()),
696716
})
697717
setCommenting(null)
698718
}}

0 commit comments

Comments
 (0)