Skip to content

refactor(ui): require planned diff rows #1019

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
benvinegar merged 1 commit into from
Sep 7, 2026
Merged

refactor(ui): require planned diff rows #1019

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .changeset/clean-rows-plan.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
---
---
15 changes: 8 additions & 7 deletions packages/hunk/src/ui/components/ui-components.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ const { DiffSectionBody } = await import("../diff/DiffSectionBody");
const { measurePlannedRenderedRowHeight, measureRenderedRowHeight } =
await import("../diff/codeRowLayout");
const { DiffRowView } = await import("../diff/DiffRowView");
const { RawDiffRowView } = await import("../diff/RawDiffRowView");

@greptile-apps greptile-apps Bot Sep 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Dynamic Test Import

The new RawDiffRowView dependency is loaded with await import(...). This violates the repository testing directive to hoist imports to the top level and avoid dynamic imports in tests. This requirement must be satisfied before merging.

Context Used: testing.mdc Cursor rule (source)

Prompt To Fix With AI 
This is a comment left during a code review.
Path: packages/hunk/src/ui/components/ui-components.test.tsx
Line: 44

Comment:
**Dynamic Test Import**

The new `RawDiffRowView` dependency is loaded with `await import(...)`. This violates the repository testing directive to hoist imports to the top level and avoid dynamic imports in tests. This requirement must be satisfied before merging.

