fix(web): authorized-client revoke no longer silently leaves the row - #6282
fix(web): authorized-client revoke no longer silently leaves the row#6282RioPlay wants to merge 1 commit into
Conversation
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.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
There was a problem hiding this comment.
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
| if (!result.revoked) { | ||
| throw new Error("That pairing link is no longer active."); |
There was a problem hiding this comment.
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
| if (!result.revoked) { | ||
| throw new Error("That client session is no longer active."); |
There was a problem hiding this comment.
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
ApprovabilityVerdict: 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. |
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:
POST /api/auth/clients/revokecan return 200 with{ revoked: false }when nothing matched. The client ignored that flag and treated any non-throwing response as success.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
revoked: falseas failure for client-session and pairing-link revoke (clear error toast).revokedCount > 0.Verification
vp test run apps/web/src/environments/primary/auth.revoke.test.ts apps/web/src/components/settings/ConnectionsSettings.logic.test.ts— passNote 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: falseon 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…”.
revokeServerClientSessionandrevokeServerPairingLinknow treat HTTP 200 withrevoked: falseas 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
revokeServerClientSessionandrevokeServerPairingLinkinauth.tsnow throw when the server returnsrevoked: false, preventing a silent no-op from being treated as success.withHiddenAccessId/excludeHiddenAccessRowshelpers.useEffectinConnectionsSettings.tsxreconciles the hidden-id sets against live snapshots so stale entries are cleaned up when the server data refreshes.Macroscope summarized c77a7fd.