Skip to content

Folder detail: the Remove confirmation opens below the pane's edge #73

Closed

Description

@elementalvoid

What happens

In the folder detail, clicking Remove with a long body (uploaded assets listed) opens the "Remove “x” from Immich Sync?" confirmation below the card's bottom edge. The user sees the body dim and a sliver of the confirm box's top border, with no buttons. It only becomes usable after scrolling the detail card down.

Worse: focus moves to the confirmation while it is off screen (focus_trap + window.focus(&self.confirm_focus)), so a keyboard user presses Remove and gets focused-but-invisible controls.

Why

crates/app/src/folder_panel.rs:

  • The detail card is the scroll container: render() builds div().id("folder-panel") … .max_h(viewport.height - px(96.)).overflow_y_scroll().
  • The confirmation is appended to the end of the scrollable body (detail_body → body.child(self.confirmation(...))), i.e. after the Remove button that triggered it, which is already the last row of a tall body.
  • ask_remove() only sets confirming = true and moves focus; nothing scrolls it into view.

detail_body's own comment says the confirmation is "not body content" — the layout does not honor that.

Why the tests missed it

crates/app/tests/folder_detail.rs defines scroll_into_view(...) and calls it before touching folder-detail-remove-confirm in every test that uses it. The harness scrolls for the user, so the suite stayed green on a bug the reporter hit immediately.

Agreed fix

Replace the inline confirmation with the gpui-component AlertDialog (already available as gpui_kit::component::dialog::AlertDialog; gpui_kit 0.7.2 re-exports gpui-component 0.7.2, and shell.rs already opens windows through gpui_kit::open_window, which wraps in Root). Style it after the docs' "Delete Account" example:

  • DialogHeader / DialogTitle = Remove “{folder}” from Immich Sync?
  • DialogDescription = the existing "Its uploaded assets stay on the server…" copy
  • DialogFooter with DialogClose → Cancel, DialogAction → Remove folder (ButtonVariant::Danger), both flex_1

Open it imperatively from ask_remove via window.open_alert_dialog, keeping the same test ids (folder-detail-remove-confirm, folder-detail-remove-cancel).

Decisions already made

  • Keep the panel body dimming while the alert is open: confirming stays as the dim flag, .opacity(0.4) wrapper stays.
  • Delete the inline machinery: confirmation(), confirm_focus, and the Folder detail: open a source folder's full view from its card #43 structural assertions about the confirmation living inside the dialog body. Focus handling and Escape belong to the dialog (focus trap + restore to the Remove button on close; keyboard(true) makes Escape cancel).
  • escape() collapses back to plain dismiss(), because Escape reaches the dialog, not the panel host.
  • The alert's builder may re-run when the dialog layer is rebuilt, so capture the folder name and a WeakEntity<FolderPanel> before opening and keep the closure cheap.

Regression test

The fix is only done when the confirm is reachable without any test-side scrolling: after clicking Remove, render_frame, then assert the alert's confirm button exists and its bounds are inside the window's reachable area (no scroll_into_view call), plus Cancel/Escape keep the folder, Remove drops it, and a refused removal keeps the detail open with its reason.

Rejected alternative

Pinning the confirmation as a footer below a scrolling body inside the card. It also fixes the visibility, but it keeps a hand-rolled confirmation next to a component that exists for exactly this, and it leaves the "answers are appended to the body" trap in place.

Activity

  1. elementalvoid commented on Oct 28, 2026

    @elementalvoid
    OwnerAuthor

    Fixed on main in bdd1c7f and verified on an arm64 DMG.

    The Remove confirmation is now the gpui-component AlertDialog — a window-level layer, so it opens on screen whatever the detail body has scrolled to, instead of being appended below the fold. The dialog's own footer carries Cancel and Remove folder, which also suppresses the alert's default OK/Cancel row (the first cut shipped four buttons). Focus trapping, Escape, and returning focus to Remove come from the dialog layer; the panel body still dims behind it, and the dashboard closes the alert with the panel.

    The tests that had been calling scroll_into_view before clicking the confirm no longer do: they now assert the confirm is on screen with no scrolling, that it is its own layer rather than panel content, that Tab reaches its own controls, and that it renders one pair of buttons.

    Follow-up in the tracker: release-please PR #84 bumps this to 0.2.1.

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingready-for-agentFully specified, ready for an AFK agent

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions