Skip to content

fix(studio): stop the grouping dialog opening off the bottom of the window - #3421

Merged
vanceingalls merged 1 commit into
mainfrom
fix/group-dialog-offscreen-v2
Aug 23, 2026
Merged

fix(studio): stop the grouping dialog opening off the bottom of the window#3421
vanceingalls merged 1 commit into
mainfrom
fix/group-dialog-offscreen-v2

Conversation

@vanceingalls

Copy link
Copy Markdown
Collaborator

Supersedes #3417, which was written on top of #3413#3415 and went stale when those merged. Rebuilt on current main and — more importantly — rebuilt on main's own resolveFloatingPanelPosition rather than the second, parallel helper I had written. Please close #3417 in favour of this.

The bug

Reported as "the grouping button didn't work — I clicked it and nothing happened."

The dialog was opening. It positioned itself raw:

style={{ position: "fixed", left: anchorRect.left, top: anchorRect.bottom + 4 }}

No flip, no clamp. This button lives in a track header at the bottom of the studio window, so anchorRect.bottom + 4 puts the dialog past the viewport edge. It was the last floating surface in the timeline with no viewport handling at all — the FX popover beside it got its cap in #3413, and every portal got its tier in #3414.

The fix

It now goes through resolveFloatingPanelPosition — the helper RenderQueue and propertyPanelColor already position with — so it flips above the anchor when there is no room below and clamps so neither edge leaves the viewport.

GROUP_DIALOG_SIZE is a declared estimate in the same style as FORMAT_PANEL_SIZE and COLOR_PICKER_SIZE: w-56 is exact, only the flip decision reads the height, and the clamp keeps the dialog on screen either way.

Measured in the test: an anchor at top: 760 in a 768px-tall window previously produced top: 780px — 12px below the bottom edge. It now lands at top: 644px, fully inside the viewport (644 + 112 = 756, against a 756 limit).

Why it shipped

The existing group-pointer test passes with or without the fix. happy-dom reports an all-zero rect for an unlaid-out button, so the dialog landed at top: 4px — on screen, and nothing like the real app. A geometry test that never sets a geometry proves nothing.

The two new tests set a realistic anchor: one at the bottom edge, one hard against the right edge. Both verified to fail against the raw positioning and pass with the helper.

Deliberately not included

A toast for the grouping write's silent elements.length < 2 bail. The path is real in code — the button renders on clipCount > 1 while the write bails on resolved count — but I could not reach it from the UI: the offer only appears on a track with 2+ ungrouped clips, and sub-composition audio arrives as separate single-clip rows, so the button never renders there. Adding a message for a branch I cannot demonstrate, plus the file split it would force to stay under the 600-line studio cap, is not justified by the evidence I have.

Also worth recording from chasing this: I reproduced the environment on both the dev build and published 0.8.8 and could not reproduce a failing write. Clicking the real button groups the clips and updates the timeline. My first "reproduction" was my own test error — a text-match click that hit the paragraph rather than the button.

Checks

tsc --noEmit clean · full studio suite 4380 passed / 0 failed · oxlint 0/0 · oxfmt --check clean · fallow audit --base origin/main no issues (its one duplication finding is inherited) · both touched files well under the 600-line limit.

🤖 Generated with Claude Code

…indow

Reported as "the grouping button did nothing — I clicked it and nothing
happened". The dialog WAS opening. It positioned itself at
`anchorRect.bottom + 4` with no flip and no clamp, and this button lives in a
track header at the bottom of the studio window, so it opened past the viewport
edge. It was the last floating surface in the timeline with no viewport handling
at all.

It now goes through `resolveFloatingPanelPosition`, the helper the other body
portals already position with (`RenderQueue`, `propertyPanelColor`), so it flips
above the anchor when there is no room below and clamps so neither edge leaves
the viewport. `GROUP_DIALOG_SIZE` is a declared estimate in the same style as
`FORMAT_PANEL_SIZE` and `COLOR_PICKER_SIZE`: `w-56` is exact, only the flip
decision reads the height, and the clamp keeps the dialog on screen either way.

Two tests, at a realistic bottom-of-window anchor and hard against the right
edge. Both verified to fail against the raw positioning.

Worth noting why this shipped: the existing `group-pointer` test passes with or
without the fix. happy-dom reports an all-zero rect for an unlaid-out button, so
the dialog landed at top:4 — on screen, and nothing like the real app. A geometry
test that never sets a geometry proves nothing.

