Skip to content
Open
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
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 @@ -90,9 +92,11 @@ function DuckPlayerDefault({ advance }) {
}
video.currentTime = 0;
try {
/** @type {Promise<void>} */
const frameReady = new Promise((resolve) => video.requestVideoFrameCallback(() => resolve()));
await video.play();
await frameReady;
} catch (error) {
// Ignore errors - we can assume that our browsers support playback
console.error(error);
}
};
Expand All @@ -102,20 +106,20 @@ function DuckPlayerDefault({ advance }) {
if (video) video.currentTime = 0;
};

// Auto-play after bubble entry animation (400ms delay + 267ms duration = 667ms)
// Auto-play after bottom bubble entry animation (650ms delay + 267ms duration = 917ms)
useEffect(() => {
const id = setTimeout(
() => {
play(videoFor('with'));
setState((prev) => ({ ...prev, phase: isReducedMotion ? 'settled' : 'playing' }));
},
isReducedMotion ? 0 : 667,
isReducedMotion ? 0 : 917,
);
return () => clearTimeout(id);
}, []); // exclude isReducedMotion from deps — must not re-fire if reduced motion changes after mount

const toggle = () => {
const { target, phase, reverse } = state;
const toggle = async () => {
const { target, phase, reverse } = stateRef.current;
if (phase === 'initial') {
// Queue or cancel a reverse so auto-play will switch to "without" once the "with" video ends
setState({ target, phase, reverse: !reverse });
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 @@ -13,6 +13,9 @@ import { useFlip } from '../hooks/useFlip';
import cn from 'classnames';
import styles from './MakeDefaultContent.module.css';

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

/**
* Top bubble content for the makeDefaultSingle step.
* Shows title (changes after user makes default), comparison table, and Skip/Make Default buttons.
Expand Down Expand Up @@ -78,6 +81,11 @@ export function MakeDefaultContent({ advance, updateSystemValue }) {
})();
};

const defaultBubbleDelay = 400;
const defaultOffset = 250;
const parsedOffset = bubbleFadeInDelayOverride ? Number.parseInt(bubbleFadeInDelayOverride, 10) : defaultOffset;
const staggerDelay = defaultBubbleDelay + (Number.isNaN(parsedOffset) ? defaultOffset : parsedOffset);
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicated stagger delay calculation risks future divergence

Low Severity

The stagger delay calculation — parsing bubbleFadeInDelay URL param, applying a default offset of 250ms, and adding it to a base delay of 400ms — is duplicated identically in both SingleStep.js and MakeDefaultContent.js, including the same module-level URLSearchParams parse. Both also independently declare bubbleFadeInDelayOverride. If the default values or logic change in one file but not the other, they'll silently diverge.

Additional Locations (1)
Fix in Cursor Fix in Web


return (
<Container class={styles.root}>
<div class={styles.titleContainer}>
Expand All @@ -95,17 +103,19 @@ export function MakeDefaultContent({ advance, updateSystemValue }) {
/>
</div>

<ComparisonTable />
<div class={styles.content} style={{ '--stagger-delay': `${staggerDelay}ms` }}>
<ComparisonTable />

<div class={styles.actions}>
{skipButtonMounted && (
<Button buttonRef={skipButtonRef} class={styles.skipButton} variant="secondary" onClick={advance}>
{t('skipButton')}
<div class={styles.actions}>
{skipButtonMounted && (
<Button buttonRef={skipButtonRef} class={styles.skipButton} variant="secondary" onClick={advance}>
{t('skipButton')}
</Button>
)}
<Button buttonRef={primaryButtonRef} disabled={isPending} onClick={showSkipButton ? enableDefaultBrowser : advance}>
{showSkipButton ? t('makeDefaultButton') : t('nextButton')}
</Button>
)}
<Button buttonRef={primaryButtonRef} disabled={isPending} onClick={showSkipButton ? enableDefaultBrowser : advance}>
{showSkipButton ? t('makeDefaultButton') : t('nextButton')}
</Button>
</div>
</div>
</Container>
);
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 All @@ -22,6 +23,15 @@
}
}

.content {
display: flex;
flex-direction: column;
align-items: center;
align-self: stretch;
gap: var(--sp-8); /* 32px — match Container gap */
animation: stagger-fade-in 267ms cubic-bezier(0.66, 0, 0.34, 1) var(--stagger-delay, 650ms) backwards;
}

.actions {
position: relative;
display: flex;
Expand All @@ -38,3 +48,14 @@
.skipButton {
flex: 1;
}

@keyframes stagger-fade-in {
from { opacity: 0; }
to { opacity: 1; }
}

@media (prefers-reduced-motion: reduce) {
.content {
animation: none;
}
}
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 = 250; // Default 250ms 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.
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