Skip to content

feat: add chat-pinned workspace actions - #2106

Closed
Casey-Mo wants to merge 1 commit into
nesquena:masterfrom
Casey-Mo:feat/chat-pinned-workspaces
Closed

feat: add chat-pinned workspace actions#2106
Casey-Mo wants to merge 1 commit into
nesquena:masterfrom
Casey-Mo:feat/chat-pinned-workspaces

Conversation

@Casey-Mo

Copy link
Copy Markdown

Summary

  • split workspace dropdown behavior into explicit New chat, Move chat, Use for new chats, and Show sessions actions
  • keep workspace state pinned to the chat/session instead of silently mutating the active chat on navigation
  • add backend /api/session/update protection so workspace moves are rejected during active/pending streams while model-only updates remain allowed
  • preserve compatibility paths for blank-page/default workspace handling and [Workspace::v1] prompt context
  • add focused regression tests plus an operator handoff doc

Verification

  • python -m pytest tests/test_workspace_selector_actions.py tests/test_workspace_system_message.py tests/test_workspace_display_prefix.py tests/test_issue1913_workspace_prefix_sentinel.py tests/test_profile_default_workspace_823.py tests/test_workspace_blank_page_fix.py tests/test_issue1464_workspace_dropdown_filter.py tests/test_workspace_panel_session_list.py tests/test_default_workspace_fallback.py tests/test_empty_session_no_disk_write.py tests/test_issue2066_stale_sidebar_spinner.py -q
    • 66 passed
  • Kanban ops verification previously ran the broader local smoke/update gate:
    • 90 passed, 0 failed
    • no unresolved conflicts or conflict markers
    • non-destructive update simulation passed: temp worktree + local patch + stash -u + pull --ff-only + stash pop + git diff --check
    • local WebUI service restarted and served HermesWebUI/0.51.45-dirty during smoke

Manual smoke covered

  • created Chat B in another workspace while Chat A was running
  • updated/moved Chat B independently
  • returned to Chat A and restored its workspace context
  • moving busy Chat A returned expected 409: Session is still streaming; wait for the current turn to finish before moving workspace.

Split workspace dropdown behavior into explicit new-chat, move-chat, default-workspace, and sidebar-filter actions. Guard session workspace moves during active/pending streams on both client and backend, preserve workspace prefix compatibility, and add focused regression coverage plus operator handoff notes.
@Casey-Mo

Copy link
Copy Markdown
Author

Claude Opus review

Model: Claude Opus via Claude Code (claude-clean --model opus)

Code Review: PR #2106 — Chat-pinned workspaces

Verdict

APPROVE WITH NON-BLOCKING CONCERNS — No blocking issues. The core change (workspace pinned per-session with explicit dropdown actions and a server-side 409 guard) is sound, well-tested, and backward-compatible. Several non-blocking items below should be considered before/after merge.

Blocking Issues

None.

Non-blocking Issues

1. static/ui.js — hardcoded operator-specific hostname mapping (_dashboardBrowserUrl)
The new publicDashboardUrls={'webui.kmc-ai.com':'https://hermes.kmc-ai.com/'} bakes a specific operator's tunnel topology into the public static bundle shipped to every Hermes WebUI install. This is operator config leaking into product code. Suggest moving to a server-injected config (e.g., extend an existing /api/config payload) or at minimum behind a build-time variable. Not a security issue (no secret), but it will hit anyone who self-hosts behind a similarly-named hostname.

2. static/sw.jsSW_BUILD_NONCE is declared but unused
The constant is added with a comment that it "forces iOS/Safari to install a fresh SW," but CACHE_NAME does not include SW_BUILD_NONCE. The reinstall actually happens because the SW file bytes changed, not because the cache key bumped. Either inline the nonce into CACHE_NAME so the cache also invalidates, or drop the constant — current state is misleading dead code.

