Fix #605: Code-review toggle stuck on loading for unset docs #610

Merged

crueber merged 1 commit from fix/issue-605 into main

2026-09-15 22:35:58 +00:00

crueber commented

2026-09-15 22:33:59 +00:00

Owner

Option A sentinel split: unseeded is undefined (signals, prefill, Show/disabled gates); null keeps meaning seeded-but-unset → checked. extractSelfApproval/withSelfApproval untouched. Tests: new settings-review-sentinel-605.test.js (5 tests) + settings-review-586 sentinel pin update; full-minus-smoke 1564/1564 green; vite build + esbuild SDK green; go vet clean. Docs: 12_web_ui.md FIXED (Forgejo #605) same commit. No new deps; Tailwind untouched; 390px reasoned (sentinel-only, no layout change).

crueber added 1 commit

2026-09-15 22:34:00 +00:00

web: fix #605 Code-review toggle stuck on loading for unset docs 25baf6adef

Settings GeneralTab seeded the self-approval toggle with a null
sentinel, but extractSelfApproval() returns null for unset (the common
case) — seeding null never left the sentinel, so the <Show> gate and
the save disabled stayed on 'loading…' forever. Option A: unseeded is
undefined (signals, prefill, Show/disabled gates); null keeps meaning
seeded-but-unset → checked. Docs: 12_web_ui.md FIXED (Forgejo #605).
Tests: new settings-review-sentinel-605.test.js; settings-review-586
sentinel pin updated.

crueber commented

2026-09-15 22:35:45 +00:00

Author

Owner

Independent review — verdict: APPROVE (no fix commit; working tree left untouched)

Checked the full origin/main..origin/fix/issue-605 diff in /tmp/walhub-605 against every acceptance bullet of #605. All green.

1. Sentinel swap is complete — no missed getSelf() === null / !== null reads.
Grep over web/src/pages/Settings.jsx finds exactly four getSelf() reads, all on the new sentinel: signal init createSignal(undefined) x2 (value + base), prefill getSelf() === undefined, <Show when={getSelf() !== undefined}>, disabled={getSelf() === undefined}. Zero remaining getSelf() === null / !== null strings anywhere in the file. (The other === null sentinels in the file — getText/getVis/getFlags — belong to sibling knobs whose extractors never return null-for-unset, so they are not the same bug.) The bug cannot reintroduce through a missed read.

2. extractSelfApproval / withSelfApproval byte-identical — contract preserved.
git diff origin/main touches nothing under web/src/lib/; repoReview.js is unchanged since #586 (ae42bd6). null-for-unset and the append/replace paths are intact.

3. checked / allowed expressions unchanged (!== false); PUT round-trip traced.
checked={getSelf() !== false} and const allowed = getSelf() !== false are byte-identical to pre-fix. Render matrix: unset (null) → checked, true → checked, false → unchecked — as required. Round-trip note (deliberate, no surprise): saving an untouched unset knob writes allow_self_approval = true explicitly via withSelfApproval(current, true) rather than omitting the key. Both readings are semantically identical under the default-ON contract, and the choice is forced by the existing allowed expression rather than new logic — fine as-is. selfDirty() (getSelf() !== getSelfBase()) load trace: undefined/undefined → not dirty while loading; effect seeds both to the same value (null/null for unset) → still clean; only the toggle's onChange (booleans only) can diverge them → "unsaved changes" appears exactly on user edit. No dirty-check surprise.

4. No-clobber preserved — the effect cannot reseed after user edit.
The gate is doc !== undefined && getSelf() === undefined, and the only writers are the effect itself (writes seed values, never undefined), the boolean-only onChange, and save/failure-reseed (booleans or fresh seeds, never undefined). Once seeded, the condition can never go true again — user edits are safe.

5. Explicit true/false render correctly — same !== false expression covers both without branching (pinned by the new test).

6. Docs amendment (law 12): docs/go/12_web_ui.md gains the FIXED (Forgejo #605) entry in the same change. No new deps (law 1): the new test imports only node:test, node:assert/strict, node:module, node:fs.

7. Tests fail pre-fix, pass post-fix. New settings-review-sentinel-605.test.js (5 tests) + updated settings-review-586 pin pass on the branch (7/7 with the 586 file). I temporarily restored origin/main's Settings.jsx and re-ran the new file: it fails (assertion error on the sentinel pins), then restored the fix byte-for-byte (git status clean apart from untracked web/node_modules). Full unit suite: 1566/1567 pass; the single failure is smoke.test.js ("built SPA shell served at / and /setup", 403 vs 200) — it requires a live server on :8080 (something else is answering in this environment) and is excluded by the project's own "unit files minus smoke" convention; this diff touches no server code, so it is unrelated and pre-existing.

No defects found — nothing to fix, no commit pushed.

## Independent review — verdict: APPROVE (no fix commit; working tree left untouched) Checked the full `origin/main..origin/fix/issue-605` diff in `/tmp/walhub-605` against every acceptance bullet of #605. All green. **1. Sentinel swap is complete — no missed `getSelf() === null` / `!== null` reads.** Grep over `web/src/pages/Settings.jsx` finds exactly four `getSelf()` reads, all on the new sentinel: signal init `createSignal(undefined)` x2 (value + base), prefill `getSelf() === undefined`, `<Show when={getSelf() !== undefined}>`, `disabled={getSelf() === undefined}`. Zero remaining `getSelf() === null` / `!== null` strings anywhere in the file. (The other `=== null` sentinels in the file — `getText`/`getVis`/`getFlags` — belong to sibling knobs whose extractors never return null-for-unset, so they are not the same bug.) The bug cannot reintroduce through a missed read. **2. `extractSelfApproval` / `withSelfApproval` byte-identical — contract preserved.** `git diff origin/main` touches nothing under `web/src/lib/`; `repoReview.js` is unchanged since #586 (`ae42bd6`). null-for-unset and the append/replace paths are intact. **3. `checked` / `allowed` expressions unchanged (`!== false`); PUT round-trip traced.** `checked={getSelf() !== false}` and `const allowed = getSelf() !== false` are byte-identical to pre-fix. Render matrix: unset (null) → checked, `true` → checked, `false` → unchecked — as required. Round-trip note (deliberate, no surprise): saving an untouched unset knob writes `allow_self_approval = true` explicitly via `withSelfApproval(current, true)` rather than omitting the key. Both readings are semantically identical under the default-ON contract, and the choice is forced by the existing `allowed` expression rather than new logic — fine as-is. `selfDirty()` (`getSelf() !== getSelfBase()`) load trace: `undefined/undefined` → not dirty while loading; effect seeds both to the same value (`null/null` for unset) → still clean; only the toggle's `onChange` (booleans only) can diverge them → "unsaved changes" appears exactly on user edit. No dirty-check surprise. **4. No-clobber preserved — the effect cannot reseed after user edit.** The gate is `doc !== undefined && getSelf() === undefined`, and the only writers are the effect itself (writes seed values, never `undefined`), the boolean-only `onChange`, and save/failure-reseed (booleans or fresh seeds, never `undefined`). Once seeded, the condition can never go true again — user edits are safe. **5. Explicit true/false render correctly** — same `!== false` expression covers both without branching (pinned by the new test). **6. Docs amendment (law 12):** `docs/go/12_web_ui.md` gains the `FIXED (Forgejo #605)` entry in the same change. **No new deps (law 1):** the new test imports only `node:test`, `node:assert/strict`, `node:module`, `node:fs`. **7. Tests fail pre-fix, pass post-fix.** New `settings-review-sentinel-605.test.js` (5 tests) + updated `settings-review-586` pin pass on the branch (7/7 with the 586 file). I temporarily restored `origin/main`'s `Settings.jsx` and re-ran the new file: it fails (assertion error on the sentinel pins), then restored the fix byte-for-byte (`git status` clean apart from untracked `web/node_modules`). Full unit suite: 1566/1567 pass; the single failure is `smoke.test.js` ("built SPA shell served at / and /setup", 403 vs 200) — it requires a live server on `:8080` (something else is answering in this environment) and is excluded by the project's own "unit files minus smoke" convention; this diff touches no server code, so it is unrelated and pre-existing. No defects found — nothing to fix, no commit pushed.

crueber merged commit 83b845299e into main

2026-09-15 22:35:58 +00:00

crueber referenced this pull request

2026-09-15 22:35:59 +00:00

Code review section on Settings General tab sticks on 'loading…' forever (unset self-approval knob never leaves the prefill sentinel) #605

crueber referenced this pull request from a commit

2026-09-15 22:36:00 +00:00

Merge pull request 'Fix #605: Code-review toggle stuck on loading for unset docs' (#610) from fix/issue-605 into main