-
Notifications
You must be signed in to change notification settings - Fork 352
fix(desktop): prevent collapsed workbar flash #3794
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,24 +46,35 @@ const WorkbarSurface = lazy(() => | |
| })), | ||
| ); | ||
|
|
||
| function SessionWorkbarFallback() { | ||
| function SessionWorkbarFallback(props: { | ||
| hidden: boolean; | ||
| rightCollapsed: boolean; | ||
| bottomOpen: boolean; | ||
| }) { | ||
| const copy = getShellCopy(useUiLocale()).app; | ||
| if (props.hidden || (props.rightCollapsed && !props.bottomOpen)) return null; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P3] This re-derives a rule that already has an owner. const visible =
!props.hidden &&
(placement === 'right' ? !props.rightCollapsed : props.bottomOpen);and the fallback now expresses it a second time, in a different shape. The two agree today, which is why this is P3 — but a fallback drifting from the resolved surface is exactly the bug this PR is fixing, so leaving a second copy of the rule behind reopens the same seam. The surface also renders both cards always and hides the invisible one with |
||
| const placements: SessionWorkbarPlacement[] = []; | ||
| if (!props.rightCollapsed) placements.push('right'); | ||
| if (props.bottomOpen) placements.push('bottom'); | ||
| return ( | ||
| <div className="maka-workbar-workspace-contents"> | ||
| <Card | ||
| variant="transparent" | ||
| padding={0} | ||
| height="100%" | ||
| className="maka-session-workbar maka-session-workbar-frame" | ||
| data-placement="right" | ||
| role="status" | ||
| aria-busy="true" | ||
| aria-label={copy.loadingWorkbarLabel} | ||
| > | ||
| <div className="maka-lazy-fallback" data-surface="panel"> | ||
| <Spinner size="sm" shade="subtle" label={copy.loadingWorkbar} /> | ||
| </div> | ||
| </Card> | ||
| {placements.map((placement) => ( | ||
| <Card | ||
| key={placement} | ||
| variant="transparent" | ||
| padding={0} | ||
| height="100%" | ||
| className="maka-session-workbar maka-session-workbar-frame" | ||
| data-placement={placement} | ||
| role="status" | ||
| aria-busy="true" | ||
| aria-label={copy.loadingWorkbarLabel} | ||
| > | ||
| <div className="maka-lazy-fallback" data-surface="panel"> | ||
| <Spinner size="sm" shade="subtle" label={copy.loadingWorkbar} /> | ||
| </div> | ||
| </Card> | ||
| ))} | ||
| </div> | ||
| ); | ||
| } | ||
|
|
@@ -163,7 +174,15 @@ export function WorkbarHost({ model: props }: { model: WorkbarHostModel }) { | |
| )} | ||
| {props.activeId && ( | ||
| <div className="maka-workbar-layout-vars" style={style}> | ||
| <Suspense fallback={<SessionWorkbarFallback />}> | ||
| <Suspense | ||
| fallback={ | ||
| <SessionWorkbarFallback | ||
| hidden={props.hidden} | ||
| rightCollapsed={props.rightCollapsed} | ||
| bottomOpen={props.bottomOpen} | ||
| /> | ||
| } | ||
| > | ||
| <WorkbarSurface | ||
| key={props.activeId} | ||
| sessionId={props.activeId} | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The observer detects by sampling rather than by record:
inspect()re-queries the live DOM, so a card added and removed inside one mutation batch leaves records behind but nothing for the query to find. It caught the real regression — you verified it red onmain— so this is not a problem today, just the part that would quietly stop catching things. Reading the added nodes out ofrecordswould make the detection independent of how fast the flash is.