refactor(app): merge sidebar pin and status slots (#150)#193
Conversation
Top-level rows now share a single 24px leading slot with priority running > permission > error > unseen > pin > empty, saving 28px on the title's left baseline. When a status is active the pin button is not rendered; users unpin via the row context menu or the ... dots menu.
Locks in the title's left baseline across idle / hover / pinned states and asserts the pin button sits inside the row's first 24px slot rather than a separate column.
📝 WalkthroughWalkthroughThis PR implements a unified left-side indicator slot for session rows. A new E2E test verifies that pinning a session merges the pin button into the status indicator slot while preserving title baseline stability. The Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/app/src/pages/layout/sidebar-items.tsx (1)
110-126:⚠️ Potential issue | 🟠 MajorMove the pin action outside the anchor element.
Nesting a button inside an
<a>element is invalid HTML and creates accessibility issues with screen readers and keyboard navigation. Theindicator()component (containing the pin button) should be rendered as a sibling of the link, not as its content.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/src/pages/layout/sidebar-items.tsx` around lines 110 - 126, The anchor element A currently contains the indicator() (which renders the pin button), causing invalid nested interactive controls; move the indicator() render out of the A so it becomes a sibling element before (or after) the A, preserve the styling container (the div with class "shrink-0 size-6...") and its color from props.tint(), and ensure the pin button’s event handlers remain bound to the indicator component while keeping the A’s href/navigation behavior and its onClick logic (props.warmPress, props.warmFocus, sidebarOpened(), clearHoverProjectSoon()) unchanged; update any layout or wrapping container to maintain spacing and accessibility (keyboard focus order) after moving indicator().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/app/e2e/sidebar/sidebar-leading-slot.spec.ts`:
- Around line 5-55: Add a second regression scenario that pins a session which
has an active status indicator (e.g., running or permission-needed) and then
asserts the same layout/keyboard affordances as the idle case: create or update
the test session to a running/permission state (using the existing
withSession/sdk helper or an SDK call to set session status), navigate with
gotoSession, pin the row (reuse row.locator('[data-action="session-row-menu"]')
and menu click), find the pinnedRow and its pinnedTitle, and assert the title
baseline and hover behavior (reuse leftOf), plus verify the unpin control
remains keyboard-reachable and in the leading slot (reuse pinButton selector and
the getBoundingClientRect checks); ensure the new assertions mirror the idle
pinned assertions so the spec fails if the active-status indicator displaces or
hides the unpin control.
In `@packages/app/src/pages/layout/sidebar-items.tsx`:
- Around line 102-107: The indicator function currently returns
props.leadingSlot only as a fallback, removing the pin control when isWorking(),
hasPermissions(), hasError(), or unseenCount() > 0 are true; change indicator
(and its callers if necessary) to always render props.leadingSlot in the DOM
(e.g., render it alongside the status indicator rather than as a fallback) so
the pin control remains keyboard-focusable and reachable; keep the visual status
elements returned when props.isWorking(), props.hasPermissions(),
props.hasError(), or props.unseenCount() > 0, but ensure those status elements
do not remove or replace props.leadingSlot (use aria-hidden on decorative
indicators or separate DOM nodes) so pinned rows retain a direct keyboard unpin
path.
---
Outside diff comments:
In `@packages/app/src/pages/layout/sidebar-items.tsx`:
- Around line 110-126: The anchor element A currently contains the indicator()
(which renders the pin button), causing invalid nested interactive controls;
move the indicator() render out of the A so it becomes a sibling element before
(or after) the A, preserve the styling container (the div with class "shrink-0
size-6...") and its color from props.tint(), and ensure the pin button’s event
handlers remain bound to the indicator component while keeping the A’s
href/navigation behavior and its onClick logic (props.warmPress,
props.warmFocus, sidebarOpened(), clearHoverProjectSoon()) unchanged; update any
layout or wrapping container to maintain spacing and accessibility (keyboard
focus order) after moving indicator().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 34cf931d-e215-4e23-8f4d-3b9f3b028484
📒 Files selected for processing (2)
packages/app/e2e/sidebar/sidebar-leading-slot.spec.tspackages/app/src/pages/layout/sidebar-items.tsx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
packages/app/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (packages/app/AGENTS.md)
Always prefer
createStoreover multiplecreateSignalcalls in SolidJS
Files:
packages/app/src/pages/layout/sidebar-items.tsxpackages/app/e2e/sidebar/sidebar-leading-slot.spec.ts
packages/app/e2e/**/*.spec.ts
📄 CodeRabbit inference engine (packages/app/e2e/AGENTS.md)
packages/app/e2e/**/*.spec.ts: Import test utilities from../fixturesinstead of@playwright/test
Test files should be named with the patternfeature-name.spec.ts
Use lowercase, descriptive test names (e.g., 'sidebar can be toggled')
Use camelCase for variable names in tests
Use SCREAMING_SNAKE_CASE for constants in tests
Use fixture-managed cleanup withwithSession(sdk, title, callback)for temporary sessions instead of callingsdk.session.delete(...)directly
Prefer theprojectfixture for tests that need a dedicated project with LLM mocking
Usedata-component,data-action, or semantic roles for selectors instead of CSS class names or IDs
UsemodKeyfrom utils for cross-platform keyboard shortcuts (Meta on Mac, Control on Linux/Windows)
In terminal tests, type through the browser usingrunTerminal()andwaitTerminalReady()instead of writing to the PTY through the SDK
Never use wall-clock waits likepage.waitForTimeout(...)to make a test pass
Wait on observable state withexpect(...),expect.poll(...), or existing helpers instead of assuming work is finished after an action
Use locator assertions liketoBeVisible(),toHaveCount(0), andtoHaveAttribute(...)for normal UI state verification
Useexpect.poll(...)for probing mock or backend state rather than transient DOM visibility
Prefer fluent helpers and drivers when they make intent obvious and reduce locator-heavy noise in tests
Use direct locators when the interaction is simple and a helper would not add clarity
When validating routing, assert against canonical or resolved workspace slugs using shared helpers from../actionsto account for Windows canonicalization
Test one feature per test file
Callproject.trackSession(sessionID, directory?)andproject.trackDirectory(directory)for any resources created outside the fixture so teardown can clean them up
Files:
packages/app/e2e/sidebar/sidebar-leading-slot.spec.ts
🧠 Learnings (6)
📚 Learning: 2026-04-23T07:23:23.849Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 180
File: packages/app/src/components/session/session-new-view.tsx:13-18
Timestamp: 2026-04-23T07:23:23.849Z
Learning: In pawwork (Astro-Han/pawwork), prefer using `createStore` instead of multiple `createSignal` calls only when the signals represent **coupled** object state that is updated together (i.e., there is at least one shared batch-update site where the state is changed in the same transaction). If the state fields are **independent** and are mutated by separate handlers (e.g., one handler updates only `selectedSkill` while another updates only `mode`), keep them as individual `createSignal` calls—using `createStore` for truly independent fields adds boilerplate without behavioral benefit.
Applied to files:
packages/app/src/pages/layout/sidebar-items.tsxpackages/app/e2e/sidebar/sidebar-leading-slot.spec.ts
📚 Learning: 2026-04-20T14:36:04.099Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.099Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Use lowercase, descriptive test names (e.g., 'sidebar can be toggled')
Applied to files:
packages/app/e2e/sidebar/sidebar-leading-slot.spec.ts
📚 Learning: 2026-04-20T14:36:04.099Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.099Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Test one feature per test file
Applied to files:
packages/app/e2e/sidebar/sidebar-leading-slot.spec.ts
📚 Learning: 2026-04-20T14:36:04.099Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.099Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Use locator assertions like `toBeVisible()`, `toHaveCount(0)`, and `toHaveAttribute(...)` for normal UI state verification
Applied to files:
packages/app/e2e/sidebar/sidebar-leading-slot.spec.ts
📚 Learning: 2026-04-22T08:49:44.563Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 126
File: packages/desktop-electron/src/main/index-sidecar-source.test.ts:3-11
Timestamp: 2026-04-22T08:49:44.563Z
Learning: In `packages/desktop-electron/src/main/index-sidecar-source.test.ts` (Astro-Han/pawwork), the test intentionally uses `expect(source).toContain` / `expect(source).not.toContain` string matching against the raw `index.ts` source text as a lightweight sidecar contract guard. The maintainer has explicitly chosen not to introduce an AST parser (e.g., `babel/parser` or acorn) for this purpose. Do not flag these string-based assertions as fragile or suggest converting them to AST-based matching.
Applied to files:
packages/app/e2e/sidebar/sidebar-leading-slot.spec.ts
📚 Learning: 2026-04-20T14:36:04.099Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.099Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Test files should be named with the pattern `feature-name.spec.ts`
Applied to files:
packages/app/e2e/sidebar/sidebar-leading-slot.spec.ts
There was a problem hiding this comment.
Code Review
This pull request introduces a new E2E test to verify that pinning a session in the sidebar does not cause horizontal layout shifts. It refactors the SessionRow and SessionItem components to merge the leadingSlot into the status indicator area, ensuring that elements like the pin button occupy the same space as status indicators rather than creating a separate column. I have no feedback to provide.
|
Addressing the "move the pin action outside the anchor" outside-diff note: The codebase already nests interactive elements inside Moving the leading slot out as a sibling of The HTML-spec concern is real (interactive inside interactive is technically not allowed). Modern browsers handle it, event propagation works, and accessibility trees tolerate it. If a real screen-reader regression surfaces, we'll revisit with the absolute-overlay approach. Not changing in this PR. |
Summary
Top-level session rows now share a single 24px leading slot instead of a pin-button column plus a separate status column. The slot priority is
running > permission > error > unseen > pin > empty. When a status is active, the pin button is not rendered in the DOM; users unpin via the row context menu or the...dots menu (both existing paths). Child sessions are unchanged.Closes #150. The right-side action slot still transitions
w-0 → w-6on hover, which shrinks the title's truncation area — tracked separately in #192.Measurement (Playwright, 4 states)
Title baseline shifts left by exactly 28px (24px freed slot + 4px gap) and stays stable across all four states, matching the spec in #150.
Design notes
The pin button is rendered as the indicator's fallback inside
SessionRow, not as a sibling column. This technically nests a<button>inside<A>, which the existingInlineEditoralready does when editing a title. The button'sstopPropagation()on click blocks anchor navigation.For the pinned + running (or permission / error / unseen) case, the pin button is hidden rather than stacked on top of the spinner. The issue body called out an "overlay" variant as a candidate but not a requirement; the context menu and dots menu both already offer unpin, so the simpler priority switch was chosen over absolute positioning.
Test plan
bun turbo typecheck)packages/app/src/pages/layout(34 pass)bun run test:e2e:local -- sidebar/, 7 pass, 3 pre-existing skips)sidebar-leading-slot.spec.ts— asserts title baseline is stable across idle / hover / pinned states and the pin button sits inside the row's first 24px slotbun dev:desktop: idle unpinned row is visually clean, hover reveals outline pin, pinned row shows filled pin, child sessions have no pin buttonSummary by CodeRabbit
Tests
Bug Fixes