Menu ▾ ▴

#60 feat(git,accounts,ui,settings): in-app forge sign-in, reconnect & session health

closed
nobody
enhancement (85)
2026-07-17
2026-07-17
Anonymous
No

Originally created by: theBGuy
Originally owned by: theBGuy

This change introduces a complete in-app forge sign-in and session health experience, allowing users to sign in to GitHub and GitLab directly from the app (no terminal required), receive proactive expiry warnings, and seamlessly reconnect expired or revoked sessions. The motivation is to eliminate context-switching to the terminal for auth flows, improve trust in session state (robust anti-flap detection), and ensure users are aware of expiring credentials before disruptions occur.

Forge session health core

  • Adds new session health detection logic in src-tauri/src/forge/session.rs, with robust anti-flap rules (double-probe before declaring broken, never alarm on transient outages).
  • Introduces SessionHealth and related types (SessionState, etc.) across backend and frontend (src-tauri/src/forge/session.rs, src/lib/git/types.ts).
  • Exports new Tauri commands in src-tauri/src/lib.rs for session health and reconnect control: forge_session_health, forge_accounts_health, forge_reconnect, forge_reconnect_cancel.
  • Minor exports and visibility adjustments in src-tauri/src/github/pr.rs and src-tauri/src/forge/mod.rs for integration.

In-app sign-in/reconnect flows

  • Implements a global, cancellable ReconnectDialog in src/features/accounts/ReconnectDialog.tsx with device-code flow (GitHub), browser flow (GitLab), and status streaming via Tauri/Channels.
  • Adds the dialog to the app shell in src/App.tsx and exposes open/close actions via store changes in src/lib/stores/ui.ts.
  • Frontend reconnect flows invoke backend via new APIs in src/lib/git/api.ts and manage UI state diligently.

Session expiry warnings

  • Presents a quiet, dismissible expiry notice (with days-remaining countdown) on PR/Issue panels, leveraging health and settings data: src/features/accounts/SessionExpiryNotice.tsx, called in src/features/pulls/PullRequestsPanel.tsx and src/features/issues/IssuesPanel.tsx.
  • Bitbucket token expiry is optionally user-supplied in src/lib/settings/api.ts, src/features/settings/AccountsSection.tsx.

Forge readiness UX (when not signed-in or session is broken)

  • Updates "not ready" panels for GitHub/GitLab (src/features/repository/ForgeNotReady.tsx) to distinguish new session, expired session, and network blips, surfacing the ReconnectDialog and affording both CLI and terminal fallback.
  • All surfaces gracefully handle "offline" (inconclusive) probes.

Accounts settings & palette integration

  • Overhauls the Accounts section (src/features/settings/AccountsSection.tsx) to surface session health state badges per account, show "Reconnect" on expired/logged-out sessions, and merge expiry data for Bitbucket and other providers.
  • Palette "Reconnect forge session" command (with shortcut, registry wired in src/lib/hotkeys/registry.ts) opens the in-app reconnect experience, context-aware by provider/session state (src/features/repository/RepositoryView.tsx).
  • Bitbucket expiry field and validation added, plus updated helpers for days-until, date formatting.

Documentation & messaging updates

  • Readme and help content revised to describe in-app forge sign-in, reconnect, and session expiry flow (README.md, src/features/help/content.ts, site/src/data/capabilities.ts).

Changelog

  • New entry in changelog.d/added-forge-session-health.md summarizing the features at user level.

Related

Tickets: #74

