Conversation
There was a problem hiding this comment.
Pull request overview
Implements issue #35 by making the sessions sidebar resizable (with persisted width) and adjusting session rows so titles can use the full available width while action icons overlay on hover.
Changes:
- Added a resizable sessions sidebar (180–480px) backed by
layout.sidebar.width()persisted inopencode.layout. - Updated session row layout to remove reserved right padding and add a hover overlay (gradient + background) behind action icons.
- Extended the reusable
ResizeHandleAPI with anonDragEndcallback.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
app-prefixable/src/pages/layout.tsx |
Uses the new layout sidebar width for panel sizing, adds the resize handle, and updates session title/icon hover behavior. |
app-prefixable/src/context/layout.tsx |
Adds persisted sidebar.width state to the layout context (localStorage opencode.layout). |
app-prefixable/src/components/resize-handle.tsx |
Adds optional onDragEnd callback support for drag lifecycle handling. |
Comments suppressed due to low confidence (1)
app-prefixable/src/components/resize-handle.tsx:67
onDragEndis only invoked in the normal mouseup path. If the handle unmounts orcleanup()runs while dragging (e.g., responsive logic hides the sidebar mid-drag),onDragEndwon’t fire and callers can get stuck in a “dragging” state. Consider callingprops.onDragEnd?.()fromcleanup()/ theonCleanuphandler as well (while ensuring it only runs once).
const threshold = props.collapseThreshold ?? 0
if (props.onCollapse && threshold > 0 && current < threshold) {
props.onCollapse()
}
if (props.onDragEnd) props.onDragEnd()
}
cleanup = () => {
document.body.style.userSelect = ""
document.body.style.overflow = ""
document.removeEventListener("mousemove", onMouseMove)
document.removeEventListener("mouseup", onMouseUp)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
app-prefixable/src/pages/layout.tsx
Outdated
| <div | ||
| class="absolute right-0 top-0 bottom-0 hidden group-hover:flex items-center rounded-r-md" | ||
| > | ||
| <Archive class="w-4 h-4" /> | ||
| </button> | ||
| <div | ||
| class="w-6 h-full" |
There was a problem hiding this comment.
The hover action overlay is an absolutely positioned sibling over the right side of the row; when it’s visible it will intercept pointer events and make the underlying session link not clickable in that region (clicking on the gradient/background does nothing). Consider setting pointer-events: none on the overlay container/gradient and pointer-events: auto on the actual icon button container so link clicks still work outside the icons.
app-prefixable/src/pages/layout.tsx
Outdated
| background: `linear-gradient(to right, transparent, ${isActive(session.id) ? "var(--surface-inset)" : "var(--background-stronger)"})`, | ||
| }} | ||
| /> | ||
| <div | ||
| class="flex items-center pr-1.5" | ||
| style={{ | ||
| background: isActive(session.id) ? "var(--surface-inset)" : "var(--background-stronger)", |
There was a problem hiding this comment.
For inactive sessions, the row background is set to var(--surface-inset) on hover, but the new icon overlay background/gradient uses var(--background-stronger). In dark mode this will look like a mismatched block behind the icons. Since the overlay only appears on hover, consider matching the overlay background to the hovered row background (e.g., var(--surface-inset) for non-active rows as well) so it blends with the highlight.
| background: `linear-gradient(to right, transparent, ${isActive(session.id) ? "var(--surface-inset)" : "var(--background-stronger)"})`, | |
| }} | |
| /> | |
| <div | |
| class="flex items-center pr-1.5" | |
| style={{ | |
| background: isActive(session.id) ? "var(--surface-inset)" : "var(--background-stronger)", | |
| background: "linear-gradient(to right, transparent, var(--surface-inset))", | |
| }} | |
| /> | |
| <div | |
| class="flex items-center pr-1.5" | |
| style={{ | |
| background: "var(--surface-inset)", |
app-prefixable/src/pages/layout.tsx
Outdated
| (e.currentTarget.style.color = | ||
| "var(--icon-weak)") | ||
| } | ||
| title="Archive session" |
There was a problem hiding this comment.
The "Archive session" icon button relies on title for its accessible name, but it doesn’t have an aria-label (unlike the restore button below). Please add aria-label="Archive session" so screen readers have a reliable label.
| title="Archive session" | |
| title="Archive session" | |
| aria-label="Archive session" |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
app-prefixable/src/components/resize-handle.tsx:67
onDragEndis only invoked from the mouseup handler. If theResizeHandleunmounts while dragging (e.g., the panel gets collapsed/hidden before mouseup), the cleanup runs butonDragEndwon’t fire, which can leave consumers (like the sidebar transition-disabling state) stuck in a "dragging" state. Consider invokingprops.onDragEnd?.()from thecleanupfunction as well (guarded so it only runs once).
const onMouseUp = () => {
document.body.style.userSelect = ""
document.body.style.overflow = ""
document.removeEventListener("mousemove", onMouseMove)
document.removeEventListener("mouseup", onMouseUp)
cleanup = null
const threshold = props.collapseThreshold ?? 0
if (props.onCollapse && threshold > 0 && current < threshold) {
props.onCollapse()
}
if (props.onDragEnd) props.onDragEnd()
}
cleanup = () => {
document.body.style.userSelect = ""
document.body.style.overflow = ""
document.removeEventListener("mousemove", onMouseMove)
document.removeEventListener("mouseup", onMouseUp)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <div | ||
| class="absolute right-0 top-0 bottom-0 hidden group-hover:flex items-center rounded-r-md" | ||
| > | ||
| <ArchiveRestore class="w-4 h-4" /> | ||
| </button> | ||
| <div | ||
| class="w-6 h-full" | ||
| style={{ | ||
| background: `linear-gradient(to right, transparent, ${isActive(session.id) ? "var(--surface-inset)" : "var(--background-stronger)"})`, | ||
| }} | ||
| /> | ||
| <div | ||
| class="flex items-center pr-1.5" | ||
| style={{ | ||
| background: isActive(session.id) ? "var(--surface-inset)" : "var(--background-stronger)", | ||
| }} |
There was a problem hiding this comment.
In the archived sessions list, the hover action overlay is an absolutely positioned sibling of the link and currently allows pointer events. This will intercept clicks on the right side of the row, preventing navigation when users click near the action icons area. Consider matching the active-sessions overlay behavior by disabling pointer events on the overlay container and re-enabling them only on the actual button container/button.
app-prefixable/src/pages/layout.tsx
Outdated
| background: `linear-gradient(to right, transparent, ${isActive(session.id) ? "var(--surface-inset)" : "var(--background-stronger)"})`, | ||
| }} | ||
| /> | ||
| <div | ||
| class="flex items-center pr-1.5" | ||
| style={{ | ||
| background: isActive(session.id) ? "var(--surface-inset)" : "var(--background-stronger)", |
There was a problem hiding this comment.
Archived session action overlay background/gradient uses var(--background-stronger) for non-active rows, but the row hover state sets the row background to var(--surface-inset). This creates a visible mismatch behind the icons on hover. Align the overlay background with the row background (e.g., use var(--surface-inset) on hover/active, or otherwise compute the correct value) so the overlay blends correctly.
| background: `linear-gradient(to right, transparent, ${isActive(session.id) ? "var(--surface-inset)" : "var(--background-stronger)"})`, | |
| }} | |
| /> | |
| <div | |
| class="flex items-center pr-1.5" | |
| style={{ | |
| background: isActive(session.id) ? "var(--surface-inset)" : "var(--background-stronger)", | |
| background: | |
| "linear-gradient(to right, transparent, var(--surface-inset))", | |
| }} | |
| /> | |
| <div | |
| class="flex items-center pr-1.5" | |
| style={{ | |
| background: "var(--surface-inset)", |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const threshold = props.collapseThreshold ?? 0 | ||
| if (props.onCollapse && threshold > 0 && current < threshold) { | ||
| props.onCollapse() | ||
| } | ||
| if (props.onDragEnd) props.onDragEnd() | ||
| } |
There was a problem hiding this comment.
onDragEnd is only invoked from the onMouseUp handler. If the handle unmounts during an active drag (e.g., responsive layout hides it, route change, or parent conditional Show toggles), cleanup() runs but onDragEnd never fires, which can leave parent UI state (like “dragging”) stuck. Consider tracking a dragging flag and calling props.onDragEnd?.() from cleanup() (and/or onCleanup) when a drag was in progress.
| class={`shrink-0 flex flex-col ${sidebarDragging() ? "" : "transition-all duration-200"}`} | ||
| style={{ | ||
| width: showSidebar() ? "256px" : "0px", | ||
| width: showSidebar() ? `${layout.sidebar.width()}px` : "0px", |
There was a problem hiding this comment.
sidebarDragging is set to true during onResize and reset via onDragEnd, but onDragEnd won’t run if the resize handle unmounts mid-drag (e.g., window resized below the breakpoint or navigation to /settings). That can leave sidebarDragging() stuck true, permanently disabling the sidebar transition classes. Add an effect to reset sidebarDragging whenever showSidebar() becomes false (and/or ensure ResizeHandle triggers onDragEnd from cleanup).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
app-prefixable/src/pages/layout.tsx
Outdated
| @@ -678,26 +685,46 @@ export function Layout(props: ParentProps) { | |||
| {session.title || "Untitled"} | |||
| </span> | |||
There was a problem hiding this comment.
<span class="truncate"> inside a flex row typically needs min-w-0 (and often flex-1) to actually ellipsize instead of overflowing/clipping without an ellipsis. Consider updating the title span (or the <A> container) to include min-w-0 + a flexible width so long session titles reliably truncate while still using the full row width.
app-prefixable/src/pages/layout.tsx
Outdated
| @@ -772,27 +799,46 @@ export function Layout(props: ParentProps) { | |||
| {session.title || "Untitled"} | |||
| </span> | |||
There was a problem hiding this comment.
Same flexbox truncation concern for archived sessions: without min-w-0 (and typically flex-1) on the title flex item, truncate can fail to ellipsize and instead overflow/clip. Adjust the archived title span / row layout so truncation is guaranteed.
| sidebar: { | ||
| width: sidebarWidth, | ||
| resize: (width: number) => { | ||
| setSidebarWidth(width); | ||
| persist(); | ||
| }, |
There was a problem hiding this comment.
sidebar.resize currently calls persist() on every mousemove-driven resize update. Since localStorage.setItem is synchronous, this can noticeably impact drag performance. Consider updating the signal on every move but deferring persistence until drag end (you now have onDragEnd on ResizeHandle), or otherwise throttling persistence.
| const [sidebarWidth, setSidebarWidth] = createSignal( | ||
| initial.sidebar?.width ?? DEFAULT_SIDEBAR_WIDTH, | ||
| ); |
There was a problem hiding this comment.
Sidebar width is loaded from persisted state without validation. If opencode.layout contains an out-of-range or non-finite value, the sidebar can render far outside the intended 180–480px bounds. Consider clamping/sanitizing the initial sidebarWidth (and any subsequent resize inputs) to the allowed range before storing/using it.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| interface LayoutState { | ||
| review: PanelState; | ||
| info: PanelState; | ||
| sidebar?: { width?: number }; |
There was a problem hiding this comment.
LayoutState.sidebar is typed as optional (sidebar?: ...), but loadState() always returns a sidebar object and the provider relies on it (using non-null assertions). Making sidebar required in LayoutState (and in the parsed state normalization) would remove the need for initial.sidebar! and make the type reflect actual runtime behavior.
| sidebar?: { width?: number }; | |
| sidebar: { width?: number }; |
app-prefixable/src/pages/layout.tsx
Outdated
| <div | ||
| class="w-6 h-full" | ||
| style={{ | ||
| background: `linear-gradient(to right, transparent, var(--surface-inset))`, | ||
| }} | ||
| class="p-1 rounded transition-colors" | ||
| style={{ color: "var(--icon-weak)" }} | ||
| onMouseEnter={(e) => | ||
| (e.currentTarget.style.color = | ||
| "var(--icon-base)") | ||
| } | ||
| onMouseLeave={(e) => | ||
| (e.currentTarget.style.color = | ||
| "var(--icon-weak)") | ||
| } | ||
| title="Rename session" | ||
| aria-label="Rename session" | ||
| > | ||
| <Pencil class="w-3.5 h-3.5" /> | ||
| </button> | ||
| <button | ||
| onClick={(e) => { | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| archiveAndNavigate(session); | ||
| /> | ||
| <div | ||
| class="flex items-center gap-0.5 pr-1.5" | ||
| style={{ | ||
| "pointer-events": "auto", | ||
| background: "var(--surface-inset)", | ||
| }} |
There was a problem hiding this comment.
The action-icons overlay background/gradient is hard-coded to var(--surface-inset) for all sessions. Per the sidebar styling, inactive rows are on var(--background-stronger) (and can also show the overlay via group-focus-within without the hover background being applied), so this can look inconsistent and may not meet the “active uses surface-inset, otherwise background-stronger” requirement. Consider deriving the overlay background from isActive(session.id) (and/or ensuring the row background is set for focus-within as well) so the overlay matches the correct row/background state.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sidebar: { | ||
| width: sidebarWidth, | ||
| resize: (width: number) => { | ||
| setSidebarWidth(Math.max(SIDEBAR_MIN_WIDTH, Math.min(SIDEBAR_MAX_WIDTH, width))); |
There was a problem hiding this comment.
review.resize() and info.resize() both call persist() internally, but sidebar.resize() only updates the signal and requires callers to remember to call persist() separately. This inconsistency makes it easy to introduce future bugs where sidebar width changes aren’t saved (e.g., if another caller is added later). Consider persisting inside sidebar.resize() as well (or rename/split APIs so it’s explicit when persistence occurs).
| setSidebarWidth(Math.max(SIDEBAR_MIN_WIDTH, Math.min(SIDEBAR_MAX_WIDTH, width))); | |
| setSidebarWidth(Math.max(SIDEBAR_MIN_WIDTH, Math.min(SIDEBAR_MAX_WIDTH, width))); | |
| persist(); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const [sidebarWidth, setSidebarWidth] = createSignal( | ||
| Math.max(SIDEBAR_MIN_WIDTH, Math.min(SIDEBAR_MAX_WIDTH, | ||
| initial.sidebar.width ?? DEFAULT_SIDEBAR_WIDTH, | ||
| )), |
There was a problem hiding this comment.
The sidebar width clamping here can still produce NaN if initial.sidebar.width is non-numeric (e.g., corrupted localStorage). Math.min/max with NaN returns NaN, which later becomes an invalid CSS width. Consider validating with Number.isFinite(...) and falling back to DEFAULT_SIDEBAR_WIDTH before clamping.
| const [sidebarWidth, setSidebarWidth] = createSignal( | |
| Math.max(SIDEBAR_MIN_WIDTH, Math.min(SIDEBAR_MAX_WIDTH, | |
| initial.sidebar.width ?? DEFAULT_SIDEBAR_WIDTH, | |
| )), | |
| const initialSidebarWidth = | |
| typeof initial.sidebar.width === "number" && | |
| Number.isFinite(initial.sidebar.width) | |
| ? initial.sidebar.width | |
| : DEFAULT_SIDEBAR_WIDTH; | |
| const [sidebarWidth, setSidebarWidth] = createSignal( | |
| Math.max( | |
| SIDEBAR_MIN_WIDTH, | |
| Math.min(SIDEBAR_MAX_WIDTH, initialSidebarWidth), | |
| ), |
| size={layout.sidebar.width()} | ||
| min={180} | ||
| max={480} | ||
| onResize={(width) => { |
There was a problem hiding this comment.
The min/max sidebar widths are now defined in context/layout.tsx (SIDEBAR_MIN_WIDTH/SIDEBAR_MAX_WIDTH) but duplicated here as literals (180/480). This can drift over time and cause the ResizeHandle constraints to disagree with the persisted clamp. Consider reusing a single source of truth (e.g., exporting constants or exposing them on layout.sidebar).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Closes #35