Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/app/analyzer/drive-panel/drive-panel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ export class DrivePanel {
protected disconnect(): void {
this.auth.signOut();
this.account.logout();
this.drive.resetError();
}

protected rescan(): void {
Expand Down
29 changes: 22 additions & 7 deletions src/app/core/auth.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -98,16 +98,31 @@ export class AuthService {
}

return new Promise<void>((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<boolean> {
if (!this.tokenClient) return Promise.resolve(false);
return new Promise<boolean>((resolve) => {
this.pendingSignInResolvers.push(resolve);
this.tokenClient!.requestAccessToken({ prompt: '' });
});
}

signOut(): void {
if (window.gapi?.client) {
window.gapi.client.setToken(null);
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -244,12 +259,12 @@ export class AuthService {

private async onTokenReceived(): Promise<void> {
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));
}
}
40 changes: 38 additions & 2 deletions src/app/core/drive.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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;
}

Expand Down Expand Up @@ -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({
Expand All @@ -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<typeof vi.fn>).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', () => {
Expand Down
120 changes: 79 additions & 41 deletions src/app/core/drive.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
});

Expand Down Expand Up @@ -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([]);
Expand Down Expand Up @@ -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<void> {
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<void> {
const currentToken = ++this.activeLoadToken;
this.appState.loading.set(true);
Expand Down Expand Up @@ -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<string | null> {
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<void> {
Expand Down Expand Up @@ -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;
Expand All @@ -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}`);
}
}
3 changes: 3 additions & 0 deletions src/app/top-nav/top-nav.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}
Expand Down
Loading