Skip to content

feat(sessions): optional per-active-workspace sidebar filter#2951

Open
Sanjays2402 wants to merge 2 commits into
nesquena:masterfrom
Sanjays2402:feat/2549-workspace-session-filter
Open

feat(sessions): optional per-active-workspace sidebar filter#2951
Sanjays2402 wants to merge 2 commits into
nesquena:masterfrom
Sanjays2402:feat/2549-workspace-session-filter

Conversation

@Sanjays2402
Copy link
Copy Markdown
Contributor

@Sanjays2402 Sanjays2402 commented May 25, 2026

Slice A from the maintainer's plan in #2549: a Preferences toggle that scopes the sidebar to the active workspace, defaulting to today's behaviour (show all).

What this does

  • New boolean setting show_all_workspaces in _SETTINGS_DEFAULTS (default True), added to _SETTINGS_BOOL_KEYS so the existing validator covers it.
  • New checkbox in Settings > Preferences directly below "Show non-WebUI sessions", with matching data-i18n keys (settings_label_all_workspaces, settings_desc_all_workspaces) added to every locale block.
  • renderSessionListFromCache runs a workspace filter right after the existing project filter and before the archived filter. It reads window._showAllWorkspaces and compares each row's stored s.workspace against the active workspace path (preferring S.session.workspace and falling back to _currentWorkspaceDetail.path from panels.js).
  • boot.js hydrates window._showAllWorkspaces from settings on load and clears it on logout.
  • switchToWorkspace calls renderSessionListFromCache after the workspace update so the sidebar reflects the new scope immediately.
  • The toggle handler re-renders the list before debouncing the autosave, so the effect is instant.

Default True keeps existing users on the current behaviour. Rows missing a workspace value are kept (defensive; matches how cron/CLI rows without resolved workspaces behave today).

Verification

  • pytest tests/test_issue2549_workspace_filter.py — 7 passed (new file covering default, filter ordering, settings wiring, i18n).
  • pytest tests/test_chinese_locale.py tests/test_japanese_locale.py tests/test_korean_locale.py tests/test_russian_locale.py tests/test_spanish_locale.py tests/test_turkish_locale.py — 33 passed (locale parity still holds with the two new keys).
  • node --check on boot.js, i18n.js, panels.js, sessions.js.
  • python3 -m py_compile api/config.py.

Closes #2549

Deviation from spec (worth flagging)

The maintainer's plan said use _currentWorkspaceDetail.path as the active workspace. This PR prefers S.session.workspace and falls back to _currentWorkspaceDetail.path only when the former is missing. _currentWorkspaceDetail is only set after the user opens Settings → Workspaces (default null at static/panels.js:27), so the spec as written would silently no-op for any user who never opens that panel. Preferring S.session.workspace makes the filter work on first launch.

Review-feedback fixes (8bd6f42)

  • Pinned rows now bypass the workspace filter (matching how !s.workspace rows do) so the ★ Pinned group stays visible across workspace switches.
  • Added an empty-state branch ("No sessions in this workspace yet.") when the workspace filter alone yields zero rows.
  • CHANGELOG note that path comparison is case-sensitive in this slice; Slice C will normalize casing in a follow-up.

Issue nesquena#2549 Slice A. Adds a Settings > Preferences toggle 'Show sessions
from all workspaces' (default on) that, when turned off, hides sidebar
rows whose stored workspace path does not match the active workspace.

The filter runs after the existing project filter inside
renderSessionListFromCache and reads window._showAllWorkspaces, mirroring
how show_cli_sessions / _showAllProfiles already scope the list along
other axes. Re-renders on workspace switch and on toggle change.

Closes nesquena#2549
@nesquena-hermes
Copy link
Copy Markdown
Collaborator

Summary

Thanks @Sanjays2402 — this lands the Slice A shape the maintainer scoped in the issue thread, including the show_all_workspaces-defaults-true backward-compat behavior. I read the full diff against master, plus the PR head at static/sessions.js:3086-3146, static/panels.js:4854-4895, api/config.py:4487-4491, and the new tests/test_issue2549_workspace_filter.py. I also cross-checked the row's workspace shape against api/models.py:459 (where s.workspace is set to str(Path(workspace).expanduser().resolve())).

Code-quality is good and the test file is appropriately scoped (file-level assertions that catch regressions without coupling to internals). I want to flag three behavioral concerns before merge plus one improvement on the spec.

Code reference

The filter at static/sessions.js:3131-3141 (PR head):

const _showAllWs=(typeof window!=='undefined')?window._showAllWorkspaces!==false:true;
const _activeWsPath=(typeof S!=='undefined'&&S.session&&S.session.workspace)
  ||(typeof _currentWorkspaceDetail!=='undefined'&&_currentWorkspaceDetail?_currentWorkspaceDetail.path:null);
const workspaceFiltered=(_showAllWs||!_activeWsPath)
  ?projectFiltered
  :projectFiltered.filter(s=>!s.workspace||s.workspace===_activeWsPath);

And the active-workspace update in switchToWorkspace at static/panels.js:4884-4888:

S.session.workspace=path;
// Trigger a sidebar re-render when the per-workspace filter is active (#2549).
if(typeof renderSessionListFromCache==='function') renderSessionListFromCache();

Diagnosis

1. Pinned sessions from other workspaces silently disappear (most important)

