From dca56507e91a2f49565708cda331368b3a910d72 Mon Sep 17 00:00:00 2001 From: Tomek Zebrowski Date: Sat, 15 Aug 2026 22:15:26 +0200 Subject: [PATCH] fix: recover from expired Drive sessions instead of silently going stale DriveService.findFolderId() swallowed every API error, including 401/403, and reported an expired token as "Required Drive folders not found" -- so AuthService.signOut() never fired, isLoggedIn stayed true, and the only way to refresh was a manual logout/login. Real errors now propagate to listFiles(), which first tries AuthService.silentRefresh() (a popup-free token refresh) and retries once before giving up, so routine hourly token expiry recovers invisibly. Only if that fails does it sign out of both AuthService (Drive) and AccountService (My Giulia account/top-nav) together, so the UI doesn't keep showing the user as signed in with an empty file list. Co-Authored-By: Claude Sonnet 5 --- src/app/analyzer/drive-panel/drive-panel.ts | 1 + src/app/core/auth.service.ts | 29 +++-- src/app/core/drive.service.spec.ts | 40 ++++++- src/app/core/drive.service.ts | 120 +++++++++++++------- src/app/top-nav/top-nav.ts | 3 + 5 files changed, 143 insertions(+), 50 deletions(-) diff --git a/src/app/analyzer/drive-panel/drive-panel.ts b/src/app/analyzer/drive-panel/drive-panel.ts index 02cbdfd..f9cd95e 100644 --- a/src/app/analyzer/drive-panel/drive-panel.ts +++ b/src/app/analyzer/drive-panel/drive-panel.ts @@ -30,6 +30,7 @@ export class DrivePanel { protected disconnect(): void { this.auth.signOut(); this.account.logout(); + this.drive.resetError(); } protected rescan(): void { diff --git a/src/app/core/auth.service.ts b/src/app/core/auth.service.ts index a363671..f63602e 100644 --- a/src/app/core/auth.service.ts +++ b/src/app/core/auth.service.ts @@ -37,7 +37,7 @@ export class AuthService { readonly gisInited = signal(false); private tokenClient: GoogleTokenClient | null = null; - private pendingSignInResolvers: Array<() => void> = []; + private pendingSignInResolvers: Array<(success: boolean) => void> = []; constructor( private readonly preferences: PreferencesService, @@ -98,16 +98,31 @@ export class AuthService { } return new Promise((resolve) => { - this.pendingSignInResolvers.push(resolve); + this.pendingSignInResolvers.push(() => resolve()); if (this.tokenClient) { this.tokenClient.requestAccessToken({ prompt: '' }); } else { console.error('Token client not ready.'); - this.resolvePendingSignIns(); + this.resolvePendingSignIns(false); } }); } + /** + * Requests a fresh Drive access token without a popup, relying on the browser's existing + * Google session. GIS access tokens expire hourly even while the user's underlying Google + * session is still valid, so DriveService calls this to recover from an expired token before + * falling back to a full sign-out -- avoids logging the user out of the whole My Giulia account + * (see AccountService) on every routine hourly expiry. + */ + silentRefresh(): Promise { + if (!this.tokenClient) return Promise.resolve(false); + return new Promise((resolve) => { + this.pendingSignInResolvers.push(resolve); + this.tokenClient!.requestAccessToken({ prompt: '' }); + }); + } + signOut(): void { if (window.gapi?.client) { window.gapi.client.setToken(null); @@ -211,7 +226,7 @@ export class AuthService { callback: (resp) => { if (resp.error !== undefined) { console.error('Auth Error:', resp.error); - this.resolvePendingSignIns(); + this.resolvePendingSignIns(false); return; } @@ -244,12 +259,12 @@ export class AuthService { private async onTokenReceived(): Promise { await this.fetchUserDetails(); - this.resolvePendingSignIns(); + this.resolvePendingSignIns(this.isLoggedIn()); } - private resolvePendingSignIns(): void { + private resolvePendingSignIns(success: boolean): void { const resolvers = this.pendingSignInResolvers; this.pendingSignInResolvers = []; - resolvers.forEach((resolve) => resolve()); + resolvers.forEach((resolve) => resolve(success)); } } diff --git a/src/app/core/drive.service.spec.ts b/src/app/core/drive.service.spec.ts index bd08dd8..ce4d587 100644 --- a/src/app/core/drive.service.spec.ts +++ b/src/app/core/drive.service.spec.ts @@ -15,6 +15,7 @@ function makeAuthFake(isLoggedInInitial = true) { user: vi.fn().mockReturnValue(null), signIn: vi.fn().mockResolvedValue(undefined), signOut: vi.fn(), + silentRefresh: vi.fn().mockResolvedValue(false), getAccessToken: vi.fn().mockReturnValue('token-abc'), } as unknown as AuthService; } @@ -23,6 +24,7 @@ function makeAccountFake(hasDriveFeature = true) { return { hasFeature: vi.fn().mockReturnValue(hasDriveFeature), loginWithGoogle: vi.fn().mockResolvedValue({ ok: true }), + logout: vi.fn(), } as unknown as AccountService; } @@ -180,7 +182,7 @@ describe('DriveService', () => { expect(drive.error()).toContain('Required Drive folders not found'); }); - it('listFiles() signs out and reports a session-expired error on 401', async () => { + it('listFiles() signs out of Drive and the account, and reports a session-expired error on 401 when silent refresh fails', async () => { const list = vi .fn() .mockResolvedValueOnce({ @@ -196,9 +198,43 @@ describe('DriveService', () => { const drive = create(); await drive.listFiles(); + expect(auth.silentRefresh).toHaveBeenCalled(); expect(setToken).toHaveBeenCalledWith(null); expect(auth.signOut).toHaveBeenCalled(); - expect(drive.error()).toContain('Session expired'); + expect(account.logout).toHaveBeenCalled(); + expect(drive.error()).toContain('session expired'); + }); + + it('listFiles() recovers from a 401 via silent refresh without signing out', async () => { + const list = vi + .fn() + .mockResolvedValueOnce({ + result: { files: [{ id: 'root-1', name: 'mygiulia' }] }, + }) + .mockResolvedValueOnce({ + result: { files: [{ id: 'sub-1', name: 'trips' }] }, + }) + .mockRejectedValueOnce({ status: 401, message: 'nope' }) + .mockResolvedValueOnce({ + result: { files: [{ id: 'root-1', name: 'mygiulia' }] }, + }) + .mockResolvedValueOnce({ + result: { files: [{ id: 'sub-1', name: 'trips' }] }, + }) + .mockResolvedValueOnce({ + result: { files: [{ id: 'f1', name: 'trip-1700000000000-120.json' }] }, + }); + vi.stubGlobal('gapi', { client: { drive: { files: { list } } } }); + (auth.silentRefresh as ReturnType).mockResolvedValue(true); + + const drive = create(); + await drive.listFiles(); + + expect(auth.silentRefresh).toHaveBeenCalled(); + expect(auth.signOut).not.toHaveBeenCalled(); + expect(account.logout).not.toHaveBeenCalled(); + expect(drive.files()).toHaveLength(1); + expect(drive.error()).toBeNull(); }); it('filteredSortedFiles() filters by name and sorts by timestamp', () => { diff --git a/src/app/core/drive.service.ts b/src/app/core/drive.service.ts index 7f987c5..fd84080 100644 --- a/src/app/core/drive.service.ts +++ b/src/app/core/drive.service.ts @@ -174,10 +174,16 @@ export class DriveService { private readonly dataProcessor: DataProcessorService, private readonly bus: EventBusService ) { + /** + * Only clears `files` here, not `error` -- error clearing is left to the specific signOut + * call sites (DrivePanel's manual "Sign out", TopNav's account-menu sign-out, both via + * `resetError()`) so it doesn't race handleApiError(), which sets a "session expired" + * message in the same tick it calls auth.signOut(); this effect flushes asynchronously and + * would otherwise wipe that message right back out. + */ effect(() => { if (!this.auth.isLoggedIn()) { this.files.set([]); - this.error.set(null); } }); @@ -309,6 +315,11 @@ export class DriveService { } } + /** Clears a stale error banner on a deliberate manual sign-out (DrivePanel, TopNav), as opposed to handleApiError()'s automatic one, which sets its own message. */ + resetError(): void { + this.error.set(null); + } + clearRecentHistory(): void { localStorage.removeItem(RECENT_KEY); this.recentIds.set([]); @@ -379,26 +390,43 @@ export class DriveService { this.files.set([]); try { - const rootId = await this.findFolderId(DRIVE_ROOT_FOLDER); - const subFolderId = rootId - ? await this.findFolderId(DRIVE_SUB_FOLDER, rootId) - : null; - - if (!subFolderId) { - this.error.set( - `Required Drive folders not found ("${DRIVE_ROOT_FOLDER}/${DRIVE_SUB_FOLDER}").` - ); - return; - } - - await this.fetchJsonFiles(subFolderId); + await this.scanTripsFolder(); } catch (error) { - this.handleApiError(error); + if (this.isAuthError(error) && (await this.auth.silentRefresh())) { + try { + await this.scanTripsFolder(); + } catch (retryError) { + this.handleApiError(retryError); + } + } else { + this.handleApiError(error); + } } finally { this.loading.set(false); } } + private async scanTripsFolder(): Promise { + const rootId = await this.findFolderId(DRIVE_ROOT_FOLDER); + const subFolderId = rootId + ? await this.findFolderId(DRIVE_SUB_FOLDER, rootId) + : null; + + if (!subFolderId) { + this.error.set( + `Required Drive folders not found ("${DRIVE_ROOT_FOLDER}/${DRIVE_SUB_FOLDER}").` + ); + return; + } + + await this.fetchJsonFiles(subFolderId); + } + + private isAuthError(error: unknown): boolean { + const status = (error as { status?: number } | undefined)?.status; + return status === 401 || status === 403; + } + async loadFile(fileName: string, id: string): Promise { const currentToken = ++this.activeLoadToken; this.appState.loading.set(true); @@ -471,31 +499,34 @@ export class DriveService { return `${h}h ${remainingM}m`; } + /** + * Deliberate deviation from legacy/src/drive.js's `findFolderId`: legacy swallows every + * error here (including 401/403) and returns null, which listFiles() then reports as + * "Required folders not found" -- masking an expired/invalid token as a data problem, so + * AuthService.signOut() never fires and the app is stuck logged-in with an empty file list + * until the user manually logs out and back in. Real API errors now propagate to + * listFiles()'s catch so expired-session handling (silent refresh, then sign-out) can run. + */ private async findFolderId( name: string, parentId = 'root' ): Promise { - try { - const variants = [ - name, - name.toLowerCase(), - name.charAt(0).toUpperCase() + name.slice(1), - ]; - const nameQuery = variants.map((v) => `name = '${v}'`).join(' or '); - const query = `mimeType='application/vnd.google-apps.folder' and (${nameQuery}) and '${parentId}' in parents and trashed=false`; - - const response = await window.gapi!.client.drive.files.list({ - q: query, - fields: 'files(id, name)', - pageSize: 1, - }); - return response.result.files.length > 0 - ? response.result.files[0].id - : null; - } catch (error) { - console.error(`Drive: Error locating folder "${name}":`, error); - return null; - } + const variants = [ + name, + name.toLowerCase(), + name.charAt(0).toUpperCase() + name.slice(1), + ]; + const nameQuery = variants.map((v) => `name = '${v}'`).join(' or '); + const query = `mimeType='application/vnd.google-apps.folder' and (${nameQuery}) and '${parentId}' in parents and trashed=false`; + + const response = await window.gapi!.client.drive.files.list({ + q: query, + fields: 'files(id, name)', + pageSize: 1, + }); + return response.result.files.length > 0 + ? response.result.files[0].id + : null; } private async fetchJsonFiles(folderId: string): Promise { @@ -543,6 +574,14 @@ export class DriveService { return match ? parseInt(match[1], 10) : 0; } + /** + * On 401/403, listFiles() has already tried AuthService.silentRefresh() and failed, so the + * Drive session is unrecoverable without user interaction. The whole app is Google-only (see + * AccountService), so there's no separate identity to preserve -- sign out of both AuthService + * (Drive) and AccountService (My Giulia account/top-nav) together, otherwise the top-nav keeps + * showing the user as signed in while their Drive files are gone with no way to recover short + * of a manual logout/login. + */ private handleApiError(error: unknown): void { const err = error as { status?: number; @@ -552,12 +591,11 @@ export class DriveService { if (err.status === 401 || err.status === 403) { window.gapi?.client?.setToken(null); this.auth.signOut(); + this.account.logout(); + this.error.set('Your Google session expired. Please sign in again.'); + return; } const msg = err.result?.error?.message || err.message || 'Unknown error'; - this.error.set( - err.status === 401 - ? 'Session expired. Please sign in again.' - : `Drive error: ${msg}` - ); + this.error.set(`Drive error: ${msg}`); } } diff --git a/src/app/top-nav/top-nav.ts b/src/app/top-nav/top-nav.ts index 318fd0a..a271174 100644 --- a/src/app/top-nav/top-nav.ts +++ b/src/app/top-nav/top-nav.ts @@ -4,6 +4,7 @@ import { AccountService } from '../core/account.service'; import { AppStateService } from '../core/app-state.service'; import { AuthService } from '../core/auth.service'; import { DataProcessorService } from '../core/data-processor.service'; +import { DriveService } from '../core/drive.service'; import { DynoService } from '../core/dyno.service'; import { EventBusService } from '../core/event-bus.service'; import { HistogramService } from '../core/histogram.service'; @@ -37,6 +38,7 @@ export class TopNav { protected readonly uiState = inject(UiStateService); protected readonly auth = inject(AuthService); protected readonly account = inject(AccountService); + private readonly drive = inject(DriveService); protected readonly appState = inject(AppStateService); protected readonly mathChannels = inject(MathChannelsService); protected readonly dyno = inject(DynoService); @@ -136,6 +138,7 @@ export class TopNav { protected signOut(): void { this.account.logout(); this.auth.signOut(); + this.drive.resetError(); this.profileOpen.set(false); this.accountMenuOpen.set(false); }