crueber/walhub

Watch

Fork 0

Commits tab: page 2 (+skip) throws 'Maximum call stack size exceeded' and sticks on 'loading history…' forever #529

New issue

Closed

opened 2026-09-14 18:24:58 +00:00 by crueber · 3 comments

crueber commented

2026-09-14 18:24:58 +00:00

Owner

Commits tab: page 2 (+skip) throws "Maximum call stack size exceeded" and sticks on "loading history…" forever

What's requested

Fix the commits-history pager: navigating from page 1 to any "?skip=N" window (older →) throws RangeError: Maximum call stack size exceeded inside Solid's effect system, surfaces a toast with the data-layer cache key (sha:<40-hex>:commits::35 — i.e. key sha:{sha}:commits:{path}:{skip} with empty path, page size 35), and leaves the page permanently on the loading history… fallback. Diagnosis is static (code reading + user evidence: toast chip + message, page-2 hang); nothing was reproduced locally.

Evidence

Toast chip sha:…:commits::35 matches the page-2 cache key built in web/src/pages/Commits.jsx (CommitList, lines ~216-228): `sha:${sha}:commits:${path()}:${skip()}` — empty path, skip=35 (second window of the default n=35 page, internal/api/commits.go line 30). The chip is the tray entry's key, set by reportError(err, key) in web/src/lib/data.js (start(), line ~196). So the failing unit is the Commits page-2 useData window, not the network call itself.
After the error the page never recovers: h() (Commits.jsx line ~229) returns getPage(), and a failed useData entry sets only entry.error (data.js start()), never a value — the <Show when={h()} fallback={<p>loading history…</p>}> fallback renders indefinitely.
Server side is healthy: internal/api/bind_wal.go Commits() is a single non-recursive git log --skip=… --max-count=n+1 render, ETag'd immutable (ccSWR/ccImmutable per head SHA). No server recursion exists.

Recursion mechanism (exact)

The page-2 branch of CommitList calls useData(...) INSIDE a createEffect body (Commits.jsx lines 216-228):

const [getPage, setPage] = createSignal(undefined);
createEffect(() => {
  const first = getFirst();
  if (!first || (skip() === 0 && !path())) return setPage(undefined);
  const sha = first.sha;
  const key = `sha:${sha}:commits:${path()}:${skip()}`;
  const [get] = useData(key, () => props.repoClient.commits({...}), SHA_TTL);
  setPage(get());
});

useData (data.js) itself opens a createEffect that reads entry.signal[0]() — a signal write/read cycle entered synchronously from inside a running effect:

The outer effect runs → useData is invoked during effect execution → its inner createEffect runs startIfStale → start() → the async fetcher throws synchronously or the fetch rejects.
start()'s catch calls reportError(err, key) (the toast) and entry.error = err; entry.signal is never set with a value.
setPage(get()) in the outer effect then depends on the inner effect's signal; because the inner createEffect was created inside an active effect scope, Solid nests its scope under the outer effect. Any re-run of the inner effect (retry, key re-read, setValue(entry.signal[0]()) on a hot path) re-triggers the outer effect, which calls useData again, creating a new nested effect each cycle — the effect graph grows without bound and Solid's synchronous dependency flush blows the JS stack: RangeError: Maximum call stack size exceeded.
The thrown error escapes the effect; entry.error is set but no value ever lands in the signal, so h() stays undefined and the page hangs on loading history… even though page 1's cached data (getFirst()) is still in the cache and would render fine.

Note h()'s guard (skip() === 0 && !path() ? getFirst() : getPage()) means the bug only fires beyond the first window — exactly the reported "page 2" trigger.

Fix

