Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions src/main/popups/titleCoachmark.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,36 @@ describe('positionCoachmark beak tracking', () => {
return placement.x + COACHMARK_SHADOW_GUTTER + placement.beakFraction * cardWidth
}

it.each([
[200, { leftX: 500, rightX: 540 }],
[280, { leftX: 500, rightX: 540 }],
[280, { leftX: 1130, rightX: 1150 }]
])(
'reports a card centre that lands on the anchor (card %s, clamped or not)',
(width, anchor) => {
// Asserted against the ANCHOR, not against the formula. An earlier version of this test
// compared `cardLeftInView` to the gutter and re-used the same width it fed in, so both
// sides reduced to the same constant and it could not fail for any input. This varies
// the card width and the anchor — including one that clamps at the right edge — and
// checks the property that actually matters.
const placement = positionCoachmark({
anchor: { ...anchor, bottomY: 36 },
bubble: { width, height: 90 },
parentBounds: { width: 1200, height: 800 }
})
const anchorCentre = (anchor.leftX + anchor.rightX) / 2
const cardCentreInWindow = placement.x + placement.cardCentreInView
const clamped = placement.x <= 0 || placement.x + placement.width >= 1200
if (clamped) {
// Clamped, the card cannot sit on the anchor — but it must still be centred in its
// own view, which is what the beak fraction is then measured against.
expect(placement.cardCentreInView).toBeCloseTo(placement.width / 2, 5)
} else {
expect(cardCentreInWindow).toBeCloseTo(anchorCentre, 5)
}
}
)

it('centres the beak when the card is not clamped', () => {
const placement = positionCoachmark({
anchor: { leftX: 500, rightX: 540, bottomY: 36 },
Expand Down
26 changes: 23 additions & 3 deletions src/main/popups/titleCoachmark.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import type { CoachmarkBeakPayload } from '../../types/ipc'
import { ipcMain } from 'electron'
import type { BrowserWindow, WebContents } from 'electron'
import { TITLEBAR_HEIGHT } from '../lib/titleBarOverlay'
Expand Down Expand Up @@ -91,6 +92,11 @@ export interface CoachmarkPlacement {
width: number
height: number
beakFraction: number
/** Where the card's midpoint belongs within the view, in CSS px. Sent so placement comes
* from the geometry main already computed rather than from the page's own idea of its
* width — and as a centre rather than an edge, so it holds whatever width the card
* actually renders at. */
cardCentreInView: number
}

/** Compute popup bounds centering the card under the anchor, clamped to the parent, plus where
Expand Down Expand Up @@ -130,7 +136,20 @@ export function positionCoachmark(opts: {
// the beak to the very corner the margin exists to keep it off.
const beakMargin = Math.min(0.5, COACHMARK_BEAK_EDGE_MARGIN / cardWidth)
const beakFraction = Math.min(1 - beakMargin, Math.max(beakMargin, rawFraction))
return { x, y, width: viewWidth, height: viewHeight, beakFraction }
// The card's CENTRE inside the view, which is the view's own midpoint.
//
// Sent because the renderer cannot derive it safely. Centring with auto margins measures the
// page's own width, and that width is sometimes still the pre-resize value — the page had
// not processed the new bounds yet. Measured at 8px off, on a page still reporting 300
// inside a 316-wide view.
//
// The CENTRE rather than the left edge, deliberately: a left offset is only correct while
// the card renders exactly as wide as `bubble.width` said it would, so it would trade a
// stale-viewport failure for a stale-width one — including on the fallback show, where the
// view is sized before any measurement exists. Pinning the centre is right for any rendered
// width, because the renderer offsets by half of whatever the card actually is.
const cardCentreInView = viewWidth / 2
return { x, y, width: viewWidth, height: viewHeight, beakFraction, cardCentreInView }
}

let _coachmarkTokenSeq = 0
Expand Down Expand Up @@ -255,14 +274,15 @@ function repositionAndShow(
): void {
if (!entry.pendingAnchor || entry.view.isDestroyed()) return
const parentBounds = entry.view.parentWindow.getContentBounds()
const { beakFraction, ...bounds } = positionCoachmark({
const { beakFraction, cardCentreInView, ...bounds } = positionCoachmark({
anchor: entry.pendingAnchor,
bubble,
parentBounds
})
entry.view.popup.setBounds(bounds)
// Tell the card where to draw its beak now that the final, possibly clamped, x is known.
entry.view.popup.webContents.send('comfy-titletooltip:set-beak', { beakFraction })
const beakPayload: CoachmarkBeakPayload = { beakFraction, cardCentreInView }
entry.view.popup.webContents.send('comfy-titletooltip:set-beak', beakPayload)
// Focus so the dismiss button is keyboard-reachable.
entry.view.showOnTop({ focus: true })

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Low — The placement is pushed over async IPC while the view is shown in the same turn, so the first painted frame renders before the renderer applies cardLeftInView — and on the first show cmCardLeft is still null, so that frame uses exactly the stale-width CSS centring this change exists to avoid, then jumps one gutter. Showing only after the renderer acknowledges the beak push, or seeding the offset in the config push, would avoid the visible correction. Raised by 1 of 8 reviewers (claude-opus-5-thinking-max adversarial).

}
Expand Down
19 changes: 16 additions & 3 deletions src/preload/comfyTitleTooltipPreload.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import type { CoachmarkBeakPayload } from '../types/ipc'
import { contextBridge, ipcRenderer } from 'electron'
import type { IpcRendererEvent } from 'electron'

Expand Down Expand Up @@ -33,7 +34,7 @@ export interface ComfyTitleTooltipBridge {
onConfig(cb: (config: TitleTooltipConfig) => void): () => void
/** Beak position, pushed after main has measured the card and settled its final (possibly
* clamped) bounds. Separate from the config push because it is only knowable then. */
onBeak(cb: (payload: { beakFraction: number }) => void): () => void
onBeak(cb: (payload: CoachmarkBeakPayload) => void): () => void
/** Coachmark dismiss button; no-op for the tooltip variant. `configToken` names the card
* the click landed on, so main can discard a click from a card it has since replaced. */
dismissCoachmark(configToken: string): void
Expand Down Expand Up @@ -74,8 +75,20 @@ const bridge: ComfyTitleTooltipBridge = {
},
onBeak: (cb) => {
const handler = (_event: IpcRendererEvent, data: unknown): void => {
const raw = (data as { beakFraction?: unknown } | undefined)?.beakFraction
if (typeof raw === 'number' && Number.isFinite(raw)) cb({ beakFraction: raw })
const payload = data as { beakFraction?: unknown; cardCentreInView?: unknown } | undefined
const raw = payload?.beakFraction
const centre = payload?.cardCentreInView
if (typeof raw === 'number' && Number.isFinite(raw)) {
cb({
beakFraction: raw,
// `null` rather than a guess: an older main that does not send it must fall back to
// CSS centring, not to a bogus offset.
// Non-negative as well as finite: a negative or NaN centre would place the card
// off its own view, and the renderer treats null as "fall back to CSS centring".
cardCentreInView:
typeof centre === 'number' && Number.isFinite(centre) && centre >= 0 ? centre : null
})
}
}
ipcRenderer.on('comfy-titletooltip:set-beak', handler)
return () => ipcRenderer.removeListener('comfy-titletooltip:set-beak', handler)
Expand Down
38 changes: 34 additions & 4 deletions src/renderer/src/comfyTitleTooltip/TitleTooltipApp.vue
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
<script setup lang="ts">
import type { CoachmarkBeakPayload } from '../../../types/ipc'
import { nextTick, onMounted, onUnmounted, ref, useTemplateRef, watch } from 'vue'

/**
Expand Down Expand Up @@ -33,7 +34,7 @@ interface Bridge {
dismissCoachmark?(configToken: string): void
/** Beak position as a fraction of the card's width, pushed once main has measured the card
* and settled its final bounds. */
onBeak?(cb: (payload: { beakFraction: number }) => void): () => void
onBeak?(cb: (payload: CoachmarkBeakPayload) => void): () => void
/** Coachmark secondary action — retires the card the same way dismiss does, and lets the
* owning feature run its follow-up (e.g. opening Settings). */
actionCoachmark?(configToken: string): void
Expand All @@ -50,6 +51,18 @@ const cmActionLabel = ref<string>('')
/** Defaults to centred, which is what a card with no clamp and a correct anchor resolves to
* anyway — so a missed push degrades to the old behaviour rather than to a detached beak. */
const cmBeakFraction = ref<number>(0.5)
/** Where the card's midpoint belongs inside the view, as MAIN computed it — `null` until it
* arrives, and on an older main that never sends it, which falls back to the CSS centring.
*
* Placement comes from main because centring here measures this page's own width, and that
* width is sometimes still the pre-resize value: the view has new bounds and the page has not
* processed them. Centring against it puts the card one gutter off the anchor, and the beak
* is pinned to the card, so the whole thing points beside the bell. Measured at 8px, with the
* page reporting 300 inside a 316-wide view.
*
* A centre rather than a left edge, so it stays correct at whatever width the card actually
* renders — `translateX(-50%)` offsets by half of the real card, not half of an assumed one. */
const cmCardCentre = ref<number | null>(null)
const themeBg = ref<string>('#211927')
const themeText = ref<string>('#ffffff')
const themeBorder = ref<string>('#38303d')
Expand Down Expand Up @@ -110,8 +123,14 @@ onMounted(() => {
if (cfg.theme.accent) themeAccent.value = cfg.theme.accent
void measureAndAck()
})
unsubBeak = bridge?.onBeak?.(({ beakFraction }) => {
unsubBeak = bridge?.onBeak?.(({ beakFraction, cardCentreInView }) => {
cmBeakFraction.value = Math.min(1, Math.max(0, beakFraction))
// `?? null` and a finiteness guard, not a `=== null` test: an older preload sends
// `undefined`, which would otherwise reach the style binding and emit `undefinedpx`.
cmCardCentre.value =
typeof cardCentreInView === 'number' && Number.isFinite(cardCentreInView)
? Math.max(0, cardCentreInView)
: null
})
bridge?.ready()
// Re-measure if Inter loads mid-session (after the initial ack) so main can
Expand Down Expand Up @@ -155,7 +174,16 @@ onUnmounted(() => {
:style="{
background: themeBg,
color: themeText,
borderColor: coachmarkBorder
borderColor: coachmarkBorder,
...(cmCardCentre === null
? {}
: {
marginLeft: '0',
marginRight: '0',
position: 'relative',
left: `${cmCardCentre}px`,
transform: 'translateX(-50%)'
})
}"
>
<span
Expand Down Expand Up @@ -246,7 +274,9 @@ onUnmounted(() => {
display: block;
width: max-content;
max-width: 280px;
/* `margin-top` for the beak; `auto` inline so the card CENTRES in the view.
/* `margin-top` for the beak; `auto` inline as the FALLBACK centring — main normally sends
an explicit centre (`cmCardCentre`) which overrides this inline, because centring here
depends on the page's own width and that is sometimes still the pre-resize value.
Body's flex centring does not reach it: `#app` is `width: 100%`, so the flex item that
gets centred is a full-width box and the card inside it stays flush-left. Main sizes the
view as the card plus a shadow gutter each side and centres that VIEW on the bell, so a
Expand Down
19 changes: 19 additions & 0 deletions src/types/ipc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,25 @@ export type { AuthStatus, Workspace }
import type { BetaActivationNotice } from '../main/lib/betaActivationNotice'
export type { BetaActivationNotice }

/** Payload of `comfy-titletooltip:set-beak`: where the coachmark card and its beak belong.
*
* Declared here because three processes have to agree on it — main sends it, the preload
* validates it, the renderer applies it — and an IPC boundary gives no compile error when
* they drift. `ipcMain.send` is untyped and `ipcRenderer.on` hands back `unknown`, so
* independent declarations would disagree silently and the card would land in the wrong
* place with everything still building.
*/
export interface CoachmarkBeakPayload {
/** Where the beak sits along the card, 0..1 from its left edge. */
beakFraction: number
/** Where the card's midpoint belongs within its view, in CSS px.
*
* A centre rather than an edge so it holds at whatever width the card actually renders,
* and `null` when main did not send one — the renderer then falls back to CSS centring
* rather than to a guess. */
cardCentreInView: number | null
}

/** Every renderer-safe Build catalog state. */
export type DevPlatformBuildState =
| 'installable'
Expand Down
Loading