Implement shared compact split and unified tool-call diff layout (#270)
# PR Title Implement shared compact split and unified tool-call diff layout --- Fixes #268 # PR Description ## Summary This PR makes tool-call diffs more compact in both `Unified` and `Split` views by reducing wasted horizontal space in line-number gutters and content indentation. ## What changed - introduced a shared compact-diff framework for tool-call diffs - kept mobile-specific policy limited to: - forcing unified mode below the breakpoint - enabling wrap only in mobile unified mode - added mode-specific compact applicators in the diff viewer: - unified applicator - split applicator - reduced gutter width waste by measuring rendered line-number text and tightening column width around it - removed unnecessary right-side content padding - aligned `+` / `-` markers closer to the left edge across both views - simplified cleanup after gatekeeper review by removing extra plumbing and residue ## Screenshots ### Before <img width="581" height="341" alt="image" src="https://github.com/user-attachments/assets/ec47b256-749a-4afc-8879-aaf33f0b46b6" /> ### After <img width="470" height="586" alt="image" src="https://github.com/user-attachments/assets/7258a5a2-47c4-408d-84bc-1b497761c7ad" /> ## Architectural approach This change intentionally uses: - shared policy in `packages/ui/src/components/tool-call/diff-render.tsx` - shared helper/measurement logic in `packages/ui/src/components/diff-viewer.tsx` - mode-specific applicators where unified and split DOM differ - CSS for shared visual spacing and alignment cleanup The goal was to keep the implementation architecturally clean and avoid building separate duplicated compact-diff features for: - mobile vs desktop - unified vs split Instead, the feature shares one compact-diff concept and only diverges where the upstream diff DOM requires separate handling. ## Files changed - `packages/ui/src/components/tool-call/diff-render.tsx` - `packages/ui/src/components/diff-viewer.tsx` - `packages/ui/src/styles/messaging/tool-call.css` - `packages/ui/src/types/message.ts` ## Validation Manual validation was performed in the running UI. Verified manually: - compact unified gutters on mobile - compact unified gutters on desktop - compact split gutters on desktop - tighter operator alignment in both modes Also verified: - `npm run typecheck` passes ## Notes - This PR is intended to address the compact diff layout problem described in the related issue. - Diff-specific CSS still lives in `tool-call.css`; future extraction into a smaller dedicated stylesheet is possible but not required for this change. --------- Co-authored-by: Shantur Rathore <i@shantur.com>
This commit is contained in:
VooDisss
@@ -1,4 +1,4 @@
|
||||
import { createMemo, Show, createEffect, onCleanup } from "solid-js"
|
||||
import { createMemo, Show, createEffect } from "solid-js"
|
||||
import { DiffView, DiffModeEnum } from "@git-diff-view/solid"
|
||||
import "@git-diff-view/solid/styles/diff-view-pure.css"
|
||||
import { disableCache } from "@git-diff-view/core"
|
||||
@@ -20,6 +20,7 @@ interface ToolCallDiffViewerProps {
|
||||
filePath?: string
|
||||
theme: "light" | "dark"
|
||||
mode: DiffViewMode
|
||||
wrap?: boolean
|
||||
onRendered?: () => void
|
||||
cachedHtml?: string
|
||||
cacheEntryParams?: CacheEntryParams
|
||||
@@ -31,11 +32,183 @@ type DiffData = {
|
||||
hunks: string[]
|
||||
}
|
||||
|
||||
type CaptureContext = {
|
||||
theme: ToolCallDiffViewerProps["theme"]
|
||||
mode: DiffViewMode
|
||||
diffText: string
|
||||
cacheEntryParams?: CacheEntryParams
|
||||
function measureTextWidth(container: HTMLElement, text: string, source: HTMLElement) {
|
||||
const computed = window.getComputedStyle(source)
|
||||
const probe = document.createElement("span")
|
||||
probe.textContent = text || ""
|
||||
probe.style.position = "absolute"
|
||||
probe.style.visibility = "hidden"
|
||||
probe.style.pointerEvents = "none"
|
||||
probe.style.display = "inline-block"
|
||||
probe.style.width = "auto"
|
||||
probe.style.maxWidth = "none"
|
||||
probe.style.whiteSpace = "nowrap"
|
||||
probe.style.fontFamily = computed.fontFamily
|
||||
probe.style.fontSize = computed.fontSize
|
||||
probe.style.fontWeight = computed.fontWeight
|
||||
probe.style.fontStyle = computed.fontStyle
|
||||
probe.style.letterSpacing = computed.letterSpacing
|
||||
probe.style.fontVariant = computed.fontVariant
|
||||
probe.style.textTransform = computed.textTransform
|
||||
probe.style.lineHeight = computed.lineHeight
|
||||
container.appendChild(probe)
|
||||
const width = Math.ceil(probe.getBoundingClientRect().width)
|
||||
probe.remove()
|
||||
return width
|
||||
}
|
||||
|
||||
function computeCompactWidth(
|
||||
container: HTMLElement,
|
||||
entries: Array<{ text: string; source: HTMLElement }>,
|
||||
maxWidthPx = 40,
|
||||
) {
|
||||
const measuredLabelWidthPx = entries.reduce((max, entry) => {
|
||||
return Math.max(max, measureTextWidth(container, entry.text, entry.source))
|
||||
}, 0)
|
||||
const fallbackTextLength = entries.reduce((max, entry) => Math.max(max, entry.text.length), 1)
|
||||
const fallbackWidthPx = Math.round(fallbackTextLength * 7 + 4)
|
||||
return Math.max(2, Math.min(maxWidthPx, measuredLabelWidthPx > 0 ? measuredLabelWidthPx + 2 : fallbackWidthPx))
|
||||
}
|
||||
|
||||
function applyCompactUnifiedGutter(container: HTMLElement, wrap: boolean) {
|
||||
const tableWrapper = container.querySelector<HTMLElement>(".unified-diff-table-wrapper")
|
||||
const table = container.querySelector<HTMLTableElement>(".unified-diff-table")
|
||||
const numberCol = container.querySelector<HTMLTableColElement>(".unified-diff-table-num-col")
|
||||
const gutterRows = container.querySelectorAll<HTMLElement>(".diff-line-num")
|
||||
const hunkGutters = container.querySelectorAll<HTMLElement>(".diff-line-hunk-action, .diff-line-widget-wrapper, .diff-line-extend-wrapper")
|
||||
const entries: Array<{ gutter: HTMLElement; label: HTMLElement; text: string }> = []
|
||||
|
||||
if (table) {
|
||||
if (wrap) {
|
||||
table.classList.add("table-fixed")
|
||||
table.style.tableLayout = "fixed"
|
||||
table.style.width = "100%"
|
||||
table.style.minWidth = "100%"
|
||||
} else {
|
||||
table.classList.remove("table-fixed")
|
||||
table.style.tableLayout = "auto"
|
||||
table.style.width = "max-content"
|
||||
table.style.minWidth = "100%"
|
||||
}
|
||||
}
|
||||
|
||||
gutterRows.forEach((gutter) => {
|
||||
const oldSpan = gutter.querySelector<HTMLElement>("[data-line-old-num]")
|
||||
const newSpan = gutter.querySelector<HTMLElement>("[data-line-new-num]")
|
||||
const spacer = gutter.querySelector<HTMLElement>(".shrink-0")
|
||||
const flexWrapper = gutter.querySelector<HTMLElement>(":scope > .flex")
|
||||
const currentLabel = gutter.querySelector<HTMLElement>(":scope > .tool-call-diff-compact-line-number")
|
||||
|
||||
const oldText = oldSpan?.textContent?.trim() ?? ""
|
||||
const newText = newSpan?.textContent?.trim() ?? ""
|
||||
const hasUsableNew = newText.length > 0 && newText !== "0"
|
||||
const hasUsableOld = oldText.length > 0 && oldText !== "0"
|
||||
const visibleText = hasUsableNew ? newText : hasUsableOld ? oldText : newText || oldText
|
||||
|
||||
if (flexWrapper) flexWrapper.style.display = "none"
|
||||
if (spacer) spacer.style.display = "none"
|
||||
if (oldSpan) { oldSpan.style.display = "none"; oldSpan.style.width = "auto" }
|
||||
if (newSpan) { newSpan.style.display = "none"; newSpan.style.width = "auto" }
|
||||
|
||||
gutter.style.paddingLeft = "1px"
|
||||
gutter.style.paddingRight = "1px"
|
||||
gutter.style.textAlign = "left"
|
||||
|
||||
const label = currentLabel ?? document.createElement("span")
|
||||
label.className = "tool-call-diff-compact-line-number"
|
||||
label.textContent = visibleText
|
||||
label.setAttribute("aria-hidden", visibleText ? "false" : "true")
|
||||
if (!currentLabel) gutter.appendChild(label)
|
||||
|
||||
entries.push({ gutter, label, text: visibleText })
|
||||
})
|
||||
|
||||
const gutterWidthPx = computeCompactWidth(container, entries.map((entry) => ({ text: entry.text, source: entry.label })))
|
||||
const gutterWidth = `${gutterWidthPx}px`
|
||||
const compactAsideWidth = `${Math.max(8, gutterWidthPx - 10)}px`
|
||||
|
||||
if (tableWrapper) {
|
||||
tableWrapper.style.setProperty("--diff-aside-width", compactAsideWidth)
|
||||
tableWrapper.style.setProperty("--diff-aside-width--", compactAsideWidth)
|
||||
}
|
||||
if (numberCol) {
|
||||
numberCol.style.width = gutterWidth
|
||||
}
|
||||
|
||||
entries.forEach(({ gutter, label }) => {
|
||||
gutter.style.width = gutterWidth
|
||||
gutter.style.minWidth = gutterWidth
|
||||
gutter.style.maxWidth = gutterWidth
|
||||
label.style.width = "auto"
|
||||
label.style.maxWidth = "none"
|
||||
})
|
||||
|
||||
hunkGutters.forEach((gutter) => {
|
||||
gutter.style.width = gutterWidth
|
||||
gutter.style.minWidth = gutterWidth
|
||||
gutter.style.maxWidth = gutterWidth
|
||||
gutter.style.paddingLeft = "0"
|
||||
gutter.style.paddingRight = "0"
|
||||
})
|
||||
}
|
||||
|
||||
function applyCompactSplitGutter(container: HTMLElement) {
|
||||
const oldWrapper = container.querySelector<HTMLElement>(".old-diff-table-wrapper")
|
||||
const newWrapper = container.querySelector<HTMLElement>(".new-diff-table-wrapper")
|
||||
const numberCells = Array.from(container.querySelectorAll<HTMLElement>(".diff-line-old-num, .diff-line-new-num"))
|
||||
const hunkActions = Array.from(container.querySelectorAll<HTMLElement>(".diff-line-hunk-action, .diff-line-widget-wrapper, .diff-line-extend-wrapper"))
|
||||
const numberSpans = numberCells
|
||||
.map((cell) => ({ cell, span: cell.querySelector<HTMLElement>("[data-line-num]") }))
|
||||
.filter((entry): entry is { cell: HTMLElement; span: HTMLElement } => Boolean(entry.span))
|
||||
|
||||
const gutterWidthPx = computeCompactWidth(
|
||||
container,
|
||||
numberSpans.map(({ span }) => ({ text: span.textContent?.trim() ?? "", source: span })),
|
||||
64,
|
||||
)
|
||||
const gutterWidth = `${gutterWidthPx}px`
|
||||
|
||||
;[oldWrapper, newWrapper].forEach((wrapper) => {
|
||||
if (wrapper) {
|
||||
wrapper.style.setProperty("--diff-aside-width", gutterWidth)
|
||||
}
|
||||
})
|
||||
|
||||
numberCells.forEach((cell) => {
|
||||
cell.style.width = gutterWidth
|
||||
cell.style.minWidth = gutterWidth
|
||||
cell.style.maxWidth = gutterWidth
|
||||
cell.style.paddingLeft = "2px"
|
||||
cell.style.paddingRight = "2px"
|
||||
cell.style.textAlign = "left"
|
||||
cell.style.whiteSpace = "nowrap"
|
||||
cell.style.overflowWrap = "normal"
|
||||
cell.style.wordBreak = "normal"
|
||||
})
|
||||
|
||||
numberSpans.forEach(({ span }) => {
|
||||
span.style.whiteSpace = "nowrap"
|
||||
span.style.overflowWrap = "normal"
|
||||
span.style.wordBreak = "normal"
|
||||
})
|
||||
|
||||
hunkActions.forEach((cell) => {
|
||||
cell.style.width = gutterWidth
|
||||
cell.style.minWidth = gutterWidth
|
||||
cell.style.maxWidth = gutterWidth
|
||||
cell.style.paddingLeft = "0"
|
||||
cell.style.paddingRight = "0"
|
||||
})
|
||||
}
|
||||
|
||||
function applyCompactDiffLayout(container: HTMLElement, mode: DiffViewMode, wrap = false) {
|
||||
if (mode === "unified") {
|
||||
applyCompactUnifiedGutter(container, wrap)
|
||||
return
|
||||
}
|
||||
if (mode === "split") {
|
||||
applyCompactSplitGutter(container)
|
||||
}
|
||||
}
|
||||
|
||||
export function ToolCallDiffViewer(props: ToolCallDiffViewerProps) {
|
||||
@@ -67,12 +240,15 @@ export function ToolCallDiffViewer(props: ToolCallDiffViewerProps) {
|
||||
const contextKey = createMemo(() => {
|
||||
const data = diffData()
|
||||
if (!data) return ""
|
||||
return `${props.theme}|${props.mode}|${props.diffText}`
|
||||
return `${props.theme}|${props.mode}|${props.wrap ? "wrap" : "nowrap"}|${props.diffText}`
|
||||
})
|
||||
|
||||
createEffect(() => {
|
||||
const cachedHtml = props.cachedHtml
|
||||
if (cachedHtml) {
|
||||
if (diffContainerRef) {
|
||||
applyCompactDiffLayout(diffContainerRef, props.mode, Boolean(props.wrap))
|
||||
}
|
||||
// When we are given cached HTML, we rely on the caller's cache
|
||||
// and simply notify once rendered.
|
||||
props.onRendered?.()
|
||||
@@ -83,9 +259,10 @@ export function ToolCallDiffViewer(props: ToolCallDiffViewerProps) {
|
||||
if (!key) return
|
||||
if (!diffContainerRef) return
|
||||
if (lastCapturedKey === key) return
|
||||
|
||||
|
||||
requestAnimationFrame(() => {
|
||||
if (!diffContainerRef) return
|
||||
applyCompactDiffLayout(diffContainerRef, props.mode, Boolean(props.wrap))
|
||||
const markup = diffContainerRef.innerHTML
|
||||
if (!markup) return
|
||||
lastCapturedKey = key
|
||||
@@ -95,6 +272,7 @@ export function ToolCallDiffViewer(props: ToolCallDiffViewerProps) {
|
||||
html: markup,
|
||||
theme: props.theme,
|
||||
mode: props.mode,
|
||||
wrap: props.wrap,
|
||||
})
|
||||
}
|
||||
props.onRendered?.()
|
||||
@@ -122,7 +300,7 @@ export function ToolCallDiffViewer(props: ToolCallDiffViewerProps) {
|
||||
diffViewMode={props.mode === "split" ? DiffModeEnum.Split : DiffModeEnum.Unified}
|
||||
diffViewTheme={props.theme}
|
||||
diffViewHighlight
|
||||
diffViewWrap={false}
|
||||
diffViewWrap={Boolean(props.wrap)}
|
||||
diffViewFontSize={13}
|
||||
/>
|
||||
</ErrorBoundary>
|
||||
@@ -131,7 +309,7 @@ export function ToolCallDiffViewer(props: ToolCallDiffViewerProps) {
|
||||
</div>
|
||||
}
|
||||
>
|
||||
<div innerHTML={props.cachedHtml} />
|
||||
<div ref={diffContainerRef} innerHTML={props.cachedHtml} />
|
||||
</Show>
|
||||
</div>
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user