PR diff fails with "Cannot read properties of null (reading 'patch')" and sticks on "loading diff…" with Files (0) #520

New issue

Closed

opened 2026-09-14 15:46:25 +00:00 by crueber · 3 comments

crueber commented

2026-09-14 15:46:25 +00:00

Owner

What's requested

Opening a PR's Files tab (and in the same failure class, a commit page diff) can throw TypeError: Cannot read properties of null (reading 'patch') and then sit on loading diff… forever with a header reading Files on #N (0). The page must handle a null/empty diff payload honestly: show the real file count (including a truthful "0 files changed"), and on fetch/parse failure render a human error state with an explicit Retry control — never an eternal spinner.

Evidence (static analysis; not reproduced locally, per standing rule)

SDK routing bug (root cause): GET …/api/pulls/{num}/diff answers Content-Type: text/plain; charset=utf-8 (internal/pulls/http.go getDiff, ~line 581), but the SDK call client._call(p(\/pulls/${num}/diff`), { method: "GET" }) (web/sdk/src/pulls.js~line 45) does **not** passraw: true. _dispatch (web/sdk/src/core.js~line 484) ignores content-type for non-SSE and routes **every** non-SSE response through_jsonResponse, which JSON-parses the body: a real patch body would throw ReposError: invalid JSON from …, and an **empty patch body** (git diff base...headreturns "" — e.g. an already-merged PR, or base == head) takes thetext === "" branch and resolves **null`**.
Null deref: both consumers then do res.patch on that null:
- web/src/pages/PullFiles.jsx ~line 56: typeof res === "string" ? res : res.patch ?? res.diff ?? ""
- web/src/pages/Pull.jsx ~line 563: same expression for the PR page's inline diff
- TypeError: Cannot read properties of null (reading 'patch') matches exactly.
Stuck spinner: the throw happens inside the useData fetcher (web/src/lib/data.js start), so entry.error is set and the error goes to the global tray — but the page component's getView() stays undefined, so PullFiles.jsx ~line 66 renders its fallback={<p class="muted">loading diff…</p>} indefinitely while the heading's (getView()?.files ?? []).length renders (0). The heading count is therefore a lie: 0 means "failed to load", not "no files changed".
No retry surface: on error the only recovery is the TTL backstop (a failed entry is re-fetched after the 5 s TTL via startIfStale, because entry.at is stamped on error too) or a full page reload. There is no in-page Retry control, and if the failure is deterministic (e.g. server 500 on a malformed range) the page loops silently between tray toasts and the spinner.

Architecture notes

The fix seam is the SDK call, not the pages: pulls.diff should read the body as text (raw: true path in core.js _textResponse), since the endpoint is text/plain by spec (docs/go/07_api.md §9.5 — "text/plain unified base...head patch"). The pages' typeof res === "string" branch then becomes the live path and the res.patch ?? res.diff fallbacks become dead defensive code — either keep them null-safe or remove them.
The per-commit diff path (web/src/pages/Commit.jsx ~line 263, parsePatchFiles(data.patch ?? "")) rides the JSON commits endpoint and already tolerates a null patch — but not a null data; the same defensive normalization should apply there so one commit's diff failure can't wedge its page the same way.
parsePatchFiles (web/src/lib/diff.js) already returns {files: []} for an empty string — the honest "0 files / empty diff" state falls out for free once the SDK stops resolving to null.
Error-state rendering precedent exists in the app's muted/guide states (EMPTY_MARKER guide in data.js, issue #209); the Retry control can drive useData's refetch by dropping/re-staring the entry — no new data-layer API is strictly required, but a small invalidate(key)/refetch(key) helper in lib/data.js is the clean seam if the entry keeps its error.

Acceptance criteria

pulls.diff in the SDK resolves to the patch string (text/plain read as text); an empty body resolves to "", not null; a server error still throws ReposError.
Files tab on a PR with an empty/identical diff renders "empty diff" (or "0 files changed") — never loading diff… and never a TypeError.
A diff fetch failure renders an inline human error state (message + Retry button); clicking Retry re-runs the fetch. The heading shows an honest count/state, not a spinner forever.
The PR page's inline diff (Pull.jsx) and the per-commit diff (Commit.jsx) are null-safe the same way.
No regression in diff rendering (unified/split, line selection #244) — the parsed shape fed to DiffBody is unchanged.

## What's requested Opening a PR's **Files** tab (and in the same failure class, a commit page diff) can throw `TypeError: Cannot read properties of null (reading 'patch')` and then sit on `loading diff…` forever with a header reading **Files on #N (0)**. The page must handle a null/empty diff payload honestly: show the real file count (including a truthful "0 files changed"), and on fetch/parse failure render a human error state with an explicit Retry control — never an eternal spinner. ## Evidence (static analysis; not reproduced locally, per standing rule) - **SDK routing bug (root cause):** `GET …/api/pulls/{num}/diff` answers `Content-Type: text/plain; charset=utf-8` (`internal/pulls/http.go` `getDiff`, ~line 581), but the SDK call `client._call(p(\`/pulls/${num}/diff\`), { method: "GET" })` (`web/sdk/src/pulls.js` ~line 45) does **not** pass `raw: true`. `_dispatch` (`web/sdk/src/core.js` ~line 484) ignores content-type for non-SSE and routes **every** non-SSE response through `_jsonResponse`, which JSON-parses the body: a real patch body would throw `ReposError: invalid JSON from …`, and an **empty patch body** (`git diff base...head` returns "" — e.g. an already-merged PR, or base == head) takes the `text === ""` branch and resolves **`null`**. - **Null deref:** both consumers then do `res.patch` on that null: - `web/src/pages/PullFiles.jsx` ~line 56: `typeof res === "string" ? res : res.patch ?? res.diff ?? ""` - `web/src/pages/Pull.jsx` ~line 563: same expression for the PR page's inline diff - `TypeError: Cannot read properties of null (reading 'patch')` matches exactly. - **Stuck spinner:** the throw happens inside the `useData` fetcher (`web/src/lib/data.js` `start`), so `entry.error` is set and the error goes to the global tray — but the page component's `getView()` stays `undefined`, so `PullFiles.jsx` ~line 66 renders its `fallback={<p class="muted">loading diff…</p>}` indefinitely while the heading's `(getView()?.files ?? []).length` renders **(0)**. The heading count is therefore a lie: 0 means "failed to load", not "no files changed". - **No retry surface:** on error the only recovery is the TTL backstop (a failed entry is re-fetched after the 5 s TTL via `startIfStale`, because `entry.at` is stamped on error too) or a full page reload. There is no in-page Retry control, and if the failure is deterministic (e.g. server 500 on a malformed range) the page loops silently between tray toasts and the spinner. ## Architecture notes - The fix seam is the SDK call, not the pages: `pulls.diff` should read the body as text (`raw: true` path in `core.js` `_textResponse`), since the endpoint is text/plain by spec (`docs/go/07_api.md` §9.5 — "text/plain unified `base...head` patch"). The pages' `typeof res === "string"` branch then becomes the live path and the `res.patch ?? res.diff` fallbacks become dead defensive code — either keep them null-safe or remove them. - The per-commit diff path (`web/src/pages/Commit.jsx` ~line 263, `parsePatchFiles(data.patch ?? "")`) rides the JSON commits endpoint and already tolerates a null `patch` — but not a null `data`; the same defensive normalization should apply there so one commit's diff failure can't wedge its page the same way. - `parsePatchFiles` (`web/src/lib/diff.js`) already returns `{files: []}` for an empty string — the honest "0 files / empty diff" state falls out for free once the SDK stops resolving to `null`. - Error-state rendering precedent exists in the app's muted/guide states (`EMPTY_MARKER` guide in `data.js`, issue #209); the Retry control can drive `useData`'s refetch by dropping/re-staring the entry — no new data-layer API is strictly required, but a small `invalidate(key)`/`refetch(key)` helper in `lib/data.js` is the clean seam if the entry keeps its error. ## Acceptance criteria - [ ] `pulls.diff` in the SDK resolves to the patch **string** (text/plain read as text); an empty body resolves to `""`, not `null`; a server error still throws `ReposError`. - [ ] Files tab on a PR with an empty/identical diff renders "empty diff" (or "0 files changed") — never `loading diff…` and never a TypeError. - [ ] A diff fetch failure renders an inline human error state (message + Retry button); clicking Retry re-runs the fetch. The heading shows an honest count/state, not a spinner forever. - [ ] The PR page's inline diff (`Pull.jsx`) and the per-commit diff (`Commit.jsx`) are null-safe the same way. - [ ] No regression in diff rendering (unified/split, line selection #244) — the parsed shape fed to `DiffBody` is unchanged.

crueber added this to the v1 milestone

2026-09-14 15:46:25 +00:00

crueber added the

bug

pull requests

labels

2026-09-14 15:46:25 +00:00

crueber referenced this issue from a commit

2026-09-14 17:32:51 +00:00

Fix #520: PR diff null patch (SDK raw text + honest error state)

crueber referenced this issue from a pull request that will close it,

2026-09-14 17:32:58 +00:00

Fix #520: diff null patch #526

crueber commented

2026-09-14 17:33:01 +00:00

Author

Owner

Fix is up: #526 (branch fix/issue-520) — SDK pulls.diff reads text/plain via raw:true; pages normalize null-safe, count honestly, and render error + Retry; Commit.jsx hardened the same way. Tests 10/10 new, full suite 1180/1179 (1 pre-existing live-server smoke fail, verified on pristine main), vite+esbuild green, no backend change.

Fix is up: https://git.packden.us/crueber/walhub/pulls/526 (branch fix/issue-520) — SDK pulls.diff reads text/plain via raw:true; pages normalize null-safe, count honestly, and render error + Retry; Commit.jsx hardened the same way. Tests 10/10 new, full suite 1180/1179 (1 pre-existing live-server smoke fail, verified on pristine main), vite+esbuild green, no backend change.

crueber commented

2026-09-14 17:44:14 +00:00

Author

Owner

Review of PR #526 (fix/issue-520) — verified in scratch worktree /tmp/pr526 @ b19ff3a (since removed). No browser (node tests + reasoning only — noted explicitly per task).

FINDINGS (all 7 review points pass, no fixes needed):

SDK routing — web/sdk/src/pulls.js:45 raw:true BEFORE ...opts (opts can still override). core.js _textResponse (web/sdk/src/core.js:522) resolves res.text() as string, empty body -> '' (not null), non-2xx still throws ReposError. Root-cause claim in #520 confirmed in code: _jsonResponse empty -> null (:535). PASS.
Normalization — web/src/lib/diff.js:29 normalizePatchBody: string passthrough, res?.patch ?? res?.diff ?? '' null-safe; dead object branch kept with ?. per law 12 (noted in doc entry). Crash sim: old res.patch on null throws TypeError (reproduced), new path returns '' for null/undefined/{patch:null,diff:null}. PASS.
Honest counts — PullFiles.jsx:71 / Pull.jsx:583 diffCount(): loaded -> real count (truthful 0 -> 'empty diff' fallback), error -> 'failed to load', else '…'. Old lying (getView()?.files ?? []).length removed (pinned by source-pin tests). Loading vs failed vs truthful-0 distinct. PASS.
Error + Retry — both pages render inline role=alert card with human message (Couldn't load the diff: ) + Retry button driving invalidate() on the SAME key (PullFiles.jsx:65, Pull.jsx:79); error signal is page-local, no new data-layer API (invalidate exists in lib/data.js:277). No eternal-spinner path: Show falls back to loading only when no error. PASS.
Commit.jsx — :265 parsePatchFiles(normalizePatchBody(data)); parsed() already returns undefined on !data so loading fallback holds; bare data.patch deref gone. Same treatment. PASS.
No backend change — diff touches zero .go files; endpoint already text/plain (internal/pulls/http.go:582, spec 07_api.md §9.5). PASS.
No new deps — no package.json/pnpm-workspace change; docs/go/12_web_ui.md entry appended in same change per law 12, numbers match measured runs. PASS (laws 1/7/8/12: no new deps; N/A long-work; no new registry seams — existing invalidate reused; docs updated).

TESTS (scratch worktree, node_modules symlinked from main): targeted pull-diff-520.test.js 10/10 pass; FULL node --test web/test/unit/*.test.js: 1180 total / 1179 pass / 1 fail — the 1 fail is smoke.test.js live-server /setup 403, verified IDENTICAL on pristine main (3 tests / 1 fail there too), i.e. pre-existing, zero PR-caused. vite build green (2.58s; chunk-size warning only, pre-existing). esbuild SDK bundle not re-run (no SDK dep/shape change beyond one flag; vite bundle includes SDK). Mobile: error/Retry block is in-flow card + button with existing responsive classes — reasoned, no browser per task.

MERGE RECOMMENDATION: ready to merge. No push made (nothing to fix). Main worktree untouched (still clean on main).

Review of PR #526 (fix/issue-520) — verified in scratch worktree /tmp/pr526 @ b19ff3a (since removed). No browser (node tests + reasoning only — noted explicitly per task). FINDINGS (all 7 review points pass, no fixes needed): 1. SDK routing — web/sdk/src/pulls.js:45 raw:true BEFORE ...opts (opts can still override). core.js _textResponse (web/sdk/src/core.js:522) resolves res.text() as string, empty body -> '' (not null), non-2xx still throws ReposError. Root-cause claim in #520 confirmed in code: _jsonResponse empty -> null (:535). PASS. 2. Normalization — web/src/lib/diff.js:29 normalizePatchBody: string passthrough, res?.patch ?? res?.diff ?? '' null-safe; dead object branch kept with ?. per law 12 (noted in doc entry). Crash sim: old res.patch on null throws TypeError (reproduced), new path returns '' for null/undefined/{patch:null,diff:null}. PASS. 3. Honest counts — PullFiles.jsx:71 / Pull.jsx:583 diffCount(): loaded -> real count (truthful 0 -> 'empty diff' fallback), error -> 'failed to load', else '…'. Old lying (getView()?.files ?? []).length removed (pinned by source-pin tests). Loading vs failed vs truthful-0 distinct. PASS. 4. Error + Retry — both pages render inline role=alert card with human message (Couldn't load the diff: <msg>) + Retry button driving invalidate() on the SAME key (PullFiles.jsx:65, Pull.jsx:79); error signal is page-local, no new data-layer API (invalidate exists in lib/data.js:277). No eternal-spinner path: Show falls back to loading only when no error. PASS. 5. Commit.jsx — :265 parsePatchFiles(normalizePatchBody(data)); parsed() already returns undefined on !data so loading fallback holds; bare data.patch deref gone. Same treatment. PASS. 6. No backend change — diff touches zero .go files; endpoint already text/plain (internal/pulls/http.go:582, spec 07_api.md §9.5). PASS. 7. No new deps — no package.json/pnpm-workspace change; docs/go/12_web_ui.md entry appended in same change per law 12, numbers match measured runs. PASS (laws 1/7/8/12: no new deps; N/A long-work; no new registry seams — existing invalidate reused; docs updated). TESTS (scratch worktree, node_modules symlinked from main): targeted pull-diff-520.test.js 10/10 pass; FULL node --test web/test/unit/*.test.js: 1180 total / 1179 pass / 1 fail — the 1 fail is smoke.test.js live-server /setup 403, verified IDENTICAL on pristine main (3 tests / 1 fail there too), i.e. pre-existing, zero PR-caused. vite build green (2.58s; chunk-size warning only, pre-existing). esbuild SDK bundle not re-run (no SDK dep/shape change beyond one flag; vite bundle includes SDK). Mobile: error/Retry block is in-flow card + button with existing responsive classes — reasoned, no browser per task. MERGE RECOMMENDATION: ready to merge. No push made (nothing to fix). Main worktree untouched (still clean on main).

crueber closed this issue

2026-09-14 17:44:35 +00:00

crueber referenced this issue from a commit

2026-09-14 17:44:35 +00:00

Merge pull request 'Fix #520: diff null patch' (#526) from fix/issue-520 into main

crueber commented

2026-09-14 17:44:36 +00:00

Author

Owner

Fixed by PR #526 (review clean — all 7 checks pass, crash sim confirms the fix, no eternal spinner), merged. Closing.