mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-10-12 16:02:56 +00:00
fix: give a page without the band's chrome what the band was carrying
Second review round, three reachable defects plus one decision: - A public raw app was 150px tall. The bundle's iframe is `h-full`, and dropping its `min-h-screen` left nothing to resolve against on the share, embed and custom-path routes — none of which has a layout above them. Measured: 150px in a 900px viewport. The floor goes back for those routes and stays off inside the workspace, where the layout hands a height down and `min-h-screen` would overflow it by the band. - The band retracted out from under its own open menu on a deployed app. Last round taught the sidebar card to hold while the pointer is in a portaled menu and left its sibling alone, but the band opens menus too — the workspace disc, the fork family, every path level. One helper now, used by both closers. - `bandPeeked` outlived the page it belonged to. Clicking a control in the band navigates away, and the next page may not be one the band floats over, so the hover handlers that would clear it are gone — leaving the band already down when a full-bleed page came back. `beforeNavigate` clears it. - An embed has no band to carry a control, so the app's Edit floats over the canvas there, where it sat before the band existed. Registering it as the band's `afterName` under `navHidden` instead would have cost the app 44px of the iframe for a row holding one button. Reading "is the workspace navigation hidden" now has one definition (`isMenuHidden`), since a page has to answer it too: the flag is sticky inside an iframe, so the raw query param is the wrong thing to read. Also inset the band by the rail only when there is a rail — an embed reserved its width and crowded its own controls into a compact layout early. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
14712768cd
commit
3906774eba
@@ -160,12 +160,6 @@ The row's height matches the sidebar's own header row, so the two read as one ba
|
||||
actingWorkspaceId={content?.actingWorkspaceId}
|
||||
/>
|
||||
</div>
|
||||
{:else if content?.afterName}
|
||||
<!-- An embed has no trail, but what sits after the name is the page's own control — an
|
||||
app's Edit — and dropping it with the trail would take that with it. -->
|
||||
<div class="flex items-center min-w-0">
|
||||
{@render content.afterName()}
|
||||
</div>
|
||||
{/if}
|
||||
|
||||
{#if item && !navHidden && (item.summaryContent || item.summary)}
|
||||
|
||||
@@ -20,6 +20,7 @@
|
||||
import { getUserExt } from '$lib/user'
|
||||
import { Pen } from 'lucide-svelte'
|
||||
import { page } from '$app/state'
|
||||
import { isMenuHidden } from '$lib/components/sessions/sessionMode.svelte'
|
||||
|
||||
let {
|
||||
workspace,
|
||||
@@ -53,6 +54,11 @@
|
||||
const hideEditBtn = page.url.searchParams.get('hideEditBtn') === 'true'
|
||||
const hideRefreshBar = page.url.searchParams.get('hideRefreshBar') === 'true'
|
||||
|
||||
const showEdit = $derived(canWriteApp && !hideEditBtn)
|
||||
// Decides where Edit goes, not whether it appears: without the workspace navigation there is
|
||||
// no band to put it in. Same rule the layout hides the sidebar by.
|
||||
const menuHidden = $derived(isMenuHidden(page.url))
|
||||
|
||||
// Embedder side: mint a scoped embed token (by path) from the member's session.
|
||||
async function fetchEmbedToken(opts?: { sdkConsent?: boolean }): Promise<{ token?: string }> {
|
||||
const headers: Record<string, string> = {}
|
||||
@@ -150,17 +156,23 @@
|
||||
</div>
|
||||
|
||||
<!-- The band names the app for whoever opened it, write access or not: the route alone registers
|
||||
no item, and the breadcrumb would fall back to the section name "Apps".
|
||||
Edit is carried here too, rather than floating over the app's own canvas where it would land
|
||||
on whatever the app draws there. It sits with the name instead of at the far end of the bar:
|
||||
on a page whose header is only there while hovered, the far end is a journey across the
|
||||
window. -->
|
||||
no item, and the breadcrumb would fall back to the section name "Apps". -->
|
||||
<PageHeaderContent
|
||||
item={{ kind: 'app', path }}
|
||||
afterName={canWriteApp && !hideEditBtn ? editAction : undefined}
|
||||
afterName={showEdit && !menuHidden ? editAction : undefined}
|
||||
fullBleed
|
||||
/>
|
||||
|
||||
<!-- With a band, Edit sits with the app's name rather than at the far end of the bar: on a page
|
||||
whose header is only there while hovered, the far end is a journey across the window. An
|
||||
embed has no band — it would cost the app 44px of the iframe to carry one button — so Edit
|
||||
floats over the canvas there, which is where it sat before the band existed. -->
|
||||
{#if showEdit && menuHidden}
|
||||
<div class="absolute bottom-4 right-4 z-50">
|
||||
{@render editAction()}
|
||||
</div>
|
||||
{/if}
|
||||
|
||||
{#snippet editAction()}
|
||||
<Button
|
||||
unifiedSize="sm"
|
||||
|
||||
@@ -150,13 +150,18 @@
|
||||
{:else if app}
|
||||
{#key app}
|
||||
{#if app.raw_app && effectiveWorkspace}
|
||||
<RawAppPreview
|
||||
workspace={effectiveWorkspace}
|
||||
user={$userStore}
|
||||
secret={app.bundle_secret}
|
||||
path={app.path}
|
||||
runnables={(app.value?.runnables ?? {}) as Record<string, Runnable>}
|
||||
/>
|
||||
<!-- The bundle's iframe is `h-full`, so something has to give it a height to resolve
|
||||
against. In the workspace the layout hands one down; the share, embed and custom-path
|
||||
routes have nothing above them, and an unfloored iframe falls back to 150px. -->
|
||||
<div class={inWorkspace ? 'h-full w-full' : 'h-screen w-full'}>
|
||||
<RawAppPreview
|
||||
workspace={effectiveWorkspace}
|
||||
user={$userStore}
|
||||
secret={app.bundle_secret}
|
||||
path={app.path}
|
||||
runnables={(app.value?.runnables ?? {}) as Record<string, Runnable>}
|
||||
/>
|
||||
</div>
|
||||
{:else if app.raw_app && !effectiveWorkspace}
|
||||
<div class="px-4 mt-20">
|
||||
<Alert type="error" title="Configuration error">
|
||||
|
||||
@@ -47,6 +47,30 @@ export function withMenuHidden(url: string, workspaceId?: string): string {
|
||||
}
|
||||
}
|
||||
|
||||
// Remembers `nomenubar` for the rest of this iframe's life. Navigating inside a preview drops
|
||||
// the query param (client-side routing and full loads both), and the global nav must not pop
|
||||
// back in. sessionStorage so it survives a reload within the frame; the top window never writes
|
||||
// it, so an ordinary tab and the oauth callback still toggle normally.
|
||||
export function rememberMenuHidden(url: URL): void {
|
||||
if (typeof window === 'undefined' || window.self === window.top) return
|
||||
if (url.searchParams.get('nomenubar') !== 'true') return
|
||||
try {
|
||||
sessionStorage.setItem('nomenubar_embedded', 'true')
|
||||
} catch {}
|
||||
}
|
||||
|
||||
// Whether this page is shown without the workspace navigation. The layout drops the sidebar and
|
||||
// the band's trail, so a page whose only control lives in the band has to carry it itself — see
|
||||
// InWorkspaceAppViewer's Edit button. Read this rather than the raw query param: a preview tab
|
||||
// that has navigated no longer carries it.
|
||||
export function isMenuHidden(url: URL): boolean {
|
||||
return (
|
||||
url.searchParams.get('nomenubar') === 'true' ||
|
||||
url.pathname.startsWith('/oauth/callback/') ||
|
||||
isSessionPreviewFrame()
|
||||
)
|
||||
}
|
||||
|
||||
// True when this window is a sessions-preview iframe: embedded, with the `nomenubar` flag
|
||||
// the preview always sets and the logged layout stickies into sessionStorage.
|
||||
export function isSessionPreviewFrame(): boolean {
|
||||
|
||||
@@ -14,7 +14,11 @@
|
||||
} from '$lib/gen'
|
||||
import { capitalize, classNames, debounce, getModifierKey, sendUserToast } from '$lib/utils'
|
||||
import { useLocalStorageValue } from '$lib/svelte5Utils.svelte'
|
||||
import { isSessionPreviewFrame } from '$lib/components/sessions/sessionMode.svelte'
|
||||
import {
|
||||
isMenuHidden,
|
||||
isSessionPreviewFrame,
|
||||
rememberMenuHidden
|
||||
} from '$lib/components/sessions/sessionMode.svelte'
|
||||
import WorkspaceMenu from '$lib/components/sidebar/WorkspaceMenu.svelte'
|
||||
import SidebarContent from '$lib/components/sidebar/SidebarContent.svelte'
|
||||
import SettingsMenu from '$lib/components/sidebar/SettingsMenu.svelte'
|
||||
@@ -358,27 +362,10 @@
|
||||
$workspaceStore = queryWorkspace
|
||||
}
|
||||
|
||||
// When this window is an iframe (e.g. the sessions preview), keep the menu
|
||||
// hidden once `nomenubar` has been requested: navigating inside the preview
|
||||
// drops the query param (both client-side routing and full-document loads),
|
||||
// and we don't want the global nav to pop back in. Stickiness is stored in
|
||||
// sessionStorage so it survives full reloads within the iframe's browsing
|
||||
// context. The top window is unaffected (embedded is false there), so the
|
||||
// oauth-callback case and ordinary navigation still toggle normally.
|
||||
const embedded = typeof window !== 'undefined' && window.self !== window.top
|
||||
const requested = page.url.searchParams.get('nomenubar') === 'true'
|
||||
if (embedded && requested) {
|
||||
try {
|
||||
sessionStorage.setItem('nomenubar_embedded', 'true')
|
||||
} catch {}
|
||||
}
|
||||
let stickyEmbedded = false
|
||||
if (embedded) {
|
||||
try {
|
||||
stickyEmbedded = sessionStorage.getItem('nomenubar_embedded') === 'true'
|
||||
} catch {}
|
||||
}
|
||||
menuHidden = requested || page.url.pathname.startsWith('/oauth/callback/') || stickyEmbedded
|
||||
// Pages read the same rule (a page whose only control lives in the band has to carry it
|
||||
// itself where there is no band), so it is defined once in sessionMode.
|
||||
rememberMenuHidden(page.url)
|
||||
menuHidden = isMenuHidden(page.url)
|
||||
}
|
||||
|
||||
async function updateUserStore(workspace: string | undefined) {
|
||||
@@ -468,6 +455,10 @@
|
||||
} else {
|
||||
menuOpen = false
|
||||
}
|
||||
// The next page may not be one the band floats over, and then the band loses the hover
|
||||
// handlers that would clear this — leaving it already down when a full-bleed page comes
|
||||
// back. Clicking a control in the band (an app's Edit) is exactly that navigation.
|
||||
bandPeeked = false
|
||||
|
||||
// Inside a sessions-preview iframe, hand an editor-route navigation up to the
|
||||
// parent so it mounts the in-process editor (sharing the session runtime)
|
||||
@@ -527,13 +518,16 @@
|
||||
// and off the card both. The delay is what lets the pointer cross the gap between them: they
|
||||
// do not touch, and the leave of the one fires before the enter of the other.
|
||||
const PEEK_CLOSE_DELAY_MS = 200
|
||||
// Both the card and the band open menus, and a menu portals to `body` (overlayPortalTarget),
|
||||
// so the pointer moving into one leaves the surface that opened it. A surface has to outlive
|
||||
// its own menus or it slides out from under the one being read: hold while the pointer is in a
|
||||
// menu, and take the close from the leave of that instead.
|
||||
function pointerInMenu(): boolean {
|
||||
return !!document.querySelector('[role="menu"]:hover')
|
||||
}
|
||||
const { debounced: schedulePeekClose, clearDebounce: cancelPeekClose } = debounce(() => {
|
||||
if (!peeksOnHover) return
|
||||
// A menu opened inside the card portals to `body` (overlayPortalTarget), so the pointer
|
||||
// moving into it leaves the card. The card has to outlive the menus it opens, or it slides
|
||||
// out from under the one being read: hold while the pointer is in a menu, and take the
|
||||
// close from the leave of that instead.
|
||||
if (document.querySelector('[role="menu"]:hover')) {
|
||||
if (pointerInMenu()) {
|
||||
schedulePeekClose()
|
||||
return
|
||||
}
|
||||
@@ -564,7 +558,13 @@
|
||||
const floatBand = $derived(pageHeader.content?.barRightInset != null || bandPeek)
|
||||
// Same delay as the sidebar's peek, and for the same reason: the handle and the band do not
|
||||
// touch, so the leave of one has to survive long enough for the enter of the other.
|
||||
// The band carries menus of its own — the workspace disc, the fork family, every path level —
|
||||
// and they portal out of it just as the card's do, so it holds for the same reason.
|
||||
const { debounced: scheduleBandHide, clearDebounce: cancelBandHide } = debounce(() => {
|
||||
if (pointerInMenu()) {
|
||||
scheduleBandHide()
|
||||
return
|
||||
}
|
||||
bandPeeked = false
|
||||
}, PEEK_CLOSE_DELAY_MS)
|
||||
|
||||
@@ -1565,9 +1565,9 @@
|
||||
<!-- `menuHidden` hides the workspace navigation, not the page: an embedded Runs or detail
|
||||
page keeps its own controls, which now live in this band and nowhere else. The band
|
||||
renders without its breadcrumb there, so the embed gains no workspace nav. -->
|
||||
{#if !devOnly && (!menuHidden || pageHeader.actions.length > 0 || pageHeader.content?.afterName)}
|
||||
{#if !devOnly && (!menuHidden || pageHeader.actions.length > 0)}
|
||||
{@const rightInset = pageHeader.content?.barRightInset}
|
||||
{@const leftInset = useDrawer ? 0 : railWidth}
|
||||
{@const leftInset = useDrawer || menuHidden ? 0 : railWidth}
|
||||
<!-- One element for every placement, styled rather than branched: a page registers its
|
||||
inset on mount, and swapping between two `{#if}` arms would destroy the band and
|
||||
build another one a frame later — a blink of no header, with the content sliding
|
||||
|
||||
Reference in New Issue
Block a user