feat(web): settings modal shell + gear button (deep-linkable via ?settings)#253
feat(web): settings modal shell + gear button (deep-linkable via ?settings)#253mgabor3141 wants to merge 2 commits into
Conversation
|
| Filename | Overview |
|---|---|
| apps/gmux-web/src/main.tsx | Replaces local manageProjectsOpen state with URL-driven settingsOpen derived from loc.query.settings; introduces openSettings/closeSettings callbacks; minor duplicate-push edge case when gear is clicked while modal is already open |
| apps/gmux-web/src/settings.tsx | Rename from manage-projects.tsx + export name SettingsModal; content and logic unchanged; comments updated to reflect future tab-bar slices |
| apps/gmux-web/src/sidebar.tsx | Adds IconSettings SVG, replaces onManageProjects prop with onOpenSettings, moves LaunchButton from header into sidebar body for empty state, adds sidebar-settings-btn gear button |
| apps/gmux-web/src/styles.css | Adds CSS for .sidebar-settings-btn (gear button) and .sidebar-empty-launch (repositioned LaunchButton container); clean additions with no apparent conflicts |
Sequence Diagram
sequenceDiagram
participant User
participant Sidebar
participant App
participant Browser as Browser History
participant SettingsModal
User->>Sidebar: clicks gear button
Sidebar->>App: onOpenSettings callback
App->>Browser: "pushState to ?settings=projects"
App->>SettingsModal: "open=true"
User->>SettingsModal: Escape or backdrop click
SettingsModal->>App: onClose callback
App->>Browser: replaceState strips ?settings
App->>SettingsModal: "open=false"
User->>Browser: presses Back
Browser->>App: popstate to previous URL
App->>SettingsModal: "open=false"
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
apps/gmux-web/src/main.tsx:372-376
**Duplicate history push when settings already open**
`openSettings` unconditionally calls `loc.route(...)`, which does a `history.pushState`, even when `?settings` is already present in the URL. On desktop the gear button is always visible, so clicking it while the modal is open pushes an identical history entry — pressing back then lands on `?settings=projects` again, and only a second back actually closes the modal. A quick guard (or a `replace` when already open) prevents the stale entry.
```suggestion
const openSettings = useCallback((tab = 'projects') => {
const params = new URLSearchParams(location.search)
// If the requested tab is already active, replace rather than push so
// the back button doesn't land on a duplicate settings entry.
const alreadyOpen = params.get('settings') === tab
params.set('settings', tab)
loc.route(`${loc.path}?${params.toString()}`, alreadyOpen)
}, [loc])
```
Reviews (1): Last reviewed commit: "feat(web): settings modal shell + gear b..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const openSettings = useCallback((tab = 'projects') => { | ||
| const params = new URLSearchParams(location.search) | ||
| params.set('settings', tab) | ||
| loc.route(`${loc.path}?${params.toString()}`) | ||
| }, [loc]) |
There was a problem hiding this comment.
openSettings unconditionally calls loc.route(...), which does a history.pushState, even when ?settings is already present in the URL. On desktop the gear button is always visible, so clicking it while the modal is open pushes an identical history entry — pressing back then lands on ?settings=projects again, and only a second back actually closes the modal. A quick guard (or a replace when already open) prevents the stale entry.
| const openSettings = useCallback((tab = 'projects') => { | |
| const params = new URLSearchParams(location.search) | |
| params.set('settings', tab) | |
| loc.route(`${loc.path}?${params.toString()}`) | |
| }, [loc]) | |
| const openSettings = useCallback((tab = 'projects') => { | |
| const params = new URLSearchParams(location.search) | |
| // If the requested tab is already active, replace rather than push so | |
| // the back button doesn't land on a duplicate settings entry. | |
| const alreadyOpen = params.get('settings') === tab | |
| params.set('settings', tab) | |
| loc.route(`${loc.path}?${params.toString()}`, alreadyOpen) | |
| }, [loc]) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/gmux-web/src/main.tsx
Line: 372-376
Comment:
**Duplicate history push when settings already open**
`openSettings` unconditionally calls `loc.route(...)`, which does a `history.pushState`, even when `?settings` is already present in the URL. On desktop the gear button is always visible, so clicking it while the modal is open pushes an identical history entry — pressing back then lands on `?settings=projects` again, and only a second back actually closes the modal. A quick guard (or a `replace` when already open) prevents the stale entry.
```suggestion
const openSettings = useCallback((tab = 'projects') => {
const params = new URLSearchParams(location.search)
// If the requested tab is already active, replace rather than push so
// the back button doesn't land on a duplicate settings entry.
const alreadyOpen = params.get('settings') === tab
params.set('settings', tab)
loc.route(`${loc.path}?${params.toString()}`, alreadyOpen)
}, [loc])
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed: openSettings now replaces instead of pushing when the requested tab is already active (replace || alreadyActive), so clicking the gear while the modal is open no longer stacks a duplicate history entry. (The replace param itself lands in the hosts-tab PR up the stack, where tab switches also replace.)
Try this PRcurl -sSfL https://gmux.app/install-pr.sh | sh -s -- 253Built from |
mgabor3141
force-pushed
the
feat/local-host-multihost
branch
from
fb13d69 to
4a53371
Compare
1 participant
Closes #249.
Summary
First slice of the home/settings restructure (#249): introduce a deep-linkable Settings modal and a gear button, replacing the standalone "Manage projects" modal. Shell + entry point only — the modal's content is unchanged (still today's add/discovery UI); the tab bar and the configured-project manage-list arrive in later slices (#250/#251).
Changes
manage-projects.tsx→settings.tsx, exportManageProjectsModal→SettingsModal. Content/behavior otherwise unchanged.?settingsquery param read fromloc.query, not the path. The path-derivedviewis untouched, so opening settings over a live session keeps the terminal/websocket mounted. Open pushes a history entry (back closes); close replaces. Open/close preserve any other query params (onlysettingsis touched).aria-label/title"Settings". NewIconSettings(sliders glyph)..sidebar-empty-launch), so the header is just logo + gear when you have no projects.onManageProjectsprop →onOpenSettings; the "Add a project" hint link and the gear both open settings.Testing
?settings=projectsover the preserved home view; deep-linking?settings=projectsopens directly; close strips the param; unrelated query params survive open/close.