Do not instantiate useData inside the effect. Hoist it to component scope with a reactive key, the Tree.jsx DocTabs pattern (web/src/pages/Tree.jsx ~line 103): useData(() => (pageKey() ? \sha:${sha}:commits:${path}:${skip}` : "commits:none"), () => (pageKey() ? repoClient.commits({...}) : null), SHA_TTL)` — a null/"none" key means "do not fetch", so the first page and later windows share one subscription and no nested effect is ever created.
While in there: a failed useData window currently renders as an eternal loading history…. Add an error state (inline retry, the DegradedNotice precedent) so a page-2 fetch failure shows a human-readable error instead of an eternal pulse; no raw RangeError strings in the tray for this path.

Architecture notes

useData supports a reactive GETTER key (documented in data.js) — that is the intended way to change keys inside a component; calling the hook itself inside an effect is the misuse.
The sha:<sha>:commits:* entries are SHA_TTL=Infinity immutable windows; nothing server-side needs to change.
Toast chips expose raw cache keys ({e.key} in App.jsx tray). Acceptable as a debug affordance, but the acceptance criteria below cover not reaching that path on expected errors.

Acceptance criteria

Page 2 (and every subsequent older → window) of the Commits tab loads without a RangeError; no tray entry appears on success.
No useData/useResolved call site lives inside a createEffect body (grep sweep of web/src/pages/*.jsx — Commits.jsx lines 216-228 is the offender; confirm no siblings share the pattern).
A failed page-2 fetch renders an inline human-readable error with retry, never an indefinite loading history….
Page-1 behavior unchanged (resolve → sha-addressed first window; ?path= windows still work).
The commit-graph lanes still derive per visible window after the refactor (memo reads the same window getter).

# Commits tab: page 2 (+skip) throws "Maximum call stack size exceeded" and sticks on "loading history…" forever ## What's requested Fix the commits-history pager: navigating from page 1 to any "?skip=N" window (`older →`) throws `RangeError: Maximum call stack size exceeded` inside Solid's effect system, surfaces a toast with the data-layer cache key (`sha:<40-hex>:commits::35` — i.e. key `sha:{sha}:commits:{path}:{skip}` with empty path, page size 35), and leaves the page permanently on the `loading history…` fallback. Diagnosis is static (code reading + user evidence: toast chip + message, page-2 hang); nothing was reproduced locally. ## Evidence - Toast chip `sha:…:commits::35` matches the page-2 cache key built in `web/src/pages/Commits.jsx` (`CommitList`, lines ~216-228): `` `sha:${sha}:commits:${path()}:${skip()}` `` — empty `path`, `skip=35` (second window of the default n=35 page, `internal/api/commits.go` line 30). The chip is the tray entry's `key`, set by `reportError(err, key)` in `web/src/lib/data.js` (`start()`, line ~196). So the failing unit is the Commits page-2 `useData` window, not the network call itself. - After the error the page never recovers: `h()` (Commits.jsx line ~229) returns `getPage()`, and a failed `useData` entry sets only `entry.error` (data.js `start()`), never a value — the `<Show when={h()} fallback={<p>loading history…</p>}>` fallback renders indefinitely. - Server side is healthy: `internal/api/bind_wal.go` `Commits()` is a single non-recursive `git log --skip=… --max-count=n+1` render, ETag'd immutable (ccSWR/ccImmutable per head SHA). No server recursion exists. ## Recursion mechanism (exact) The page-2 branch of `CommitList` calls **`useData(...)` INSIDE a `createEffect` body** (Commits.jsx lines 216-228): ```jsx const [getPage, setPage] = createSignal(undefined); createEffect(() => { const first = getFirst(); if (!first || (skip() === 0 && !path())) return setPage(undefined); const sha = first.sha; const key = `sha:${sha}:commits:${path()}:${skip()}`; const [get] = useData(key, () => props.repoClient.commits({...}), SHA_TTL); setPage(get()); }); ``` `useData` (data.js) itself opens a `createEffect` that reads `entry.signal[0]()` — a signal write/read cycle entered synchronously from inside a running effect: 1. The outer effect runs → `useData` is invoked during effect execution → its inner `createEffect` runs `startIfStale` → `start()` → the async fetcher throws synchronously or the fetch rejects. 2. `start()`'s catch calls `reportError(err, key)` (the toast) and `entry.error = err`; `entry.signal` is never set with a value. 3. `setPage(get())` in the outer effect then depends on the inner effect's signal; because the inner `createEffect` was created *inside* an active effect scope, Solid nests its scope under the outer effect. Any re-run of the inner effect (retry, key re-read, `setValue(entry.signal[0]())` on a hot path) re-triggers the outer effect, which calls `useData` again, creating a *new* nested effect each cycle — the effect graph grows without bound and Solid's synchronous dependency flush blows the JS stack: `RangeError: Maximum call stack size exceeded`. 4. The thrown error escapes the effect; `entry.error` is set but no value ever lands in the signal, so `h()` stays undefined and the page hangs on `loading history…` even though page 1's cached data (`getFirst()`) is still in the cache and would render fine. Note `h()`'s guard (`skip() === 0 && !path() ? getFirst() : getPage()`) means the bug only fires beyond the first window — exactly the reported "page 2" trigger. ## Fix - Do not instantiate `useData` inside the effect. Hoist it to component scope with a reactive key, the Tree.jsx DocTabs pattern (`web/src/pages/Tree.jsx` ~line 103): `useData(() => (pageKey() ? \`sha:${sha}:commits:${path}:${skip}\` : "commits:none"), () => (pageKey() ? repoClient.commits({...}) : null), SHA_TTL)` — a null/"none" key means "do not fetch", so the first page and later windows share one subscription and no nested effect is ever created. - While in there: a failed `useData` window currently renders as an eternal `loading history…`. Add an error state (inline retry, the DegradedNotice precedent) so a page-2 fetch failure shows a human-readable error instead of an eternal pulse; no raw `RangeError` strings in the tray for this path. ## Architecture notes - `useData` supports a reactive GETTER key (documented in data.js) — that is the intended way to change keys inside a component; calling the hook itself inside an effect is the misuse. - The `sha:<sha>:commits:*` entries are SHA_TTL=Infinity immutable windows; nothing server-side needs to change. - Toast chips expose raw cache keys (`{e.key}` in App.jsx tray). Acceptable as a debug affordance, but the acceptance criteria below cover not reaching that path on expected errors. ## Acceptance criteria - [ ] Page 2 (and every subsequent `older →` window) of the Commits tab loads without a RangeError; no tray entry appears on success. - [ ] No `useData`/`useResolved` call site lives inside a `createEffect` body (grep sweep of `web/src/pages/*.jsx` — Commits.jsx lines 216-228 is the offender; confirm no siblings share the pattern). - [ ] A failed page-2 fetch renders an inline human-readable error with retry, never an indefinite `loading history…`. - [ ] Page-1 behavior unchanged (resolve → sha-addressed first window; `?path=` windows still work). - [ ] The commit-graph lanes still derive per visible window after the refactor (memo reads the same window getter).

crueber added the

git storage

label

2026-09-14 18:25:18 +00:00

crueber added this to the v1 milestone

2026-09-14 18:25:18 +00:00

crueber added the

issues

label

2026-09-14 18:28:09 +00:00

crueber referenced this issue from a commit

2026-09-14 21:42:57 +00:00

Fix #529: hoist Commits page-2 useData out of createEffect, add inline window error

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

2026-09-14 21:42:58 +00:00

Fix #529: commits stack overflow #541

crueber commented

2026-09-14 21:43:01 +00:00

Author

Owner

Fixed by #541 (branch fix/issue-529): page-2 useData hoisted out of the createEffect into a top-level reactive-key subscription (DocTabs pattern), failed windows render an inline error with retry instead of the eternal loading fallback, page-1 path unchanged. 8/8 new tests green, vite build green; only failure in the full suite is the ambient live-server smoke subtest (/setup 403 on the standing :8080, unrelated).

crueber commented

2026-09-14 21:50:04 +00:00

Author

Owner

Review of PR #541 (fix/issue-529), verified in scratch worktree /tmp/pr541 (removed afterward; main worktree untouched, still clean on main).

FINDINGS (all acceptance criteria hold):

Nesting eliminated — PASS. web/src/pages/Commits.jsx:238 useData is now at component scope with reactive key () => pageKey() ?? "commits:none"; the old lines 216-228 hook-in-effect is gone. Remaining createEffect bodies (:208 setViewed publish, :253 pageError clear) contain no data hooks. Independent sweep of all 126 web/src .jsx/.js files for useData(/useResolved(/useDataRefetchable( inside createEffect bodies: CLEAN (broader than the new test's pages-only sweep, same verdict).
Page-2 path — PASS. Single subscription, exactly one .commits( call site (grep), key stays sha:{sha}:commits:{path}:{skip} with SHA_TTL; sentinel "commits:none" resolves null and never fetches. Tree.jsx:109 doctabs:none precedent confirmed real.
Page-1 identical — PASS. h() (:262) serves getFirst() whenever pageKey() is null, i.e. exactly the old skip()===0 && !path() branch; useResolved line untouched. One deliberate, documented delta: empty/unresolved-first ?skip windows now render the guide via getFirst() instead of firing a doomed commits call with sha undefined (noted in the 12_web_ui.md entry).
Failure states — PASS. Fetcher catch records pageError and rethrows (tray contract intact); fallback renders role=alert 'Could not load this page of history.' + Retry driving invalidate(k) (:253-263, :284-296). Per-window (key-change clears, still-failing re-arms), message generic, pending windows keep the pulse. No eternal spinner on any page-2 path. Note (pre-existing, out of scope): a page-1 resolve failure still shows the loading fallback + tray entry, same as before — not a regression.
Graph memo — PASS. createMemo still reads h()?.commits (unchanged line), so lanes derive per visible window; pinned by test.
Scope/hygiene — PASS. Diff is exactly 3 files (12_web_ui.md decision entry + Commits.jsx + commits-page-529.test.js); no backend/SDK change, no package.json/lock diff (invalidate is an existing data.js export, law 1 holds); docs entry accurately describes the shipped behavior including the no-useDataError-accessor decision. Laws 7/8: no task/channel changes, no new registry seams.

TESTS (scratch worktree, node_modules symlinked from main):

node --test web/test/unit/*.test.js: 1231 total / 1230 pass / 1 fail — the single failure is smoke.test.js's live-server subtest (/setup 403 from the standing :8080, needs a live Go server; untouched by this frontend-only diff, matches PR description). New commits-page-529.test.js: 8/8 green.
vite build green (2.48s); esbuild SDK bundle green (pnpm shim unavailable under mise here, so both build:ui/build:sdk steps were run directly via node_modules/.bin).
No browser drive (per review instructions: node tests + reasoning; mobile-viewport N/A — error card reuses existing card/btn classes, no new layout).

No fixes needed; nothing pushed.

MERGE RECOMMENDATION: ready to merge.

Review of PR #541 (fix/issue-529), verified in scratch worktree /tmp/pr541 (removed afterward; main worktree untouched, still clean on main). FINDINGS (all acceptance criteria hold): 1. Nesting eliminated — PASS. web/src/pages/Commits.jsx:238 useData is now at component scope with reactive key () => pageKey() ?? "commits:none"; the old lines 216-228 hook-in-effect is gone. Remaining createEffect bodies (:208 setViewed publish, :253 pageError clear) contain no data hooks. Independent sweep of all 126 web/src .jsx/.js files for useData(/useResolved(/useDataRefetchable( inside createEffect bodies: CLEAN (broader than the new test's pages-only sweep, same verdict). 2. Page-2 path — PASS. Single subscription, exactly one .commits( call site (grep), key stays sha:{sha}:commits:{path}:{skip} with SHA_TTL; sentinel "commits:none" resolves null and never fetches. Tree.jsx:109 doctabs:none precedent confirmed real. 3. Page-1 identical — PASS. h() (:262) serves getFirst() whenever pageKey() is null, i.e. exactly the old skip()===0 \&\& !path() branch; useResolved line untouched. One deliberate, documented delta: empty/unresolved-first ?skip windows now render the guide via getFirst() instead of firing a doomed commits call with sha undefined (noted in the 12_web_ui.md entry). 4. Failure states — PASS. Fetcher catch records pageError and rethrows (tray contract intact); fallback renders role=alert 'Could not load this page of history.' + Retry driving invalidate(k) (:253-263, :284-296). Per-window (key-change clears, still-failing re-arms), message generic, pending windows keep the pulse. No eternal spinner on any page-2 path. Note (pre-existing, out of scope): a page-1 resolve failure still shows the loading fallback + tray entry, same as before — not a regression. 5. Graph memo — PASS. createMemo still reads h()?.commits (unchanged line), so lanes derive per visible window; pinned by test. 6. Scope/hygiene — PASS. Diff is exactly 3 files (12_web_ui.md decision entry + Commits.jsx + commits-page-529.test.js); no backend/SDK change, no package.json/lock diff (invalidate is an existing data.js export, law 1 holds); docs entry accurately describes the shipped behavior including the no-useDataError-accessor decision. Laws 7/8: no task/channel changes, no new registry seams. TESTS (scratch worktree, node_modules symlinked from main): - node --test web/test/unit/*.test.js: 1231 total / 1230 pass / 1 fail — the single failure is smoke.test.js's live-server subtest (/setup 403 from the standing :8080, needs a live Go server; untouched by this frontend-only diff, matches PR description). New commits-page-529.test.js: 8/8 green. - vite build green (2.48s); esbuild SDK bundle green (pnpm shim unavailable under mise here, so both build:ui/build:sdk steps were run directly via node_modules/.bin). - No browser drive (per review instructions: node tests + reasoning; mobile-viewport N/A — error card reuses existing card/btn classes, no new layout). No fixes needed; nothing pushed. MERGE RECOMMENDATION: ready to merge.

crueber closed this issue

2026-09-14 21:50:22 +00:00

crueber referenced this issue from a commit

2026-09-14 21:50:22 +00:00

Merge pull request 'Fix #529: commits stack overflow' (#541) from fix/issue-529 into main

crueber commented

2026-09-14 21:50:23 +00:00

Author

Owner

Fixed by PR #541 (review clean — all 6 checks pass, repo-wide sweep confirms zero hook-in-effect sites), merged. Closing.

crueber referenced this issue

2026-09-14 22:37:35 +00:00