Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,12 @@ class T3ReviewDiffModule : Module() {
AsyncFunction("scrollToTop") { view: T3ReviewDiffView, animated: Boolean ->
view.scrollToTop(animated)
}
AsyncFunction("setRowsJson") { view: T3ReviewDiffView, rowsJson: String ->
AsyncFunction("setRowsJson") {
view: T3ReviewDiffView,
rowsJson: String,
contentResetKey: String
->
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
view.setContentResetKey(contentResetKey)
view.setRowsJson(rowsJson)
}
AsyncFunction("setTokensJson") { view: T3ReviewDiffView, tokensJson: String ->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,8 @@ public class T3ReviewDiffModule: Module {
// Large, frequently changing JSON values cannot be regular Fabric props. Expo's
// prop adapter compares strings on the main thread before invoking a setter, which
// makes a syntax-token patch capable of blocking a frame by itself.
AsyncFunction("setRowsJson") { (view: T3ReviewDiffView, rowsJson: String) in
AsyncFunction("setRowsJson") { (view: T3ReviewDiffView, rowsJson: String, contentResetKey: String) in
view.setContentResetKey(contentResetKey)
view.setRowsJson(rowsJson)
}

Expand Down
46 changes: 41 additions & 5 deletions apps/mobile/src/features/diffs/nativeReviewDiffSurface.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ const mocks = vi.hoisted(() => ({
tokenCompletion: null as Promise<boolean> | null,
onDebug: undefined as NativeReviewDiffViewProps["onDebug"],
autoViewport: false,
contentResetKeyLandsLate: false,
rows: vi.fn(),
tokens: vi.fn(),
highlight: vi.fn(),
Expand All @@ -30,22 +31,30 @@ function RawView({
ref: nativeRef,
onDebug,
initialFileId,
contentResetKey,
contentResetKey = "",
}: NativeReviewDiffViewProps & { ref: Ref<unknown> }) {
const [content, setContent] = useState({ resetKey: contentResetKey, rows: "" });
const rows = content.resetKey === contentResetKey ? content.rows : "";
// Native setContentResetKey clears rows unless the key is unchanged.
const setContentResetKey = (resetKey: string) =>
setContent((current) => (current.resetKey === resetKey ? current : { resetKey, rows: "" }));
const [tokenRowIds, setTokenRowIds] = useState<string[]>([]);
// Android Fabric can apply a prop update after an AsyncFunction call made later.
if (!mocks.contentResetKeyLandsLate && content.resetKey !== contentResetKey) {
setContentResetKey(contentResetKey);
}
useLayoutEffect(() => {
mocks.onDebug = onDebug;
});
useImperativeHandle(nativeRef, () => ({
async setRowsJson(value: string) {
async setRowsJson(value: string, resetKey?: string) {
mocks.rows(value);
if (mocks.registrationFailures > 0) {
mocks.registrationFailures -= 1;
throw new Error("Unable to find the 'T3ReviewDiffSurface' view for this native tag");
}
setContent({ resetKey: contentResetKey, rows: value });
if (resetKey !== undefined) setContentResetKey(resetKey);
setContent((current) => ({ ...current, rows: value }));
setContentResetKey(contentResetKey);
if (mocks.autoViewport) {
const decoded = JSON.parse(value) as NativeReviewDiffData["rows"];
onDebug?.({
Expand Down Expand Up @@ -79,7 +88,7 @@ function RawView({
}));
return (
<div data-token-rows={tokenRowIds.join(",")}>
{rows ? "Rendered diff" : "Blank native surface"}
{content.rows ? "Rendered diff" : "Blank native surface"}
</div>
);
}
Expand Down Expand Up @@ -141,6 +150,7 @@ function setup(integrated = false) {
mocks.tokenCompletion = null;
mocks.onDebug = undefined;
mocks.autoViewport = false;
mocks.contentResetKeyLandsLate = false;
mocks.rows.mockClear();
mocks.tokens.mockClear();
mocks.highlight
Expand Down Expand Up @@ -251,6 +261,20 @@ function setup(integrated = false) {
async mountPullRequest(targetFileId?: string) {
await act(async () => root.render(<PullRequestDiff targetFileId={targetFileId} />));
},
async showSection(contentResetKey: string) {
await act(async () =>
root.render(
<NativeDiff
rowsJson='[{"kind":"line","id":"r0"}]'
contentResetKey={contentResetKey}
appearanceScheme="light"
themeJson="{}"
rowHeight={20}
contentWidth={300}
/>,
),
);
},
async collapseFiles(fileIds: ReadonlyArray<string>) {
collapsedFileIds = fileIds;
await act(async () => root.render(<PullRequestDiff />));
Expand Down Expand Up @@ -321,6 +345,18 @@ it("retries transient native registration without replacing the diff with an err
expect(screen.container.textContent).toBe("Rendered diff");
});

it("renders identical rows after switching sections when the reset key prop lands late", async () => {
using screen = setup(true);
await screen.showSection("thread:git:changes");
await screen.deliverPayloads();
expect(screen.container.textContent).toBe("Rendered diff");

mocks.contentResetKeyLandsLate = true;
await screen.showSection("thread:git:working-tree");
await screen.deliverPayloads();
expect(screen.container.textContent).toBe("Rendered diff");
});

it.each([false, undefined])(
"keeps colors queued until native accepts them and recovers rejected patches (%s)",
async (rejected) => {
Expand Down
14 changes: 8 additions & 6 deletions apps/mobile/src/features/diffs/nativeReviewDiffSurface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ export function isNativeReviewDiffDrawEvent(payload: Readonly<Record<string, unk
}

interface NativeReviewDiffViewRef {
readonly setRowsJson: (rowsJson: string) => Promise<void>;
readonly setRowsJson: (rowsJson: string, contentResetKey: string) => Promise<void>;
readonly setTokensJson: (tokensJson: string) => Promise<void>;
readonly setTokensPatchJson: (tokensPatchJson: string) => Promise<boolean>;
readonly scrollToFile: (fileId: string, animated: boolean) => Promise<void>;
Expand Down Expand Up @@ -216,8 +216,7 @@ function useNativeReviewDiffPayload(
}

const view = nativeRef.current;
const command: ((payload: string) => Promise<void | boolean>) | undefined = view?.[method];
if (!view || !command) {
if (!view?.[method]) {
if (attempts < NATIVE_REVIEW_DIFF_PAYLOAD_RETRY_FRAMES) {
attempts += 1;
frame = requestAnimationFrame(dispatch);
Expand All @@ -229,8 +228,12 @@ function useNativeReviewDiffPayload(
}

void Promise.resolve()
.then(() => {
if (!cancelled) return command.call(view, payload);
.then((): Promise<void | boolean> | undefined => {
if (cancelled) return;
// Fabric can apply the contentResetKey prop after this call, so rows carry the key.
return method === "setRowsJson"
? view.setRowsJson(payload, resetKey ?? "")
: view[method](payload);
})
.then((accepted) => {
if (cancelled) return;
Expand Down Expand Up @@ -264,7 +267,6 @@ function useNativeReviewDiffPayload(
cancelAnimationFrame(frame);
}
};
// oxlint-disable-next-line react/exhaustive-effect-dependencies -- A new resetKey redelivers an unchanged payload after the diff surface resets.
}, [method, nativeRef, payload, resetKey, onFailure, onDelivered]);
}

Expand Down
Loading