Skip to content

Sidebar peek - #4355

Open
hzub wants to merge 4 commits into
mainfrom
OMNI-2342-sidebar-peek
Open

Sidebar peek#4355
hzub wants to merge 4 commits into
mainfrom
OMNI-2342-sidebar-peek

Conversation

@hzub

@hzub hzub commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Related issue

Closes #

Summary

Adds a hover-to-peek affordance to the collapsed sidebar. With the sidebar
closed, dwelling for 1s on the header's panel-toggle button floats the sidebar
in as a rounded, ringed card overlaying the content (rather than pushing main
aside like a real open). Leaving the card dismisses it after a short grace
period; a toggle inside the peeking card pins it fully open, and clicking the
header button opens it directly.

  • Peek is a desktop hover affordance only — on mobile the toggle keeps
    opening the full-screen overlay, so a tap's synthetic pointerenter never
    triggers a peek.
  • The peek card floats a few px off the viewport edge, is width-capped, ringed
    (ring-1 ring-border), shadowed, and animates in (fade + slide-from-left).
    Resize is disabled while peeking — the card is a fixed-width flyout, not a
    resizable panel.
  • Open/peek timers are cancellable: leaving the button before 1s cancels the
    pending peek; re-entering the card during the close grace period cancels the
    dismiss. Both timers clear on unmount.

Test Plan

  • npm run type-check — clean
  • npm run lint — clean
  • npx vitest run src/shell/Sidebar — 210/210 pass
  • npx vitest run src/shell/ChatHeader — 12/12 pass
  • Manual (desktop): hover the collapsed sidebar's toggle ~1s → card peeks in
    off the edge, no resize handle; move away before 1s → nothing opens; leave the
    card → dismisses after the grace period; jitter back onto it within that
    window → stays; in-card toggle pins it fully open.
  • Manual (narrow / <768px viewport): tapping the toggle opens the full overlay,
    never peeks.

Demo

Screen.Recording.2026-08-07.at.16.42.34.mov

Type of change

  • Bug fix
  • Feature
  • UI / frontend change
  • Refactor / chore
  • Docs
  • Test / CI
  • Breaking change

Test coverage

  • Unit tests added / updated
  • Integration tests added / updated
  • E2E tests added / updated
  • Manual verification completed
  • Existing tests cover this change

Coverage notes

The peek behaviour is hover/timer-driven layout on the existing Sidebar and
ChatHeader; existing suites (210 Sidebar + 12 ChatHeader) cover the components
and pass with the new optional props. The hover-dwell, grace-period dismiss, and
mobile gating were verified manually per the Test Plan; no new automated timer
tests were added.

Changelog

Hover the collapsed sidebar toggle to peek the conversation list without pinning it open.

@github-actions github-actions Bot added the size/M Pull request size: M label Aug 7, 2026
@omnigent-ci

omnigent-ci Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Polly AI Review

Sidebar peek — Review

