Conversation
Contributor
📝 WalkthroughWalkthroughThis PR refactors token handling to introduce automatic token refresh, timestamp tracking, and guaranteed-valid token provision through a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as API Client 1 & 2
participant ensureValidToken as ensureValidToken()
participant Storage as Storage
participant TokenRefresh as hsTokenRenew()
Client->>ensureValidToken: ensureValidToken(username)
ensureValidToken->>Storage: Get user from storage
Storage-->>ensureValidToken: User object
ensureValidToken->>ensureValidToken: isTokenExpired(user)?
alt Token Valid
ensureValidToken-->>Client: Return current token
else Token Expired
alt Refresh Already Pending
ensureValidToken->>ensureValidToken: Get pending promise
ensureValidToken-->>ensureValidToken: Wait for promise
else Start New Refresh
ensureValidToken->>TokenRefresh: Refresh with refreshToken
TokenRefresh-->>Storage: Updated user (new token + timestamp)
Storage-->>ensureValidToken: Store complete
end
ensureValidToken-->>Client: Return refreshed token
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/app/publish/_api/use-save-draft.ts (1)
82-100:⚠️ Potential issue | 🟠 MajorAdd a guard to validate the token before passing it to SDK calls.
ensureValidTokenreturnsPromise<string | undefined>— it can be undefined whengetUser(username)finds no matching entry in storage. While theactiveUser?.usernamecheck at line 51 guards against missing React-state user, storage can become inconsistent (e.g., cleared mid-session). Currently the token is passed directly toupdateDraftandaddDraftwithout validation, causing the backend to return a confusing auth error instead of failing early with a clear message.Add this guard after line 82:
const token = await ensureValidToken(username); if (!token) { throw new Error("[Draft] Failed to obtain a valid token"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/publish/_api/use-save-draft.ts` around lines 82 - 100, ensureValidToken returns string | undefined but its result is passed directly into updateDraft and addDraft; add a guard after the ensureValidToken call that checks the token from ensureValidToken(username) and throws a clear Error (e.g. "[Draft] Failed to obtain a valid token") when falsy so updateDraft and addDraft are never called with an undefined token; update the block around ensureValidToken, updateDraft, and addDraft in use-save-draft.ts accordingly.
🧹 Nitpick comments (3)
apps/web/src/features/chat/mattermost-api.ts (1)
81-97: Duplicate token refresh logic — potential race withensureValidToken.This block calls
hsTokenRenewdirectly instead of using the newensureValidTokenutility. If another part of the app triggersensureValidToken(orrefreshTokenInBackground) for the same user concurrently, two independent refresh requests hit the auth server with the same refresh token. Depending on the server's token rotation policy, one refresh could invalidate the other's newly issued token.Consider replacing the inline refresh with
ensureValidTokenand retrying bootstrap with the returned token:♻️ Suggested refactor
- try { - console.log("Chat token expired, attempting auto-refresh..."); - - // Call HiveSigner to refresh tokens - const refreshedTokens = await hsTokenRenew(refreshToken); - - // Update tokens in global store (saves to localStorage) - const currentUser = users.find((u) => u.username === username); - addUser({ - username: refreshedTokens.username, - accessToken: refreshedTokens.access_token, - refreshToken: refreshedTokens.refresh_token, - expiresIn: refreshedTokens.expires_in, - tokenObtainedAt: Date.now(), - postingKey: currentUser?.postingKey, - loginType: currentUser?.loginType - }); - - console.log("✓ Chat tokens refreshed successfully"); - - // Retry bootstrap with refreshed tokens - res = await fetch("/api/mattermost/bootstrap", { + try { + console.log("Chat token expired, attempting auto-refresh..."); + const freshAccessToken = await ensureValidToken(username || ""); + const freshRefreshToken = getRefreshToken(username || ""); + console.log("✓ Chat tokens refreshed successfully"); + + res = await fetch("/api/mattermost/bootstrap", { method: "POST", headers: { "Content-Type": "application/json" }, body: JSON.stringify({ username, - accessToken: refreshedTokens.access_token, - refreshToken: refreshedTokens.refresh_token, + accessToken: freshAccessToken, + refreshToken: freshRefreshToken, displayName: username, community }) });This would require importing
ensureValidTokenfrom@/utils.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/chat/mattermost-api.ts` around lines 81 - 97, Replace the direct hsTokenRenew call and manual token handling with the shared token helper to avoid concurrent refresh races: import and call ensureValidToken for the given username (instead of hsTokenRenew), use the returned token object to call addUser (preserving postingKey/loginType from currentUser) and then retry the bootstrap logic with the new access token; remove the direct hsTokenRenew invocation so refreshTokenInBackground/ensureValidToken manage rotation consistently.apps/web/src/features/shared/login/hooks/use-user-select.ts (1)
39-40: Consider using the returned token or verifying the refresh succeeded.
ensureValidTokenreturns the (possibly refreshed) access token, but its return value is discarded here. If the refresh fails,ensureValidTokensilently returns the stale token and the user switch proceeds anyway. If the intent is best-effort refresh this is fine, but if a valid token is required before switching, you should check the result or at least log a warning when the token couldn't be refreshed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/shared/login/hooks/use-user-select.ts` around lines 39 - 40, The call site in use-user-select (the hook that invokes ensureValidToken(user.username)) currently ignores the returned token; update the logic to capture the returned value from ensureValidToken and verify it succeeded before proceeding with the user switch: call const token = await ensureValidToken(user.username), then if token is null/undefined or otherwise indicates an expired/invalid value, either abort the switch (return/throw) or emit a warning via the same logger used in this module; if best-effort is intended, at minimum log a warning when the returned token is stale/unchanged so callers can diagnose refresh failures.apps/web/src/utils/user-token.ts (1)
38-45: Legacy sessions will trigger a refresh on everygetAccessTokencall until a refresh succeeds.When
tokenObtainedAtisundefined(legacy sessions),isTokenExpiredalways returnstrue. This means everygetAccessTokencall triggersrefreshTokenInBackground. The dedup guard inrefreshTokenInBackgroundprevents concurrent requests, but once the promise resolves and is deleted frompendingRefreshes, the nextgetAccessTokenwill trigger another refresh — because the in-memory state was never updated (only localStorage was). This will keep happening until the component re-reads from localStorage.This is a minor concern since
refreshTokenInBackgroundupdates localStorage withtokenObtainedAt, so subsequent calls togetUser(which reads from localStorage) will find the updated timestamp. Just ensure this is the intended migration path for legacy sessions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/utils/user-token.ts` around lines 38 - 45, isTokenExpired currently returns true for legacy sessions when tokenObtainedAt is undefined, causing every getAccessToken call to re-trigger refreshTokenInBackground until localStorage is re-read; fix by updating the in-memory user state when a background refresh succeeds so subsequent getAccessToken/isTokenExpired checks see the new tokenObtainedAt. Concretely, inside refreshTokenInBackground (the function that writes tokenObtainedAt to localStorage and manages pendingRefreshes), after a successful refresh update the in-memory cached user object returned by getUser (or the module-level user cache) to include the new tokenObtainedAt/expiresIn and persist that change so isTokenExpired no longer treats the session as legacy; keep the existing dedupe via pendingRefreshes intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/app/publish/_api/use-schedule.ts`:
- Around line 137-139: ensureValidToken(author) may return undefined, so add a
guard in use-schedule before calling addSchedule: check the token variable
returned from ensureValidToken and if it's undefined, short-circuit with a clear
authentication error (throw or return a structured error/response) instead of
calling addSchedule; mirror the guard/handling used in use-save-draft.ts to
ensure downstream API calls never receive an undefined token and to surface a
proper auth failure.
In `@apps/web/src/app/submit/_api/save-draft.ts`:
- Around line 79-83: After calling ensureValidToken(username) in save-draft.ts
you must guard against it returning undefined before passing the token into
updateDraft or addDraft: check the token variable (from ensureValidToken) and if
undefined exit early (return an error/throw or respond with appropriate status)
rather than calling updateDraft(token, ...) or addDraft(token, ...); update the
logic around the editingDraft branch and the addDraft path to use this guard so
neither updateDraft nor addDraft receives an undefined token.
In `@apps/web/src/app/submit/_api/schedule.ts`:
- Around line 104-106: ensureValidToken(author) can return undefined, so add a
guard before calling addSchedule to avoid passing an undefined token; in the
function invoking ensureValidToken (where addSchedule is called), check the
returned token from ensureValidToken(author) and if it's undefined either throw
a controlled error or return early (e.g., surface an authentication/validation
error to the caller) rather than calling addSchedule(token, ...). Update any
related error logging or user-facing messages so they reflect the auth-missing
case.
In `@apps/web/src/utils/user-token.ts`:
- Around line 118-162: The function ensureValidToken currently catches
hsTokenRenew errors and returns the stale user.accessToken, which hides
failures; change the catch block inside the refreshPromise to return undefined
(or re-throw) instead of the expired token so callers can detect a refresh
failure — specifically modify the async IIFE in ensureValidToken (the block that
calls hsTokenRenew and builds updatedUser, and which currently references
hsTokenRenew, updateUserInStorage, pendingRefreshes, and user) to: on error log
the error, return undefined (or re-throw the error if you prefer
caller-handling), and still delete pendingRefreshes in the finally so concurrent
refresh deduping cleans up correctly. Ensure the promise stored in
pendingRefreshes reflects the new return type (Promise<string | undefined>).
- Around line 86-96: getAccessToken currently returns a possibly stale token and
triggers refreshTokenInBackground without waiting, so update critical call sites
to obtain a guaranteed-valid token by using ensureValidToken() (or awaiting a
new async wrapper that awaits refreshTokenInBackground) instead of
getAccessToken; specifically replace usages in setup-external-create.tsx (around
line ~100), setup-external-import.tsx (around ~133), hive-signer.ts (around ~29)
and any direct API calls for chat/image
uploads/publish/schedule/save-draft/user-select to call ensureValidToken (or
convert the calling flow to async and await token refresh) while leaving
non-critical query-based usages unchanged.
- Around line 47-79: The issue is that updateUserInStorage only writes refreshed
tokens to localStorage, leaving the in-memory Zustand users array stale; to fix,
after any successful refresh in refreshTokenInBackground and ensureValidToken
replace the call to updateUserInStorage (or follow it) with an invocation of the
Zustand action addUser (or otherwise dispatch an event) so the updatedUser
object (the same structure created in refreshTokenInBackground and returned from
ensureValidToken) is added to the store; if addUser isn't in scope, accept a
store action or callback into these functions (or emit a lightweight update
event) and call that with the updatedUser so both localStorage and the
useGlobalStore/state.users are kept in sync (mirroring the mattermost-api.ts
pattern).
---
Outside diff comments:
In `@apps/web/src/app/publish/_api/use-save-draft.ts`:
- Around line 82-100: ensureValidToken returns string | undefined but its result
is passed directly into updateDraft and addDraft; add a guard after the
ensureValidToken call that checks the token from ensureValidToken(username) and
throws a clear Error (e.g. "[Draft] Failed to obtain a valid token") when falsy
so updateDraft and addDraft are never called with an undefined token; update the
block around ensureValidToken, updateDraft, and addDraft in use-save-draft.ts
accordingly.
---
Nitpick comments:
In `@apps/web/src/features/chat/mattermost-api.ts`:
- Around line 81-97: Replace the direct hsTokenRenew call and manual token
handling with the shared token helper to avoid concurrent refresh races: import
and call ensureValidToken for the given username (instead of hsTokenRenew), use
the returned token object to call addUser (preserving postingKey/loginType from
currentUser) and then retry the bootstrap logic with the new access token;
remove the direct hsTokenRenew invocation so
refreshTokenInBackground/ensureValidToken manage rotation consistently.
In `@apps/web/src/features/shared/login/hooks/use-user-select.ts`:
- Around line 39-40: The call site in use-user-select (the hook that invokes
ensureValidToken(user.username)) currently ignores the returned token; update
the logic to capture the returned value from ensureValidToken and verify it
succeeded before proceeding with the user switch: call const token = await
ensureValidToken(user.username), then if token is null/undefined or otherwise
indicates an expired/invalid value, either abort the switch (return/throw) or
emit a warning via the same logger used in this module; if best-effort is
intended, at minimum log a warning when the returned token is stale/unchanged so
callers can diagnose refresh failures.
In `@apps/web/src/utils/user-token.ts`:
- Around line 38-45: isTokenExpired currently returns true for legacy sessions
when tokenObtainedAt is undefined, causing every getAccessToken call to
re-trigger refreshTokenInBackground until localStorage is re-read; fix by
updating the in-memory user state when a background refresh succeeds so
subsequent getAccessToken/isTokenExpired checks see the new tokenObtainedAt.
Concretely, inside refreshTokenInBackground (the function that writes
tokenObtainedAt to localStorage and manages pendingRefreshes), after a
successful refresh update the in-memory cached user object returned by getUser
(or the module-level user cache) to include the new tokenObtainedAt/expiresIn
and persist that change so isTokenExpired no longer treats the session as
legacy; keep the existing dedupe via pendingRefreshes intact.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
New Features