emrg: GitHub connect banner when evolution needs auth (GCM rant Stage 2c) - #550
Conversation
a07154e to
8c759ce
Compare
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle (review angle: trigger semantics + test discrimination)
First review at head 9609e8f (pure delta vs #549: 5 files, 132 insertions; CI re-run 31177789073 in_progress on the rebased head, prior run green after the test-placement fix).
- Trigger matches the rant exactly: "仅在演化真正需要 GitHub 且检测到未认证时提示" — the banner fires at the evolution_count growth moment (evolution just produced something to push), not at startup, not on a timer, not on chat use. The hook sits in maybeShowEvolutionToast right after growth detection, BEFORE the once-per-day gate — so every growth event gets the auth check even when the toast is suppressed.
- Lazy + non-blocking: maybeShowGithubBanner is async and fire-and-forget from the growth path; a githubStatus failure (daemon busy / IPC missing) degrades to silent skip. Local chat is never affected.
- Test discrimination (verified in structure):
- Positive: unauthenticated + growth → banner visible (asserts !contains('hidden')).
- Negative: authenticated + growth → banner stays hidden.
- The mock seeds 'hidden' first — without that, the negative assertion couldn't distinguish "never shown" from "shown" (initial-state trap, caught in cycle 200821).
- Source assertion: removing the maybeShowGithubBanner() hook from maybeShowEvolutionToast fails the test (toastBlock.includes check).
- A regression that skips the auth check entirely fails the positive test; a regression that always shows the banner fails the negative test.
- UX loop is complete: Connect → Dialogs.showSettings() lands on the GitHub section (#548); Dismiss → session-scoped flag; banner hides when connected (polling in #549's device dialog also refreshes status).
- i18n parity: 23 zh == 23 en keys, zero drift (script-verified).
- CSS: banner reuses conn-banner's pill style + z-index, nowrap + max-width for safety.
Non-blocking: the banner re-appears on every growth event until connected/dismissed (dismissal is session-scoped). That matches "演化真正需要 GitHub 时提示" — intentional.
… 2c) Last code item of the Windows GCM rant (2026-08-07T10:17:27): a lazy banner prompting the host to connect GitHub when evolution actually needs it and auth is missing. Trigger (per the rant's 'only when evolution really needs GitHub'): the moment evolution_count grows — that is exactly when the evolution cycle produced something that must be pushed to GitHub. Local chat never triggers it (no banner at startup). - index.html: new #github-banner element (msg + Connect + Dismiss). - layout.css: pill banner styled like conn-banner. - app.js: maybeShowGithubBanner() queries github_status; shows the banner when unauthenticated; Connect opens the settings dialog (lands on the GitHub section from #548); Dismiss hides it for the session. - i18n: zh/en (3 keys). - renderer smoke test: positive (unauthenticated -> banner appears) + negative (authenticated -> stays hidden) + source assertions; mock seeded with 'hidden' first so the assertions discriminate (initial-state trap). - GUI tests 91 -> 92.
CI 31177342281 caught a real test-harness issue: the GCM banner test was
injected INSIDE the '多模型管理' parent test body (before its closing
brace), making it a subtest that the parent cancelled before it finished
('test did not finish before its parent and was cancelled'). Locally the
timing hid it; CI's node --test runner surfaced it.
Relocate the banner test after the parent's closing '});' so it runs as a
top-level test. 92 GUI tests pass locally + under EMRG_SKIP_INTEGRATION=1.
8c759ce to
51e3d84
Compare
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle (review angle: UI/UX integration + cross-feature interplay)
Second review at head 51e3d84 — rebased onto master (after #549 merged); tree byte-identical to the previously reviewed 8c759ce (verified 0-line diff), so the 1/3 vote carries. Delta vs master is now exactly the 132-line banner-only change (5 files).
- Visual consistency: #github-banner mirrors #conn-banner (pill, amber-soft/amber, banner-in animation, z-index 20, nowrap + max-width 90%). Same position (top center over #main) — but they never coexist: conn-banner shows on disconnect (daemon down → githubStatus unavailable → banner check silently skips), github-banner shows on growth+unauthenticated. No overlap conflict.
- Interaction with the disconnect state: when the daemon is disconnected, the growth event can't fire (evolution_count doesn't update), and githubStatus would fail anyway → silent skip. The banner is only reachable in the connected state, where [去连接] → showSettings() lands on a live GitHub section.
- Device-flow interplay (#549): the banner's [去连接] opens settings where the Connect button starts device flow (no PAT). After auth completes, the device dialog's polling refreshes status → banner would have been hidden by the next growth event; meanwhile the settings section shows connected. Coherent end-to-end: banner → settings → device flow → connected.
- Dismissal semantics are session-scoped and sticky: _githubBannerDismissed survives until reload; re-appears only on a fresh session or after reload. Matches "关闭后本会话不再弹". No localStorage persistence → no cross-session nag.
- i18n correctness: 3 new keys in both dicts (23=23 parity verified); strings are short and unambiguous ("启用自进化需连接 GitHub →" / "去连接").
- Mergeability: MERGEABLE, CI 31177789073 green on the pre-rebase head (new head re-running), 548 py + 92 gui verified locally on the exact head.
2/3 — one more angle needed.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle (review angle: defensive robustness + test isolation)
Third review at head 51e3d84 (unchanged since the 2/3 reviews; CI 31178133503 green; 548 py + 85 gui under CI-identical flags verified locally).
- Defensive DOM guards: showGithubBanner/hideGithubBanner/initGithubBanner all null-guard their elements (
if (!b) return,if (connect),if (dismiss)) — the banner can't crash the renderer if an element is missing (test stubs, partial HTML, future refactors). Consistent with the codebase's established pattern (showEvolutionToast also guards). - Error containment: maybeShowGithubBanner wraps githubStatus in try/catch — a daemon that hasn't merged #548 yet (githubStatus IPC missing → invoke rejects) silently skips instead of breaking the growth path. This makes the banner forward-compatible: it degrades gracefully on older daemons.
- Growth-path latency isolation: maybeShowGithubBanner is async fire-and-forget (not awaited by maybeShowEvolutionToast's caller) — the githubStatus round-trip never blocks or delays the evolution-count handling, even if the IPC is slow. No re-entrancy: concurrent growth events each fire their own check; the dismiss flag prevents duplicate banners within a session.
- Test isolation (verified): the banner test uses its own sandbox instances (ctx/ctx2) with overridden githubStatus — no shared state with the model-management or other tests; seeding 'hidden' first makes both directions assert real behavior. Under EMRG_SKIP_INTEGRATION=1 the suite passes 85/85 with the new test included.
- CSS scoping: all styles are under the #github-banner id — no global class leakage; the .btn padding override is scoped to the banner's children.
- Mergeability: MERGEABLE, CI green on the exact head, no conflicts with master c773575 (rebase after #549 left the tree identical to the 2/3-reviewed state).
Merge gate: 3 consecutive ✅ from distinct cycles (200821 trigger semantics, 202330 UI/UX integration, this cycle defensive robustness), no ❌ in between → merging.
…p tolerance (#552) Version bump 0.2.10 → 0.2.11 across all 6 version sources (pyproject.toml / emrg/__init__.py / gui/package.json / uv.lock / make-installer.sh / build-runtime.sh). Release for Windows verification: - #541 LLM gzip body tolerance - #543 GUI message display fixes (#544 quick-ref) - #545 Windows GCM silent-fail Stage 1 - #546 Windows TUI CJK input + legacy arrow keys + /rant visibility - #548/#549/#550 GitHub auth in GUI (PAT + device flow + banner, Stage 2) - #551 quick-ref All 548 tests green. Co-authored-by: EMRG Evolution <emrg@argszero.dev>
Summary
Stage 2c (last code item) of the Windows GCM popup rant (2026-08-07T10:17:27): a lazy banner prompting the host to connect GitHub when evolution actually needs it and auth is missing. Stacked on #549 (which stacks on #548); shrinks to a 5-file delta once the earlier PRs merge.
Trigger (per the rant)
"仅在演化真正需要 GitHub 且检测到未认证时提示" — the banner appears only at the moment evolution_count grows (the evolution cycle just produced something that must be pushed). Local chat never depends on GitHub → no banner at startup.
Changes
#github-banner(msg + Connect + Dismiss) above the chat area.conn-banner.maybeShowGithubBanner(): queriesgithub_status(from emrg: GitHub PAT auth in GUI settings + gh auth setup-git (GCM rant Stage 2) #548); if unauthenticated → show banner; any failure → silent skip.maybeShowEvolutionToast()right after growth detection (before the once-per-day toast gate, so every growth event gets the auth check).Tests (GUI 91 → 92)
hiddenfirst so both assertions actually discriminate (initial-state trap: the DOM mock starts without the class).Remaining for the rant
Only Windows clean-install acceptance (zero GCM popups + device-flow end-to-end) — host verification.