Skip to content

fix(GlassPanes): upgrade off flat SSR fallback after mount #26

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.

Sign up for GitHub

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

Open
yousefh409 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(GlassPanes): upgrade off flat SSR fallback after mount #26

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 22 additions & 8 deletions src/components/GlassPanes.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
*/

import type { CSSProperties, HTMLAttributes } from "react";
import { useEffect, useMemo } from "react";
import { useEffect, useMemo, useState } from "react";
import { resolveColor, useInView, usePrefersReducedMotion } from "../utils";
import { GOLDEN_RATIO_FRAC, MIN_SPEED } from "../utils/constants";
import { supportsBackdropFilter } from "../utils/featureDetect";
Expand All @@ -66,8 +66,8 @@ const MAX_COUNT = 8;
const PANE_ANGLE_DEG = 8;

/** Pane blur depth range, px — depth is the point: far panes frost lightly, near panes heavily. */
const MIN_BLUR_PX = 8;
const BLUR_SPAN_PX = 18; // 8-26px
const MIN_BLUR_PX = 4;
const BLUR_SPAN_PX = 8; // 4-12px
Comment on lines +69 to +70

@devin-ai-integration devin-ai-integration Bot Jul 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔍 Blur depth range halved — significantly reduced visual depth differentiation

The blur range was reduced from 8–26px (MIN_BLUR_PX=8, BLUR_SPAN_PX=18) to 4–12px (MIN_BLUR_PX=4, BLUR_SPAN_PX=8 at src/components/GlassPanes.tsx:69-70). The component's core visual premise — documented in the header comment at lines 4-6 — is that each pane gets a DIFFERENT blur strength so overlaps read as physical depth. The old 18px span gave a 3.25× ratio between the lightest and heaviest frost (8px vs 26px); the new 8px span gives only a 3× ratio (4px vs 12px) with much less absolute difference. At 4px minimum blur, the lightest-frosted pane may not read as frosted at all on high-DPI screens. This is a deliberate visual tuning choice but worth confirming it still achieves the intended depth layering effect in practice.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.


/** How far past each container edge the slot row extends, % of width —
* absorbs the sideways shift the shared rotation causes at the container's
Expand Down Expand Up @@ -101,7 +101,7 @@ const PANE_SHADOW =
const KEYFRAMES_CSS = `
@keyframes ${SLIDE_KEYFRAMES_NAME} {
0% { transform: rotate(var(--fluidkit-panes-rot)) translate(0%, 0%); }
50% { transform: rotate(var(--fluidkit-panes-rot)) translate(var(--fluidkit-panes-drift), -2.5%); }
50% { transform: rotate(var(--fluidkit-panes-rot)) translate(var(--fluidkit-panes-drift), -3.5%); }
100% { transform: rotate(var(--fluidkit-panes-rot)) translate(0%, 0%); }
}
`;
Expand Down Expand Up @@ -133,7 +133,7 @@ function layoutPanes(colors: string[] | undefined, count: number, speed: number)
// covers every point regardless of count.
widthPct: spacing + MIN_OVERLAP_PCT + frac * OVERLAP_SPAN_PCT,
blurPx: Math.round(MIN_BLUR_PX + frac * BLUR_SPAN_PX),
driftPct: `${i % 2 === 0 ? 4 : -4}%`,
driftPct: `${i % 2 === 0 ? 13 : -13}%`,

@devin-ai-integration devin-ai-integration Bot Jul 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 Tripled pane drift breaks the full-surface coverage guarantee, exposing gaps at default settings

The pane slide distance is tripled (driftPct changed from ±4% to ±13% at GlassPanes.tsx:136) without increasing the overlap margins that absorb it, so neighboring panes can drift apart far enough to expose the background.

Impact: At the default count of 3, and at count=2, visible gaps flash between the glass panes mid-animation, breaking the documented "every point is always behind at least one pane" guarantee.

Coverage-guarantee math showing the gap

The overlap comment at src/components/GlassPanes.tsx:78 still says "drift amplitude ≈ 4% of pane width" but the drift is now 13%. The overlap constants (MIN_OVERLAP_PCT=12, OVERLAP_SPAN_PCT=8 at lines 80-81) were sized for the old 4% drift and are now too small.

CSS translate(13%, …) moves a pane by 13% of its own width. Two adjacent panes alternate direction (even → +13%, odd → −13%), so at the animation midpoint they move apart by 0.13 × width_i + 0.13 × width_j in container-width terms.

count=3 (default):

  • spacing = 116/3 ≈ 38.7%, min pane width ≈ 50.7%
  • Worst-case gap opening = 0.13 × 50.7 + 0.13 × 55.6 ≈ 13.8% of container
  • Static overlap between adjacent panes ≈ 12%
  • Net: −1.8% → a gap of ~1.8% of the container width

count=2:

  • spacing = 58%, min pane width ≈ 70%
  • Worst-case gap opening ≈ 18.8% of container
  • Static overlap ≈ 12%
  • Net: −6.8% → a gap of ~6.8% of the container width

The old ±4% drift kept the worst-case gap opening well under the 12% minimum overlap (e.g., count=2: 0.04×70×2 = 5.6% < 12%). The fix is either to reduce the drift back toward the old value, or to increase MIN_OVERLAP_PCT to at least 2 × 0.13 × max_pane_width (roughly 26% for count=2).

Prompt for agents 
The drift amplitude at line 136 was changed from ±4% to ±13% of pane width, but the overlap constants MIN_OVERLAP_PCT (line 80) and OVERLAP_SPAN_PCT (line 81) were not increased to compensate. The comment at line 78 also still references the old 4% value.

The coverage guarantee requires that the static overlap between adjacent panes exceeds the maximum drift-induced gap opening. The gap opening equals 0.13 * width_pane_i + 0.13 * width_pane_j (two adjacent panes moving in opposite directions). For count=2, this is about 18.8% of container width, far exceeding the 12% minimum overlap.

Two approaches to fix:
1. Reduce drift back to a value the current overlap can absorb. The old ±4% worked. Something up to about ±5% would be safe with the current 12% minimum overlap.
2. Increase MIN_OVERLAP_PCT to absorb the new ±13% drift. For count=2 (worst case, pane width ~70%), the overlap needs to be at least 2 * 0.13 * 78 ≈ 20.3%, so MIN_OVERLAP_PCT should be at least ~24% (accounting for jitter). This would make panes significantly wider and more overlapping.

Also update the comment at line 78 to reflect whichever drift value is chosen.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@devin-ai-integration devin-ai-integration Bot Jul 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Missing changelog entry for user-visible behavior changes

Several user-facing behaviors are changed — pane drift amplitude, blur range, and SSR/hydration rendering — but no CHANGELOG.md entry is added (CONTRIBUTING.md:43-44), so downstream consumers have no record of the visual and rendering changes.

Impact: Users upgrading the library won't know the glass-pane animation and SSR behavior changed.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@chatgpt-codex-connector chatgpt-codex-connector Bot Jul 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Keep single-pane coverage during drift

When consumers pass count={1}, this new 13% translate moves the only pane by 13% of its 128% width at the 50% keyframe; with the existing layout its interval shifts from [-16%, 112%] to about [0.64%, 128.64%], leaving a visible uncovered strip on the left edge. The component documents and tests that counts are clamped to 1–8 and should tile the full surface, so the widened drift needs matching bleed/width or a one-pane exception.

Useful? React with 👍 / 👎.

periodS,
// Negative delay starts each pane mid-cycle instead of all in-phase
// at 0%, so panes don't visibly synchronize.
Expand All @@ -159,8 +159,20 @@ export function GlassPanes({
style,
...rest
}: GlassPanesProps) {
const prefersReducedMotion = usePrefersReducedMotion();
const rawPrefersReducedMotion = usePrefersReducedMotion();
const { ref, inView } = useInView<HTMLDivElement>();

// Feature detection (reduced-motion + backdrop-filter support) is
// static-safe during SSR and the first client render — it assumes reduced
// motion and no backdrop-filter. Without re-checking after mount the panes
// stay stuck on that flat, non-animated fallback in the browser. Flip a
// mounted flag in an effect so we re-render once with the real values.
const [mounted, setMounted] = useState(false);
useEffect(() => {
setMounted(true);
}, []);

const prefersReducedMotion = mounted ? rawPrefersReducedMotion : true;
Comment on lines +170 to +175

@devin-ai-integration devin-ai-integration Bot Jul 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Missing test updates for changed animation and SSR behavior

The PR changes drift amplitude, blur depth range, and adds a new SSR-to-client hydration path (the mounted state gate at GlassPanes.tsx:170-175), but no tests are added or updated (CONTRIBUTING.md:41), so the new behavior is untested.

Impact: Regressions in the SSR fallback upgrade path or the new drift/blur values won't be caught by CI.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

const animating = !prefersReducedMotion && inView;

useEffect(() => {
Expand All @@ -170,8 +182,10 @@ export function GlassPanes({
const clampedSpeed = Math.max(speed, MIN_SPEED);
const clampedCount = Math.min(MAX_COUNT, Math.max(1, Math.round(count)));
// Guarded pure function (never throws, false in SSR) — same detector the
// engine's resolveMaterial uses, safe to call during render.
const frosted = supportsBackdropFilter();
// engine's resolveMaterial uses, safe to call during render. Gate on
// `mounted` so SSR/first render stays on the flat fallback and the client
// upgrades to frosted glass after mount.
const frosted = mounted && supportsBackdropFilter();
Comment on lines 182 to +188

@devin-ai-integration devin-ai-integration Bot Jul 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔍 Extra render cycle from mounted-state hydration gate

The useState(false) + useEffect(() => setMounted(true), []) pattern at lines 170–173 guarantees a second render on every mount. During the first render, prefersReducedMotion is forced to true and frosted is forced to false, so the initial DOM has animation-name: none and no backdrop-filter. The post-mount render then flips both to their real values, causing a visual flash from flat/static to frosted/animated. This is a standard SSR hydration-safety pattern and is intentional per the comment, but consumers may notice a brief frame of the fallback appearance on client-side-only apps where SSR safety isn't needed. Other components in this repo (e.g., MeshGradient, Silk) do NOT use this pattern — they call supportsBackdropFilter() and usePrefersReducedMotion() directly during render without a mounted gate — so this creates an inconsistency across the ambient background family.

(Refers to lines 170-188)

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

const panes = useMemo(
() => layoutPanes(colors, clampedCount, clampedSpeed),
[colors, clampedCount, clampedSpeed]
Comment on lines 189 to 191

@devin-ai-integration devin-ai-integration Bot Jul 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📝 Info: useMemo dependency on colors array identity may cause unnecessary recalculations after mount

The useMemo at line 189–192 depends on colors by reference identity. Since the mounted-state pattern now forces a second render immediately after mount, if the parent passes a new array literal on each render (e.g., <GlassPanes colors={['red', 'blue']} />), the pane layout will be recomputed on the post-mount render even though the colors haven't semantically changed. This is a pre-existing concern (array identity instability is always a risk with inline literals), but the extra render from the mounted gate makes it fire one more time than before. The computation in layoutPanes is lightweight so the performance impact is negligible, but it's worth noting for consistency with the "byte-identical pane styles" determinism promise in the header comment (line 30–31) — the styles ARE identical, just computed twice.

(Refers to lines 189-192)

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Expand Down