Interaction helpers, sticky Milo header, and the /milo experience - #8
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR adds a sticky site header, a standalone Milo page, view-transition and reduced-motion support, improved Milo animation cleanup and pointer handling, and shared hover and touch-target styling helpers. ChangesSite experience updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Visitor
participant Header
participant MiloPage
participant MiloAnimation
Visitor->>Header: Select Milo navigation
Header->>MiloPage: Navigate to /milo/
MiloPage->>MiloAnimation: Render and start Milo
MiloAnimation->>MiloAnimation: Track visibility and pointer input
Visitor->>MiloPage: Close or press Escape
MiloPage->>Header: Navigate back or assign /
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The /milo referrer check now requires a real same-origin path so lookalike hosts cannot pass, Escape is one-shot per page view, tap extensions grow to 0.8rem vertical to clear the 44px minimum the old minHeight guaranteed, and a hidden header Milo idles on a slow recheck instead of a per-frame rAF. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
hoverRule() now carries the hover-gated globalStyle shape used at four sites, both square icon buttons compose one shared base, and the header's reveal sentinel rides inside Header.astro instead of leaning on BaseLayout. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@coderabbitai full review Context for the review: this branch adds, in milestone commits — (1) mobile-safe CSS interaction helpers ( Known, deliberate trade-offs (please don't re-flag): the header Milo's A multi-agent self-review already ran and its confirmed findings were fixed in |
|
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/milo/animate.ts (1)
140-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument side-effecting functions with
@sideEffect.
startMilosand the new lifecycle/input closures mutate DOM state, schedule work, or manage listeners. Add@sideEffectto their JSDoc/comments.As per coding guidelines, use
@sideEffecton non-pure functions (including exports and closures) while keeping pure functions untagged.Also applies to: 195-246, 302-356
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/milo/animate.ts` around lines 140 - 149, Add `@sideEffect` documentation to the startMilos function and the side-effecting lifecycle/input closures in the referenced sections, including DOM mutation, scheduling, and listener management functions. Keep pure helper functions untagged and preserve the existing behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/styles/interaction.ts`:
- Line 9: Export the SelectorRule type, or replace it with an existing public
vanilla-extract type, so the exported hover API no longer exposes a private
type. Update the related definitions and hover usage in this section
consistently while preserving the existing selector typing.
---
Nitpick comments:
In `@src/components/milo/animate.ts`:
- Around line 140-149: Add `@sideEffect` documentation to the startMilos function
and the side-effecting lifecycle/input closures in the referenced sections,
including DOM mutation, scheduling, and listener management functions. Keep pure
helper functions untagged and preserve the existing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 13ef142c-48e0-4a9d-91e5-532eb9622d70
📒 Files selected for processing (14)
src/components/BaseHead.astrosrc/components/BaseLayout.astrosrc/components/Header.astrosrc/components/milo/animate.tssrc/pages/milo.astrosrc/pages/tools/_3d-to-svg/studio.css.tssrc/styles/breadcrumbs.css.tssrc/styles/breakpoints.tssrc/styles/footer.css.tssrc/styles/global.css.tssrc/styles/header.css.tssrc/styles/icon-button.css.tssrc/styles/interaction.tssrc/styles/milo-page.css.ts
Mobile browsers leave the visual viewport offset below the layout viewport while their toolbar collapses mid-scroll, which slid the fixed header's top out of sight until the toolbar returned. Reproduced with touch-gesture probes (visualViewport.offsetTop held at 16px during downward flings); tracking that offset in `top` keeps the whole bar on screen. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Exports the SelectorRule type the hover helper references, splits the frame loop into plan/advance/rest helpers so every function clears the complexity threshold, tags startMilos per the @sideeffect convention, and sweeps new comments free of em dashes and prose semicolons. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@coderabbitai full review Context for this second pass — everything from your first review is resolved, and five commits landed after it:
Same deliberate trade-offs as before (please don't re-flag): the Most valuable focus for this pass: the restructured frame loop in |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Changed files and focused commit diffs ==='
git diff --stat HEAD~5..HEAD
for commit in 99d98f8 7df8a42 eca7fde; do
printf '\n=== %s ===\n' "$commit"
git show --format=fuller --find-renames --find-copies --stat "$commit"
git show --format= --find-renames --find-copies -- \
src/components/milo/animate.ts \
src/components/BaseHead.astro \
src/components/Header.astro \
src/styles/header.css.ts \
"$commit" 2>/dev/null || true
done
printf '%s\n' '=== Focused file outlines ==='
for file in \
src/components/milo/animate.ts \
src/components/BaseHead.astro \
src/components/Header.astro \
src/styles/header.css.ts \
src/pages/milo.astro \
src/styles/milo-page.css.ts; do
if [ -f "$file" ]; then
printf '\n--- %s ---\n' "$file"
ast-grep outline "$file" --items all || true
fi
done
printf '%s\n' '=== Focused source ==='
for file in \
src/components/milo/animate.ts \
src/components/BaseHead.astro \
src/components/Header.astro \
src/styles/header.css.ts \
src/pages/milo.astro \
src/styles/milo-page.css.ts; do
if [ -f "$file" ]; then
printf '\n--- %s ---\n' "$file"
cat -n "$file"
fi
done
printf '%s\n' '=== Related lifecycle and test references ==='
rg -n -C 3 --glob '!node_modules/**' \
'startMilos\s*\(|data-milo-started|visualViewport|pageswap|pagereveal|view-transition|IntersectionObserver|visibilitychange|Navigation API|navigation\.activation' \
src test tests 2>/dev/null || true
printf '%s\n' '=== Project scripts/configuration ==='
for file in package.json astro.config.mjs astro.config.ts vite.config.ts; do
[ -f "$file" ] && { printf '\n--- %s ---\n' "$file"; cat -n "$file"; }
doneLength of output: 96709 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Correct focused diffs ==='
for commit in 99d98f8 7df8a42 eca7fde; do
printf '\n=== %s ===\n' "$commit"
git show --format= --find-renames --find-copies "$commit" -- \
src/components/milo/animate.ts \
src/components/BaseHead.astro \
src/components/Header.astro \
src/styles/header.css.ts \
src/components/milo/Milo.astro \
src/components/BaseLayout.astro \
src/pages/milo.astro
done
printf '%s\n' '=== Relevant commit ancestry ==='
git log --oneline --decorate -12
git merge-base HEAD origin/main 2>/dev/null || true
printf '%s\n' '=== Header and component call sites ==='
for file in src/components/BaseLayout.astro src/components/milo/Milo.astro; do
if [ -f "$file" ]; then
printf '\n--- %s ---\n' "$file"
cat -n "$file"
fi
done
printf '%s\n' '=== Tests and test outlines ==='
fd -a -e ts -e tsx . src | rg '\.test\.(ts|tsx)$' || true
for file in $(fd -a -e ts -e tsx . src | rg '\.test\.(ts|tsx)$' || true); do
printf '\n--- %s ---\n' "$file"
ast-grep outline "$file" --items all || true
cat -n "$file"
done
printf '%s\n' '=== Exact transition and viewport references ==='
rg -n -C 8 --glob '!node_modules/**' \
'(pageswap|pagereveal|skipTransition|visualViewport|offsetTop|data-site-header|data-header-sentinel)' \
srcLength of output: 36811 🤖 web_search executed:
Length of output: 14590
I also checked the restructured Milo frame loop and the cross-document transition gate. The ✅ Action performedFull review finished. |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@coderabbitai The startup-alignment finding is fixed in 0c9017e — |
|
Tip For best results, initiate chat on the files or code changes.
🐇 ✅ |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/Header.astro (1)
58-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a CSS variable instead of the inline
topwrite.
header.style.topbypasses the normal cascade for a value that's otherwise fully defined inheader.css.ts(top: 0). AcreateVar()+fallbackVar()default inheader.css.ts, updated here viasetElementVar(header, offsetVar, ...), would keep the static rule in the stylesheet and let only the runtime value live in the script, per this repo's convention for runtime-varying styles.As per coding guidelines: "Use
createVarplussetElementVarfor runtime-varying values; keep the static rule in.css.tsand move only the runtime value" and "Treat imperativeelement.styleas a last resort for styling, and use it only whendata-*variants or CSS variables do not fit."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Header.astro` around lines 58 - 66, Replace the direct header.style.top assignment in the visualViewport alignment handler with the repository’s CSS-variable pattern: define an offset variable and fallbackVar default in header.css.ts while retaining the static top rule, then use setElementVar(header, offsetVar, ...) in Header.astro for runtime viewport offsets.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/Header.astro`:
- Around line 58-66: Update the visualViewport setup in Header.astro to invoke
the existing align function immediately after defining it and before registering
the resize and scroll listeners, so the header reflects the current offsetTop
even when no viewport event fires initially.
---
Nitpick comments:
In `@src/components/Header.astro`:
- Around line 58-66: Replace the direct header.style.top assignment in the
visualViewport alignment handler with the repository’s CSS-variable pattern:
define an offset variable and fallbackVar default in header.css.ts while
retaining the static top rule, then use setElementVar(header, offsetVar, ...) in
Header.astro for runtime viewport offsets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ea34f016-f79c-4248-a18f-19f9217af3cf
📒 Files selected for processing (14)
src/components/BaseHead.astrosrc/components/BaseLayout.astrosrc/components/Header.astrosrc/components/milo/animate.tssrc/pages/milo.astrosrc/pages/tools/_3d-to-svg/studio.css.tssrc/styles/breadcrumbs.css.tssrc/styles/breakpoints.tssrc/styles/footer.css.tssrc/styles/global.css.tssrc/styles/header.css.tssrc/styles/icon-button.css.tssrc/styles/interaction.tssrc/styles/milo-page.css.ts
| const {visualViewport} = window; | ||
| if (visualViewport) { | ||
| const align = () => { | ||
| header.style.top = `${visualViewport.offsetTop}px`; | ||
| }; | ||
|
|
||
| visualViewport.addEventListener('resize', align, {passive: true}); | ||
| visualViewport.addEventListener('scroll', align, {passive: true}); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Call align() once before relying on the resize/scroll events.
align() is only invoked reactively. If visualViewport.offsetTop is already non-zero when this script runs (e.g. BFCache restoration mid-gesture, or the toolbar already partially collapsed on load), the header stays at its CSS default top: 0 until the next visual-viewport event fires.
🛠️ Proposed fix
const {visualViewport} = window;
if (visualViewport) {
const align = () => {
header.style.top = `${visualViewport.offsetTop}px`;
};
+ align();
visualViewport.addEventListener('resize', align, {passive: true});
visualViewport.addEventListener('scroll', align, {passive: true});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const {visualViewport} = window; | |
| if (visualViewport) { | |
| const align = () => { | |
| header.style.top = `${visualViewport.offsetTop}px`; | |
| }; | |
| visualViewport.addEventListener('resize', align, {passive: true}); | |
| visualViewport.addEventListener('scroll', align, {passive: true}); | |
| } | |
| const {visualViewport} = window; | |
| if (visualViewport) { | |
| const align = () => { | |
| header.style.top = `${visualViewport.offsetTop}px`; | |
| }; | |
| align(); | |
| visualViewport.addEventListener('resize', align, {passive: true}); | |
| visualViewport.addEventListener('scroll', align, {passive: true}); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/Header.astro` around lines 58 - 66, Update the visualViewport
setup in Header.astro to invoke the existing align function immediately after
defining it and before registering the resize and scroll listeners, so the
header reflects the current offsetTop even when no viewport event fires
initially.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Milestone-by-milestone branch for the sticky header + expandable Milo work. Each milestone landed as one commit; a multi-agent self-review then ran over the whole branch and its confirmed findings were fixed in two follow-up commits.
Milestones
07486cb):hover()gates every hover state behind@media (hover: hover);tapExtension()grows coarse-pointer hit boxes with padding + negative margin (no layout shift). Swept all six previously-unguarded hover sites. Breadcrumbs intentionally keepminHeight(their list is an overflow-x scroll container, which would clip an extended hit area).b4b4527): Milo skips his ~8 per-framesetAttributewrites when the pose is unchanged, so an idle Milo paints nothing.a48c23b):prefers-reduced-motionrenders a static portrait (and reacts live to preference flips);pointerdown/isPrimarytap-to-look.b5a8181,3393ff8,1d25b4f): slim header (wordmark left, Milo centered, Blog/Tools right) that slides in past an 8rem sentinel via IntersectionObserver and stays until you return to top; on narrow screens the wordmark collapses to "Home" so the centered Milo always fits./milo/page (13035ab): fullscreen responsive Milo (min(92vw, 82dvh), cannot clip), safe-area-aware close button with Escape support,touch-action: nonestage for drag-following.c921b07): native cross-document view transitions (@view-transition+ sharedview-transition-name) so the header Milo scales up into/milo/; reduced-motion gated; Firefox falls back to an instant navigation.Post-review fixes
acfeab8drops the touch-event scroll-follow path (browsers deliver touchmove during scroll too inconsistently); taps andtouch-action: nonedrags cover touch reliably.d8e8de7hardens the/miloclose flow (referrer origin check requires a real/delimiter; Escape is one-shot per page view), restores the 44px coarse-pointer tap minimum (0.8remvertical extensions), and idles a hidden header Milo on a 300mscheckVisibilityrecheck instead of a per-frame rAF loop.b04e929deduplicates the hover-gatedglobalStyleshape (hoverRule()), extracts a sharediconButtonbase for the studio copy button and the /milo close button, and moves the reveal sentinel insideHeader.astro.Known trade-off: the header Milo's
view-transition-nameis gated ondata-stuck="true". On a non-BFCache back-navigation the restored page is captured before the IntersectionObserver runs, so the return morph degrades to a plain crossfade. The alternative (ungated) would morph the giant Milo into an invisible hidden-header corner, which is worse. BFCache restores — the common case — morph correctly.🤖 Generated with Claude Code
Summary by CodeRabbit