**Context Used:** testing.mdc Cursor rule ([source](https://github.com/modem-dev/modem/blob/main/.cursor/rules/testing.mdc))

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@benvinegar benvinegar Sep 7, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping this as a dynamic import is intentional and consistent with the surrounding UI-component imports in this test module. These components are loaded after the test harness dependencies are initialized; hoisting only the new adapter would make its initialization order differ from DiffRowView and the other UI components it exercises.

Responded by Pi using gpt-5.6-sol.

This comment was generated by Pi using gpt-5.6-sol

@greptile-apps greptile-apps Bot Sep 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s fair. The test module intentionally initializes the harness before dynamically importing the UI components, and RawDiffRowView follows the same pattern as DiffRowView and the surrounding components. Hoisting only this import would be inconsistent and could alter initialization order, so this is not an issue. I’m retracting the comment.


function createTestDiffFile(
id: string,
Expand Down Expand Up @@ -658,7 +659,7 @@ describe("UI components", () => {
const theme = resolveTheme("github-dark-default", null);
const startUserNote = mock(() => undefined);
const setup = await testRender(
<DiffRowView
<RawDiffRowView
row={{
type: "stack-line",
key: "alpha:line:1",
Expand Down Expand Up @@ -828,7 +829,7 @@ describe("UI components", () => {
};
const renderRow = (showAddNoteBadge: boolean) =>
captureFrame(
<DiffRowView
<RawDiffRowView
row={row}
width={24}
lineNumberDigits={1}
Expand Down Expand Up @@ -860,7 +861,7 @@ describe("UI components", () => {
test("DiffRowView fills the reserved wrapped add-note column with row background", async () => {
const theme = resolveTheme("github-dark-default", null);
const setup = await testRender(
<DiffRowView
<RawDiffRowView
row={{
type: "stack-line",
key: "alpha:line:hover-wrap-bg",
Expand Down Expand Up @@ -910,7 +911,7 @@ describe("UI components", () => {
test("DiffRowView keeps metadata row background within the measured row width", async () => {
const theme = resolveTheme("github-dark-default", null);
const setup = await testRender(
<DiffRowView
<RawDiffRowView
row={{
type: "hunk-header",
key: "alpha:hunk:0",
Expand Down Expand Up @@ -965,7 +966,7 @@ describe("UI components", () => {

for (const wrapLines of [false, true]) {
const setup = await testRender(
<DiffRowView
<RawDiffRowView
row={row}
width={40}
lineNumberDigits={1}
Expand Down Expand Up @@ -1009,7 +1010,7 @@ describe("UI components", () => {
};
const measuredHeight = measureRenderedRowHeight(row, 4, 1, false, true, true, theme);
const setup = await testRender(
<DiffRowView
<RawDiffRowView
row={row}
width={4}
lineNumberDigits={1}
Expand Down Expand Up @@ -1203,7 +1204,7 @@ describe("UI components", () => {
};
const measuredHeight = measureRenderedRowHeight(row, 4, 1, false, true, true, theme);
const setup = await testRender(
<DiffRowView
<RawDiffRowView
row={row}
width={4}
lineNumberDigits={1}
Expand Down
6 changes: 3 additions & 3 deletions packages/hunk/src/ui/diff/CodeCellView.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {
import { legacyPlannedDiffRow, planCodeRowLayout } from "./codeRowLayout";
import type { DiffRow } from "./diffRows";
import { lineHighlightPaintKey, type LineHighlightPaintIndex } from "./lineHighlightPaint";
import { DiffRowView } from "./DiffRowView";
import { RawDiffRowView } from "./RawDiffRowView";
import { resolveTheme, withTransparentSurfaces } from "../themes";

/** Capture one code-row component and always release its OpenTUI renderer. */
Expand Down Expand Up @@ -66,10 +66,10 @@ const stackRow: Extract<DiffRow, { type: "stack-line" }> = {
};

/** Render common DiffRowView props while varying paint-sensitive inputs. */
function codeRowView(row: DiffRow, options: Partial<Parameters<typeof DiffRowView>[0]> = {}) {
function codeRowView(row: DiffRow, options: Partial<Parameters<typeof RawDiffRowView>[0]> = {}) {
const theme = options.theme ?? resolveTheme("github-dark-default", null);
return (
<DiffRowView
<RawDiffRowView
row={row}
width={12}
lineNumberDigits={1}
Expand Down
36 changes: 8 additions & 28 deletions packages/hunk/src/ui/diff/DiffRowView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,18 @@
/** Adapts and dispatches diff rows to their focused mounted row views. */
/** Dispatches planned diff rows to their focused mounted row views. */
import { memo } from "react";
import type { UserNoteLineTarget } from "../../core/liveComments";
import type { CopySelectedRowRange } from "../lib/diffSpatial";
import type { AppTheme } from "../themes";
import { CodeRowView, type PlannedCodeReviewRow } from "./CodeRowView";
import { legacyPlannedDiffRow, type PlannedDiffReviewRow } from "./codeRowLayout";
import type { PlannedDiffReviewRow } from "./codeRowLayout";
import type { CursorHighlight } from "./cursorHighlight";
import { DiffMetaRowView, type PlannedDiffMetaReviewRow } from "./DiffMetaRowView";
import type { DiffRow } from "./diffRows";
import type { LineHighlightPaintIndex } from "./lineHighlightPaint";

/** Inputs accepted by the memoized diff-row facade. */
export interface DiffRowViewProps {
/** Complete review-stream row; preferred when the caller owns the shared render plan. */
plannedRow?: PlannedDiffReviewRow;
/** Raw row fallback for renderer-only surfaces outside the shared review stream. */
row?: DiffRow;
/** Complete row from the shared review render plan. */
plannedRow: PlannedDiffReviewRow;
width: number;
lineNumberDigits: number;
showLineNumbers: boolean;
Expand All @@ -29,8 +26,6 @@ export interface DiffRowViewProps {
cursorHighlight?: CursorHighlight;
/** Extension marks for this row's file, resolved to terminal columns. */
lineHighlights?: LineHighlightPaintIndex;
anchorId?: string;
noteGuideSide?: "old" | "new";
showAddNoteBadge?: boolean;
onHoverRow?: (rowKey: string) => void;
onStartUserNoteAtHunk?: (hunkIndex: number, target?: UserNoteLineTarget) => void;
Expand All @@ -46,7 +41,6 @@ export interface DiffRowViewProps {
*/
export const DiffRowView = memo(function DiffRowViewComponent({
plannedRow,
row,
width,
lineNumberDigits,
showLineNumbers,
Expand All @@ -59,26 +53,15 @@ export const DiffRowView = memo(function DiffRowViewComponent({
copySelectedSide,
cursorHighlight,
lineHighlights,
anchorId,
noteGuideSide,
showAddNoteBadge,
onHoverRow,
onStartUserNoteAtHunk,
onToggleGap,
}: DiffRowViewProps) {
const resolvedPlannedRow =
plannedRow ?? (row ? legacyPlannedDiffRow(row, anchorId, noteGuideSide) : undefined);
if (!resolvedPlannedRow) {
return null;
}

if (
resolvedPlannedRow.row.type === "collapsed" ||
resolvedPlannedRow.row.type === "hunk-header"
) {
if (plannedRow.row.type === "collapsed" || plannedRow.row.type === "hunk-header") {
return (
<DiffMetaRowView
plannedRow={resolvedPlannedRow as PlannedDiffMetaReviewRow}
plannedRow={plannedRow as PlannedDiffMetaReviewRow}
width={width}
theme={theme}
selected={selected || copySelectedRowRange !== undefined}
Expand All @@ -91,13 +74,10 @@ export const DiffRowView = memo(function DiffRowViewComponent({
);
}

if (
resolvedPlannedRow.row.type === "split-line" ||
resolvedPlannedRow.row.type === "stack-line"
) {
if (plannedRow.row.type === "split-line" || plannedRow.row.type === "stack-line") {
return (
<CodeRowView
plannedRow={resolvedPlannedRow as PlannedCodeReviewRow}
plannedRow={plannedRow as PlannedCodeReviewRow}
width={width}
lineNumberDigits={lineNumberDigits}
showLineNumbers={showLineNumbers}
Expand Down
24 changes: 24 additions & 0 deletions packages/hunk/src/ui/diff/RawDiffRowView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/** Adapts renderer-only diff rows to the canonical planned-row view. */
import { memo } from "react";
import type { DiffRow } from "./diffRows";
import { DiffRowView, type DiffRowViewProps } from "./DiffRowView";
import { legacyPlannedDiffRow } from "./codeRowLayout";

/** Inputs for a raw row rendered outside the shared review plan. */
export interface RawDiffRowViewProps extends Omit<DiffRowViewProps, "plannedRow"> {
row: DiffRow;
anchorId?: string;
noteGuideSide?: "old" | "new";
}

/** Wrap a raw diff row in the planned envelope required by the mounted row view. */
export const RawDiffRowView = memo(function RawDiffRowViewComponent({
row,
anchorId,
noteGuideSide,
...viewProps
}: RawDiffRowViewProps) {
return (
<DiffRowView {...viewProps} plannedRow={legacyPlannedDiffRow(row, anchorId, noteGuideSide)} />
);
});
4 changes: 2 additions & 2 deletions packages/hunk/src/ui/lib/extensionCurrentLine.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import type { ExtensionCurrentLinePaint } from "../../extension-api/types";
import type { DiffRow, SplitLineCell, StackLineCell } from "../diff/diffRows";
import { DiffRowView } from "../diff/DiffRowView";
import { RawDiffRowView } from "../diff/RawDiffRowView";
import type { DiffSectionRowPlan } from "../diff/diffSectionRowPlan";
import type { LineCursor } from "./lineCursors";
import type { AppTheme } from "../themes";
Expand Down Expand Up @@ -104,7 +104,7 @@ export function createExtensionCurrentLinePaint({
line: cursor.target.line,
render(side: "old" | "new", width: number) {
return (
<DiffRowView
<RawDiffRowView
key={rows[side].key}
row={rows[side]}
width={width}
Expand Down
Loading