Blocking issues

  1. Required onOpen prop breaks type-check and existing tests. SidebarProps.onOpen is declared non-optional (onOpen: () => void; at Sidebar.tsx:251), but ~20 existing test render sites mount <Sidebar open onClose={vi.fn()} /> without it — e.g. Sidebar.test.tsx:218,585,1269,1633, Sidebar.rowActions.test.tsx:210, Sidebar.shiftSelect.test.tsx:173, Sidebar.archive.test.tsx:111, Sidebar.delete.test.tsx:108,271, and more. Since web/tsconfig.app.json includes all of src, npm run type-check (and the test suite's TS compile) will fail. Either make onOpen optional (defaulting to a no-op, matching how onOpenSearch is handled) or update every call site.

  2. sidebarOpen/sidebarPeek can go inconsistent via unwrapped global toggles. The new peek-aware paths correctly clear peek (onOpenAppShell.tsx:1401, header full-open→1446, onClose1400). But two existing global toggles were not updated: onToggleLeft: () => setSidebarOpen((prev) => !prev) (AppShell.tsx:1012) and onToggleLeftSidebar: () => setSidebarOpen((prev) => !prev) (1696). If either the sidebar hotkey or command-palette toggle fires while peeking, sidebarPeek stays true while sidebarOpen also becomes true, leaving the panel styled as a floating peek card (is-peek md:absolute md:inset-2 …) while the rest of the layout treats it as a fully-open pushing panel. These toggles should clear sidebarPeek (and ideally treat "open or peek" as the current-open state when computing the next value).

Security vulnerabilities

None found. The change is client-side layout/UI state only — no new network, deserialization, or auth surface.

Non-blocking notes

  • Leftover dead code: commented // hidden: peek, inside the cn() object at Sidebar.tsx:756 — either wire it up or remove it.
  • Comment/behavior mismatches: the ChatHeader comment says "Dwell on the toggle for 1s to peek," but the timer is 400ms (ChatHeader.tsx:188/205). The Sidebar comment says the card floats "4px off the viewport edge (capped at 300px wide)," but the code uses md:inset-2 (~8px) and md:max-w-[400px] (Sidebar.tsx:714/718). Align comments to the real values.
  • Accessibility: effectiveOpen now includes peek (Sidebar.tsx:652), so a hover-triggered peek makes the sidebar tabbable and visible to assistive tech (drives aria-hidden/inert at 689/693). Reasonable for an interactive flyout, but it's a hover-driven AT behavior change worth a manual pass (focus trapping/return, screen-reader announcement on an unrequested reveal).
  • Timer cleanup otherwise looks sound: the header peek timer cancels on click/leave/unmount, and the sidebar close timer is guarded by peek and cleaned up on unmount. No leak or stale-closure bug found on the peek-false path.

Summary

Solid, well-commented UI feature, but not mergeable as-is. Two real blockers: the newly-required onOpen prop will fail type-check against many existing <Sidebar> test call sites, and two un-updated global sidebar toggles (AppShell.tsx:1012 and 1696) can leave sidebarOpen and sidebarPeek simultaneously true, producing a broken hybrid layout. Fix the prop optionality/call sites and route the remaining toggles through peek-clearing logic, then clean up the dead comment and comment/value mismatches. No security concerns.


Automated review by Polly · workflow run

@hzub

hzub commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author

/review

@omnigent-ci

omnigent-ci Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Polly AI Review

Blocking issues

None. The peek state machine is coherent: effectiveOpen now includes peek so the panel isn't inert/aria-hidden while floating; both timers (peek-open in ChatHeader, peek-close in Sidebar) are cancellable and cleared on unmount via useEffect(() => cancelX, [cancelX]); mobile is gated by an early return in onPeekSidebar; and the resize handle is correctly suppressed with !peek. The onOpenSidebar signature widening (() => void(peek?: boolean) => void) and the new optional Sidebar props (onOpen, peek) are backward-compatible, so existing suites keep passing.

Security vulnerabilities

None. This is purely client-side hover/timer-driven layout — no injection surface, no auth/boundary change, no dependency or lockfile changes.

Non-blocking notes

  • Leftover dead code. The header row's className was changed to cn("...", { /* hidden: peek, */ }) — the object is empty (the only key is commented out), so cn gets a no-op arg. Either implement the intended "hide header while peeking" behavior or drop the object and revert to the plain string.
  • New comments contradict the code (and PR description). The added ChatHeader comment says "Dwell on the toggle for 1s to peek" but the timeout is setTimeout(..., 400) (400ms). The Sidebar peek comment says the card floats "4px off the viewport edge (capped at 300px wide)" but the code uses md:inset-2 (8px) and md:max-w-[400px]. These misleading comments will confuse future readers; align them (or the values) with reality.
  • Dismissal edge case. Once the peek fires, the header trigger button unmounts (it's hidden when sidebarOpen || sidebarPeek is true), so dismissal relies solely on the floating card's onPointerLeave. If the pointer moves away without ever entering the card, onPointerLeave never fires and the peek can persist until the card is hovered-then-left. In practice the card slides in over/near the trigger so the pointer usually lands on it, but a fast pointer path could leave it stuck open.
  • Coverage. Consistent with the PR's own note, no automated tests exercise the hover-dwell, grace-period dismiss, or mobile gating — the core new logic is verified manually only. A couple of fake-timer tests would lock in the cancel/dismiss behavior cheaply.

Summary

A clean, well-scoped desktop-only UI feature with no blocking or security concerns; the timer lifecycle and mobile gating are handled correctly and existing tests remain green. The only cleanups worth making before merge are removing the commented-out hidden: peek dead code and fixing the newly-added comments whose stated values (1s dwell, 4px offset, 300px cap) don't match the implemented 400ms / 8px / 400px. Optional fake-timer tests would be a worthwhile follow-up given the logic is currently manual-verified only.


Automated review by Polly · workflow run

@hzub

hzub commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author

/review

@github-actions github-actions Bot added size/L Pull request size: L and removed size/M Pull request size: M labels Aug 7, 2026
@omnigent-ci

omnigent-ci Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Polly AI Review

Review: Sidebar peek

1. Blocking issues

None. The state machine is sound: toggleLeftSidebar collapses open || peek and always clears peek, so the shell never lands in the illegal "open and peek both true" state; onClose/onOpen/onOpenSidebar(peek?) all reset the pair consistently, and both hover timers are cleared on unmount. No correctness bug, broken contract, or data-loss path in the diff.

2. Security vulnerabilities

None. Pure client-side UI/layout change — no auth boundary, deserialization, path, or network surface touched. No lockfile or dependency/extras changes.

3. Non-blocking notes

  • Dwell delay contradicts the stated behavior. The code uses setTimeout(..., 400), but the adjacent comment ("Dwell on the toggle for 1s"), the PR summary ("dwelling for 1s"), and the Test Plan ("~1s") all claim 1000ms. Either the constant or the docs/comment is wrong — reconcile so the advertised affordance matches reality.
  • Width cap comment mismatch. The peek card comment says "capped at 300px wide" but the class is md:max-w-[400px]. Harmless, but the comment is misleading.
  • Peek can get stuck open. Auto-dismiss is only scheduled by the <aside>'s onPointerLeave, which requires the pointer to have entered the card first. After the 400ms dwell fires, the card appears at md:inset-2 (top-left) while the pointer is still over the header toggle. If the user moves the pointer away without ever crossing into the card, no close timer is armed and the peek lingers until they enter+leave the card or toggle. Worth confirming this matches the intended dismiss model.
  • Dead commented code. The header cn(..., { // hidden: peek }) reduces to cn(base, {}) — leftover scaffolding; drop it or wire it up.
  • Coverage gap acknowledged but real. The new hover-dwell, grace-period dismiss, and mobile gating (useIsMobileViewport) are timer/pointer logic with several edge cases (cancel-before-dwell, re-enter-during-grace, mobile tap suppression) and have no automated tests. Existing suites pass but don't exercise the new paths. A couple of fake-timer tests would lock in the intended behavior cheaply.

4. Summary

A well-scoped, self-contained frontend feature with careful state handling and thorough manual verification. No blocking or security concerns. The only items worth addressing before merge are cosmetic-but-confusing doc/comment mismatches (the 400ms-vs-"1s" dwell being the most notable) and the "peek never dismisses if the card is never entered" edge case; adding a few timer-based tests would harden the new hover logic. Approve pending those clarifications.


Automated review by Polly · workflow run

hzub added 4 commits August 7, 2026 14:58
Signed-off-by: Hubert Zub <hubert.zub@gmail.com>
The peek work made onOpen a required prop, but the Sidebar.*.test.tsx
harnesses don't pass it, breaking the typecheck. Mirror the onOpenSearch
convention: optional with a no-op default.

Co-authored-by: Isaac
Signed-off-by: Hubert Zub <hubert.zub@gmail.com>
Signed-off-by: Hubert Zub <hubert.zub@gmail.com>
Signed-off-by: Hubert Zub <hubert.zub@gmail.com>
@hzub
hzub force-pushed the OMNI-2342-sidebar-peek branch from a56ba88 to f3aab6e Compare August 7, 2026 14:58
@github-actions github-actions Bot added size/M Pull request size: M and removed size/L Pull request size: L labels Aug 7, 2026
@hzub hzub added the ui-preview label Aug 7, 2026
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

UI Preview is ready for this PR 🚀

URL https://omnigent-ui-preview-pr-4355-3272836215725701.aws.databricksapps.com
Commit f3aab6e
Run https://github.com/omnigent-ai/omnigent/actions/runs/31191036461

Note

This preview is only accessible to maintainers with workspace access.
It serves the UI only -- connect your own host (omnigent run … --server <url>) to drive a real session.
The preview updates automatically when new commits are pushed.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant