feat(sidebar): add session pinning#319
Merged
Merged
Conversation
roxi3906
force-pushed
the
claude/gracious-maxwell-ad91bf
branch
from
July 23, 2026 06:12
2f5fb5b to
63a92cc
Compare
- Add optional field to PersistedChatSession; only an explicit true is restored on load so legacy session files stay unpinned by default - Add togglePinned action to session store; flips the flag without updating updatedAt so pinning never disturbs the last-active sort order within each section; persisted automatically via the existing reference-diff saver - Split sidebar session list into Pinned / Active sections; the Pinned section only renders when at least one conversation is pinned - Add Pin / Unpin menu item at the top of each session's dropdown menu, with icon switching between Pin and PinOff based on current state - Wire onTogglePin callback through WorkspacePage - Add store tests (toggle, updatedAt unchanged, persistence roundtrip) and sidebar render tests (section ordering, menu item wiring) Closes aipoch#288 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Replace raw radix-ui DropdownMenu with shadcn @/components/ui/dropdown-menu to align with the rest of the workspace (ComposerAgentControlsMenu, etc.) - Apply dark surface treatment: bg-text-000/text-bg-000 matching the app's tooltip token convention; border-transparent removes the light-mode border - Hover state overridden to bg-white/10 (white overlay) so items remain readable against the dark background - Delete item uses text-red-400 / data-[highlighted]:bg-red-500/15, a red accent suited for dark surfaces (replaces danger-900 light hover) - Remove all DropdownMenu.Separator dividers per design direction - Drop now-unused sessionMenuContentClassName, sessionMenuItemClassName, sessionMenuDangerItemClassName constants; keep sessionMenuIconClassName Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
roxi3906
force-pushed
the
claude/gracious-maxwell-ad91bf
branch
from
July 23, 2026 06:15
63a92cc to
69c3840
Compare
Switch the session action dropdown from the dark tooltip surface back to the shadcn default light popover (bg-popover/text-popover-foreground) to match the app's other menus. Delete item uses the danger-000/danger-900 token pair suited for the light surface. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
roxi3906
marked this pull request as ready for review
July 23, 2026 06:40
ewen-poch
added a commit
that referenced
this pull request
Jul 23, 2026
…eminders
A conversation that fails to START (createSession) throws its setup-guidance
error from settings/service.ts:resolveActiveAgentBackend — model↔framework
incompatible, no active provider, unsupported Codex bridge model, missing Claude
executable. Unlike the resume path, this path does not route through the resume
softener, and the error crosses IPC wrapped ("Error invoking remote method …"),
so it was persisted verbatim and shown the "Report error" button — a wrong-config
setup problem masquerading as a reportable app bug.
- Export the four setup-guidance messages (and an active-model-incompatible
builder + prefix) as shared constants in run-error-classification. service.ts
builds its throws from these SAME constants, so the text can never drift from
what the classifier recognizes (the fix the reviewer asked for).
- Recognize them as expected (exact-match set + the framework-name-independent
incompatible prefix) so the report button is hidden.
- Unwrap the IPC wrapper on the createSession failure path (generalize the resume
unwrapper to unwrapIpcErrorDetail) so the persisted text is clean and matches.
Also rebased onto current main (was 9 behind), so the session-pinning feature
(#319) the AI reviewer read as "removed" is correctly present.
Tests: classifier recognizes each config constant + the builder (drift guard);
the createSession failure path unwraps an IPC-wrapped incompat error and marks it
non-reportable. 500 tests, typecheck (node + web), lint clean.
ewen-poch
added a commit
that referenced
this pull request
Jul 23, 2026
…362) * fix(workspace): report only unknown run failures, not expected ones The "Report error" button opened a GitHub issue for every failed run, including failures the app already recognizes and explains — a missing workspace, a resume timeout, an incompatible model, an image-replay mismatch, or a well-understood provider error (bad key, rate limit, quota, overloaded, resource-not-found). Those are the user's or the provider's to resolve, not app bugs, so surfacing "Report error" for them invites noise in the issue tracker. Add a shared, pure leaf module (src/shared/run-error-classification.ts, mirroring media-overflow.ts) that classifies a persisted run-error string as expected vs unknown, and gate the button on it: an expected failure keeps its actionable message but drops the report affordance; only an unknown/opaque failure (or one with no message) offers it. The crafted messages now live as single-source constants that both the producers (getResumeFailureMessage, the image-replay guard, and main's describePromptError via a shared prefix) and the classifier reference, so the two cannot drift. A drift-guard test feeds describePromptError's real output through the classifier. Classifying the persisted string at display time keeps recognition working across a reload with no schema change. * fix(workspace): anchor provider-error match to avoid swallowing app errors Address review on #362: - P1: PROVIDER_ERROR_PATTERN matched bare numbers and single generic words (401, quota, billing, overloaded, "rate limit"), so ordinary app errors (EACCES: permission denied, Failed to parse rate limit configuration, Record 503 could not be decoded) were classified as expected and lost their Report button. Anchor recognition to signals absent from app prose: exact underscore slugs, an HTTP status code with an explicit HTTP/status marker, a status code immediately followed by its canonical reason phrase, or an unmistakable multi-word reason phrase. - P2: the code+reason branch now covers every 5xx the spec names (500/502/503/ 504), and the HTTP-marker branch covers any 3-digit code, so 500 Internal Server Error is expected. Add a 500 test. - P3: move the shared isReportableRunFailure import into the shared import group in ConversationPanel.tsx. - Add a false-positive regression test for the app-error prose above; switch the billing case to the billing_hard_limit slug. * fix(workspace): tighten provider-error token boundaries and billing phrase * fix(workspace): bound billing phrases, recognize verbatim provider auth/5xx * fix(workspace): classify run-failure reportability structurally at the ACP layer Determine whether a failed run offers Report error from where the failure originated, not from its message text. The agent tags an upstream model/provider failure structurally (APIError / errorKind provider-error / provider not-found); runtime.ts marks the error event providerError, which flows through failRun into a persisted session.errorReportable so only genuinely unexpected ACP-layer exceptions stay reportable. The renderer text tier now recognizes ONLY the app's own crafted reminder strings; it no longer guesses provider errors from text. - shared/acp: add providerError to the error event (copied through pushEvent). - prompt-error: export isProviderPromptError (APIError + provider-error kind + not-found). - runtime: tag the prompt-failed event with providerError. - session-store: failRun(opts.reportable); persist errorReportable; clear it wherever error clears; artifact errors are reportable. - session-persistence: round-trip errorReportable. - workspace-events + useWorkspaceAgentRuntime: pass the structural flag; the rejection path reads this turn's error event (baselined) instead of guessing. - task-api: propagate the flag on the headless path. - run-error-classification: drop the provider-error regex; keep only the exact crafted-message set. - Tests across all layers: isProviderPromptError, providerError tagging, failRun reportability, persistence round-trip, and the rejection path. * fix(workspace): recognize createSession config failures as expected reminders A conversation that fails to START (createSession) throws its setup-guidance error from settings/service.ts:resolveActiveAgentBackend — model↔framework incompatible, no active provider, unsupported Codex bridge model, missing Claude executable. Unlike the resume path, this path does not route through the resume softener, and the error crosses IPC wrapped ("Error invoking remote method …"), so it was persisted verbatim and shown the "Report error" button — a wrong-config setup problem masquerading as a reportable app bug. - Export the four setup-guidance messages (and an active-model-incompatible builder + prefix) as shared constants in run-error-classification. service.ts builds its throws from these SAME constants, so the text can never drift from what the classifier recognizes (the fix the reviewer asked for). - Recognize them as expected (exact-match set + the framework-name-independent incompatible prefix) so the report button is hidden. - Unwrap the IPC wrapper on the createSession failure path (generalize the resume unwrapper to unwrapIpcErrorDetail) so the persisted text is clean and matches. Also rebased onto current main (was 9 behind), so the session-pinning feature (#319) the AI reviewer read as "removed" is correctly present. Tests: classifier recognizes each config constant + the builder (drift guard); the createSession failure path unwraps an IPC-wrapped incompat error and marks it non-reportable. 500 tests, typecheck (node + web), lint clean. * fix(workspace): don't force-report a non-recovered context overflow Both failure-writing paths derived reportability as `!providerError`, explicitly passing `reportable: true` for every non-provider error. That value wins over failRun's text tier (opts.reportable ?? isReportableRunFailure), so a context-overflow that did NOT start recovery — a repeat overflow inside the cooldown, nothing to replay, or a detached session — was persisted with the "Report error" button even though the text tier recognizes overflow as an expected, non-reportable failure. The overflow can carry providerError=false (the agent CLI's client-side "Request too large (max 32MB)" ceiling trips before dispatch, so it is neither an APIError nor tagged provider-error), which made the `isMediaOverflowError` branch in isReportableRunFailure dead code on the event path. Fix: only a provider-tagged failure forces reportable=false; everything else leaves reportable undefined so the text tier decides. This keeps opaque ACP-layer failures reportable while letting a non-recovered overflow stay non-reportable, and makes the event path and the rejection path agree on the same rule. Tests: assert errorReportable===false for the no-recovery overflow (fails against the old forced-true expression — verified), errorReportable===true for an opaque ACP-layer event, and a new provider-tagged event-path case that stays hidden. 295 acp/shared tests, typecheck (node + web), lint clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the pin feature requested in #288.
Users can now pin any conversation to a dedicated Pinned section at the top of the sidebar. The section appears only when at least one conversation is pinned and disappears when all are unpinned. Multiple conversations can be pinned simultaneously, and pinned state survives app restarts.
Changes
Data layer
pinned?: booleantoPersistedChatSessioninshared/session-persistence.ts. Only an explicittrueis restored on load; legacy session files without the field default to unpinned with no migration required.Store
togglePinned(sessionId)action to the session store. Flips thepinnedflag and intentionally does not updateupdatedAt, so pinning never disturbs the last-active sort order within each section. Persisted automatically through the existing reference-diff saver — no IPC or persistence-bridge changes needed.Sidebar UI
PinandPinOffbased on the current state.onTogglePincallback wired throughWorkspacePage.Testing
togglePinnedtoggles the flag, leavesupdatedAtunchanged, and the flag round-trips throughtoPersistedSession.onTogglePinwith the correct session.No architecture changes
Sessions remain file-based; the database is not involved. The
pinnedfield flows through the existing spread-based strip/hydrate path with no extra plumbing.🤖 Generated with Claude Code