Skip to content

fix(evolve): invalid state after logout - #626

Open
cooper (czxtm) wants to merge 1 commit into
push-ukowyrysxuvmfrom
push-nwttyssqwnls
Open

fix(evolve): invalid state after logout#626
cooper (czxtm) wants to merge 1 commit into
push-ukowyrysxuvmfrom
push-nwttyssqwnls

Conversation

@czxtm

Copy link
Copy Markdown
Member

If using any nixmac-hosted model, if you log out then try to evolve, it
errors due to you no longer being authenticated and therefore unable to
use the provider.

If using any nixmac-hosted model, if you log out then try to evolve, it
errors due to you no longer being authenticated and therefore unable to
use the provider.
@darkmatter

darkmatter Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

🎨 Storybook preview

Open Storybook preview

Updated for 9261d51


⚠️ Detected UI changes (1)

These stories' HTML snapshots changed. I've added screenshots + links to the changed stories below. Review them carefully then accept the changes to regenerate baselines and include them in this PR:

Marketing/Landing Page › Hero App Screenshot

Marketing/Landing Page › Hero App Screenshot


Accept UI changes

  • Click here to accept these changes

Alternatively, you can run bun run test:update-snapshots locally to re-generate the baselines and then push the changes to this PR.

What does this do?

The screenshots above show UI changes detected by the Storybook
snapshot tests run on this PR. Each image is the rendered output of
a Storybook story from the code in this PR branch; the snapshot
test compared it against the committed baseline in
__snapshots__/ and flagged the difference.

Checking the box tells the darkmatter[bot] to regenerate the
baselines from this PR's current code and commit them directly to
this branch. The new baselines become the source of truth for
future runs — only accept after confirming the visual changes are
intentional.

Comparison baseline: the committed __snapshots__/ files on this
PR branch (carried forward from develop). Accept updates them in
place on this branch.

@github-actions

github-actions Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor
Warnings
⚠️

PR description is missing a ## Test Plan (or ## Testing Instructions) section. Add one describing how a reviewer can verify your change, or check No test plan needed if no testing is needed.

⚠️

No Linear issue ID found in this PR's title, description, or branch name (expected something like ENG-123). Add one so this work is traceable in Linear, or add #no-linear to the PR description to acknowledge it's intentionally untracked.

📋 PR Overview

Lines changed 236 (+227 / -9)
Files 2 added, 8 modified, 0 deleted
Draft / WIP no
Has Test Plan no
Linear issue no
No Test Plan Needed no
New UI components no
New Storybook stories no
New Rust modules no
New TS source files yes (1)
New tests yes (1)
package.json touched no
Cargo.toml touched no
Infra / CI touched no

🔬 Coverage

Report Lines Statements Functions Branches
apps/native/coverage/coverage-summary.json 36.1% 35.8% 31.4% 30.4%

Generated by 🚫 dangerJS against 9261d51

@czxtm
cooper (czxtm) marked this pull request as ready for review August 1, 2026 15:23

@prelint prelint Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning

The hook calls client.account.status() directly via oRPC to fetch auth state, rather than reading it through a viewmodel sync module.

apps/native/src/hooks/use-hosted-model-auth-guard.ts:43

1 finding(s) posted as inline comments.

.status()
.then((status) => {
if (!cancelled && shouldPromptForHostedModelAuth(preferences, status)) {
nav.openSettings("ai-models", "hosted-auth");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning

The hook calls client.account.status() directly via oRPC to fetch auth state, rather than reading it through a viewmodel sync module. The documented constraint is explicit: "Backend state must reach the UI through exactly one path: the viewmodel/.ts sync modules." Auth status (signedIn) is not surfaced anywhere in apps/native/src/viewmodel/, so this PR introduces a second, undeclared path for backend state to reach the UI — exactly what the architectural rule prohibits.

@prelint

prelint Bot commented Aug 1, 2026

Copy link
Copy Markdown

Ship with changes Hosted model auth guard: proactive redirect on logout

Product decisions in this change

Agree with concerns 1. When a user has a nixmac-hosted model selected but is no longer signed in, the app proactively redirects them to Settings with a contextual alert — instead of waiting for them to submit a prompt and receive an authentication error.

Catching the misconfiguration before the user invests effort in composing a prompt is the right instinct. The concern is scope: the guard fires once per app launch by setting a checked flag after the first evaluation. If a user is signed in when the app starts, uses the app, then logs out mid-session, the guard never re-fires — they will still hit the raw authentication error when they try to evolve. The PR description frames the bug as "if you log out then try to evolve" without qualifying whether that means across restarts or within the same session. For the across-restart case the fix is solid. For the in-session case it is not a fix.

Option What the user experiences Gap Effort to reverse
Current (once-per-launch) Clean startup redirect if already logged out Mid-session logout still hits raw error Minimal — add reactive subscriber
Reactive on auth state change Prompt appears whenever sign-out is detected, regardless of session age Potentially jarring if shown mid-prompt-compose Low — auth state is already in ViewModel

Agree 2. The auth guard checks both the evolve provider and the summary provider — either role using nixmac hosted inference triggers the warning.

Both roles make authenticated API calls. A user who only has the summary provider set to nixmac could be surprised when summarization silently fails after logout. Checking both roles is complete and correct. The abstraction (hasNixmacHostedModelSelected) is well-named and the tests cover both branches.

Disagree 3. The warning alert persists across all settings tabs — it does not clear when the user navigates to the Account tab to log back in, nor when they successfully authenticate.

When the user clicks

Agree with concerns 4. The guard makes a live API call to check auth status rather than reading cached auth state from the ViewModel.

Using a live check avoids acting on stale cached state that might reflect a previous session. The concern is offline behavior: if the device has no network at startup, the call fails, the error is swallowed, and a logged-out user with a hosted model selected sees nothing. They only discover the problem when they try to evolve — which will also fail. A fallback to cached auth state (if available) on network error would give better offline-first behavior.

Agree 5. Users are given two resolution paths — log back in or select another provider — presented as peers of equal prominence.

Both are valid resolutions. Some users genuinely want to stop using the hosted model (e.g., they've subscribed to OpenRouter). Others just want to re-authenticate. Presenting both avoids forcing re-authentication as the only path. The "Log back in" button is primary (solid) and "Select another model" is secondary (outline), which appropriately weights toward the simpler recovery path.

Agree with concerns 6. The destructive alert styling is changed globally — removing text-destructive coloring from the body, keeping only the border and icon in destructive color — as part of this bug fix.

The visual change (red border + icon, normal text color) is defensible: it makes the alert feel like a callout rather than a full-system error, which matches how the hosted-auth alert is used. The concern is that this is a global change to all destructive alerts bundled into a targeted bug fix. Any existing destructive alert in the app — whether surfacing a critical error, a data-loss warning, or a build failure — will now have muted text. The change may be correct but deserves separate product review with a broader view of where destructive alerts appear.

Agree with concerns 7. The contextual prompt is encoded as a URL search parameter on the settings route, preserved when navigating between settings tabs.

Encoding the prompt in the route means deep-linking into a specific settings state is possible and the prompt survives tab navigation — both useful properties. The concern is that there is no mechanism to clear the prompt once the user resolves the issue (as discussed above). A well-bounded alternative would be component-local state (a useState flag) that dismisses when the user clicks either CTA, avoiding the persistence problem entirely at the cost of deep-link support that is unlikely to matter here.

Agree 8. The guard will not prompt during onboarding — it waits for `onboardingState.completedAt` to be set.

Prompting a user to fix their provider configuration before they have finished initial setup would be premature and confusing. Gating on onboarding completion is the correct ordering.

Open questions

  • Does the logout flow in nixmac cause the DarwinWidget component tree to unmount and remount? If so, the once-per-launch limitation is less severe because the checked ref would reset, making the guard re-run after logout. If not, mid-session logout is still unhandled.

  • Are there other destructive alerts in the app today (build failures, dangerous-tool warnings, configuration errors) that would be affected by the global alert styling change? The PR fixes one visual problem but may introduce muted styling in contexts where high-urgency color was appropriate.

  • What is the expected user journey for someone who has inadvertently been logged out — do they know they were using a hosted model, or will they be surprised by the prompt? Does the alert copy assume too much familiarity with the distinction between hosted and BYOK models?

Recommendation

Ship with changes
The startup-guard pattern and the two-CTA resolution flow are the right product instincts. However, the alert persisting visibly after the user has already acted to resolve it (by navigating to Account) is a concrete confusion to fix before shipping, and the question of whether mid-session logouts are actually caught should be confirmed. The global alert styling change should also be intentionally reviewed rather than landing as a side effect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant