Merged
Conversation
…as animations - Add Firefox detection via is-firefox class on <html> - Create firefox-performance.css with targeted overrides: - Disable backdrop-filter (replaced with opaque backgrounds) - Remove persistent will-change that causes memory bloat - Simplify filter animations and text-shadow glows - Limit wildcard transitions to interactive elements only - MatrixRain: disable shadowBlur, throttle to 30fps, reduce columns 30% - ParticleBackground: halve particle count, throttle to 30fps, reduce connection distance (cuts O(n²) calculations significantly) - CRTEffect: replace wasteful setInterval with RAF-based tick - useScrollAnimation: cache layout values in useParallax to eliminate getBoundingClientRect() layout thrashing on scroll, add RAF debouncing - PageTransition: use opacity flash instead of filter animation on Firefox, fix pre-existing anime.js v4 API mismatch
This reverts commit fa442da.
📝 WalkthroughWalkthroughThis pull request introduces Firefox browser detection and corresponding performance optimizations across the application. A new guidance document is added for architectural reference. Animation loops are refactored from interval-based to RAF-driven approaches, component rendering is conditionally tuned for Firefox (particle counts, frame throttling, visual effects), and a new Firefox-specific CSS stylesheet disables expensive effects like backdrop-filter and global transitions, replacing them with opaque alternatives. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
This reverts commit bea38af.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main.tsx (1)
7-8: Consider extracting Firefox detection to a shared utility.The
navigator.userAgent.includes('Firefox')check is duplicated in four places. A shared utility would improve maintainability if detection logic needs updating.♻️ Optional: Create shared utility
// src/utils/browserDetection.ts export const isFirefox = typeof navigator !== 'undefined' && navigator.userAgent.includes('Firefox');Then import and reuse in
main.tsxand component files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.tsx` around lines 7 - 8, Extract the repeated Firefox check into a shared utility (e.g., export a constant/function named isFirefox) and replace all occurrences of navigator.userAgent.includes('Firefox') with imports of isFirefox; ensure the utility guards against SSR by checking typeof navigator !== 'undefined', export it (isFirefox) from a central module, and update main.tsx and the other components to import and use isFirefox instead of duplicating the userAgent check.src/css/firefox-performance.css (1)
115-130: Scope the global transition kill-switch more narrowly.Line 115 disables transitions for all descendants with
!important; non-whitelisted interactive elements (label, select, details, summary) lose transition behavior. Prefer targeted suppression of expensive properties instead of a universal reset.Affected elements not in whitelist
- `` (ContactPage.tsx, BlogPage.tsx, ContactForm.tsx) - `` (BlogPage.tsx) - `` / `` (ErrorBoundary.tsx) 🤖 Prompt for AI Agents Verify each finding against the current code and only fix it if needed. In `@src/css/firefox-performance.css` around lines 115 - 130, Replace the overly broad ".is-firefox * { transition-property: none !important; }" rule with a targeted suppression of only expensive properties (e.g. transform, opacity, filter, box-shadow, backdrop-filter) so you don't strip transitions from innocuous elements, and add the missing interactive elements to the whitelist: include .is-firefox label, .is-firefox select, .is-firefox details, and .is-firefox summary in the same selector group as .is-firefox a, .is-firefox button, .is-firefox input, .is-firefox textarea, .is-firefox header, .is-firefox [class*="card"], .is-firefox [class*="btn"], .is-firefox .menu-toggle, .is-firefox nav ul; keep the existing transition-duration and the !important only on the targeted properties so performance-sensitive transitions are disabled while preserving expected transitions on label/select/details/summary. 🤖 Prompt for all review comments with AI agents Verify each finding against the current code and only fix it if needed. Inline comments: In `@src/css/firefox-performance.css`: - Around line 93-95: The selector .is-firefox [data-theme="matrix"] body::after is unreachable because data-theme is set on the same element as .is-firefox; update the selector to combine them on the same element (e.g. .is-firefox[data-theme="matrix"] body::after or html.is-firefox[data-theme="matrix"] body::after) so the scanline opacity override actually matches and applies. In `@src/hooks/useScrollAnimation.ts`: - Around line 182-191: The visibility calculation uses a stale windowHeight captured once; modify updateLayout() to also refresh windowHeight = window.innerHeight (in addition to elementTop and elementHeight) and ensure updateLayout is invoked on window resize by adding a resize event listener that calls updateLayout (and remove the listener in the cleanup). Update references to elementTop, elementHeight, windowHeight, and updateLayout accordingly so visibility checks use the current window height. --- Nitpick comments: In `@src/css/firefox-performance.css`: - Around line 115-130: Replace the overly broad ".is-firefox * { transition-property: none !important; }" rule with a targeted suppression of only expensive properties (e.g. transform, opacity, filter, box-shadow, backdrop-filter) so you don't strip transitions from innocuous elements, and add the missing interactive elements to the whitelist: include .is-firefox label, .is-firefox select, .is-firefox details, and .is-firefox summary in the same selector group as .is-firefox a, .is-firefox button, .is-firefox input, .is-firefox textarea, .is-firefox header, .is-firefox [class*="card"], .is-firefox [class*="btn"], .is-firefox .menu-toggle, .is-firefox nav ul; keep the existing transition-duration and the !important only on the targeted properties so performance-sensitive transitions are disabled while preserving expected transitions on label/select/details/summary. In `@src/main.tsx`: - Around line 7-8: Extract the repeated Firefox check into a shared utility (e.g., export a constant/function named isFirefox) and replace all occurrences of navigator.userAgent.includes('Firefox') with imports of isFirefox; ensure the utility guards against SSR by checking typeof navigator !== 'undefined', export it (isFirefox) from a central module, and update main.tsx and the other components to import and use isFirefox instead of duplicating the userAgent check. 🪄 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: defaults Review profile: CHILL Plan: Pro Run ID: a8ae6e76-0096-4a40-b417-2485d165cee9 📥 Commits Reviewing files that changed from the base of the PR and between 68da9d1 and d5251b5. 📒 Files selected for processing (9) .claude/CLAUDE.md src/components/CRTEffect.tsx src/components/MatrixRain.tsx src/components/PageTransition.tsx src/components/ParticleBackground.tsx src/css/firefox-performance.css src/css/main.css src/hooks/useScrollAnimation.ts src/main.tsx
Comment on lines
+93
to
+95
| .is-firefox [data-theme="matrix"] body::after { | ||
| opacity: 0.2; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== data-theme assignments in source =="
rg -nP 'data-theme|setAttribute\(\s*["'\'']data-theme["'\'']|dataset\.theme' --type=ts --type=tsx --type=js --type=jsx --type=html src
echo
echo "== matrix + body::after selectors =="
rg -nP '\[data-theme="matrix"\].*body::after|body\[data-theme="matrix"\]::after|body::after' --type=css src/cssRepository: ThomasJButler/thomasjbutler.github.io
Length of output: 164
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Searching for data-theme assignments =="
rg 'data-theme' src --type=ts --type=js --type=html -n
echo ""
echo "== CSS selectors involving data-theme in firefox-performance.css =="
cat -n src/css/firefox-performance.css | grep -A2 -B2 'data-theme' || echo "Not found with grep, checking with ripgrep..."
rg 'data-theme' src/css/firefox-performance.css -n
echo ""
echo "== Check main.tsx for is-firefox class application =="
rg 'is-firefox' src -n
echo ""
echo "== Check for body element with data-theme =="
rg 'body.*data-theme|data-theme.*body' src --type=ts --type=js --type=html -nRepository: ThomasJButler/thomasjbutler.github.io
Length of output: 4195
Fix unreachable CSS selector for scanline opacity override.
Line 93 (.is-firefox [data-theme="matrix"] body::after) is unreachable because data-theme is applied to the html element itself (via document.documentElement.setAttribute('data-theme', theme) in ThemeContext.tsx), not to a descendant. The current selector tries to match [data-theme="matrix"] as a descendant of .is-firefox (which is the html element), making it impossible to match.
Proposed fix
-.is-firefox [data-theme="matrix"] body::after {
+.is-firefox[data-theme="matrix"] body::after {
opacity: 0.2;
}Combine the .is-firefox and [data-theme="matrix"] selectors on the same element (html) since both conditions are applied to it.
📝 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.
Suggested change
| .is-firefox [data-theme="matrix"] body::after { | |
| opacity: 0.2; | |
| } | |
| .is-firefox[data-theme="matrix"] body::after { | |
| opacity: 0.2; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/css/firefox-performance.css` around lines 93 - 95, The selector
.is-firefox [data-theme="matrix"] body::after is unreachable because data-theme
is set on the same element as .is-firefox; update the selector to combine them
on the same element (e.g. .is-firefox[data-theme="matrix"] body::after or
html.is-firefox[data-theme="matrix"] body::after) so the scanline opacity
override actually matches and applies.
Comment on lines
+182
to
+191
| // Cache layout values to avoid getBoundingClientRect() on every scroll frame | ||
| let elementTop = element.offsetTop; | ||
| let elementHeight = element.offsetHeight; | ||
| const windowHeight = window.innerHeight; | ||
| let rafPending = false; | ||
|
|
||
| const updateLayout = () => { | ||
| elementTop = element.offsetTop; | ||
| elementHeight = element.offsetHeight; | ||
| }; |
There was a problem hiding this comment.
windowHeight is not updated on resize, causing stale visibility calculations.
The windowHeight is captured once at line 185 but updateLayout() (lines 188-191) only updates elementTop and elementHeight. After a window resize, windowHeight remains stale, making the visibility check at line 199 incorrect.
🐛 Proposed fix
+ let windowHeight = window.innerHeight;
- const windowHeight = window.innerHeight;
let rafPending = false;
const updateLayout = () => {
elementTop = element.offsetTop;
elementHeight = element.offsetHeight;
+ windowHeight = window.innerHeight;
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/useScrollAnimation.ts` around lines 182 - 191, The visibility
calculation uses a stale windowHeight captured once; modify updateLayout() to
also refresh windowHeight = window.innerHeight (in addition to elementTop and
elementHeight) and ensure updateLayout is invoked on window resize by adding a
resize event listener that calls updateLayout (and remove the listener in the
cleanup). Update references to elementTop, elementHeight, windowHeight, and
updateLayout accordingly so visibility checks use the current window height.
ThomasJButler
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
setIntervalin CRTEffect with RAF, eliminates layout thrashing inuseParallaxscroll handler, fixes anime.js v4 API mismatch in PageTransition.Files changed
CLAUDE.mdsrc/main.tsxis-firefoxclass)src/css/firefox-performance.csssrc/css/main.csssrc/components/MatrixRain.tsxsrc/components/ParticleBackground.tsxsrc/components/CRTEffect.tsxsrc/components/PageTransition.tsxsrc/hooks/useScrollAnimation.tsTest plan
npm run buildpassesSummary by CodeRabbit
Style
Refactor
Documentation