Skip to content

fix(settings): reset to General on bare /settings so browser Back can't strand a stale tab #415

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ruzin merged 1 commit into from
Jul 24, 2026
Merged

fix(settings): reset to General on bare /settings so browser Back can't strand a stale tab #415

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
33 changes: 22 additions & 11 deletions app/renderer/src/routes/Settings.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,17 @@
return id === 'transcription' ? 'ai' : id;
}

// Resolve the `?tab=` param of a route to a nav tab, or null when absent or
// not a known deep-link id. Single definition shared by the first-mount
// initialTab and the route-reactive sync effect so the two can't drift.
function tabFromRoute(route: string): SettingsTabId | null {
const requested = getRouteParam(route, 'tab');
if (requested && (DEEP_LINK_IDS as readonly string[]).includes(requested)) {
return resolveTab(requested as DeepLinkId);
}
return null;
}

// ---------------------------------------------------------------------------
// Settings page — a full takeover of the main app chrome (Granola/Wispr
// style): the folder/meeting sidebar is replaced entirely by SettingsNav
Expand All @@ -76,23 +87,23 @@
// Used by the sidebar's "Sign in to organisation" CTA to land users
// directly on the org sign-in form rather than the General tab.
const route = useRoute();
const initialTab = React.useMemo<SettingsTabId>(() => {
const requested = getRouteParam(route, 'tab');
if (requested && (DEEP_LINK_IDS as readonly string[]).includes(requested)) {
return resolveTab(requested as DeepLinkId);
}
return 'general';
}, []); // Intentional — only consume the URL param on first mount.
const initialTab = React.useMemo<SettingsTabId>(
() => tabFromRoute(route) ?? 'general',
[], // Intentional — only consume the URL param on first mount.

Check warning on line 92 in app/renderer/src/routes/Settings.tsx

View workflow job for this annotation

GitHub Actions / Lint (renderer) 

React Hook React.useMemo has a missing dependency: 'route'. Either include it or remove the dependency array
);
const [tab, setTab] = React.useState<SettingsTabId>(initialTab);
// Keep the visible tab in sync when the `?tab=` param changes AFTER mount —
// e.g. the ⌘K settings search navigates to /settings?tab=<id> while Settings
// is already open. initialTab (above) only consumes the param on first mount,
// so without this the hash would update but the tab wouldn't switch.
//
// A bare `/settings` (no param) resets to General — the same meaning the
// absent param has on first mount. This matters for browser Back: nav-rail
// clicks push `?tab=` entries onto the hash history (route is the single
// source of truth since #411), so Back can land on the bare route and must
// not leave a stale tab visible.
React.useEffect(() => {
const requested = getRouteParam(route, 'tab');
if (requested && (DEEP_LINK_IDS as readonly string[]).includes(requested)) {
setTab(resolveTab(requested as DeepLinkId));
}
setTab(tabFromRoute(route) ?? 'general');
}, [route]);
const version = useAppVersion();
// Templates' own editor is a full-page takeover with its own header/back
Expand Down
18 changes: 18 additions & 0 deletions e2e/specs/settings-cmdk-search.t1.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,24 @@ test('search still works after a manual nav-rail tab switch (#405 regression)',
await expect(page.locator(settingsPage)).not.toContainText('Launch on login');
});

test('browser Back to bare /settings resets the visible tab to General (#405)', async ({
launchApp,
}) => {
const { page } = await launchApp(launchOpts);
await openSettings(page); // bare /settings → General

// Nav to AI via the rail — pushes /settings?tab=ai onto the hash history.
await page.locator('[data-settings-nav="ai"]').click();
await expect(page.locator(settingsPage)).toContainText('AI provider');

// Browser Back returns to the bare route. The route→tab effect must treat
// the absent param like first mount (General), not leave the AI tab stale.
await page.goBack();
await expect.poll(() => page.evaluate(() => window.location.hash)).toBe('#/settings');
await expect(page.locator(settingsPage)).toContainText('Launch on login');
await expect(page.locator(settingsPage)).not.toContainText('AI provider');
});

// Drift guard: a few index titles must still match the labels their tabs
// actually render, so a renamed control can't leave a stale search entry that
// jumps to a tab where nothing matches. Uses only cross-platform settings.
Expand Down
Loading