3. static/sessions.js loadSession() — boot-restore clears localStorage on any error, not just 404
Old behavior: only 404 cleared hermes-webui-session. New behavior: any failure on the saved-session restore path (including transient 5xx or network blips) wipes the saved active-session pointer and rethrows. Mobile users on flaky links may now lose their last session after a single transient error. Suggest restricting isSavedBootRestore cleanup to e.status===404 (or 404+410), keeping transient 5xx recoverable on retry.

4. api/routes.py/reset GET is unauthenticated and inline-scripted
The route serves a self-clearing HTML page with inline <script>(async function(){...})()</script>. Two notes:

  • Inline script requires CSP script-src 'unsafe-inline' or a nonce. Recent commit 96ca83b fix(security): drop unsafe-eval tightened CSP; verify 'unsafe-inline' is still allowed on this route's response or this won't actually run on browsers enforcing strict CSP.
  • Endpoint is reachable by any HTTP client. Effect is bounded to the caller's browser (cookies/storage), so blast radius is self-only — but unauth + inline-script combo is worth a one-line note.

5. api/routes.py /api/session/update — busy check holds session lock while returning 409
The 409 return happens inside with _get_session_agent_lock(body["session_id"]):. The context manager will release the lock on return, so this is correct, but the pattern is slightly unusual. Consider hoisting the bad(...) call after the with block for clarity. Not a functional issue.

6. api/routes.py GET /api/session — CLI sessions hard-coded to read_only: True
Previously bool((cli_meta or {}).get("read_only")); now always True. This removes any path where cli_meta could signal a mutable CLI session. Comment justifies it; just confirm there's no flow (e.g., resumed CLI sessions surfaced through state.db that the user can continue) that depended on the old conditional behavior. The companion static/messages.js change blocking send() on is_cli_session is consistent.

7. api/streaming.py_workspace_system_message semantics changed
The new message drops "This tag is the single authoritative source ... It overrides any prior workspace mentioned in this system prompt, memory, or conversation history." Practically, since _workspace_context_prefix is still prepended to every user message, agents should still pick up the active workspace. But long-running migrated sessions whose system prompt previously included the stronger override language may now have weaker priming. Worth eyeballing one or two real long sessions after deploy.

8. static/panels.jsmoveCurrentChatToWorkspace does not distinguish server 409 from generic errors
The client checks _isCurrentSessionBusyForWorkspaceMove() before POSTing, but if the server returns 409 (e.g., race where the client thought it was idle but pending_user_message got set server-side), it falls into the generic catch(e){setStatus(t('switch_failed')+e.message);} and surfaces a less helpful message than the server-supplied "Session is still streaming..." Consider checking e.status===409 and routing to showToast(t('workspace_busy_switch')) or surfacing e.message.

9. static/i18n.js — new keys added only to one locale block
The diff shows the new workspace_action_* keys in a single LOCALES entry. If other languages exist in the same file, those locales will fall back to English (assuming fallback is implemented). Worth a grep to confirm fallback behavior and to update other locales if the project conventionally translates eagerly.