Deliberately NOT included: a toast for the grouping write's silent
`elements.length < 2` bail. That path is real in code but I could not reach it
from the UI — the button only renders on a track with 2+ ungrouped clips, and
sub-composition audio arrives as separate single-clip rows, so the offer never
appears there. Adding a message for an unreachable branch, plus the file split it
would force to stay under the 600-line studio cap, is not justified by evidence.

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exact-head review at 6babfb2019879f4448b89d2cdfaafaa7031b6a91.

The fix reuses the existing viewport contract instead of creating a second positioning path: TimelineFxButton.tsx:21-37 delegates centering, flip, and clamp to resolveFloatingPanelPosition, and :97-106 applies only the returned fixed coordinates to the portal. The new geometry witnesses at TimelineFxButton.test.tsx:77-99 set real bottom/right-edge rectangles, so both fail against the old raw positioning instead of passing on happy-dom zero geometry.

I re-ran the helper against the two test rectangles plus the missing left-edge case; outputs are {198,644,top}, {788,120,bottom}, and {12,24,bottom} respectively. Targeted oxlint and oxfmt are clean. All exact-head CI contexts are settled with no failures, including Test, Typecheck, both Windows jobs, preview parity, and CodeQL. The branch is five commits behind current main, but none touches the component, its tests, or the shared helper.

No blocker or important finding.

Verdict: APPROVE
Reasoning: The grouping dialog now uses the established viewport-aware positioning boundary, and realistic geometry tests pin both failure axes that made the control appear inert.

— Magi

@vanceingalls
vanceingalls merged commit dd0626a into main Aug 23, 2026
45 checks passed
@vanceingalls
vanceingalls deleted the fix/group-dialog-offscreen-v2 branch August 23, 2026 00:04
vanceingalls added a commit that referenced this pull request Aug 23, 2026
Twelve of the stack's feature commits landed on main as squashes (#3274
through #3292, plus #3401's canary removal); the 96 review-and-fix commits
that followed them here did not, and main moved 64 commits on in the
meantime. This reconciles the two.

58 files conflicted. 44 were audio-only — main's side there is the
squashed form of commits this branch already carries and has since
superseded, so the branch side stands. The rest needed real work, in both
directions:

**Taken from main, absent here.**
- `ensureAudioGroupInertStyle` (#3278's review). An `<hf-audio-group>` is
  an unknown custom element, so it still takes a flex/grid slot and can
  open a line box — adding a group shifted authored layout. The helper and
  its `init.ts` call never came back to the branch, and this branch is
  what emits the element.
- `#3383`'s ended-audio replay: `canSeekEndedMediaBackward` and its five
  siblings in `media.ts`, with all six tests. Not present here in any
  form.
- `#3380`'s `asetpts=N/SR/TB` between `apad` and `atrim`. Also applied to
  `mixGroupMembers`, the group submix, which is new on this branch and so
  had the same bug in a path main's fix could not reach: delayed members
  padded then amix'd, where a group of four or more silently loses one.
- `#3401`'s `displayNumber` thread. The header derives its row from the
  group-aware order and the undo label from ascending element keys, so
  once a group exists the same click said "Hide track 2" and recorded
  "Hide track 1".
- `#3413`/`#3421`'s viewport handling — the popover's height cap and
  `inset()`, and `resolveFloatingPanelPosition` for the grouping dialog,
  which lives in a track header at the bottom of the window.
- Two extractions this branch had inline and at exactly the 600-line cap:
  `useTimelineDeleteOps` and `editingModeSlice`. Bodies were identical.

**Kept from the branch, against main.** Mute and solo are gone by
deliberate breaking change (`remove mute and solo from tracks and groups`,
`remove the group volume slider and level meter`), so eight files main
still carries are deleted again, `PlayerControls` keeps no
`previewIframeRef` (it existed only to feed `SoloBanner`), the
group-levels branch comes out of main's new `previewMessageRouter`, and
`STRIP_H` goes with the bus strip it sized. Main's
`TimelineTrackPlainHeader.test.tsx` is rewritten against the control that
actually exists — the visibility eye, withheld from an audible audio row
and offered back once hidden, which is the only way out of `data-hidden`.

**Unioned.** `TimelineFxPopover` — main's positioning, this branch's
audition telemetry (`auditionPresetChain`, `storedChain`,
`onAuditionTracked`); `SKILL.md` — main's #3416 "keep the carve group a
voice group" beside this branch's bus section, with the canary paragraph
dropped since the canaries no longer exist.

Every port is mutation-checked. core 2508, studio 4460, lint 528, engine
1630, cli 2813, sdk 549, producer green; tsc, oxlint, oxfmt, fallow and
the 600-line cap clean.
@miguel-heygen miguel-heygen mentioned this pull request Aug 23, 2026
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants