Fix(webapp): Dynamically load spline on 404 on page #2834
Conversation
|
WalkthroughReplaces the Spline React component usage in ErrorDisplay with a newly added TriggerRotatingLogo component. Adds TriggerRotatingLogo which lazy-loads the external Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Review CompleteYour review story is ready! Comment !reviewfast on this PR to re-generate the story. |
![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
Fix all issues with AI Agents 🤖
In @apps/webapp/app/components/ErrorDisplay.tsx:
- Around line 59-67: The useEffect that injects the Spline viewer script can
cause race conditions and hydration mismatches because <spline-viewer> is
rendered immediately; update the ErrorDisplay component to track loading state
(e.g., isSplineReady / splineError) and only render the <spline-viewer> element
when the script has loaded successfully, attach script.onload and script.onerror
handlers to set those states, include cleanup to remove listeners in the effect,
and when appending the script set integrity and crossOrigin attributes (using
the SRI hash) to mitigate CDN security risks and show a user-friendly
fallback/UI when splineError is true.
- Line 64: The CDN script src is pointing to a non-existent @splinetool/viewer
version 1.12.29 which will cause runtime load failures; update the string
assigned to script.src in ErrorDisplay (the line setting script.src = "...") to
reference a released version such as 1.12.23 so the browser can successfully
load the Spline viewer.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/webapp/app/components/ErrorDisplay.tsxapps/webapp/package.json
💤 Files with no reviewable changes (1)
- apps/webapp/package.json
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
Files:
apps/webapp/app/components/ErrorDisplay.tsx
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
apps/webapp/app/components/ErrorDisplay.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Files:
apps/webapp/app/components/ErrorDisplay.tsx
apps/webapp/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access all environment variables through the
envexport ofenv.server.tsinstead of directly accessingprocess.envin the Trigger.dev webapp
Files:
apps/webapp/app/components/ErrorDisplay.tsx
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: When importing from@trigger.dev/corein the webapp, use subpath exports from the package.json instead of importing from the root path
Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webapp
Files:
apps/webapp/app/components/ErrorDisplay.tsx
**/*.{js,ts,jsx,tsx,json,md,css,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier
Files:
apps/webapp/app/components/ErrorDisplay.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
apps/webapp/app/components/ErrorDisplay.tsx (2)
10-22: LGTM: TypeScript declaration for custom element.The global JSX namespace extension correctly declares the
spline-viewercustom element for TypeScript. The use ofinterfacehere is appropriate sinceJSX.IntrinsicElementsmust be an interface.
89-93: Custom element usage is correct, pending script loading fix.The
spline-viewercustom element is used correctly with appropriate attributes. However, this depends on the race condition fix from lines 59-67 to ensure the element is defined before rendering.
| import Spline from "@splinetool/react-spline"; | ||
| import { type ReactNode } from "react"; | ||
| import { type ReactNode, useEffect } from "react"; | ||
|
|
There was a problem hiding this comment.
Let's pull this out into a separate TriggerRotatingLogo component so it doesn't contaminate this file.
Also pump the issue below into Claude and see what it thinks about the race condition
Code Review: PR #2834 - Load spline 404 on pageSummaryThis PR removes the bundled ✅ What's Good
|
samejr
changed the title
PR Review: Load Spline 404 on PageThanks for this PR! The approach of dynamically loading the Spline viewer to reduce bundle size is solid. Here's my feedback: ✅ What's Good
🔍 Suggestions & Considerations1. Script Cleanup on UnmountThe script element is appended to useEffect(() => {
if (customElements.get("spline-viewer")) {
setIsSplineReady(true);
return;
}
const script = document.createElement("script");
script.type = "module";
script.src = "https://unpkg.com/@splinetool/viewer@1.12.29/build/spline-viewer.js";
script.onload = () => setIsSplineReady(true);
document.head.appendChild(script);
return () => {
document.head.removeChild(script);
};
}, []);However, if this component could be re-mounted multiple times, you'd want to avoid removing the script (since the web component would still be registered). The current approach is actually safer for that scenario. 2. Consider Pinning with Integrity HashUsing unpkg is convenient, but consider adding a Subresource Integrity (SRI) hash for security: script.integrity = "sha384-..."; // Generate from the specific version
script.crossOrigin = "anonymous";This ensures the script hasn't been tampered with. Alternatively, if self-hosting is an option, that would provide more control over the dependency. 3. TypeScript Declaration ScopeThe global JSX namespace extension works but pollutes the global type space. Consider if this could be scoped to this file only, though for a web component like this, the global declaration is a common pattern. 4. Version PinningGood job pinning the version ( 📋 Minor Nit
🧪 Test CoverageThere don't appear to be existing tests for 🔒 Security
📦 Performance
Overall: This is a clean, well-structured change. The approach of loading Spline dynamically rather than bundling it is the right call for a component only used on error pages. The suggestions above are minor improvements rather than blocking issues. LGTM with optional improvements! 👍 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @apps/webapp/app/components/TriggerRotatingLogo.tsx:
- Around line 21-34: Prevent multiple script inserts and handle load errors by
introducing a single shared loader promise (e.g., window.__splineLoader) used
inside the useEffect in TriggerRotatingLogo; replace the naive
customElements.get + direct append logic with: if
customElements.get("spline-viewer") setIsSplineReady(true) else if
window.__splineLoader await or then on that promise to call
setIsSplineReady(true) or setIsSplineReady(false) on rejection, and if no
window.__splineLoader create it, append one script tag with src
"https://unpkg.com/@splinetool/viewer@1.12.29/build/spline-viewer.js", wire
script.onload to resolve the promise and script.onerror to reject it, and store
that promise on window.__splineLoader so simultaneous mounts share it.
- Line 30: The script tag in TriggerRotatingLogo.tsx sets script.src to
"@splinetool/viewer@1.12.29" which appears newer than the latest published
1.12.23; verify whether 1.12.29 is intentional (pre-release) or change the src
to the latest stable version (e.g., 1.12.23) and add Subresource Integrity and
CORS attributes by setting script.integrity to the correct SRI hash and
script.crossOrigin = "anonymous" where the script element is created (refer to
the script.src assignment in TriggerRotatingLogo.tsx).
🧹 Nitpick comments (1)
apps/webapp/app/components/TriggerRotatingLogo.tsx (1)
40-52: Add accessibility attribute for decorative content.Since this is a decorative element (indicated by
pointer-events-none), it should be hidden from assistive technologies.♿ Proposed fix to improve accessibility
return ( <motion.div className="pointer-events-none absolute inset-0 overflow-hidden" + aria-hidden="true" initial={{ opacity: 0 }} animate={{ opacity: 1 }} transition={{ delay: 0.5, duration: 2, ease: "easeOut" }} >
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/webapp/app/components/ErrorDisplay.tsxapps/webapp/app/components/TriggerRotatingLogo.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webapp/app/components/ErrorDisplay.tsx
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
Files:
apps/webapp/app/components/TriggerRotatingLogo.tsx
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
apps/webapp/app/components/TriggerRotatingLogo.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Files:
apps/webapp/app/components/TriggerRotatingLogo.tsx
apps/webapp/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access all environment variables through the
envexport ofenv.server.tsinstead of directly accessingprocess.envin the Trigger.dev webapp
Files:
apps/webapp/app/components/TriggerRotatingLogo.tsx
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: When importing from@trigger.dev/corein the webapp, use subpath exports from the package.json instead of importing from the root path
Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webapp
Files:
apps/webapp/app/components/TriggerRotatingLogo.tsx
**/*.{js,ts,jsx,tsx,json,md,css,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier
Files:
apps/webapp/app/components/TriggerRotatingLogo.tsx
🧠 Learnings (1)
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use lifecycle functions (init, cleanup, onStart, onSuccess, onFailure, handleError) for task event handling
Applied to files:
apps/webapp/app/components/TriggerRotatingLogo.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: claude-review
- GitHub Check: Analyze (javascript-typescript)
PR Review: Dynamically load Spline on 404 pageSummaryThis PR removes the ✅ What looks good
|
PR Review: Dynamically load Spline on 404 pageThank you for this PR! The approach of dynamically loading the Spline library is a solid improvement - it removes a bundled dependency and only loads the 3D viewer when needed (on error pages). Here's my feedback: ✅ What's Good
🔍 Suggestions & Questions
🔒 SecurityLoading from unpkg.com is standard practice. The comment about SRI hashes not being stable is accurate - unpkg doesn't guarantee content-addressable hashes. If stricter security is needed in the future, consider self-hosting or using a CDN with SRI support. 🧪 Test CoverageThis is a UI component with browser-specific behavior (custom elements, dynamic script loading). Testing would require a browser environment. Given this is a 404 page component and the logic is relatively straightforward, manual testing is probably sufficient. Worth verifying:
Minor NitThe framer-motion import was removed from Overall: This is a clean refactor that improves bundle size without sacrificing functionality. The code is well-structured with good attention to edge cases (duplicate loads, custom element registration checks). 👍 Looks good to merge after considering the optional feedback above. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @apps/webapp/app/components/TriggerRotatingLogo.tsx:
- Around line 25-55: The promise handlers in useEffect
(window.__splineLoader.then(...) and the onload/onerror handlers that call
setIsSplineReady) can call setState after the component unmounts; fix by adding
a mounted flag inside the useEffect (e.g., let mounted = true), replace direct
setIsSplineReady(...) calls with guarded calls (if (mounted)
setIsSplineReady(true/false)), and return a cleanup function that sets mounted =
false so any late-resolving window.__splineLoader or script callbacks won't
update state after unmount; ensure you update both places where setIsSplineReady
is used (the existing window.__splineLoader.then/catch and the
script.onload/script.onerror resolver/rejector handlers).
🧹 Nitpick comments (1)
apps/webapp/app/components/TriggerRotatingLogo.tsx (1)
4-20: TypeScript declarations look good; consider stricter typing for loading-anim-type.The JSX intrinsic element and Window interface augmentations are well-structured. The
loading-anim-typeprop could optionally use a string union type for better type safety if the Spline viewer supports a fixed set of values.♻️ Example with stricter typing
declare global { namespace JSX { interface IntrinsicElements { "spline-viewer": React.DetailedHTMLProps< React.HTMLAttributes<HTMLElement> & { url?: string; - "loading-anim-type"?: string; + "loading-anim-type"?: "spinner-small-light" | "spinner-small-dark" | "none"; }, HTMLElement >; } }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
apps/webapp/app/components/TriggerRotatingLogo.tsx
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
Files:
apps/webapp/app/components/TriggerRotatingLogo.tsx
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
apps/webapp/app/components/TriggerRotatingLogo.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Files:
apps/webapp/app/components/TriggerRotatingLogo.tsx
apps/webapp/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access all environment variables through the
envexport ofenv.server.tsinstead of directly accessingprocess.envin the Trigger.dev webapp
Files:
apps/webapp/app/components/TriggerRotatingLogo.tsx
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: When importing from@trigger.dev/corein the webapp, use subpath exports from the package.json instead of importing from the root path
Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webapp
Files:
apps/webapp/app/components/TriggerRotatingLogo.tsx
**/*.{js,ts,jsx,tsx,json,md,css,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier
Files:
apps/webapp/app/components/TriggerRotatingLogo.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
| useEffect(() => { | ||
| // Already registered from a previous render | ||
| if (customElements.get("spline-viewer")) { | ||
| setIsSplineReady(true); | ||
| return; | ||
| } | ||
|
|
||
| // Another mount already started loading - share the same promise | ||
| if (window.__splineLoader) { | ||
| window.__splineLoader.then(() => setIsSplineReady(true)).catch(() => setIsSplineReady(false)); | ||
| return; | ||
| } | ||
|
|
||
| // First mount: create script and shared loader promise | ||
| const script = document.createElement("script"); | ||
| script.type = "module"; | ||
| // Version pinned; SRI hash omitted as unpkg doesn't guarantee hash stability across deploys | ||
| script.src = "https://unpkg.com/@splinetool/viewer@1.12.29/build/spline-viewer.js"; | ||
|
|
||
| window.__splineLoader = new Promise<void>((resolve, reject) => { | ||
| script.onload = () => resolve(); | ||
| script.onerror = () => reject(); | ||
| }); | ||
|
|
||
| window.__splineLoader.then(() => setIsSplineReady(true)).catch(() => setIsSplineReady(false)); | ||
|
|
||
| document.head.appendChild(script); | ||
|
|
||
| // Intentionally no cleanup: once the custom element is registered globally, | ||
| // removing the script would break re-mounts while providing no benefit | ||
| }, []); |
There was a problem hiding this comment.
Race condition successfully addressed; prevent setState after unmount.
The shared window.__splineLoader promise elegantly solves the race condition flagged in the previous review. However, the promise handlers on lines 34 and 49 call setIsSplineReady without checking if the component is still mounted, which could cause warnings or unexpected behavior if the component unmounts before the script loads.
🔧 Proposed fix using cleanup pattern
export function TriggerRotatingLogo() {
const [isSplineReady, setIsSplineReady] = useState(false);
useEffect(() => {
+ let mounted = true;
+
// Already registered from a previous render
if (customElements.get("spline-viewer")) {
setIsSplineReady(true);
return;
}
// Another mount already started loading - share the same promise
if (window.__splineLoader) {
- window.__splineLoader.then(() => setIsSplineReady(true)).catch(() => setIsSplineReady(false));
+ window.__splineLoader
+ .then(() => mounted && setIsSplineReady(true))
+ .catch(() => mounted && setIsSplineReady(false));
return;
}
// First mount: create script and shared loader promise
const script = document.createElement("script");
script.type = "module";
// Version pinned; SRI hash omitted as unpkg doesn't guarantee hash stability across deploys
script.src = "https://unpkg.com/@splinetool/viewer@1.12.29/build/spline-viewer.js";
window.__splineLoader = new Promise<void>((resolve, reject) => {
script.onload = () => resolve();
script.onerror = () => reject();
});
- window.__splineLoader.then(() => setIsSplineReady(true)).catch(() => setIsSplineReady(false));
+ window.__splineLoader
+ .then(() => mounted && setIsSplineReady(true))
+ .catch(() => mounted && setIsSplineReady(false));
document.head.appendChild(script);
// Intentionally no cleanup: once the custom element is registered globally,
// removing the script would break re-mounts while providing no benefit
+
+ return () => {
+ mounted = false;
+ };
}, []);🤖 Prompt for AI Agents
In @apps/webapp/app/components/TriggerRotatingLogo.tsx around lines 25 - 55, The
promise handlers in useEffect (window.__splineLoader.then(...) and the
onload/onerror handlers that call setIsSplineReady) can call setState after the
component unmounts; fix by adding a mounted flag inside the useEffect (e.g., let
mounted = true), replace direct setIsSplineReady(...) calls with guarded calls
(if (mounted) setIsSplineReady(true/false)), and return a cleanup function that
sets mounted = false so any late-resolving window.__splineLoader or script
callbacks won't update state after unmount; ensure you update both places where
setIsSplineReady is used (the existing window.__splineLoader.then/catch and the
script.onload/script.onerror resolver/rejector handlers).
Remove the spline dependency and load it dynamically on the 404 page.
Uses a web component with a workaround for React.