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

New issue

Closed

opened 2026-09-15 21:49:11 +00:00 by crueber · 1 comment

crueber commented

2026-09-15 21:49:11 +00:00

Owner

Code review section on Settings → General sticks on "loading…" forever

What's broken

On the repo Settings page, General tab, the "Code review" section never renders its toggle — it shows the fallback loading… permanently. The "Save code review" button stays disabled. Affects any repo whose settings TOML has no [review] allow_self_approval key — i.e. every fresh/unedited repo, because unset is the common case.

Mechanism (static analysis, no local repro)

Two pieces of the #586 implementation disagree about what null means:

web/src/lib/repoReview.js extractSelfApproval() (lines 27–42): by its own documented contract, an unset knob returns null ("null = unset (renders checked: the server default is allowed)"). Only explicit true/false literals read.
web/src/pages/Settings.jsx prefill effect (lines 153–159): uses getSelf() === null as the "not yet prefilled" sentinel:
```
if (doc !== undefined && getSelf() === null) {
  const seed = extractSelfApproval(...);
  setSelf(seed);      // seed is null for an unset knob
  setSelfBase(seed);
}
```
For an unset knob the effect runs, seeds null, and the condition stays true forever — the signal never leaves the sentinel value.
web/src/pages/Settings.jsx line 427 gates the toggle on the same sentinel:
```
<Show when={getSelf() !== null} fallback={<p class="muted text-sm">loading…</p>}>
```
getSelf() is permanently null, so the toggle never renders and the section shows loading… forever. Line 448's disabled={getSelf() === null} keeps the save button disabled for the same reason.

The sibling feature-toggles section does NOT have this bug because extractFeatures() (web/src/lib/repoFeatures.js line 39) returns a concrete object ({ issues: true, … }) for unset, never null — the prefill seed is distinguishable from "unseeded".

Note the dirty check (selfDirty(), line 245: getSelf() !== getSelfBase()) happens to work with nulls (both null → not dirty), so the only visible symptom is the eternal loading fallback + disabled save.

Fix options (implementer's call)

The clean shape is separating "not yet seeded" from "unset" so the default-ON contract in repoReview.js is preserved:

Option A (recommended): give the prefill a distinct sentinel. Initialize getSelf/getSelfBase to undefined (unseeded) instead of null; the prefill effect checks getSelf() === undefined; the <Show> gate becomes getSelf() !== undefined; the save button's disabled check likewise. null then means only "unset → render checked" per the module contract.
Option B: track a separate selfSeeded boolean signal set true after the first successful seed; gate the <Show> and button on it. Slightly more state, same effect.

Either way, keep extractSelfApproval()'s null-for-unset contract untouched — withSelfApproval()'s append path and the headless tests in web/test/unit/ (if any cover repoReview) rely on it. A regression test asserting "unset TOML → toggle renders checked (not loading)" belongs in the unit suite, mirroring the settingsNav/repoReview headless-test pattern.

Acceptance criteria

A repo with no [review] section (or no allow_self_approval key) shows the toggle rendered checked on Settings → General — never loading….
The "Save code review" button is enabled once the settings doc has loaded.
An explicit allow_self_approval = false renders unchecked; = true renders checked.
User edits are still never clobbered by re-prefill (the null-sentinel intent of the original effect).
Headless unit test covers the unset-TOML → checked-render path.

# Code review section on Settings → General sticks on "loading…" forever ## What's broken On the repo Settings page, General tab, the "Code review" section never renders its toggle — it shows the fallback `loading…` permanently. The "Save code review" button stays disabled. Affects any repo whose settings TOML has no `[review] allow_self_approval` key — i.e. every fresh/unedited repo, because unset is the common case. ## Mechanism (static analysis, no local repro) Two pieces of the #586 implementation disagree about what `null` means: 1. **`web/src/lib/repoReview.js` `extractSelfApproval()` (lines 27–42)**: by its own documented contract, an *unset* knob returns `null` ("null = unset (renders checked: the server default is allowed)"). Only explicit `true`/`false` literals read. 2. **`web/src/pages/Settings.jsx` prefill effect (lines 153–159)**: uses `getSelf() === null` as the "not yet prefilled" sentinel: ```js if (doc !== undefined && getSelf() === null) { const seed = extractSelfApproval(...); setSelf(seed); // seed is null for an unset knob setSelfBase(seed); } ``` For an unset knob the effect runs, seeds `null`, and the condition stays true forever — the signal never leaves the sentinel value. 3. **`web/src/pages/Settings.jsx` line 427** gates the toggle on the same sentinel: ```jsx <Show when={getSelf() !== null} fallback={<p class="muted text-sm">loading…</p>}> ``` `getSelf()` is permanently `null`, so the toggle never renders and the section shows `loading…` forever. Line 448's `disabled={getSelf() === null}` keeps the save button disabled for the same reason. The sibling feature-toggles section does NOT have this bug because `extractFeatures()` (`web/src/lib/repoFeatures.js` line 39) returns a concrete object (`{ issues: true, … }`) for unset, never `null` — the prefill seed is distinguishable from "unseeded". Note the dirty check (`selfDirty()`, line 245: `getSelf() !== getSelfBase()`) happens to work with nulls (both null → not dirty), so the only visible symptom is the eternal loading fallback + disabled save. ## Fix options (implementer's call) The clean shape is separating "not yet seeded" from "unset" so the default-ON contract in `repoReview.js` is preserved: - **Option A (recommended)**: give the prefill a distinct sentinel. Initialize `getSelf`/`getSelfBase` to `undefined` (unseeded) instead of `null`; the prefill effect checks `getSelf() === undefined`; the `<Show>` gate becomes `getSelf() !== undefined`; the save button's disabled check likewise. `null` then means only "unset → render checked" per the module contract. - **Option B**: track a separate `selfSeeded` boolean signal set true after the first successful seed; gate the `<Show>` and button on it. Slightly more state, same effect. Either way, keep `extractSelfApproval()`'s null-for-unset contract untouched — `withSelfApproval()`'s append path and the headless tests in `web/test/unit/` (if any cover repoReview) rely on it. A regression test asserting "unset TOML → toggle renders checked (not loading)" belongs in the unit suite, mirroring the settingsNav/repoReview headless-test pattern. ## Acceptance criteria - [ ] A repo with no `[review]` section (or no `allow_self_approval` key) shows the toggle rendered **checked** on Settings → General — never `loading…`. - [ ] The "Save code review" button is enabled once the settings doc has loaded. - [ ] An explicit `allow_self_approval = false` renders unchecked; `= true` renders checked. - [ ] User edits are still never clobbered by re-prefill (the null-sentinel intent of the original effect). - [ ] Headless unit test covers the unset-TOML → checked-render path.

crueber added the

bug

issues

labels

2026-09-15 21:49:17 +00:00

crueber added this to the v1 milestone

2026-09-15 21:49:17 +00:00

crueber added the

pull requests

label

2026-09-15 21:49:58 +00:00

crueber referenced this issue from a commit

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

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

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

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

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

crueber referenced this issue

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

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

crueber closed this issue

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

crueber commented

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

Author

Owner

Fixed by #610 (merged): prefill sentinel is now undefined (unseeded) with null keeping its unset→checked contract — all four getSelf() reads swapped, contract module untouched, no-clobber preserved. Saving an untouched unset knob writes allow_self_approval=true (semantically identical under default-ON). Verified: 1564 unit green (smoke excluded, pre-existing), vite/esbuild green, independent review APPROVE.

crueber referenced this issue 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