The filter drops every row whose s.workspace !== _activeWsPath, including pinned ones. Pinned is the user's explicit "keep this visible regardless of how I'm scoping" signal — the rest of the sidebar respects that (the ★ Pinned group renders before time-bucketed groups at static/sessions.js:3274). With the filter on, a user who pinned a session in workspace A and then switches to workspace B loses that pin from the sidebar entirely.

Two options:

  • Recommended: exempt pinned rows from the workspace filter the same way you already exempt rows missing s.workspace:

    const workspaceFiltered=(_showAllWs||!_activeWsPath)
      ?projectFiltered
      :projectFiltered.filter(s=>s.pinned||!s.workspace||s.workspace===_activeWsPath);
  • Alternative: document this as a known scope and add it to the toggle's description text. I don't think this is the right call — pinning is a stronger affordance than the per-workspace filter and should win.

2. No empty-state messaging when the filter yields zero rows

The existing project-filter empty state at static/sessions.js:3256-3261:

if(_activeProject&&sessions.length===0){
  const empty=document.createElement('div');
  empty.style.cssText='padding:20px 14px;color:var(--muted);font-size:12px;text-align:center;opacity:.7;';
  empty.textContent=_activeProject===NO_PROJECT_FILTER?'No unassigned sessions.':'No sessions in this project yet.';
  list.appendChild(empty);
}

A user who toggles the filter on, then switches to a brand-new workspace, sees a blank sidebar with no explanation. Adding a sibling branch for the workspace filter — something like:

if(!_showAllWs&&_activeWsPath&&!_activeProject&&sessions.length===0){
  const empty=document.createElement('div');
  empty.style.cssText='padding:20px 14px;color:var(--muted);font-size:12px;text-align:center;opacity:.7;';
  empty.textContent='No sessions in this workspace yet.';
  list.appendChild(empty);
}

Even better with a small "Show all workspaces" link that flips the toggle, but the text-only version is the minimum bar.

3. Case-insensitive paths (the maintainer's "Slice C" sharp edge)

The maintainer flagged this on the issue: Path.resolve() doesn't lowercase, so on APFS/NTFS a row with workspace /Users/syxc/Workspace/foo and an active workspace detail of /Users/syxc/workspace/foo (same inode, different casing) won't match under ===. The maintainer's plan said Slice C ships separately and this filter can ship without it — so this isn't a blocker, but it's worth a one-liner in the CHANGELOG or the toggle description ("workspace paths are compared exactly; Slice C will normalize case-insensitive matches"). Otherwise macOS users with the casing pattern from #2549 will hit it and reopen the issue.

A defensive option for now: use a normalized comparison that does cheap casefold without committing to a storage policy:

const norm=p=>typeof p==='string'?p.replace(/\/+$/,'').toLowerCase():p;
:projectFiltered.filter(s=>!s.workspace||norm(s.workspace)===norm(_activeWsPath))

That's lossless (doesn't change stored values, just comparison), and means Slice C only needs to decide what to do about the stored value, not what to do about the filter. But out of scope is also fine — your call.

4. Minor: _activeWsPath is more robust than the maintainer's spec

The original plan said use _currentWorkspaceDetail.path. Your code prefers S.session.workspace and falls back to _currentWorkspaceDetail.path. This is actually a strict improvement — _currentWorkspaceDetail is only set after the user opens Settings → Workspaces (look at static/panels.js:27, default null, and _renderWorkspaceDetail is the only setter), so for users who've never opened that panel, the maintainer's spec would silently no-op. Your choice to prefer S.session.workspace makes the filter work on first launch. Good call — worth calling that change out in the PR body so reviewers don't think it's a deviation.

Test plan after fixes

  1. Pin a session in workspace A. Switch to workspace B, toggle filter on. Confirm the pinned session still appears in the ★ Pinned group.
  2. Toggle filter on, switch to a brand-new empty workspace. Confirm an empty-state message appears.
  3. Switch between workspaces with the filter on. Confirm the sidebar updates immediately (already tested via switchToWorkspace re-render call).
  4. Toggle the checkbox in Settings → Preferences while on a workspace with sessions. Confirm the sidebar shrinks immediately, before the autosave debounce fires.
  5. Logout, log back in. Confirm the setting persists.
  6. On macOS, create two sessions where the workspace ends up stored with different casings (cd Workspace; hermes then cd workspace; hermes). Toggle filter on. Document the behavior — they'll show as two separate workspaces. This is the Slice C case.

Optional polish

  • All 12 non-English locale blocks got the English value for settings_label_all_workspaces / settings_desc_all_workspaces. Consistent with how recent features ship (the parity tests pass on placeholder English strings), so not blocking. Native translations can be a follow-up PR.

LGTM in shape — defaults-true keeps existing users untouched, the filter runs at the right point in the chain, the test file is well-structured, and the i18n placeholders match house style. Pinned exemption and the empty-state message are the two changes I'd want before merge.

@Sanjays2402
Copy link
Copy Markdown
Contributor Author

Both real, fixed in 8bd6f42: pinned rows now bypass the workspace filter the same way !s.workspace rows do, and added an empty-state branch (No sessions in this workspace yet.) when the workspace filter alone yields zero rows. Also added a CHANGELOG note that path comparison is case-sensitive in this slice with Slice C to follow \u2014 went with that over the defensive norm() so the casing policy stays a Slice C decision. S.session.workspace preference is now called out in the PR body too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add option to filter sessions by workspace (hide sessions from other workspaces)

2 participants