Skip to content

fix(web): authorized-client revoke no longer silently leaves the row - #6282

Open
RioPlay wants to merge 1 commit into
pingdotgg:mainfrom
RioPlay:fix/web-revoke-authorized-session-ui
Open

fix(web): authorized-client revoke no longer silently leaves the row#6282
RioPlay wants to merge 1 commit into
pingdotgg:mainfrom
RioPlay:fix/web-revoke-authorized-session-ui

Conversation

@RioPlay

@RioPlay RioPlay commented Aug 12, 2026

Copy link
Copy Markdown

Problem

One-off Revoke on Settings → Connections → Authorized clients could look like a no-op: the button flashed "Revoking…", then the row stayed, with no error. Reported on Windows.

Cause

Two client gaps stacked:

  1. POST /api/auth/clients/revoke can return 200 with { revoked: false } when nothing matched. The client ignored that flag and treated any non-throwing response as success.
  2. The list only updated from the live auth-access stream (clientRemoved). There was no optimistic hide and no local check of the HTTP result, so a no-op or a lagging stream left the row in place.

Windows was where it was noticed; the contract bug is cross-platform (not a Windows-only code path).

Fix

  • Treat revoked: false as failure for client-session and pairing-link revoke (clear error toast).
  • Hide the row as soon as revoke succeeds, then prune the hide set when the stream catches up.
  • Same optimistic hide for "Revoke others" when revokedCount > 0.
  • Focused unit tests for the revoke contract and hide helpers.

Verification

  • vp test run apps/web/src/environments/primary/auth.revoke.test.ts apps/web/src/components/settings/ConnectionsSettings.logic.test.ts — pass
  • Manual smoke on Windows: individual revoke removed the row as expected

Note on confidence

Confirmed as a real product defect in the client/server contract and UI update path. Not proven with a captured Network response of revoked: false on the original report; the fix still closes the silent-failure path that matches the reported symptom.

Model: Grok 4.5 (xAI) · harness: Grok Build TUI


Note

Medium Risk
Touches client-side access-management revoke handling and list visibility for authorized clients. Risk is moderate because incorrect optimistic hide or revoke-result checks could briefly misrepresent access state, but server auth itself is unchanged.

Overview
Fixes Settings → Connections revoke looking like a no-op when the row stayed after “Revoking…”.

revokeServerClientSession and revokeServerPairingLink now treat HTTP 200 with revoked: false as failure and surface an error toast, instead of treating any non-throwing response as success.

Authorized clients/pairing links also hide optimistically on successful revoke (including “Revoke others”), then prune the hide set when the auth-access snapshot catches up—so lagging or dropped stream events no longer leave stale rows.

Reviewed by Cursor Bugbot for commit c77a7fd. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Fix revoked authorized-client rows silently persisting in the connections settings UI

  • revokeServerClientSession and revokeServerPairingLink in auth.ts now throw when the server returns revoked: false, preventing a silent no-op from being treated as success.
  • The connections settings UI now optimistically hides revoked pairing links and client sessions immediately after a successful revoke call, using new withHiddenAccessId / excludeHiddenAccessRows helpers.
  • A useEffect in ConnectionsSettings.tsx reconciles the hidden-id sets against live snapshots so stale entries are cleaned up when the server data refreshes.
  • Bulk "revoke other sessions" now also optimistically hides all affected rows when the server reports at least one was revoked.

Macroscope summarized c77a7fd.

Revoke treated HTTP 200 as success even when the server returned revoked:false, and the list only updated via the live auth stream. Check the revoke flag, hide the row as soon as revoke succeeds, and show a real error when nothing was revoked.
@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: fca188da-3240-49c0-96b0-6fc790093e34

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Aug 12, 2026

@macroscopeapp macroscopeapp Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One convention issue: the two new revoke no-op failures are raised as plain Errors with an unstructured message, while every other failure in environments/primary/auth.ts is a Schema.TaggedErrorClass with structured attributes and a derived message. Everything else in the diff (pure hide helpers, React state/effect wiring, the test seam usage in auth.revoke.test.ts) looks consistent with the conventions.

Posted via Macroscope — Effect Service Conventions

Comment on lines +445 to +446
if (!result.revoked) {
throw new Error("That pairing link is no longer active.");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This new failure is raised as a plain Error, so the only data it carries is an unstructured message — the rest of this module models failures as Schema.TaggedErrorClass with structured attributes and a message derived from them (PrimaryEnvironmentRequestError, PrimaryEnvironmentPairingCredentialRequiredError, ...). Consider adding a tagged error, e.g. PrimaryEnvironmentPairingLinkInactiveError with a pairingLinkId: Schema.String field, a message getter returning the existing user-visible text ("That pairing link is no longer active." so the UI string is preserved), and an exported isPrimaryEnvironmentPairingLinkInactiveError = Schema.is(...) predicate, then throw that here.

Posted via Macroscope — Effect Service Conventions

Comment on lines +501 to +502
if (!result.revoked) {
throw new Error("That client session is no longer active.");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same as the pairing-link path: a plain Error carries no structured context. Consider a Schema.TaggedErrorClass such as PrimaryEnvironmentClientSessionInactiveError with a sessionId: Schema.String attribute and a message getter returning the current text ("That client session is no longer active."), plus an exported Schema.is predicate, so callers can discriminate this no-op case instead of matching on a message string.

Posted via Macroscope — Effect Service Conventions

@macroscopeapp

macroscopeapp Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved c77a7fd

This is a self-contained UI bug fix adding optimistic row hiding when revoking authorized clients, with proper error handling and comprehensive unit tests. Changes are limited to frontend settings components with no runtime behavior impact beyond the intended fix.

You can customize Macroscope's approvability policy. Learn more.

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

Labels

size:L 100-499 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant