Skip to content

fix(web): stop displaying preemptive password modal during connecting - #228

Merged
co-l merged 2 commits into
co-l:developfrom
theshwal:devflow/theshwal/openfox/issue-4-no-password-modal-on-local
Aug 12, 2026
Merged

fix(web): stop displaying preemptive password modal during connecting#228
co-l merged 2 commits into
co-l:developfrom
theshwal:devflow/theshwal/openfox/issue-4-no-password-modal-on-local

Conversation

@theshwal

@theshwal theshwal commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes theshwal#4

The web UI was rendering <PasswordModal isOpen={true} /> in the pre-connection branch of App whenever no token was in localStorage, before GET /api/auth had resolved. Servers that do not require authentication would briefly flash a "Password Required" dialog with a stale hardcoded message before the WebSocket handshake completed, even though the server eventually returned requiresAuth: false.

Root cause

web/src/App.tsx (line 362) had a pre-connection branch that rendered the modal preemptively based solely on the absence of a local token:

if (!isReadonly && connectionStatus !== 'connected' && !showPasswordModal && !hasToken) {
  return (
    <>
      <PasswordModal isOpen={true} isRetry={passwordModalRetry} onSubmit={submitPassword} onCancel={cancelPassword} />
      <div className="h-screen flex items-center justify-center">
        <SpinnerWithText text="Connecting to server..." />
      </div>
    </>
  )
}

The state showPasswordModal is only set by the session store after /api/auth returns requiresAuth: true, or after a 401/closeCode 4000. The pre-connection choice was therefore not data-driven, and the dialog's hardcoded title ("Password Required") and body ("This server requires a password to connect.") were misleading as long as the server had not yet confirmed.

The server contract was already correct: GET /api/auth returns { requiresAuth: requiresAuth() && hasPassword(), hasPassword: hasPassword() }, and the session store handles that response correctly. The only culprit was the preemptive UI render.

Minimal fix

Removed only <PasswordModal> and the surrounding fragment from the pre-connection branch. The condition (notably !showPasswordModal) and the spinner wrapper are preserved verbatim. The modal is now shown exclusively through the second render path, controlled by the store's showPasswordModal flag.

if (!isReadonly && connectionStatus !== 'connected' && !showPasswordModal && !hasToken) {
  return (
    <div className="h-screen flex items-center justify-center">
      <SpinnerWithText text="Connecting to server..." />
    </div>
  )
}

The fix is intentionally scoped to the false-positive startup case. On servers that legitimately require authentication, the modal still appears as soon as the store flips showPasswordModal to true after /api/auth resolves.

Tests

Two regression tests were added in web/src/App.test.tsx under a new describe('App - Password modal rendering') block:

  • Negative case: with connectionStatus='reconnecting', showPasswordModal=false, and no token, the rendered DOM contains Connecting to server..., does not contain Password Required, and the modal's data-testid element is absent.
  • Positive case: with connectionStatus='reconnecting' and showPasswordModal=true, the modal is rendered with Password Required.

Both tests pass when the fix is applied. The negative test fails when the modal is reintroduced in the pre-connection branch (verified locally), confirming the test catches the regression.

Test command: npx vitest run web/src/App.test.tsx → 3/3 passed.

Quality checks

  • npm run typecheck → passed
  • npm run lint → passed
  • npm run format → passed
  • npm run test:unit → 3472/3473 passed
    • 1 pre-existing flaky test in src/server/routes/plugins.test.ts (GET /installed > returns empty list when no plugins directory exists) failed intermittently across runs. This test is environmentally dependent on the user-level plugins directory and is not related to this change. The user has explicitly acknowledged this as a pre-existing flaky test.

Out of scope

  • No changes to src/server/auth.ts or src/server/index.ts; the server-side requiresAuth()/hasPassword() logic and the truth table covered by src/server/auth.test.ts are already correct.
  • e2e/auth.test.ts remains describe.skip per its existing isolation constraints; no global reactivation was attempted.
  • No hostname/locality heuristics added to the frontend (forbidden by the issue).
  • No refactor of PasswordModal, the session store, or the WebSocket layer.

Files changed

  • web/src/App.tsx: 9 lines (corrective)
  • web/src/App.test.tsx: 229 lines (regression tests)

AI-Enhanced Development

  • AI Models: MiniMax-M3

Cache Impact

  • No

The PasswordModal was rendered in the pre-connection branch of App whenever no token was in localStorage, before /api/auth had resolved. Servers that don't require auth would briefly show "Password Required" with a stale hardcoded message before the WS handshake completed.

The condition is unchanged; only the modal itself is removed from the preemptive branch. The modal still renders via the second render path once showPasswordModal is set by the store after /api/auth confirms auth is required, or after a 401/closeCode 4000.

Adds two regression tests in web/src/App.test.tsx:

- negative: no PasswordModal when reconnecting without token and server does not require auth

- positive: PasswordModal renders via showPasswordModal state after /api/auth confirms auth required

Fixes #4
@theshwal
theshwal marked this pull request as draft August 8, 2026 22:05
@theshwal
theshwal marked this pull request as ready for review August 8, 2026 22:05
@theshwal

theshwal commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

ai-disclosure is the only failing check. The PR body already contains the required AI model disclosure. Could you please rerun that failed job?

@theshwal theshwal closed this Aug 9, 2026
@theshwal theshwal reopened this Aug 9, 2026
@co-l
co-l merged commit 35d5cbd into co-l:develop Aug 12, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants