Skip to content
Open
Show file tree
Hide file tree
Changes from 6 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
3 changes: 3 additions & 0 deletions special-pages/pages/onboarding/app/v4/components/Bubble.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { ProgressIndicator } from './ProgressIndicator';
* @property {boolean} [exiting] - When true, fades out the container via CSS animation
* @property {() => void} [onExitComplete] - Called when the fade-out animation ends
* @property {import('../data/data-types').Progress} [progress] - When provided, renders a progress badge pinned to the bubble's top edge
* @property {number} [fadeInDelay] - Delay in ms before the fade-in animation starts (default 400ms via CSS)
*/

/**
Expand Down Expand Up @@ -48,6 +49,7 @@ export function Bubble({
exiting = false,
onExitComplete,
progress,
fadeInDelay,
...props
}) {
const { isReducedMotion } = useEnv();
Expand Down Expand Up @@ -142,6 +144,7 @@ export function Bubble({
<div
ref={containerCallback}
class={cn(styles.container, isMounted.current && (exiting ? styles.fadeOut : styles.fadeIn))}
style={fadeInDelay !== undefined ? { '--fade-in-delay': `${fadeInDelay}ms` } : undefined}
onAnimationEnd={complete}
>
<div ref={contentRef} class={styles.content}>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
}

.fadeIn {
animation: fade-in 267ms cubic-bezier(0.66, 0, 0.34, 1) 400ms backwards;
animation: fade-in 267ms cubic-bezier(0.66, 0, 0.34, 1) var(--fade-in-delay, 400ms) backwards;
}

/* Clip wrapper for bottom-left tail — overflow creates the tuck/untuck effect */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ function DuckPlayerDefault({ advance }) {
const videosRef = useRef(/** @type {Record<DPTarget, HTMLVideoElement | null>} */ ({ with: null, without: null }));

const [state, setState] = useState(/** @type {DPState} */ ({ target: 'with', phase: 'initial', reverse: false }));
const stateRef = useRef(state);
stateRef.current = state;

/** @type {(target: DPTarget) => DPTarget} */
const flip = (target) => (target === 'with' ? 'without' : 'with');
Expand All @@ -89,12 +91,14 @@ function DuckPlayerDefault({ advance }) {
return;
}
video.currentTime = 0;
/** @type {Promise<void>} */
const frameReady = new Promise((resolve) => video.requestVideoFrameCallback(() => resolve()));
try {
await video.play();
} catch (error) {
// Ignore errors - we can assume that our browsers support playback
console.error(error);
}
await frameReady;
};

/** @param {HTMLVideoElement | null} video */
Expand All @@ -114,7 +118,7 @@ function DuckPlayerDefault({ advance }) {
return () => clearTimeout(id);
}, []); // exclude isReducedMotion from deps — must not re-fire if reduced motion changes after mount

const toggle = () => {
const toggle = async () => {
const { target, phase, reverse } = state;
if (phase === 'initial') {
// Queue or cancel a reverse so auto-play will switch to "without" once the "with" video ends
Expand All @@ -124,18 +128,19 @@ function DuckPlayerDefault({ advance }) {
if (!reverse) reset(videoFor(flip(target)));
setState({ target, phase: 'playing', reverse: !reverse });
} else {
// Settled: switch to the other video
// Settled: play the other video, wait for its first frame, then switch visibility
const next = flip(target);
play(videoFor(next));
await play(videoFor(next));
setState({ target: next, phase: isReducedMotion ? 'settled' : 'playing', reverse: false });
}
};

const end = () => {
if (state.reverse) {
// A reverse was queued — play the other video now
Copy link
Contributor

Choose a reason for hiding this comment

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

Async toggle/end creates state machine race condition

Medium Severity

Making toggle and end async introduces a gap between reading stateRef.current and calling setState, during which concurrent events can interleave and overwrite state. Previously these were synchronous — setState ran immediately after play (fire-and-forget), so the state machine stayed consistent. Now, in the settled branch of toggle, a rapid second click sees the same stale phase: 'settled' and re-enters the same branch, restarting the video via currentTime = 0. Worse, in end's reverse branch, if the user clicks toggle during await play(...), their intent to cancel the reverse gets silently overwritten when end's delayed setState runs with target: next.

Additional Locations (1)
Fix in Cursor Fix in Web

const next = flip(state.target);
play(videoFor(next));
const end = async () => {
const { reverse, target } = stateRef.current;
if (reverse) {
// A reverse was queued — play the other video, wait for its first frame, then switch
const next = flip(target);
await play(videoFor(next));
setState({ target: next, phase: 'playing', reverse: false });
} else {
// No reverse — just settle on the current video
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@
position: absolute;
top: 0;
left: 0;
visibility: hidden;
opacity: 0;
pointer-events: none;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
font-size: calc(18 * var(--px-in-rem));
font-weight: 400;
line-height: calc(22 * var(--px-in-rem));
text-align: center;
text-align: left;
color: var(--ds-text-primary);
word-wrap: break-word;
text-box-trim: trim-both;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

.titleContainer {
position: relative;
align-self: stretch;
}

.title {
Expand Down
13 changes: 13 additions & 0 deletions special-pages/pages/onboarding/app/v4/components/SingleStep.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ import styles from './SingleStep.module.css';
/** @type {string|null} */
const bubbleWidthOverride = new URLSearchParams(window.location.search).get('bubbleWidth');

/** @type {string|null} */
const bubbleFadeInDelayOverride = new URLSearchParams(window.location.search).get('bubbleFadeInDelay');

/**
* Main layout component for v4 steps.
* Steps with bubbles use absolute positioning; the layout measures bubble heights.
Expand Down Expand Up @@ -67,6 +70,16 @@ export function SingleStep() {
exiting={globalState.exiting}
onExitComplete={topBubble ? undefined : advance}
progress={showProgress && !topBubble ? progress : undefined}
fadeInDelay={
topBubble
? (() => {
const defaultTopDelay = 400; // Default fade-in delay from CSS
const defaultOffset = 100; // Default 100ms offset between top and bottom
const offset = bubbleFadeInDelayOverride ? Number.parseInt(bubbleFadeInDelayOverride, 10) : defaultOffset;
return defaultTopDelay + (Number.isNaN(offset) ? defaultOffset : offset);
})()
: undefined
}
>
{bottomBubble?.content}
</Bubble>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
font-size: var(--sp-6); /* 24px */
font-weight: 700;
line-height: var(--sp-7); /* 28px */
text-align: center;
text-align: left;
white-space: pre-line;
color: var(--ds-text-primary);
margin: 0;
Expand All @@ -29,7 +29,7 @@
font-size: calc(18 * var(--px-in-rem));
font-weight: 400;
line-height: calc(22 * var(--px-in-rem));
text-align: center;
text-align: left;
white-space: pre-line;
color: var(--ds-text-primary);
margin: 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
font-size: var(--sp-6); /* 24px */
font-weight: 700;
line-height: var(--sp-7); /* 28px */
text-align: center;
text-align: left;
color: var(--ds-text-primary);
word-wrap: break-word;
text-box-trim: trim-both;
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ test.describe('onboarding v4', () => {
// Simulate disabled video ending → reducer plays enabled video (toWithDuckPlayer)
await disabledVideo.dispatchEvent('ended');
await expect(enabledVideo).toBeVisible();
await expect(disabledVideo).not.toBeVisible();
await expect(disabledVideo).toHaveCSS('opacity', '0');
await expect(page.getByRole('button', { name: 'See Without Duck Player' })).toBeVisible();

// Simulate enabled video ending → withDuckPlayer
Expand Down
Loading