Add rotating word containers to hero and fix animated GIF optimization#51
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughFixes ImageOverlay to auto-detect animated GIFs and disable Next.js image optimization for them, and adds deterministic, drift-free timing to RotatingWord plus new e2e tests and POM helpers verifying three rotating word containers and exact hero tagline text. Changes
Sequence Diagram(s)(omitted — changes are component-local and do not introduce multi-component sequential interactions) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Auto-detect animated GIFs in ImageOverlay and set `unoptimized` on next/image, silencing the Next.js optimization warning for ohm-logo-progression.gif (and any future GIFs) without requiring per-callsite configuration - Add E2E tests for the multi-word rotating hero: verifies three RotatingWord containers render and display their initial words (create / thoughtful / products) on load; tightens the existing accessible-tagline assertion to the full static string - Mark the GIF warning as resolved in TODO.md https://claude.ai/code/session_011eAwNMUyVSdr5wbmctsYV3
rioredwards
force-pushed
the
claude/review-changes-mlvs7wcmz24bdddg-joKRs
branch
from
afe7e64 to
4f97eca
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
e2e/pom/home-page.ts (2)
28-30: Minor naming ambiguity:heroTaglineVisible()alongsideheroTagline().
heroTagline()returns the canonical (hidden) element;heroTaglineVisible()returns the visible one. The suffixVisiblereads as if it might return a boolean (likeisVisible()), which differs from every other Locator-returning method in this POM. A name likeheroTaglineDisplay()orheroTaglineElement()would be less ambiguous, though this is a low-priority nit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/pom/home-page.ts` around lines 28 - 30, The method name heroTaglineVisible() is ambiguous because it suggests a boolean but returns a Locator; rename it to a Locator-returning name (e.g., heroTaglineElement() or heroTaglineDisplay()) and update all references; modify the declaration of heroTaglineVisible() to the chosen name and replace every call site that currently uses heroTaglineVisible() (and any tests/fixtures importing it) so the POM remains consistent with other Locator-returning methods like heroTagline().
32-34: CSS class selector inrotatingWordContainers()is fragile — preferdata-testid.
.rotating-word-containeris a styling class; if it's ever renamed or extracted, this locator breaks silently. Adding adata-testid="rotating-word-container"to each container element in the component and switching togetByTestId(...)here would make the selector implementation-agnostic and consistent with the rest of the POM.♻️ Suggested change (after adding data-testid to the component)
- rotatingWordContainers(): Locator { - return this.heroTaglineVisible().locator(".rotating-word-container"); - } + rotatingWordContainers(): Locator { + return this.heroTaglineVisible().getByTestId("rotating-word-container"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/pom/home-page.ts` around lines 32 - 34, The locator in rotatingWordContainers() currently uses the styling class ".rotating-word-container", which is fragile; after adding data-testid="rotating-word-container" to each container in the component, update rotatingWordContainers() to use getByTestId("rotating-word-container") (scoped off heroTaglineVisible() if needed) so the POM targets the test id instead of the CSS class and becomes implementation-agnostic.components/image-overlay/image-overlay.tsx (1)
39-40:endsWith(".gif")misses URLs with query parameters; variable name overstates detection capability.Two issues in these two lines:
Query params/fragments: A CDN URL like
/ohm-logo-progression.gif?v=2orhttps://cdn.example.com/img.gif#anchorwould passendsWith(".gif")asfalse, silently leaving optimization enabled and reproducing the original warning.Naming:
isAnimatedGifimplies the code can detect animation; it only detects the.gifextension. Non-animated GIFs also getunoptimized=true. A name likeisGifis more accurate.♻️ Suggested fix
- const srcString = isStaticImage ? src.src : src; - const isAnimatedGif = srcString.toLowerCase().endsWith(".gif"); + const srcString = isStaticImage ? src.src : src; + const isGif = srcString.split(/[?#]/)[0].toLowerCase().endsWith(".gif");- unoptimized={isAnimatedGif} + unoptimized={isGif}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/image-overlay/image-overlay.tsx` around lines 39 - 40, The GIF detection misses query strings/fragments and the name overstates capability: update the logic that uses srcString and isAnimatedGif to instead compute isGif by parsing srcString (the value assigned from isStaticImage ? src.src : src) and testing only the pathname portion or using a case-insensitive regex that ignores query/fragment (e.g., match /\.gif(\?|#|$)/i) so URLs like "image.gif?v=2" or "image.gif#anchor" are detected; rename the variable from isAnimatedGif to isGif to reflect that this only checks the extension.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@components/image-overlay/image-overlay.tsx`:
- Around line 39-40: The GIF detection misses query strings/fragments and the
name overstates capability: update the logic that uses srcString and
isAnimatedGif to instead compute isGif by parsing srcString (the value assigned
from isStaticImage ? src.src : src) and testing only the pathname portion or
using a case-insensitive regex that ignores query/fragment (e.g., match
/\.gif(\?|#|$)/i) so URLs like "image.gif?v=2" or "image.gif#anchor" are
detected; rename the variable from isAnimatedGif to isGif to reflect that this
only checks the extension.
In `@e2e/pom/home-page.ts`:
- Around line 28-30: The method name heroTaglineVisible() is ambiguous because
it suggests a boolean but returns a Locator; rename it to a Locator-returning
name (e.g., heroTaglineElement() or heroTaglineDisplay()) and update all
references; modify the declaration of heroTaglineVisible() to the chosen name
and replace every call site that currently uses heroTaglineVisible() (and any
tests/fixtures importing it) so the POM remains consistent with other
Locator-returning methods like heroTagline().
- Around line 32-34: The locator in rotatingWordContainers() currently uses the
styling class ".rotating-word-container", which is fragile; after adding
data-testid="rotating-word-container" to each container in the component, update
rotatingWordContainers() to use getByTestId("rotating-word-container") (scoped
off heroTaglineVisible() if needed) so the POM targets the test id instead of
the CSS class and becomes implementation-agnostic.
Three independent setTimeout chains accumulated small timing errors separately, causing the words to drift out of their 600ms stagger after ~30-60 seconds. Root cause: each setTimeout fires "now + delay" rather than at a fixed absolute time, and tiny inaccuracies compound across cycles. Fix: record a startTimeRef epoch once on init and a cycleCountRef. Each phase transition is scheduled at startTimeRef + cycleCount * CYCLE_DURATION + phaseOffset instead of a relative delay from "now". Since CYCLE_DURATION (pauseDuration + ANIMATION_DURATION) is mathematically constant regardless of word length, no drift can accumulate. Also adds the missing clearTimeout cleanup to the phase effect. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
components/ui/RotatingWord.tsx (1)
134-134: Remove commented-out debug span before merging.This
{/* <span className="debug">... */}is a development artifact. Consider removing it to keep the component clean, or extracting it behind aprocess.env.NODE_ENV === 'development'guard if you use it frequently during development.🧹 Proposed cleanup
- {/* <span className="debug">{animationPhase}</span> */}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ui/RotatingWord.tsx` at line 134, Remove the commented-out debug span in the RotatingWord component: delete the line {/* <span className="debug">{animationPhase}</span> */} to clean up the component, or if you want to keep it for development, replace it with a conditional render that only shows in development (e.g., use process.env.NODE_ENV === 'development' && <span className="debug">{animationPhase}</span>) so the debug output (animationPhase, className "debug") is not present in production.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@components/ui/RotatingWord.tsx`:
- Line 134: Remove the commented-out debug span in the RotatingWord component:
delete the line {/* <span className="debug">{animationPhase}</span> */} to clean
up the component, or if you want to keep it for development, replace it with a
conditional render that only shows in development (e.g., use
process.env.NODE_ENV === 'development' && <span
className="debug">{animationPhase}</span>) so the debug output (animationPhase,
className "debug") is not present in production.
The hero paragraph is rendered twice (desktop + mobile layouts), both with data-testid="hero-tagline". The previous selector matched both, returning 6 containers instead of the expected 3. Use :visible to scope to the currently rendered layout. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2 participants
Summary
This PR adds a new rotating word containers feature to the hero section and fixes an issue where animated GIFs were not being properly handled by Next.js Image optimization.
Key Changes
.giffiles in the ImageOverlay component to set theunoptimizedproperty on Next.js Image, preventing optimization warnings for animated imagesheroTaglineVisible()androtatingWordContainers()methods to the HomePage POM for better test selectorsImplementation Details
.rotating-word-containerclass selector within the hero tagline elementunoptimizedproperty is conditionally applied only to animated GIFs, allowing other image formats to benefit from Next.js optimizationhttps://claude.ai/code/session_011eAwNMUyVSdr5wbmctsYV3
Summary by CodeRabbit
Bug Fixes
New Features
Tests