Test Coverage

  • tests/test_workspace_selector_actions.py (new): Reasonable coverage via string-matching against panels.js/sessions.js/routes.py/i18n.js. Brittle to whitespace/refactors but catches the targeted regressions (e.g., that newChatInWorkspace body doesn't call /api/session/update, that the model-update path remains allowed in /api/session/update even when workspace is unchanged).
  • tests/test_workspace_system_message.py (new): Asserts new wording. Pinned to specific phrases — fragile if marketing/UX rewords later.
  • Gap: No backend test for the 409 path in /api/session/update when active_stream_id/pending_user_message is set. The selector-actions test asserts the source code contains the guard but doesn't exercise it end-to-end. Worth adding a small handler-level test that constructs a session with active_stream_id set and asserts the route returns 409 + leaves s.workspace unchanged.
  • Gap: No test for _handle_workspace_set_last path validation (bad path → ValueError → 400; valid path → set_last_workspace called).
  • Gap: No client-side test that read-only/CLI sessions skip loadDir/_refreshGitBadge (covered only by manual smoke per the handoff doc).

Rollout Recommendation

Safe to ship behind normal release. Suggested order of operations:

  1. Merge.
  2. Watch for any spike in lost-session reports on mobile (item fix(frontend): use URL origin for fetch/EventSource to support revers… #3 above) — if seen, gate the localStorage cleanup to 404 only.
  3. Confirm CSP allows the inline script on /reset in production (item fix(api): resolve model provider from config to prevent misrouting #4); easy to verify by hitting /reset in a strict-CSP browser context.
  4. Open a follow-up to externalize the webui.kmc-ai.com mapping (item Portability #1) before any third party deploys this build.
  5. The aggressive cache-buster h=cli-readonly-session-20260509 on all asset URLs + SW shell version bump should force-refresh mobile clients on first load — no manual user action needed.

Rollback is clean: no schema changes; per-session workspace storage is unchanged; the new /api/workspaces/set_last and /reset are additive; reverting the patch restores prior behavior with no data migration.

@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Thanks @Casey-Mo — substantial workspace-pinning slice that materially improves the chat-vs-workspace mental model. Reviewed the diff, want to make sure the UX shape is right before the code review takes too much time.

Read on the change

  • Four explicit actions (New chat, Move chat, Use for new chats, Show sessions) replacing the implicit "select workspace → silently mutate active chat" behavior is the right direction
  • Pinning workspace to chat instead of global is a real correctness win — currently it's possible to start a turn in workspace A, navigate to a different chat (silently shifting the global pointer to workspace B), then have the original stream's tool calls resolve paths against B
  • Backend 409 on /api/session/update workspace-move during active/pending streams is the right protection; the model-only-update allowance keeps the common case unblocked
  • The operator handoff doc (docs/chat-pinned-workspaces-handoff.md) is a thoughtful artifact for a feature this size

UX gate — screenshots needed before deep code review

Since this is a substantive UI shape change (new dropdown layout, new confirm flows, new "pinned" affordance), screenshots in the PR body before I sink Opus time into the 476-LOC diff. Specifically:

Workspace dropdown — 1280px desktop and 390px mobile:

  1. Default closed state — chat in workspace A, dropdown closed, showing whatever current-workspace indicator the design lands on.
  2. Dropdown expanded — all four actions visible (New chat / Move chat / Use for new chats / Show sessions), label copy + any icons.
  3. "Move chat" mid-confirm — whatever confirm dialog or inline state warns the user that this is changing the pinned workspace for the current chat (vs. a transient navigation).
  4. "Move chat" rejected during active stream — the 409 surface from /api/session/update. How does the frontend present that to the user? Toast? Inline error? Disabled state during stream?

Cross-chat behavior — 1280px:

  1. Two chats side-by-side concept — Chat A pinned to workspace /foo, Chat B pinned to workspace /bar. Switching between them in the sidebar should not silently mutate either pin. A short screenshot or screen recording demonstrating the no-silent-mutation behavior is the most credible artifact here.

Show sessions affordance — 1280px:

  1. "Show sessions" action triggered — what does the sessions-for-this-workspace view look like? Is it a panel, a filter on the existing sidebar, a new route?

What I'll do next once screenshots land

  • Full Opus pre-review of the 16-file diff (api/models.py, api/routes.py, api/streaming.py, plus 10 frontend files, plus 2 test files, plus the handoff doc)
  • Specific focus areas: (a) the 409 path doesn't accidentally block legitimate model-only updates during streams, (b) the workspace pin is correctly carried through [Workspace::v1] prompt context, (c) static/sw.js change is scoped correctly (it's surprising in this PR — what's the service-worker side touching?), (d) the operator handoff doc's claims match the code

Labels

Adding hold + ux until screenshots land. Take your time — this is a real feature, not a quick fix, and getting the dropdown layout right pre-merge is cheaper than iterating after release.

@nesquena-hermes nesquena-hermes added hold ux User experience / visual polish labels May 12, 2026
@Casey-Mo

Copy link
Copy Markdown
Author

Closing: this change was intended for local deployment only, not upstream submission.

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

Labels

hold ux User experience / visual polish

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants