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); }