Discussion

  • Anonymous

    Anonymous - 2026-07-17
     

  • Anonymous

    Anonymous - 2026-07-17

    Originally posted by: theBGuy

    🤖 GitDesktop AI review · opus · automated

    I'll review this PR. Since large portions of the diff are truncated (the 1611-line session.rs, and parts of ReconnectDialog.tsx, AccountsSection.tsx, the panels), let me read the actual files to ground my review.Let me read the remaining truncated portions to ground the review.Let me verify a couple of things: the layout wrapper in the panels (does ConversationListPanel fill correctly?), and providerLabel.Confirmed the layout wrapper matches ConversationListPanel's own root (flex min-h-0 flex-1 flex-col), so no layout regression there, and providerLabel handles all three providers. Here's the review.

    Summary

    Adds in-app gh/glab sign-in + reconnect with anti-flap session-health probing, expiry warnings, and a cancellable child-process driver. The design is sound and unusually well-defended (input validation, token redaction, RAII cleanup, the cancel-before-register race handled, solid Rust unit tests). Not blocking, but there's a real (if slow) registry leak and a cross-surface day-count inconsistency worth fixing before merge.

    Correctness / resource management

    • should-fix — src-tauri/src/forge/session.rs, cancel_reconnect / the RECONNECT_REGISTRY. The module comment promises "Entries are removed on EVERY exit path," but cancel_reconnect uses entry(...).or_insert_with(...), so a cancel that arrives after the flow already finished re-creates an orphan tombstone that nothing ever removes. Concrete path that fires on every successful reconnect: the child exits → forge_reconnect returns → ReconnectGuard drops → unregister_reconnect removes the entry → the frontend's onEvent schedules setTimeout(onClose, 1200) → 1.2s later the dialog unmounts → ReconnectDialog's cleanup calls forgeReconnectCancel(id) → cancel_reconnect inserts a fresh Notify tombstone for that dead id that is never adopted or removed. The "Try again" path (start() cancels the prior, already-finished id) leaks the same way. It's small and bounded per attempt, but it grows unbounded over the app's lifetime and contradicts the stated invariant.
    • Fix (frontend, cleanest): track a finishedRef set when a terminal finished event arrives, and have both the unmount cleanup and start()'s prior-cancel skip forgeReconnectCancel once the session has finished. Belt-and-suspenders (backend): have cancel_reconnect notify-if-present, and when it creates a tombstone, spawn a short delayed unregister_reconnect so an unadopted tombstone can't linger past the spawn window.

    Consistency

    • should-fix — Bitbucket "days left" is computed two different ways for the same stored YYYY-MM-DD. In src/features/accounts/SessionExpiryNotice.tsx, daysUntil does Math.floor((Date.parse(iso) - now)/86_400_000) where Date.parse("2026-07-20") is parsed as UTC midnight. In src/features/settings/AccountsSection.tsx, daysUntilDate does Math.ceil((Date.parse(${date}T23:59:59) - now)/86_400_000) — local end-of-day with ceil. For a token expiring 2026-07-20, the PR/Issues banner can say "expires in 3 days" while the Accounts badge says "expires in 5 days" for the identical token. Fix: extract one shared helper (same anchor time and rounding) and use it in both surfaces.
    • Same finding, minor sub-point: SessionExpiryNotice renders "expires today" for a negative (already-lapsed) Bitbucket count, whereas AccountsSection clamps to 0. Folding both into the shared helper resolves this too.

    Nits

    • nit — src/features/repository/RepositoryView.tsx, the reconnect-forge-session hotkey. mode is sessionHealth.data?.state === "notConnected" ? "login" : "refresh"; the action is gated only on forgeProvider !== null, not on health having loaded. For a never-signed-in GitHub repo where useForgeSessionHealth hasn't resolved yet, data is undefined → mode: "refresh" → gh auth refresh errors ("no account"). It degrades to a failed-finish with fallbacks rather than a crash, but first-run from the palette can open the wrong mode. Consider defaulting to "login" when sessionHealth.data is undefined (or gating the action until health resolves).

    Posted by GitDesktop — AI output, verify before acting on it.

     

  • Anonymous

    Anonymous - 2026-07-17

    Originally posted by: theBGuy

    🤖 GitDesktop AI security audit · opus · automated

    I'll ground my review by reading the actual source that the diff truncated — especially the Rust reconnect driver and how attacker-influenceable values (host, session output) flow into subprocess spawns and streamed events.Let me quickly verify the truncated frontend files introduce no unsafe sinks (HTML injection, URL opening from untrusted data).No high-confidence, exploitable security vulnerabilities are introduced by this diff.

    I traced the security-relevant flows in the new code:

    • forge_reconnect subprocess spawn (session.rs): every input is validated before spawn — session_id against [A-Za-z0-9-]{8,64}, host restricted to [A-Za-z0-9.-], mode to login|refresh, provider matched to a fixed set; the binary names (gh/glab) are hardcoded and resolved via resolve_named. Args are passed as discrete argv elements to tokio::process::Command (no shell), and host is always bound as the value of --hostname, so no command- or argument-injection path exists.
    • Streamed reconnect output: every line is passed through redact_tokens (gh/glab token prefixes) + 300-char truncation before reaching the frontend; detail/message/login are all derived from sanitized lines. No token/secret is forwarded or logged.
    • openUrl sinks: in AccountsSection they target hardcoded constant URLs; the reconnect dialog opens only the device-flow URL for the already-configured forge host (https, GHES-support behavior gated behind explicit user action on a clearly-labeled host) — not an attacker-controlled protocol.
    • Settings/expiry: bitbucketTokenExpiresAt is a non-secret user-entered date; SessionExpiryNotice/AccountsSection render via React (no dangerouslySetInnerHTML, no eval).

    The one prior comment flagged a registry-entry leak in cancel_reconnect — that's a resource-cleanup bug, not an exploitable security issue, and falls under the excluded resource-exhaustion category.

    Posted by GitDesktop — AI output, verify before acting on it.

     

  • Anonymous

    Anonymous - 2026-07-17

    Originally posted by: theBGuy

    Triage — round 1 dispositions (all findings verified against the code; fixes land in the next push)

    • Registry tombstone leak (should-fix) — confirmed, fixing both ends. Exactly as described: after a successful flow the guard removes the entry, then the dialog's 1.2s auto-close unmount fires forgeReconnectCancel, whose entry().or_insert_with() re-creates an orphan tombstone; Try-again leaks the same way. Fix: (1) frontend clears sessionIdRef on the terminal finished event so completed flows are never cancelled; (2) backend validates the id in forge_reconnect_cancel (same grammar as forge_reconnect) and self-cleans unadopted tombstones via a delayed sweep (adoption is observable through the Arc refcount — a running flow holds a clone). The module's "removed on every exit path" invariant becomes true for cancel-created entries too. (This also covers the security audit's relay of the same finding.)
    • Bitbucket days-left divergence (should-fix) — confirmed. daysUntil floors from a UTC-midnight parse; daysUntilDate ceils to local end-of-day — same token, different counts, plus the today-vs-0 clamp drift. Fix: one shared local-calendar-day helper (pure date-to-date diff) used by the notice and the Accounts card, unified expired/today wording.
    • Palette mode before health resolves (nit) — accepted, different mechanism. Right defect, but defaulting to login when health is unresolved would silently narrow granted scopes for the common signed-in case: gh auth login requests the default scope set, while gh auth refresh "maintains previously added scopes" (its own help) — a user with workflow/extra scopes would lose them. Fix instead: gate the palette action's enabled on session health having loaded, so the mode decision is never made blind. The failure described (refresh on a never-signed-in host) already degrades to the failed-finish fallbacks, and can no longer be reached from the palette.

    Copilot's six inline findings are all confirmed too (dispositions on each thread): mode-aware terminal fallback command, finished-clears-ref, UTC-midnight date parse (folds into the shared helper), unknown-flag-only --json fallback with other non-zero → offline, a 300-char bound in sanitize_detail, and id validation in forge_reconnect_cancel. A genuinely clean 7/7-real round.

    Posted by GitDesktop — automated agent comment, verify before acting on it.

     

  • Anonymous

    Anonymous - 2026-07-17

    Originally posted by: theBGuy

    🤖 GitDesktop AI review · opus · automated

    I'll ground my review by reading the truncated files — especially session.rs (the reconnect driver and registry), and the truncated frontend portions.Let me read the rest of session.rs (tests and remaining driver code) and the truncated AccountsSection.tsx portion.Let me check the Bitbucket expiry-date input handling and clear-on-disconnect wiring in AccountsSection, plus the GitLab sign-in block, since those parts weren't shown.This is a well-executed change, and I've verified the current state against both the previous review and Copilot's findings by reading session.rs (full, incl. tests), ReconnectDialog.tsx, and the AccountsSection Bitbucket/GitHub blocks.

    Summary

    Adds in-app gh/glab sign-in + reconnect with anti-flap session-health classification, expiry warnings, and a cancellable child-process driver. The design is sound and unusually well-defended (input validation, token redaction, RAII cleanup, the cancel-before-register race, and a good spread of Rust unit tests including the new classifier/sweep/id-grammar cases). Every issue from the prior review and every Copilot item is addressed in the current diff — I found no new blocking or should-fix problems. Ship it.

    Resolved since last review

    • Registry tombstone leak (session.rs cancel_reconnect + ReconnectDialog.tsx) — fixed on both ends. The frontend now nulls sessionIdRef.current on the terminal finished event (line 122), so the unmount cleanup and start()'s prior-cancel no longer cancel an already-finished flow. As belt-and-suspenders, cancel_reconnect now distinguishes an Occupied entry (adopt, let the guard clean up) from a Vacant one (insert a tombstone), and only for the created-tombstone case schedules sweep_unadopted_tombstone after 60s, which removes the entry iff Arc::strong_count == 1 under the lock. A running flow (which holds a guard clone → count ≥ 2, and tolerates the full 900s RECONNECT_TIMEOUT) is correctly spared. Covered by sweep_removes_unadopted_tombstone / sweep_spares_adopted_entry.
    • Bitbucket days-left divergence — the two ad-hoc helpers (daysUntil floor-from-UTC-midnight, daysUntilDate ceil-to-local-EOD) are gone; both SessionExpiryNotice and AccountsSection.BitbucketAccount now compute from the shared calendarDaysUntil (pure local date-to-date diff), and the expired/today/N-days wording is unified across both surfaces. The clear-on-disconnect path (setBitbucketTokenExpiresAt(null) at line 721) is wired too.
    • Palette mode decided before health resolves — the reconnect-forge-session action is now gated forgeProvider === "bitbucket" || (forgeProvider !== null && sessionHealth.data !== undefined), so github/gitlab never derive mode from undefined health; the comment documents the deliberate choice not to default to "login" (scope-narrowing) either.

    Copilot findings — all resolved in the current diff

    • fallbackCommand for refresh → now gh auth refresh --hostname when isGitHub && mode === "refresh" (line 101–104).
    • sessionIdRef left set after finish → nulled at line 122.
    • daysUntil UTC date-only parse → replaced by calendarDaysUntil.
    • gh_status_json treating any non-zero as unknown-flag → now classify_gh_json_nonzero splits unknown-flag (→ text fallback) from other failures (→ Inconclusive → Offline), tested.
    • sanitize_detail length bound → now redacts, collapses newlines, and caps at 300 chars (tested by sanitize_detail_caps_at_300), and the gh error-field call sites route through it.
    • forge_reconnect_cancel id validation → now rejects malformed ids via valid_session_id before touching the registry (line 900–902).

    No further findings.

    Posted by GitDesktop — AI output, verify before acting on it.

     

  • Anonymous

    Anonymous - 2026-07-17

    Ticket changed by: theBGuy

    • status: open --> closed
     

Log in to post a comment.

MongoDB Logo MongoDB