Preserve Clerk session between login window opens#642
Conversation
Agent-Logs-Url: https://github.com/highperformancecoder/minsky/sessions/fee97c92-7978-4c1d-8195-ace7a1267507 Co-authored-by: highperformancecoder <3075825+highperformancecoder@users.noreply.github.com>
| await this.electronService.invoke(events.SET_AUTH_TOKEN, token); | ||
| } | ||
|
|
||
| async setSession(_token: string): Promise<void> { |
There was a problem hiding this comment.
This method discards the passed token. As a result, it never reinstates the session.
There was a problem hiding this comment.
Fixed in 7d51337. The stored authToken is a short-lived JWT which Clerk doesn't accept directly for session restoration. Instead, setSession now calls clerk.setActive({ session: sessions[0].id }) when no session is currently active but clerk.client.sessions has available sessions (populated by clerk.load()). If neither path yields an active session, it throws so the login form is shown.
Agent-Logs-Url: https://github.com/highperformancecoder/minsky/sessions/198fce2f-6377-4ea0-a84d-1c4fe6545b1b Co-authored-by: highperformancecoder <3075825+highperformancecoder@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Preserves (or attempts to preserve) an existing Clerk authentication session when reopening the Electron login window, reducing unnecessary re-authentication prompts.
Changes:
- Adds session-restore logic to
ClerkServicevia a newsetSession(token)method. - Updates
LoginComponentto read anauthTokenquery param and attempt to restore the session on init. - Updates
WindowManager.openLoginWindow()to append the storedauthTokenas a URL query parameter when opening the login route.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| gui-js/libs/ui-components/src/lib/login/login.component.ts | Reads authToken from route query params and initializes/restores Clerk session on component init. |
| gui-js/libs/core/src/lib/services/clerk/clerk.service.ts | Adds a session-restore helper that attempts to activate an existing Clerk client session. |
| gui-js/apps/minsky-electron/src/app/managers/WindowManager.ts | Includes the stored auth token in the login popup URL to enable renderer-side session restoration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private destroy$ = new Subject<void>(); | ||
|
|
||
| constructor( | ||
| private clerkService: ClerkService, | ||
| private electronService: ElectronService, | ||
| private route: ActivatedRoute | ||
| ) {} |
There was a problem hiding this comment.
destroy$/OnDestroy are currently unused for subscription cleanup (the queryParams subscription uses take(1) and completes automatically). Either remove destroy$ + ngOnDestroy, or switch to takeUntil(this.destroy$) if you intend to keep a long-lived subscription.
| async ngOnInit() { | ||
| this.route.queryParams.pipe(take(1)).subscribe((params) => { | ||
| this.initializeSession(params['authToken']); | ||
| }); | ||
| } | ||
|
|
||
| private async initializeSession(authToken: string | undefined) { | ||
| try { | ||
| await this.clerkService.initialize(); | ||
|
|
||
| if (authToken) { | ||
| await this.clerkService.setSession(authToken); | ||
| } | ||
|
|
||
| this.isAuthenticated = await this.clerkService.isSignedIn(); |
There was a problem hiding this comment.
initializeSession() is async but is invoked from the queryParams subscription without awaiting and without setting an initialization loading state. This allows the user to submit the form before clerkService.initialize() completes, which can cause signInWithEmailPassword() to fail with “Clerk is not initialized”. Consider awaiting initialization in ngOnInit (e.g., via firstValueFrom/await) and/or disabling the form until initialization finishes, and/or calling clerkService.initialize() inside onSubmit() as a guard.
| } catch (err) { | ||
| this.errorMessage = 'Failed to initialize authentication.'; | ||
| this.errorMessage = 'Session expired. Please sign in again.'; | ||
| this.isAuthenticated = false; | ||
| } |
There was a problem hiding this comment.
The catch block always shows “Session expired. Please sign in again.” even when the failure is unrelated to session expiry (e.g., Clerk initialization/config/network errors). Consider using a more general message when initialize() fails, and only showing the “session expired” message when a session restore attempt actually fails.
| async setSession(token: string): Promise<void> { | ||
| if (!this.clerk) throw new Error('Clerk not initialized'); | ||
| // clerk.load() in initialize() restores the session from browser storage if still valid. | ||
| // If no session is active but sessions exist on the client, activate the first available one. | ||
| // The token parameter is a hint that the user previously authenticated. | ||
| if (!token) return; | ||
| if (!this.clerk.session && this.clerk.client?.sessions?.length > 0) { | ||
| await this.clerk.setActive({ session: this.clerk.client.sessions[0].id }); | ||
| } | ||
| if (!this.clerk.session) { | ||
| throw new Error('Session expired or invalid'); | ||
| } |
There was a problem hiding this comment.
setSession(token) does not actually use the provided token to reinstate a session; it only checks truthiness and then activates the first session from clerk.client.sessions. This is misleading for callers and makes it hard to reason about correctness/security. Consider renaming to reflect what it does (e.g., restoreSessionFromStorage()), or using the token meaningfully (or dropping the parameter entirely).
| async setSession(token: string): Promise<void> { | |
| if (!this.clerk) throw new Error('Clerk not initialized'); | |
| // clerk.load() in initialize() restores the session from browser storage if still valid. | |
| // If no session is active but sessions exist on the client, activate the first available one. | |
| // The token parameter is a hint that the user previously authenticated. | |
| if (!token) return; | |
| if (!this.clerk.session && this.clerk.client?.sessions?.length > 0) { | |
| await this.clerk.setActive({ session: this.clerk.client.sessions[0].id }); | |
| } | |
| if (!this.clerk.session) { | |
| throw new Error('Session expired or invalid'); | |
| } | |
| private getSessionIdFromToken(token: string): string | null { | |
| const parts = token.split('.'); | |
| if (parts.length < 2) return null; | |
| try { | |
| const payload = parts[1] | |
| .replace(/-/g, '+') | |
| .replace(/_/g, '/') | |
| .padEnd(Math.ceil(parts[1].length / 4) * 4, '='); | |
| const decoded = JSON.parse(atob(payload)); | |
| return typeof decoded?.sid === 'string' ? decoded.sid : null; | |
| } catch { | |
| return null; | |
| } | |
| } | |
| async setSession(token: string): Promise<void> { | |
| if (!this.clerk) throw new Error('Clerk not initialized'); | |
| if (!token) throw new Error('Session token is required'); | |
| const sessionId = this.getSessionIdFromToken(token); | |
| if (!sessionId) { | |
| throw new Error('Session token is invalid'); | |
| } | |
| // clerk.load() in initialize() may already have restored the matching session from browser storage. | |
| if (this.clerk.session?.id === sessionId) { | |
| return; | |
| } | |
| const matchingSession = this.clerk.client?.sessions?.find((session: { id: string }) => session.id === sessionId); | |
| if (!matchingSession) { | |
| throw new Error('Session expired or invalid'); | |
| } | |
| await this.clerk.setActive({ session: matchingSession.id }); | |
| if (this.clerk.session?.id !== sessionId) { | |
| throw new Error('Session expired or invalid'); | |
| } |
| async setSession(token: string): Promise<void> { | ||
| if (!this.clerk) throw new Error('Clerk not initialized'); | ||
| // clerk.load() in initialize() restores the session from browser storage if still valid. | ||
| // If no session is active but sessions exist on the client, activate the first available one. | ||
| // The token parameter is a hint that the user previously authenticated. | ||
| if (!token) return; | ||
| if (!this.clerk.session && this.clerk.client?.sessions?.length > 0) { | ||
| await this.clerk.setActive({ session: this.clerk.client.sessions[0].id }); |
There was a problem hiding this comment.
Activating this.clerk.client.sessions[0] is an arbitrary choice if multiple sessions exist; it may activate a different session than the one the user expects. If multiple sessions are possible here, prefer selecting the most recently active session or otherwise deterministically choosing the intended session (and document the selection criteria).
| async setSession(token: string): Promise<void> { | |
| if (!this.clerk) throw new Error('Clerk not initialized'); | |
| // clerk.load() in initialize() restores the session from browser storage if still valid. | |
| // If no session is active but sessions exist on the client, activate the first available one. | |
| // The token parameter is a hint that the user previously authenticated. | |
| if (!token) return; | |
| if (!this.clerk.session && this.clerk.client?.sessions?.length > 0) { | |
| await this.clerk.setActive({ session: this.clerk.client.sessions[0].id }); | |
| private getSessionRecencyValue(session: any): number { | |
| const recencyCandidate = session?.lastActiveAt ?? session?.updatedAt ?? session?.createdAt; | |
| if (typeof recencyCandidate === 'number') { | |
| return recencyCandidate; | |
| } | |
| if (recencyCandidate instanceof Date) { | |
| return recencyCandidate.getTime(); | |
| } | |
| if (typeof recencyCandidate === 'string') { | |
| const parsedValue = Date.parse(recencyCandidate); | |
| return Number.isNaN(parsedValue) ? Number.NEGATIVE_INFINITY : parsedValue; | |
| } | |
| return Number.NEGATIVE_INFINITY; | |
| } | |
| private selectSessionToActivate(): any | null { | |
| const sessions = this.clerk?.client?.sessions; | |
| if (!Array.isArray(sessions) || sessions.length === 0) return null; | |
| return sessions.reduce((selectedSession: any, currentSession: any) => { | |
| if (!selectedSession) return currentSession; | |
| const selectedRecency = this.getSessionRecencyValue(selectedSession); | |
| const currentRecency = this.getSessionRecencyValue(currentSession); | |
| if (currentRecency > selectedRecency) return currentSession; | |
| if (currentRecency < selectedRecency) return selectedSession; | |
| const selectedId = String(selectedSession?.id ?? ''); | |
| const currentId = String(currentSession?.id ?? ''); | |
| return currentId.localeCompare(selectedId) < 0 ? currentSession : selectedSession; | |
| }, null); | |
| } | |
| async setSession(token: string): Promise<void> { | |
| if (!this.clerk) throw new Error('Clerk not initialized'); | |
| // clerk.load() in initialize() restores the session from browser storage if still valid. | |
| // If no session is active but sessions exist on the client, activate one deterministically. | |
| // Prefer the most recently active session (`lastActiveAt`), then `updatedAt`, then `createdAt`; | |
| // if timestamps are unavailable, fall back to the lexicographically smallest session id. | |
| // The token parameter is a hint that the user previously authenticated. | |
| if (!token) return; | |
| if (!this.clerk.session && this.clerk.client?.sessions?.length > 0) { | |
| const sessionToActivate = this.selectSessionToActivate(); | |
| if (sessionToActivate?.id) { | |
| await this.clerk.setActive({ session: sessionToActivate.id }); | |
| } |
| async setSession(token: string): Promise<void> { | ||
| if (!this.clerk) throw new Error('Clerk not initialized'); | ||
| // clerk.load() in initialize() restores the session from browser storage if still valid. | ||
| // If no session is active but sessions exist on the client, activate the first available one. | ||
| // The token parameter is a hint that the user previously authenticated. | ||
| if (!token) return; | ||
| if (!this.clerk.session && this.clerk.client?.sessions?.length > 0) { | ||
| await this.clerk.setActive({ session: this.clerk.client.sessions[0].id }); | ||
| } | ||
| if (!this.clerk.session) { | ||
| throw new Error('Session expired or invalid'); | ||
| } |
There was a problem hiding this comment.
Throwing Session expired or invalid when no session becomes active after attempting restore forces the caller to treat “no restorable session” as an exceptional condition. For this workflow it’s usually better to return a boolean (restored vs not) and let the UI fall back to the login form without showing an error unless the user action fails.
| async setSession(token: string): Promise<void> { | |
| if (!this.clerk) throw new Error('Clerk not initialized'); | |
| // clerk.load() in initialize() restores the session from browser storage if still valid. | |
| // If no session is active but sessions exist on the client, activate the first available one. | |
| // The token parameter is a hint that the user previously authenticated. | |
| if (!token) return; | |
| if (!this.clerk.session && this.clerk.client?.sessions?.length > 0) { | |
| await this.clerk.setActive({ session: this.clerk.client.sessions[0].id }); | |
| } | |
| if (!this.clerk.session) { | |
| throw new Error('Session expired or invalid'); | |
| } | |
| async setSession(token: string): Promise<boolean> { | |
| if (!this.clerk) throw new Error('Clerk not initialized'); | |
| // clerk.load() in initialize() restores the session from browser storage if still valid. | |
| // If no session is active but sessions exist on the client, activate the first available one. | |
| // The token parameter is a hint that the user previously authenticated. | |
| if (!token) return false; | |
| if (!this.clerk.session && this.clerk.client?.sessions?.length > 0) { | |
| await this.clerk.setActive({ session: this.clerk.client.sessions[0].id }); | |
| } | |
| return !!this.clerk.session; |
| const existingToken = StoreManager.store.get('authToken') || ''; | ||
|
|
||
| const loginWindow = WindowManager.createPopupWindowWithRouting({ | ||
| width: 420, | ||
| height: 500, | ||
| title: 'Login', | ||
| modal: false, | ||
| url: '#/headless/login', | ||
| url: `#/headless/login?authToken=${encodeURIComponent(existingToken)}`, | ||
| }); |
There was a problem hiding this comment.
Passing the (potentially long-lived) authToken in the login window URL query string risks accidental disclosure via logs, crash reports, screenshots, devtools, or navigation history. Prefer keeping tokens out of URLs: fetch the stored token from the main process via IPC after the window loads, or rely on Clerk’s own storage restoration without transmitting the token to the renderer.
setSession(token)method toClerkService- now actually reinstates the session by callingclerk.setActive()with the first available session fromclerk.client.sessionsif no session is currently activeLoginComponentto acceptauthTokenquery param viaActivatedRoute, usetake(1), delegate to privateinitializeSession()method, addngOnDestroywithdestroy$SubjectWindowManager.openLoginWindow()to retrieve storedauthTokenfromStoreManagerand pass it as URL query parameter to the login componentOriginal prompt
Preserve Clerk Session Between Login Window Opens
Problem
Currently, when "Manage Clerk session" is selected, it asks for the user to log in again, even if previously logged in. The Clerk session is not preserved between calls to open the login window.
Solution
Modify the existing
LoginComponentto accept anauthTokenquery parameter and use it to restore the Clerk session. Add aWindowManager.openLoginWindow()method that retrieves the stored token and passes it to the login component.Changes Required
1. Update
gui-js/libs/ui-components/src/lib/login/login.component.tsModify the component to:
authTokenas a query parameter viaActivatedRoutengOnInit()Changes needed:
Add a
destroy$Subject for cleanup:2. Add
openLoginWindow()togui-js/apps/minsky-electron/src/app/managers/WindowManager.tsAdd a static method that:
authTokenfromStoreManager3. Update menu item handler in
gui-js/apps/minsky-electron/src/app/managers/ApplicationMenuManager.tsFind the "Manage Clerk session" or similar menu item and update it to use
WindowManager.openLoginWindow():Testing
After these changes:
Notes
SET_AUTH_TOKENalready handles storing/clearing tokens and resolving the PromiseStoreManagervia the IPC handlerAuthorComponentandLockHandlesComponentwhich accept query parametersThe following is the prior conversation context from the user's chat exploration (may be truncated):
User: Currently, when "Manage Clerk session" is selected, it asks for the user to log in again, even if previously logged in, ie the Clerk session is not preserved between calls to WindowManager.openLoginWindow.
Assistant: I'll help you investigate why the Clerk session is not being preserved between calls to
WindowManager.openLoginWindow. Let me start by examining the current implementation.Assistant: Let me search more specifically for Clerk-related code in the PR.
Assistant: Based on my investigation of PR #635 and the codebase, I can see that the issue is that the Clerk session is not being preserved between calls to
WindowManager.openLoginWindow. While I couldn't find the exactopenLoginWindowmethod in the current codebase (it may have been added in PR #635), the problem is likely that:This pull request was created from Copilot chat.
This change is