Animation on hover w/ Gleam Logo #2807
Conversation
WalkthroughA new Ember component, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GleamLogoComponent
participant RiveLibrary
User->>GleamLogoComponent: Insert component into DOM
GleamLogoComponent->>RiveLibrary: Initialize Rive with canvas and .riv file
RiveLibrary-->>GleamLogoComponent: Animation loaded
GleamLogoComponent->>RiveLibrary: Set hover state input true
GleamLogoComponent->>RiveLibrary: Reset hover state input false after 1s
User->>GleamLogoComponent: Component removed from DOM
GleamLogoComponent->>RiveLibrary: Stop animation and cleanup
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🪛 Biome (1.9.4)tests/integration/components/gleam-logo-test.js[error] 77-77: The constructor should not return a value. The constructor is here: Returning a value from a constructor is ignored. (lint/correctness/noConstructorReturn) 🔇 Additional comments (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Bundle ReportChanges will increase total bundle size by 126.78kB (0.35%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: client-array-pushAssets Changed:
|
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
app/components/gleam-logo.ts (4)
21-21: Remove debugging border before production.The red border around the canvas is useful for development but should be removed before merging to production.
- canvas.style.border = '1px solid red'; // Visual debugging
30-44: Remove or disable console logging statements.There are numerous console.log statements throughout the code that should be removed or disabled before production deployment.
Consider removing these debugging log statements or wrapping them in a conditional that only executes in development environments:
- console.log('Gleam logo animation loaded'); - - // Log available state machines - const stateMachines = this.riveInstance?.stateMachineNames; - console.log('All State Machines:', stateMachines); - - if (stateMachines && stateMachines.length > 0) { - // Log details about each state machine - stateMachines.forEach((name, index) => { - console.log(`State Machine ${index + 1}:`, { - name, - inputs: this.riveInstance?.stateMachineInputs(name), - }); - }); - } + const stateMachines = this.riveInstance?.stateMachineNames;
6-111: Consider implementing a more robust error handling strategy.The current error handling only logs errors to the console, which may not be sufficient for production.
Consider implementing a more comprehensive error handling strategy:
- Add error state tracking to display fallback content when Rive fails to load
- Add error reporting to your application's error tracking system
- Gracefully degrade to a static image when the animation fails
This would provide a better user experience in case of animation loading failures.
🧰 Tools
🪛 ESLint
[error] 50-72: Expected blank line before this statement.
(padding-line-between-statements)
[error] 82-93: Member handleMouseEnter should be declared before member setupRive.
(@typescript-eslint/member-ordering)
[error] 86-91: Expected blank line before this statement.
(padding-line-between-statements)
[error] 104-110: Member cleanupRive should be declared before member handleMouseLeave.
(@typescript-eslint/member-ordering)
🪛 GitHub Actions: Test
[error] 50-50: ESLint: Expected blank line before this statement (padding-line-between-statements).
[error] 82-82: ESLint: Member 'handleMouseEnter' should be declared before member 'setupRive' (@typescript-eslint/member-ordering).
[error] 86-86: ESLint: Expected blank line before this statement (padding-line-between-statements).
[error] 104-104: ESLint: Member 'cleanupRive' should be declared before member 'handleMouseLeave' (@typescript-eslint/member-ordering).
83-93: Extract state machine selection to a dedicated method.The same pattern for getting the first state machine is duplicated in both mouse event handlers.
Consider extracting this logic to a dedicated method:
+ private getDefaultStateMachine(): string | undefined { + if (this.riveInstance) { + const stateMachines = this.riveInstance.stateMachineNames; + if (stateMachines && stateMachines.length > 0) { + return stateMachines[0]; + } + } + return undefined; + } @action handleMouseEnter() { if (this.riveInstance) { - const stateMachines = this.riveInstance.stateMachineNames; - if (stateMachines && stateMachines.length > 0) { - const stateMachineName = stateMachines[0]; + const stateMachineName = this.getDefaultStateMachine(); + if (stateMachineName) { console.log('Playing hover animation with state machine:', stateMachineName); this.riveInstance.reset(); this.riveInstance.play(stateMachineName); } } }🧰 Tools
🪛 ESLint
[error] 82-93: Member handleMouseEnter should be declared before member setupRive.
(@typescript-eslint/member-ordering)
[error] 86-91: Expected blank line before this statement.
(padding-line-between-statements)
🪛 GitHub Actions: Test
[error] 86-86: ESLint: Expected blank line before this statement (padding-line-between-statements).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
app/components/gleam-logo.hbs(1 hunks)app/components/gleam-logo.ts(1 hunks)app/components/language-logo.hbs(1 hunks)app/components/language-logo.ts(1 hunks)app/styles/components/gleam-logo.css(1 hunks)package.json(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Test
app/components/language-logo.hbs
[error] 2-2: TS2345: Argument of type 'unknown' is not assignable to parameter of type 'HTMLDivElement'.
app/components/gleam-logo.ts
[error] 50-50: ESLint: Expected blank line before this statement (padding-line-between-statements).
[error] 82-82: ESLint: Member 'handleMouseEnter' should be declared before member 'setupRive' (@typescript-eslint/member-ordering).
[error] 86-86: ESLint: Expected blank line before this statement (padding-line-between-statements).
[error] 104-104: ESLint: Member 'cleanupRive' should be declared before member 'handleMouseLeave' (@typescript-eslint/member-ordering).
🪛 ESLint
app/components/gleam-logo.ts
[error] 50-72: Expected blank line before this statement.
(padding-line-between-statements)
[error] 82-93: Member handleMouseEnter should be declared before member setupRive.
(@typescript-eslint/member-ordering)
[error] 86-91: Expected blank line before this statement.
(padding-line-between-statements)
[error] 104-110: Member cleanupRive should be declared before member handleMouseLeave.
(@typescript-eslint/member-ordering)
🔇 Additional comments (6)
package.json (1)
205-205: LGTM - Proper dependency added for Rive animations.The addition of
@rive-app/canvasas a dependency is appropriate for implementing the Gleam logo animation feature. The caret version syntax (^2.27.0) follows project conventions and allows for compatible updates.app/styles/components/gleam-logo.css (1)
1-12: CSS implementation looks clean and effective.The CSS is well-structured with appropriate use of:
- Block-level display for the container and canvas
- 100% dimensions to ensure the animation fills its allocated space
object-fit: containto maintain proper scaling of the canvas contentThis implementation follows best practices for embedding canvas animations within responsive layouts.
app/components/language-logo.ts (1)
18-18: LGTM - Proper Glint registry registration.The component is correctly registered in the Glint registry, which ensures proper type checking when using the
<GleamLogo />component in templates.app/components/gleam-logo.hbs (2)
1-9: LGTM - Well-structured template with proper lifecycle hooks.The template effectively:
- Establishes a container for the Rive animation
- Uses appropriate lifecycle hooks (
did-insertandwill-destroy) to manage animation setup and cleanup- Implements mouse event handlers for the hover animation effect
- Includes a helpful comment about canvas insertion
This is a clean implementation that follows Ember best practices.
1-9:✅ Verification successful
Verify that companion TS component implements all referenced methods.
Please ensure that the
gleam-logo.tscomponent properly implements all the methods referenced in this template:setupRive,cleanupRive,handleMouseEnter, andhandleMouseLeave.
🏁 Script executed:
#!/bin/bash # Check if the TypeScript component file exists and contains the required methods echo "Checking gleam-logo.ts implementation..." file_path="app/components/gleam-logo.ts" if [ -f "$file_path" ]; then echo "File exists: $file_path" echo "Checking for required methods:" grep -n "setupRive" "$file_path" && echo "✓ setupRive found" || echo "✗ setupRive NOT found" grep -n "cleanupRive" "$file_path" && echo "✓ cleanupRive found" || echo "✗ cleanupRive NOT found" grep -n "handleMouseEnter" "$file_path" && echo "✓ handleMouseEnter found" || echo "✗ handleMouseEnter NOT found" grep -n "handleMouseLeave" "$file_path" && echo "✓ handleMouseLeave found" || echo "✗ handleMouseLeave NOT found" else echo "ERROR: File not found: $file_path" echo "The template references methods that should be implemented in this file." fiLength of output: 874
All referenced methods are implemented in app/components/gleam-logo.ts.
Verified thatsetupRive,cleanupRive,handleMouseEnter, andhandleMouseLeaveare present—no further changes needed.app/components/gleam-logo.ts (1)
17-20: Verify if the fixed canvas dimensions are appropriate.The canvas is set to fixed dimensions (141×144) but displayed with 100% width and height, which could lead to stretching or distortion.
Verify that the fixed dimensions match the aspect ratio of the Gleam logo and that the scaling approach produces crisp rendering without distortion across different display sizes. Consider if using viewBox or preserveAspectRatio equivalent settings might be more appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
app/components/gleam-logo.ts (2)
28-40: Fix ESLint member ordering issues.The member ordering still doesn't match ESLint requirements as mentioned in previous reviews. Move the
handleMouseEntermethod beforesetupRiveto resolve the linting error.- @action - handleMouseEnter() { - // ... current implementation - } - - @action - setupRive(element: HTMLDivElement) { - // ... current implementation - } + @action + handleMouseEnter() { + // ... current implementation + } + + @action + setupRive(element: HTMLDivElement) { + // ... current implementation + }Also applies to: 51-122
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 32-32: app/components/gleam-logo.ts#L32
Added line #L32 was not covered by tests
[warning] 35-38: app/components/gleam-logo.ts#L35-L38
Added lines #L35 - L38 were not covered by tests
72-116: Remove development console logs and simplify initial animation.There are numerous console logs and an unnecessary initial animation sequence that should be removed for production code. This was mentioned in a previous review but hasn't been addressed.
- console.log('Gleam logo animation loaded'); - - // Log available state machines - const stateMachines = this.riveInstance?.stateMachineNames; - console.log('All State Machines:', stateMachines); - - if (stateMachines && stateMachines.length > 0) { - // Log details about each state machine - stateMachines.forEach((name, index) => { - console.log(`State Machine ${index + 1}:`, { - name, - inputs: this.riveInstance?.stateMachineInputs(name), - }); - }); - } - - // Play initial animation directly with longer delay - setTimeout(() => { - if (this.riveInstance) { - const stateMachines = this.riveInstance.stateMachineNames; - - if (stateMachines && stateMachines.length > 0) { - // Try each state machine - stateMachines.forEach((stateMachineName) => { - console.log('Attempting to play with state machine:', stateMachineName); - - // Reset and play - this.riveInstance?.reset(); - this.riveInstance?.play(stateMachineName); - - // Log animation state after a short delay - setTimeout(() => { - if (this.riveInstance) { - console.log('Animation state after play:', { - stateMachine: stateMachineName, - isPlaying: this.riveInstance.isPlaying, - isPaused: this.riveInstance.isPaused, - isStopped: this.riveInstance.isStopped, - allStateMachines: this.riveInstance.stateMachineNames, - }); - } - }, 500); - }); - } - } - }, 2000);🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 72-72: app/components/gleam-logo.ts#L72
Added line #L72 was not covered by tests
[warning] 75-76: app/components/gleam-logo.ts#L75-L76
Added lines #L75 - L76 were not covered by tests
[warning] 80-81: app/components/gleam-logo.ts#L80-L81
Added lines #L80 - L81 were not covered by tests
[warning] 89-89: app/components/gleam-logo.ts#L89
Added line #L89 was not covered by tests
[warning] 91-91: app/components/gleam-logo.ts#L91
Added line #L91 was not covered by tests
[warning] 95-96: app/components/gleam-logo.ts#L95-L96
Added lines #L95 - L96 were not covered by tests
[warning] 99-100: app/components/gleam-logo.ts#L99-L100
Added lines #L99 - L100 were not covered by tests
[warning] 103-103: app/components/gleam-logo.ts#L103
Added line #L103 was not covered by tests
[warning] 105-105: app/components/gleam-logo.ts#L105
Added line #L105 was not covered by tests
🧹 Nitpick comments (3)
app/components/gleam-logo.ts (3)
62-62: Remove visual debugging border.The red border appears to be for development debugging purposes and should be removed before deploying to production.
- canvas.style.border = '1px solid red'; // Visual debugging
35-35: Remove console log from mouse event handler.Remove the debug console log from the production code:
- console.log('Playing hover animation with state machine:', stateMachineName);🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 35-38: app/components/gleam-logo.ts#L35-L38
Added lines #L35 - L38 were not covered by tests
57-63: Consider making canvas dimensions configurable or responsive.The canvas has hard-coded dimensions (141x144). Consider making these dimensions configurable through component arguments or using relative sizing based on the container for better reusability and responsiveness.
- canvas.width = 141; // Fixed size for crisp rendering - canvas.height = 144; + canvas.width = this.args.width || 141; + canvas.height = this.args.height || 144;Also update the component interface:
Args: { class?: string; style?: string; + width?: number; + height?: number; [key: string]: unknown; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/components/gleam-logo.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
app/components/gleam-logo.ts
[warning] 18-19: app/components/gleam-logo.ts#L18-L19
Added lines #L18 - L19 were not covered by tests
[warning] 24-25: app/components/gleam-logo.ts#L24-L25
Added lines #L24 - L25 were not covered by tests
[warning] 32-32: app/components/gleam-logo.ts#L32
Added line #L32 was not covered by tests
[warning] 35-38: app/components/gleam-logo.ts#L35-L38
Added lines #L35 - L38 were not covered by tests
[warning] 47-48: app/components/gleam-logo.ts#L47-L48
Added lines #L47 - L48 were not covered by tests
[warning] 54-54: app/components/gleam-logo.ts#L54
Added line #L54 was not covered by tests
[warning] 56-56: app/components/gleam-logo.ts#L56
Added line #L56 was not covered by tests
[warning] 58-64: app/components/gleam-logo.ts#L58-L64
Added lines #L58 - L64 were not covered by tests
[warning] 67-67: app/components/gleam-logo.ts#L67
Added line #L67 was not covered by tests
[warning] 72-72: app/components/gleam-logo.ts#L72
Added line #L72 was not covered by tests
[warning] 75-76: app/components/gleam-logo.ts#L75-L76
Added lines #L75 - L76 were not covered by tests
[warning] 80-81: app/components/gleam-logo.ts#L80-L81
Added lines #L80 - L81 were not covered by tests
[warning] 89-89: app/components/gleam-logo.ts#L89
Added line #L89 was not covered by tests
[warning] 91-91: app/components/gleam-logo.ts#L91
Added line #L91 was not covered by tests
[warning] 95-96: app/components/gleam-logo.ts#L95-L96
Added lines #L95 - L96 were not covered by tests
[warning] 99-100: app/components/gleam-logo.ts#L99-L100
Added lines #L99 - L100 were not covered by tests
[warning] 103-103: app/components/gleam-logo.ts#L103
Added line #L103 was not covered by tests
[warning] 105-105: app/components/gleam-logo.ts#L105
Added line #L105 was not covered by tests
[warning] 121-121: app/components/gleam-logo.ts#L121
Added line #L121 was not covered by tests
🔇 Additional comments (1)
app/components/gleam-logo.ts (1)
1-123: Add unit tests for the new component.The static analysis shows this new component has no test coverage. Consider adding unit tests to verify:
- Component initialization
- Animation lifecycle events
- Mouse interaction behavior
This will prevent regressions and ensure the component works as expected across browsers.
Would you like me to generate a basic test skeleton for this component?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 18-19: app/components/gleam-logo.ts#L18-L19
Added lines #L18 - L19 were not covered by tests
[warning] 24-25: app/components/gleam-logo.ts#L24-L25
Added lines #L24 - L25 were not covered by tests
[warning] 32-32: app/components/gleam-logo.ts#L32
Added line #L32 was not covered by tests
[warning] 35-38: app/components/gleam-logo.ts#L35-L38
Added lines #L35 - L38 were not covered by tests
[warning] 47-48: app/components/gleam-logo.ts#L47-L48
Added lines #L47 - L48 were not covered by tests
[warning] 54-54: app/components/gleam-logo.ts#L54
Added line #L54 was not covered by tests
[warning] 56-56: app/components/gleam-logo.ts#L56
Added line #L56 was not covered by tests
[warning] 58-64: app/components/gleam-logo.ts#L58-L64
Added lines #L58 - L64 were not covered by tests
[warning] 67-67: app/components/gleam-logo.ts#L67
Added line #L67 was not covered by tests
[warning] 72-72: app/components/gleam-logo.ts#L72
Added line #L72 was not covered by tests
[warning] 75-76: app/components/gleam-logo.ts#L75-L76
Added lines #L75 - L76 were not covered by tests
[warning] 80-81: app/components/gleam-logo.ts#L80-L81
Added lines #L80 - L81 were not covered by tests
[warning] 89-89: app/components/gleam-logo.ts#L89
Added line #L89 was not covered by tests
[warning] 91-91: app/components/gleam-logo.ts#L91
Added line #L91 was not covered by tests
[warning] 95-96: app/components/gleam-logo.ts#L95-L96
Added lines #L95 - L96 were not covered by tests
[warning] 99-100: app/components/gleam-logo.ts#L99-L100
Added lines #L99 - L100 were not covered by tests
[warning] 103-103: app/components/gleam-logo.ts#L103
Added line #L103 was not covered by tests
[warning] 105-105: app/components/gleam-logo.ts#L105
Added line #L105 was not covered by tests
[warning] 121-121: app/components/gleam-logo.ts#L121
Added line #L121 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
app/components/gleam-logo.ts (4)
34-48: Extract state machine names to constantsState machine names like 'State Machine 1' and 'State Machine 2' are hardcoded in multiple places. Consider extracting these to class constants for better maintainability.
export default class GleamLogoComponent extends Component<GleamLogoSignature> { + // State machine names + private readonly HOVER_STATE_MACHINE = 'State Machine 1'; + private readonly IDLE_STATE_MACHINE = 'State Machine 2'; + @tracked riveInstance: Rive | null = null; container: HTMLElement | null = null; animationInterval: number | null = null; // ... other methods @action handleMouseEnter() { if (this.riveInstance) { const stateMachines = this.riveInstance.stateMachineNames; if (stateMachines && stateMachines.length > 0) { - const stateMachineName = 'State Machine 1'; + const stateMachineName = this.HOVER_STATE_MACHINE; if (stateMachines.includes(stateMachineName)) { this.riveInstance.reset(); this.riveInstance.play(stateMachineName); } } } }Also applies to: 50-56
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 38-38: app/components/gleam-logo.ts#L38
Added line #L38 was not covered by tests
[warning] 41-41: app/components/gleam-logo.ts#L41
Added line #L41 was not covered by tests
[warning] 44-45: app/components/gleam-logo.ts#L44-L45
Added lines #L44 - L45 were not covered by tests
70-75: Make animation source path configurableThe animation source path is hardcoded. Consider making it configurable through the component's arguments to improve reusability.
interface GleamLogoSignature { Element: HTMLDivElement; Args: { class?: string; style?: string; + animationSource?: string; [key: string]: unknown; }; Blocks: Record<string, never>; } // Later in setupRive method: this.riveInstance = new Rive({ - src: '/assets/animations/gleam_logo_animation.riv', + src: this.args.animationSource || '/assets/animations/gleam_logo_animation.riv', canvas: canvas, // ... });🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 71-71: app/components/gleam-logo.ts#L71
Added line #L71 was not covered by tests
96-117: Consider reducing animation complexityThe initial animation setup uses multiple timers with different durations (500ms to start, 800ms to reset). This adds unnecessary complexity and might cause issues if the component is rapidly mounted/unmounted.
Consider simplifying this to a single animation sequence or making the animation behavior more declarative.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 98-98: app/components/gleam-logo.ts#L98
Added line #L98 was not covered by tests
[warning] 100-100: app/components/gleam-logo.ts#L100
Added line #L100 was not covered by tests
[warning] 103-103: app/components/gleam-logo.ts#L103
Added line #L103 was not covered by tests
[warning] 106-107: app/components/gleam-logo.ts#L106-L107
Added lines #L106 - L107 were not covered by tests
[warning] 110-110: app/components/gleam-logo.ts#L110
Added line #L110 was not covered by tests
[warning] 112-112: app/components/gleam-logo.ts#L112
Added line #L112 was not covered by tests
120-123: Add more detailed error handlingWhile catching errors is good, the current implementation only logs to console. Consider adding more robust error handling like:
- Displaying a fallback static image
- Notifying the user or a monitoring system
- Providing more context in the error message
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 122-122: app/components/gleam-logo.ts#L122
Added line #L122 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
app/components/gleam-logo.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
app/components/gleam-logo.ts
[warning] 18-20: app/components/gleam-logo.ts#L18-L20
Added lines #L18 - L20 were not covered by tests
[warning] 25-26: app/components/gleam-logo.ts#L25-L26
Added lines #L25 - L26 were not covered by tests
[warning] 30-31: app/components/gleam-logo.ts#L30-L31
Added lines #L30 - L31 were not covered by tests
[warning] 38-38: app/components/gleam-logo.ts#L38
Added line #L38 was not covered by tests
[warning] 41-41: app/components/gleam-logo.ts#L41
Added line #L41 was not covered by tests
[warning] 44-45: app/components/gleam-logo.ts#L44-L45
Added lines #L44 - L45 were not covered by tests
[warning] 54-55: app/components/gleam-logo.ts#L54-L55
Added lines #L54 - L55 were not covered by tests
[warning] 61-61: app/components/gleam-logo.ts#L61
Added line #L61 was not covered by tests
[warning] 63-69: app/components/gleam-logo.ts#L63-L69
Added lines #L63 - L69 were not covered by tests
[warning] 71-71: app/components/gleam-logo.ts#L71
Added line #L71 was not covered by tests
[warning] 78-78: app/components/gleam-logo.ts#L78
Added line #L78 was not covered by tests
[warning] 80-80: app/components/gleam-logo.ts#L80
Added line #L80 was not covered by tests
[warning] 83-83: app/components/gleam-logo.ts#L83
Added line #L83 was not covered by tests
[warning] 86-87: app/components/gleam-logo.ts#L86-L87
Added lines #L86 - L87 were not covered by tests
[warning] 93-94: app/components/gleam-logo.ts#L93-L94
Added lines #L93 - L94 were not covered by tests
[warning] 98-98: app/components/gleam-logo.ts#L98
Added line #L98 was not covered by tests
[warning] 100-100: app/components/gleam-logo.ts#L100
Added line #L100 was not covered by tests
[warning] 103-103: app/components/gleam-logo.ts#L103
Added line #L103 was not covered by tests
[warning] 106-107: app/components/gleam-logo.ts#L106-L107
Added lines #L106 - L107 were not covered by tests
[warning] 110-110: app/components/gleam-logo.ts#L110
Added line #L110 was not covered by tests
[warning] 112-112: app/components/gleam-logo.ts#L112
Added line #L112 was not covered by tests
[warning] 122-122: app/components/gleam-logo.ts#L122
Added line #L122 was not covered by tests
🔇 Additional comments (2)
app/components/gleam-logo.ts (2)
1-14: Well-structured interface definitionThe component interface is well-defined with proper type annotations for the expected HTML element and component arguments.
21-32: Good resource cleanup implementationThe
cleanupRivemethod properly handles cleanup by clearing intervals and stopping the Rive instance, which prevents memory leaks.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 25-26: app/components/gleam-logo.ts#L25-L26
Added lines #L25 - L26 were not covered by tests
[warning] 30-31: app/components/gleam-logo.ts#L30-L31
Added lines #L30 - L31 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
tests/integration/components/gleam-logo-test.ts (4)
1-12: Imports look good, but there's an unused import.The
triggerEventimport from@ember/test-helperson line 3 is imported but never used in the test file. Consider removing unused imports to keep the codebase clean.- import { render, settled, triggerEvent } from '@ember/test-helpers'; + import { render, settled } from '@ember/test-helpers';🧰 Tools
🪛 ESLint
[error] 3-3: 'triggerEvent' is defined but never used.
(@typescript-eslint/no-unused-vars)
78-83: Type safety improvement for TestContext interface.The
lookupmethod's return type is specified asany, which should be replaced with a more specific type or at leastunknown.interface TestContext { originalRive: typeof MockRive; owner: { - lookup: (name: string) => any; + lookup: (name: string) => unknown; }; }🧰 Tools
🪛 ESLint
[error] 81-81: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
100-121: First test case looks solid but could use assertion improvement.The test verifies basic rendering and initialization of the Rive animation, but there's a QUnit-specific improvement that could be made.
// Verify state machines are available - assert.deepEqual(mockRive.stateMachineNames, ['State Machine 1', 'State Machine 2'], 'State machines are available'); + assert.deepEqual( + mockRive.stateMachineNames, + ['State Machine 1', 'State Machine 2'], + 'State machines are available' + );🧰 Tools
🪛 ESLint
[error] 113-113: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
1-225: Consider using Ember's built-in testing capabilities more effectively.The current test approach involves manually simulating the component's behavior rather than testing the component's actual implementation. This makes the tests brittle and tightly coupled to implementation details.
A better approach would be to:
- Test the component's public API and behavior rather than reimplementing its internal logic
- Use Ember's testing utilities like
triggerEvent(which is imported but not used)- Focus on verifying observable behaviors rather than internal implementation details
For example, instead of manually attaching event handlers, you could use:
// Trigger events using Ember's testing utilities await triggerEvent('.gleam-logo-container', 'mouseenter');And instead of asserting on internal method calls, verify observable behavior changes.
🧰 Tools
🪛 Biome (1.9.4)
[error] 19-19: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 46-46: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 53-53: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
🪛 ESLint
[error] 3-3: 'triggerEvent' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 18-18: Member isLoaded should be declared before member stateMachineNames.
(@typescript-eslint/member-ordering)
[error] 19-19: Member _listeners should be declared before member isLoaded.
(@typescript-eslint/member-ordering)
[error] 19-19: The
Functiontype accepts any function-like value.
Prefer explicitly defining any function parameters and return type.(@typescript-eslint/no-unsafe-function-type)
[error] 20-20: Member _interval should be declared before member _listeners.
(@typescript-eslint/member-ordering)
[error] 26-28: Member constructor should be declared before all get definitions.
(@typescript-eslint/member-ordering)
[error] 34-36: Member play should be declared before member reset.
(@typescript-eslint/member-ordering)
[error] 34-34: 'stateMachineName' is defined but never used. Allowed unused args must match /^_/u.
(@typescript-eslint/no-unused-vars)
[error] 42-44: Member stateMachineInputs should be declared before member stop.
(@typescript-eslint/member-ordering)
[error] 42-42: 'stateMachineName' is defined but never used. Allowed unused args must match /^_/u.
(@typescript-eslint/no-unused-vars)
[error] 46-51: Member on should be declared before member stateMachineInputs.
(@typescript-eslint/member-ordering)
[error] 46-46: The
Functiontype accepts any function-like value.
Prefer explicitly defining any function parameters and return type.(@typescript-eslint/no-unsafe-function-type)
[error] 50-50: Expected blank line before this statement.
(padding-line-between-statements)
[error] 53-61: Member off should be declared before member on.
(@typescript-eslint/member-ordering)
[error] 53-53: The
Functiontype accepts any function-like value.
Prefer explicitly defining any function parameters and return type.(@typescript-eslint/no-unsafe-function-type)
[error] 55-60: Expected blank line before this statement.
(padding-line-between-statements)
[error] 57-59: Expected blank line before this statement.
(padding-line-between-statements)
[error] 64-64: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 66-68: Expected blank line before this statement.
(padding-line-between-statements)
[error] 81-81: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 113-113: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 133-133: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 142-142: Unexpected assert.equal. Use assert.strictEqual, assert.deepEqual, or assert.propEqual.
(qunit/no-assert-equal)
[error] 157-163: Expected blank line before this statement.
(padding-line-between-statements)
[error] 159-162: Expected blank line before this statement.
(padding-line-between-statements)
[error] 173-173: Unexpected assert.equal. Use assert.strictEqual, assert.deepEqual, or assert.propEqual.
(qunit/no-assert-equal)
[error] 174-174: Unexpected assert.equal. Use assert.strictEqual, assert.deepEqual, or assert.propEqual.
(qunit/no-assert-equal)
[error] 190-190: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 220-220: Unexpected assert.equal. Use assert.strictEqual, assert.deepEqual, or assert.propEqual.
(qunit/no-assert-equal)
[error] 221-221: Unexpected assert.equal. Use assert.strictEqual, assert.deepEqual, or assert.propEqual.
(qunit/no-assert-equal)
[error] 222-222: Unexpected assert.equal. Use assert.strictEqual, assert.deepEqual, or assert.propEqual.
(qunit/no-assert-equal)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
tests/integration/components/gleam-logo-test.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
tests/integration/components/gleam-logo-test.ts
[error] 19-19: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 46-46: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 53-53: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
🪛 ESLint
tests/integration/components/gleam-logo-test.ts
[error] 3-3: 'triggerEvent' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 18-18: Member isLoaded should be declared before member stateMachineNames.
(@typescript-eslint/member-ordering)
[error] 19-19: Member _listeners should be declared before member isLoaded.
(@typescript-eslint/member-ordering)
[error] 19-19: The Function type accepts any function-like value.
Prefer explicitly defining any function parameters and return type.
(@typescript-eslint/no-unsafe-function-type)
[error] 20-20: Member _interval should be declared before member _listeners.
(@typescript-eslint/member-ordering)
[error] 26-28: Member constructor should be declared before all get definitions.
(@typescript-eslint/member-ordering)
[error] 34-36: Member play should be declared before member reset.
(@typescript-eslint/member-ordering)
[error] 34-34: 'stateMachineName' is defined but never used. Allowed unused args must match /^_/u.
(@typescript-eslint/no-unused-vars)
[error] 42-44: Member stateMachineInputs should be declared before member stop.
(@typescript-eslint/member-ordering)
[error] 42-42: 'stateMachineName' is defined but never used. Allowed unused args must match /^_/u.
(@typescript-eslint/no-unused-vars)
[error] 46-51: Member on should be declared before member stateMachineInputs.
(@typescript-eslint/member-ordering)
[error] 46-46: The Function type accepts any function-like value.
Prefer explicitly defining any function parameters and return type.
(@typescript-eslint/no-unsafe-function-type)
[error] 50-50: Expected blank line before this statement.
(padding-line-between-statements)
[error] 53-61: Member off should be declared before member on.
(@typescript-eslint/member-ordering)
[error] 53-53: The Function type accepts any function-like value.
Prefer explicitly defining any function parameters and return type.
(@typescript-eslint/no-unsafe-function-type)
[error] 55-60: Expected blank line before this statement.
(padding-line-between-statements)
[error] 57-59: Expected blank line before this statement.
(padding-line-between-statements)
[error] 64-64: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 66-68: Expected blank line before this statement.
(padding-line-between-statements)
[error] 81-81: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 113-113: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 133-133: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 142-142: Unexpected assert.equal. Use assert.strictEqual, assert.deepEqual, or assert.propEqual.
(qunit/no-assert-equal)
[error] 157-163: Expected blank line before this statement.
(padding-line-between-statements)
[error] 159-162: Expected blank line before this statement.
(padding-line-between-statements)
[error] 173-173: Unexpected assert.equal. Use assert.strictEqual, assert.deepEqual, or assert.propEqual.
(qunit/no-assert-equal)
[error] 174-174: Unexpected assert.equal. Use assert.strictEqual, assert.deepEqual, or assert.propEqual.
(qunit/no-assert-equal)
[error] 190-190: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 220-220: Unexpected assert.equal. Use assert.strictEqual, assert.deepEqual, or assert.propEqual.
(qunit/no-assert-equal)
[error] 221-221: Unexpected assert.equal. Use assert.strictEqual, assert.deepEqual, or assert.propEqual.
(qunit/no-assert-equal)
[error] 222-222: Unexpected assert.equal. Use assert.strictEqual, assert.deepEqual, or assert.propEqual.
(qunit/no-assert-equal)
🔇 Additional comments (1)
tests/integration/components/gleam-logo-test.ts (1)
85-98: Test setup and teardown looks good.The hooks setup properly stores and restores the original Rive constructor, which is a good practice to avoid test pollution.
|
|
||
| // Create and attach mock Rive instance | ||
| const mockRive = new MockRive({ canvas: canvas as HTMLCanvasElement }); | ||
| (container as any).__riveInstance = mockRive; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety by avoiding any assertions.
There are several instances where (container as any).__riveInstance is used to assign the mock Rive instance. This bypasses TypeScript's type checking and should be avoided.
Consider extending the HTMLElement interface to include the __riveInstance property, or use a WeakMap to associate containers with their Rive instances:
// At the top of the file
interface GleamLogoContainer extends HTMLElement {
__riveInstance?: MockRive;
}
// Then in the tests
const container = document.querySelector('.gleam-logo-container') as GleamLogoContainer;
container.__riveInstance = mockRive;Also applies to: 133-133, 190-190
🧰 Tools
🪛 ESLint
[error] 113-113: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
| test('it cleans up resources on destroy', async function (this: TestContext, assert) { | ||
| await render(hbs`<GleamLogo />`); | ||
|
|
||
| // Get the container and create mock Rive instance | ||
| const container = document.querySelector('.gleam-logo-container') as HTMLElement; | ||
| const canvas = container?.querySelector('canvas'); | ||
| assert.ok(canvas, 'Canvas element exists'); | ||
|
|
||
| // Create and attach mock Rive instance | ||
| const mockRive = new MockRive({ canvas: canvas as HTMLCanvasElement }); | ||
| (container as any).__riveInstance = mockRive; | ||
|
|
||
| // Track cleanup calls | ||
| let stopCalls = 0; | ||
| let resetCalls = 0; | ||
|
|
||
| mockRive.stop = () => { | ||
| stopCalls++; | ||
| }; | ||
|
|
||
| mockRive.reset = () => { | ||
| resetCalls++; | ||
| }; | ||
|
|
||
| // Simulate Rive load | ||
| mockRive.simulateLoad(); | ||
| await settled(); | ||
|
|
||
| // Add cleanup handler directly | ||
| const cleanupRive = () => { | ||
| if (mockRive) { | ||
| mockRive.stop(); | ||
| mockRive.reset(); | ||
| } | ||
| }; | ||
|
|
||
| // Trigger cleanup | ||
| cleanupRive(); | ||
| await settled(); | ||
|
|
||
| assert.equal(stopCalls, 1, 'Stop was called on destroy'); | ||
| assert.equal(resetCalls, 1, 'Reset was called on destroy'); | ||
| assert.equal(mockRive.interval, null, 'Animation interval was cleared'); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test for resource cleanup.
Similar to the previous test, this one has QUnit assertion style issues and manually recreates cleanup logic rather than testing the component's own cleanup method.
test('it cleans up resources on destroy', async function (this: TestContext, assert) {
await render(hbs`<GleamLogo />`);
// Get the container and create mock Rive instance
const container = document.querySelector('.gleam-logo-container') as HTMLElement;
const canvas = container?.querySelector('canvas');
assert.ok(canvas, 'Canvas element exists');
// Create and attach mock Rive instance
const mockRive = new MockRive({ canvas: canvas as HTMLCanvasElement });
(container as any).__riveInstance = mockRive;
// Track cleanup calls
let stopCalls = 0;
let resetCalls = 0;
mockRive.stop = () => {
stopCalls++;
};
mockRive.reset = () => {
resetCalls++;
};
// Simulate Rive load
mockRive.simulateLoad();
await settled();
// Add cleanup handler directly
const cleanupRive = () => {
if (mockRive) {
mockRive.stop();
mockRive.reset();
}
};
// Trigger cleanup
cleanupRive();
await settled();
- assert.equal(stopCalls, 1, 'Stop was called on destroy');
- assert.equal(resetCalls, 1, 'Reset was called on destroy');
- assert.equal(mockRive.interval, null, 'Animation interval was cleared');
+ assert.strictEqual(stopCalls, 1, 'Stop was called on destroy');
+ assert.strictEqual(resetCalls, 1, 'Reset was called on destroy');
+ assert.strictEqual(mockRive.interval, null, 'Animation interval was cleared');
});📝 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.
| test('it cleans up resources on destroy', async function (this: TestContext, assert) { | |
| await render(hbs`<GleamLogo />`); | |
| // Get the container and create mock Rive instance | |
| const container = document.querySelector('.gleam-logo-container') as HTMLElement; | |
| const canvas = container?.querySelector('canvas'); | |
| assert.ok(canvas, 'Canvas element exists'); | |
| // Create and attach mock Rive instance | |
| const mockRive = new MockRive({ canvas: canvas as HTMLCanvasElement }); | |
| (container as any).__riveInstance = mockRive; | |
| // Track cleanup calls | |
| let stopCalls = 0; | |
| let resetCalls = 0; | |
| mockRive.stop = () => { | |
| stopCalls++; | |
| }; | |
| mockRive.reset = () => { | |
| resetCalls++; | |
| }; | |
| // Simulate Rive load | |
| mockRive.simulateLoad(); | |
| await settled(); | |
| // Add cleanup handler directly | |
| const cleanupRive = () => { | |
| if (mockRive) { | |
| mockRive.stop(); | |
| mockRive.reset(); | |
| } | |
| }; | |
| // Trigger cleanup | |
| cleanupRive(); | |
| await settled(); | |
| assert.equal(stopCalls, 1, 'Stop was called on destroy'); | |
| assert.equal(resetCalls, 1, 'Reset was called on destroy'); | |
| assert.equal(mockRive.interval, null, 'Animation interval was cleared'); | |
| }); | |
| test('it cleans up resources on destroy', async function (this: TestContext, assert) { | |
| await render(hbs`<GleamLogo />`); | |
| // Get the container and create mock Rive instance | |
| const container = document.querySelector('.gleam-logo-container') as HTMLElement; | |
| const canvas = container?.querySelector('canvas'); | |
| assert.ok(canvas, 'Canvas element exists'); | |
| // Create and attach mock Rive instance | |
| const mockRive = new MockRive({ canvas: canvas as HTMLCanvasElement }); | |
| (container as any).__riveInstance = mockRive; | |
| // Track cleanup calls | |
| let stopCalls = 0; | |
| let resetCalls = 0; | |
| mockRive.stop = () => { | |
| stopCalls++; | |
| }; | |
| mockRive.reset = () => { | |
| resetCalls++; | |
| }; | |
| // Simulate Rive load | |
| mockRive.simulateLoad(); | |
| await settled(); | |
| // Add cleanup handler directly | |
| const cleanupRive = () => { | |
| if (mockRive) { | |
| mockRive.stop(); | |
| mockRive.reset(); | |
| } | |
| }; | |
| // Trigger cleanup | |
| cleanupRive(); | |
| await settled(); | |
| assert.strictEqual(stopCalls, 1, 'Stop was called on destroy'); | |
| assert.strictEqual(resetCalls, 1, 'Reset was called on destroy'); | |
| assert.strictEqual(mockRive.interval, null, 'Animation interval was cleared'); | |
| }); |
🧰 Tools
🪛 ESLint
[error] 190-190: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 220-220: Unexpected assert.equal. Use assert.strictEqual, assert.deepEqual, or assert.propEqual.
(qunit/no-assert-equal)
[error] 221-221: Unexpected assert.equal. Use assert.strictEqual, assert.deepEqual, or assert.propEqual.
(qunit/no-assert-equal)
[error] 222-222: Unexpected assert.equal. Use assert.strictEqual, assert.deepEqual, or assert.propEqual.
(qunit/no-assert-equal)
| test('it handles hover events', async function (assert) { | ||
| await render(hbs`<GleamLogo />`); | ||
|
|
||
| // Get the container and create mock Rive instance | ||
| const container = document.querySelector('.gleam-logo-container') as HTMLElement; | ||
| const canvas = container?.querySelector('canvas'); | ||
| assert.ok(canvas, 'Canvas element exists'); | ||
|
|
||
| // Create and attach mock Rive instance | ||
| const mockRive = new MockRive({ canvas: canvas as HTMLCanvasElement }); | ||
| (container as any).__riveInstance = mockRive; | ||
|
|
||
| // Track calls to play and reset | ||
| let playCalls = 0; | ||
| let resetCalls = 0; | ||
|
|
||
| // Override the mock methods before simulating load | ||
| mockRive.play = (stateMachineName: string) => { | ||
| playCalls++; | ||
| assert.equal(stateMachineName, 'State Machine 1', 'Hover plays correct state machine'); | ||
| }; | ||
|
|
||
| mockRive.reset = () => { | ||
| resetCalls++; | ||
| }; | ||
|
|
||
| // Simulate Rive load | ||
| mockRive.simulateLoad(); | ||
| await settled(); | ||
|
|
||
| // Add event handler directly to the container | ||
| const handleMouseEnter = () => { | ||
| if (mockRive) { | ||
| const stateMachines = mockRive.stateMachineNames; | ||
| if (stateMachines && stateMachines.length > 0) { | ||
| const stateMachineName = 'State Machine 1'; | ||
| if (stateMachines.includes(stateMachineName)) { | ||
| mockRive.reset(); | ||
| mockRive.play(stateMachineName); | ||
| } | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| container.addEventListener('mouseenter', handleMouseEnter); | ||
|
|
||
| // Trigger the event | ||
| container.dispatchEvent(new MouseEvent('mouseenter')); | ||
| await settled(); | ||
|
|
||
| assert.equal(playCalls, 1, 'Play was called once on hover'); | ||
| assert.equal(resetCalls, 1, 'Reset was called once on hover'); | ||
|
|
||
| // Clean up | ||
| container.removeEventListener('mouseenter', handleMouseEnter); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test structure and assertions for hover events test.
The hover event test has several areas for improvement:
- It manually recreates event handling logic that should be testing the component's behavior
- It uses
assert.equalinstead ofassert.strictEqualas recommended by QUnit best practices - There are missing blank lines between logical code blocks
test('it handles hover events', async function (assert) {
await render(hbs`<GleamLogo />`);
// Get the container and create mock Rive instance
const container = document.querySelector('.gleam-logo-container') as HTMLElement;
const canvas = container?.querySelector('canvas');
assert.ok(canvas, 'Canvas element exists');
// Create and attach mock Rive instance
const mockRive = new MockRive({ canvas: canvas as HTMLCanvasElement });
(container as any).__riveInstance = mockRive;
// Track calls to play and reset
let playCalls = 0;
let resetCalls = 0;
// Override the mock methods before simulating load
mockRive.play = (stateMachineName: string) => {
playCalls++;
- assert.equal(stateMachineName, 'State Machine 1', 'Hover plays correct state machine');
+ assert.strictEqual(stateMachineName, 'State Machine 1', 'Hover plays correct state machine');
};
mockRive.reset = () => {
resetCalls++;
};
// Simulate Rive load
mockRive.simulateLoad();
await settled();
// Add event handler directly to the container
const handleMouseEnter = () => {
if (mockRive) {
const stateMachines = mockRive.stateMachineNames;
if (stateMachines && stateMachines.length > 0) {
+
const stateMachineName = 'State Machine 1';
if (stateMachines.includes(stateMachineName)) {
+
mockRive.reset();
mockRive.play(stateMachineName);
}
}
}
};
container.addEventListener('mouseenter', handleMouseEnter);
// Trigger the event
container.dispatchEvent(new MouseEvent('mouseenter'));
await settled();
- assert.equal(playCalls, 1, 'Play was called once on hover');
- assert.equal(resetCalls, 1, 'Reset was called once on hover');
+ assert.strictEqual(playCalls, 1, 'Play was called once on hover');
+ assert.strictEqual(resetCalls, 1, 'Reset was called once on hover');
// Clean up
container.removeEventListener('mouseenter', handleMouseEnter);
});📝 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.
| test('it handles hover events', async function (assert) { | |
| await render(hbs`<GleamLogo />`); | |
| // Get the container and create mock Rive instance | |
| const container = document.querySelector('.gleam-logo-container') as HTMLElement; | |
| const canvas = container?.querySelector('canvas'); | |
| assert.ok(canvas, 'Canvas element exists'); | |
| // Create and attach mock Rive instance | |
| const mockRive = new MockRive({ canvas: canvas as HTMLCanvasElement }); | |
| (container as any).__riveInstance = mockRive; | |
| // Track calls to play and reset | |
| let playCalls = 0; | |
| let resetCalls = 0; | |
| // Override the mock methods before simulating load | |
| mockRive.play = (stateMachineName: string) => { | |
| playCalls++; | |
| assert.equal(stateMachineName, 'State Machine 1', 'Hover plays correct state machine'); | |
| }; | |
| mockRive.reset = () => { | |
| resetCalls++; | |
| }; | |
| // Simulate Rive load | |
| mockRive.simulateLoad(); | |
| await settled(); | |
| // Add event handler directly to the container | |
| const handleMouseEnter = () => { | |
| if (mockRive) { | |
| const stateMachines = mockRive.stateMachineNames; | |
| if (stateMachines && stateMachines.length > 0) { | |
| const stateMachineName = 'State Machine 1'; | |
| if (stateMachines.includes(stateMachineName)) { | |
| mockRive.reset(); | |
| mockRive.play(stateMachineName); | |
| } | |
| } | |
| } | |
| }; | |
| container.addEventListener('mouseenter', handleMouseEnter); | |
| // Trigger the event | |
| container.dispatchEvent(new MouseEvent('mouseenter')); | |
| await settled(); | |
| assert.equal(playCalls, 1, 'Play was called once on hover'); | |
| assert.equal(resetCalls, 1, 'Reset was called once on hover'); | |
| // Clean up | |
| container.removeEventListener('mouseenter', handleMouseEnter); | |
| }); | |
| test('it handles hover events', async function (assert) { | |
| await render(hbs`<GleamLogo />`); | |
| // Get the container and create mock Rive instance | |
| const container = document.querySelector('.gleam-logo-container') as HTMLElement; | |
| const canvas = container?.querySelector('canvas'); | |
| assert.ok(canvas, 'Canvas element exists'); | |
| // Create and attach mock Rive instance | |
| const mockRive = new MockRive({ canvas: canvas as HTMLCanvasElement }); | |
| (container as any).__riveInstance = mockRive; | |
| // Track calls to play and reset | |
| let playCalls = 0; | |
| let resetCalls = 0; | |
| // Override the mock methods before simulating load | |
| mockRive.play = (stateMachineName: string) => { | |
| playCalls++; | |
| assert.strictEqual(stateMachineName, 'State Machine 1', 'Hover plays correct state machine'); | |
| }; | |
| mockRive.reset = () => { | |
| resetCalls++; | |
| }; | |
| // Simulate Rive load | |
| mockRive.simulateLoad(); | |
| await settled(); | |
| // Add event handler directly to the container | |
| const handleMouseEnter = () => { | |
| if (mockRive) { | |
| const stateMachines = mockRive.stateMachineNames; | |
| if (stateMachines && stateMachines.length > 0) { | |
| const stateMachineName = 'State Machine 1'; | |
| if (stateMachines.includes(stateMachineName)) { | |
| mockRive.reset(); | |
| mockRive.play(stateMachineName); | |
| } | |
| } | |
| } | |
| }; | |
| container.addEventListener('mouseenter', handleMouseEnter); | |
| // Trigger the event | |
| container.dispatchEvent(new MouseEvent('mouseenter')); | |
| await settled(); | |
| assert.strictEqual(playCalls, 1, 'Play was called once on hover'); | |
| assert.strictEqual(resetCalls, 1, 'Reset was called once on hover'); | |
| // Clean up | |
| container.removeEventListener('mouseenter', handleMouseEnter); | |
| }); |
🧰 Tools
🪛 ESLint
[error] 133-133: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 142-142: Unexpected assert.equal. Use assert.strictEqual, assert.deepEqual, or assert.propEqual.
(qunit/no-assert-equal)
[error] 157-163: Expected blank line before this statement.
(padding-line-between-statements)
[error] 159-162: Expected blank line before this statement.
(padding-line-between-statements)
[error] 173-173: Unexpected assert.equal. Use assert.strictEqual, assert.deepEqual, or assert.propEqual.
(qunit/no-assert-equal)
[error] 174-174: Unexpected assert.equal. Use assert.strictEqual, assert.deepEqual, or assert.propEqual.
(qunit/no-assert-equal)
| // Mock Rive class for testing | ||
| class MockRive { | ||
| @tracked canvas: HTMLCanvasElement; | ||
| @tracked stateMachineNames: string[] = ['State Machine 1', 'State Machine 2']; | ||
| @tracked isLoaded = false; | ||
| private _listeners: Map<string, Function[]> = new Map(); | ||
| private _interval: number | null = null; | ||
|
|
||
| get interval(): number | null { | ||
| return this._interval; | ||
| } | ||
|
|
||
| constructor(options: { canvas: HTMLCanvasElement }) { | ||
| this.canvas = options.canvas; | ||
| } | ||
|
|
||
| reset() { | ||
| // Mock reset implementation | ||
| } | ||
|
|
||
| play(stateMachineName: string) { | ||
| // Mock play implementation | ||
| } | ||
|
|
||
| stop() { | ||
| // Mock stop implementation | ||
| } | ||
|
|
||
| stateMachineInputs(stateMachineName: string) { | ||
| return []; | ||
| } | ||
|
|
||
| on(event: string, callback: Function) { | ||
| if (!this._listeners.has(event)) { | ||
| this._listeners.set(event, []); | ||
| } | ||
| this._listeners.get(event)?.push(callback); | ||
| } | ||
|
|
||
| off(event: string, callback: Function) { | ||
| const listeners = this._listeners.get(event); | ||
| if (listeners) { | ||
| const index = listeners.indexOf(callback); | ||
| if (index > -1) { | ||
| listeners.splice(index, 1); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Helper method to trigger events | ||
| triggerEvent(event: string, ...args: any[]) { | ||
| const listeners = this._listeners.get(event); | ||
| if (listeners) { | ||
| listeners.forEach((callback) => callback(...args)); | ||
| } | ||
| } | ||
|
|
||
| // Helper method to simulate load | ||
| simulateLoad() { | ||
| this.isLoaded = true; | ||
| this.triggerEvent('load'); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve MockRive class with proper TypeScript patterns.
The MockRive class has several issues flagged by static analysis:
- Using the generic
Functiontype instead of specific function signatures - Using
anytype in method parameters - Inconsistent member ordering
- Missing blank lines between statements
These issues impact code quality and type safety.
// Mock Rive class for testing
class MockRive {
@tracked canvas: HTMLCanvasElement;
+ @tracked isLoaded = false;
@tracked stateMachineNames: string[] = ['State Machine 1', 'State Machine 2'];
- @tracked isLoaded = false;
- private _listeners: Map<string, Function[]> = new Map();
- private _interval: number | null = null;
+ private _listeners: Map<string, Array<(...args: unknown[]) => void>> = new Map();
+ private _interval: number | null = null;
get interval(): number | null {
return this._interval;
}
constructor(options: { canvas: HTMLCanvasElement }) {
this.canvas = options.canvas;
}
reset() {
// Mock reset implementation
}
play(stateMachineName: string) {
// Mock play implementation
}
stop() {
// Mock stop implementation
}
stateMachineInputs(stateMachineName: string) {
return [];
}
- on(event: string, callback: Function) {
+ on(event: string, callback: (...args: unknown[]) => void) {
if (!this._listeners.has(event)) {
this._listeners.set(event, []);
}
+
this._listeners.get(event)?.push(callback);
}
- off(event: string, callback: Function) {
+ off(event: string, callback: (...args: unknown[]) => void) {
const listeners = this._listeners.get(event);
if (listeners) {
const index = listeners.indexOf(callback);
+
if (index > -1) {
listeners.splice(index, 1);
}
}
}
// Helper method to trigger events
- triggerEvent(event: string, ...args: any[]) {
+ triggerEvent(event: string, ...args: unknown[]) {
const listeners = this._listeners.get(event);
if (listeners) {
+
listeners.forEach((callback) => callback(...args));
}
}
// Helper method to simulate load
simulateLoad() {
this.isLoaded = true;
this.triggerEvent('load');
}
}📝 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.
| // Mock Rive class for testing | |
| class MockRive { | |
| @tracked canvas: HTMLCanvasElement; | |
| @tracked stateMachineNames: string[] = ['State Machine 1', 'State Machine 2']; | |
| @tracked isLoaded = false; | |
| private _listeners: Map<string, Function[]> = new Map(); | |
| private _interval: number | null = null; | |
| get interval(): number | null { | |
| return this._interval; | |
| } | |
| constructor(options: { canvas: HTMLCanvasElement }) { | |
| this.canvas = options.canvas; | |
| } | |
| reset() { | |
| // Mock reset implementation | |
| } | |
| play(stateMachineName: string) { | |
| // Mock play implementation | |
| } | |
| stop() { | |
| // Mock stop implementation | |
| } | |
| stateMachineInputs(stateMachineName: string) { | |
| return []; | |
| } | |
| on(event: string, callback: Function) { | |
| if (!this._listeners.has(event)) { | |
| this._listeners.set(event, []); | |
| } | |
| this._listeners.get(event)?.push(callback); | |
| } | |
| off(event: string, callback: Function) { | |
| const listeners = this._listeners.get(event); | |
| if (listeners) { | |
| const index = listeners.indexOf(callback); | |
| if (index > -1) { | |
| listeners.splice(index, 1); | |
| } | |
| } | |
| } | |
| // Helper method to trigger events | |
| triggerEvent(event: string, ...args: any[]) { | |
| const listeners = this._listeners.get(event); | |
| if (listeners) { | |
| listeners.forEach((callback) => callback(...args)); | |
| } | |
| } | |
| // Helper method to simulate load | |
| simulateLoad() { | |
| this.isLoaded = true; | |
| this.triggerEvent('load'); | |
| } | |
| } | |
| // Mock Rive class for testing | |
| class MockRive { | |
| @tracked canvas: HTMLCanvasElement; | |
| @tracked isLoaded = false; | |
| @tracked stateMachineNames: string[] = ['State Machine 1', 'State Machine 2']; | |
| private _listeners: Map<string, Array<(...args: unknown[]) => void>> = new Map(); | |
| private _interval: number | null = null; | |
| get interval(): number | null { | |
| return this._interval; | |
| } | |
| constructor(options: { canvas: HTMLCanvasElement }) { | |
| this.canvas = options.canvas; | |
| } | |
| reset() { | |
| // Mock reset implementation | |
| } | |
| play(stateMachineName: string) { | |
| // Mock play implementation | |
| } | |
| stop() { | |
| // Mock stop implementation | |
| } | |
| stateMachineInputs(stateMachineName: string) { | |
| return []; | |
| } | |
| on(event: string, callback: (...args: unknown[]) => void) { | |
| if (!this._listeners.has(event)) { | |
| this._listeners.set(event, []); | |
| } | |
| this._listeners.get(event)?.push(callback); | |
| } | |
| off(event: string, callback: (...args: unknown[]) => void) { | |
| const listeners = this._listeners.get(event); | |
| if (listeners) { | |
| const index = listeners.indexOf(callback); | |
| if (index > -1) { | |
| listeners.splice(index, 1); | |
| } | |
| } | |
| } | |
| // Helper method to trigger events | |
| triggerEvent(event: string, ...args: unknown[]) { | |
| const listeners = this._listeners.get(event); | |
| if (listeners) { | |
| listeners.forEach((callback) => callback(...args)); | |
| } | |
| } | |
| // Helper method to simulate load | |
| simulateLoad() { | |
| this.isLoaded = true; | |
| this.triggerEvent('load'); | |
| } | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 19-19: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 46-46: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 53-53: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
🪛 ESLint
[error] 18-18: Member isLoaded should be declared before member stateMachineNames.
(@typescript-eslint/member-ordering)
[error] 19-19: Member _listeners should be declared before member isLoaded.
(@typescript-eslint/member-ordering)
[error] 19-19: The Function type accepts any function-like value.
Prefer explicitly defining any function parameters and return type.
(@typescript-eslint/no-unsafe-function-type)
[error] 20-20: Member _interval should be declared before member _listeners.
(@typescript-eslint/member-ordering)
[error] 26-28: Member constructor should be declared before all get definitions.
(@typescript-eslint/member-ordering)
[error] 34-36: Member play should be declared before member reset.
(@typescript-eslint/member-ordering)
[error] 34-34: 'stateMachineName' is defined but never used. Allowed unused args must match /^_/u.
(@typescript-eslint/no-unused-vars)
[error] 42-44: Member stateMachineInputs should be declared before member stop.
(@typescript-eslint/member-ordering)
[error] 42-42: 'stateMachineName' is defined but never used. Allowed unused args must match /^_/u.
(@typescript-eslint/no-unused-vars)
[error] 46-51: Member on should be declared before member stateMachineInputs.
(@typescript-eslint/member-ordering)
[error] 46-46: The Function type accepts any function-like value.
Prefer explicitly defining any function parameters and return type.
(@typescript-eslint/no-unsafe-function-type)
[error] 50-50: Expected blank line before this statement.
(padding-line-between-statements)
[error] 53-61: Member off should be declared before member on.
(@typescript-eslint/member-ordering)
[error] 53-53: The Function type accepts any function-like value.
Prefer explicitly defining any function parameters and return type.
(@typescript-eslint/no-unsafe-function-type)
[error] 55-60: Expected blank line before this statement.
(padding-line-between-statements)
[error] 57-59: Expected blank line before this statement.
(padding-line-between-statements)
[error] 64-64: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 66-68: Expected blank line before this statement.
(padding-line-between-statements)
|
https://www.loom.com/share/85e5c7a87b3c4f03897153e988134f3d?sid=6f78cd08-a4d6-4e4b-9899-a5c27653a10a for a preview of the animation |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
app/components/gleam-logo.ts (3)
18-125: Add integration tests to improve code coverageThe component currently lacks test coverage. Add integration tests to verify the component's behavior.
I can help create integration tests for this component. Would you like me to generate a test file that:
- Mocks the Rive library
- Tests component rendering
- Tests animation behavior on hover
- Tests cleanup
🧰 Tools
🪛 ESLint
[error] 107-118: Member cleanupRive should be declared before member handleMouseLeave.
(@typescript-eslint/member-ordering)
78-80: 🛠️ Refactor suggestionImprove error handling
Instead of just logging errors to the console, consider reporting them to a monitoring system like Sentry.
} catch (error: unknown) { - console.error('Error setting up Rive:', error); + // Report to monitoring system + // @ts-ignore - Assuming a global Sentry instance + window.Sentry?.captureException(error, { + tags: { component: 'GleamLogo' }, + extra: { message: 'Error setting up Rive animation' } + }); + console.error('Error setting up Rive:', error); }As mentioned in the previous review, errors should be reported to Sentry rather than just being logged to the console.
107-118:⚠️ Potential issueFix member ordering
According to ESLint, the
cleanupRivemethod should be declared beforehandleMouseLeave.@action -handleMouseLeave() { - if (this.riveInstance) { - this.riveInstance.stop(); - this.riveInstance.reset(); - } -} - -@action cleanupRive() { if (this.animationInterval) { clearInterval(this.animationInterval); this.animationInterval = null; } if (this.riveInstance) { this.riveInstance.stop(); this.riveInstance = null; } } @action +handleMouseLeave() { + if (this.riveInstance) { + this.riveInstance.stop(); + this.riveInstance.reset(); + } +}This issue was flagged in the static analysis hints and needs to be fixed to pass linting checks.
🧰 Tools
🪛 ESLint
[error] 107-118: Member cleanupRive should be declared before member handleMouseLeave.
(@typescript-eslint/member-ordering)
🧹 Nitpick comments (5)
app/components/gleam-logo.ts (5)
6-16: Consider simplifying the component signatureBased on the previous comments, the DOM-level attributes like
classandstyleare automatically handled by Ember's...attributes. The current signature is more complex than needed.interface GleamLogoSignature { Element: HTMLDivElement; Args: { height: number; - }; + }; - - Blocks: { - default: []; - }; }
18-21: Add default height valueAdd a default height value to ensure the component works correctly even when the height prop is not provided.
export default class GleamLogoComponent extends Component<GleamLogoSignature> { @tracked riveInstance: Rive | null = null; container: HTMLElement | null = null; animationInterval: number | null = null; + + // Default height if not provided + get height(): number { + return this.args.height ?? 100; + }
23-25: Use the getter for height consistencyIf you implement the height getter as suggested above, use it here for consistency:
get containerStyle(): string { - return `height: ${this.args.height}px; width: auto;`; + return `height: ${this.height}px; width: auto;`; }
35-36: Hardcoded dimensions should be configurableThe canvas dimensions are hardcoded to 400x400, which might not be appropriate for all use cases. Consider making these values configurable or calculate them based on the container size.
// Set initial canvas size for high quality -canvas.width = 400; // Base size for quality -canvas.height = 400; // Will adjust based on container +const scale = window.devicePixelRatio || 1; +const containerRect = element.getBoundingClientRect(); +canvas.width = containerRect.width * scale; +canvas.height = containerRect.height * scale;Per previous comments, where do the hardcoded values 400x400 come from? If these dimensions are specific to the Gleam logo animation file, it would be helpful to add a comment explaining this or make them configurable to support other animations in the future.
99-105: Simplify mouse leave handler using extracted methodSimilarly to the mouse enter handler, this can be simplified using the extracted method.
@action handleMouseLeave() { - if (this.riveInstance) { - this.riveInstance.stop(); - this.riveInstance.reset(); - } + if (!this.riveInstance) return; + + this.riveInstance.stop(); + this.riveInstance.reset(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (4)
app/components/gleam-logo.hbs(1 hunks)app/components/gleam-logo.ts(1 hunks)app/components/language-logo.hbs(1 hunks)tests/integration/components/gleam-logo-test.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/components/gleam-logo.hbs
🚧 Files skipped from review as they are similar to previous changes (2)
- app/components/language-logo.hbs
- tests/integration/components/gleam-logo-test.js
🧰 Additional context used
🪛 ESLint
app/components/gleam-logo.ts
[error] 107-118: Member cleanupRive should be declared before member handleMouseLeave.
(@typescript-eslint/member-ordering)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (1)
app/components/gleam-logo.ts (1)
48-50: Ensure the layout preserves the aspect ratioThe Rive layout is set to
Fit.Contain, which should preserve the aspect ratio. However, as noted in previous comments, if the animation file isn't square (which appears to be the case based on comments), this could lead to unexpected results.Verify that this layout configuration correctly preserves the aspect ratio of the Gleam logo animation. Consider adding a comment explaining how the aspect ratio is maintained.
app/components/gleam-logo.ts
Outdated
| @action | ||
| handleMouseEnter() { | ||
| if (this.riveInstance) { | ||
| const stateMachines = this.riveInstance.stateMachineNames; | ||
|
|
||
| if (stateMachines && stateMachines.length > 0) { | ||
| const stateMachineName = 'State Machine 1'; | ||
|
|
||
| if (stateMachines.includes(stateMachineName)) { | ||
| this.riveInstance.reset(); | ||
| this.riveInstance.play(stateMachineName); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract animation methods to reduce code duplication
The mouse enter/leave handlers have repeated code for checking state machines and playing animations. Extract this into a reusable method.
@action
handleMouseEnter() {
- if (this.riveInstance) {
- const stateMachines = this.riveInstance.stateMachineNames;
-
- if (stateMachines && stateMachines.length > 0) {
- const stateMachineName = 'State Machine 1';
-
- if (stateMachines.includes(stateMachineName)) {
- this.riveInstance.reset();
- this.riveInstance.play(stateMachineName);
- }
- }
- }
+ this.playAnimation('State Machine 1');
}
// Add a helper method to play animations
playAnimation(stateMachineName: string): void {
if (!this.riveInstance) return;
const stateMachines = this.riveInstance.stateMachineNames;
if (stateMachines?.includes(stateMachineName)) {
this.riveInstance.reset();
this.riveInstance.play(stateMachineName);
}
}This will reduce code duplication across the component and make the code more maintainable.
📝 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.
| @action | |
| handleMouseEnter() { | |
| if (this.riveInstance) { | |
| const stateMachines = this.riveInstance.stateMachineNames; | |
| if (stateMachines && stateMachines.length > 0) { | |
| const stateMachineName = 'State Machine 1'; | |
| if (stateMachines.includes(stateMachineName)) { | |
| this.riveInstance.reset(); | |
| this.riveInstance.play(stateMachineName); | |
| } | |
| } | |
| } | |
| } | |
| @action | |
| handleMouseEnter() { | |
| this.playAnimation('State Machine 1'); | |
| } | |
| // Helper method to encapsulate common animation logic | |
| private playAnimation(stateMachineName: string): void { | |
| if (!this.riveInstance) return; | |
| const stateMachines = this.riveInstance.stateMachineNames; | |
| if (stateMachines?.includes(stateMachineName)) { | |
| this.riveInstance.reset(); | |
| this.riveInstance.play(stateMachineName); | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
app/components/gleam-logo.ts (2)
99-99: 🛠️ Refactor suggestionImplement proper error reporting.
Errors are currently only logged to the console, which is insufficient for production code. Consider using a proper error reporting mechanism.
try { // Existing code... } catch (error: unknown) { console.error('Error setting up Rive:', error); + // Report to proper error monitoring system + import('ember-error-tracking').then(({ reportError }) => { + reportError(error, { + context: 'GleamLogoComponent.handleDidInsert', + extra: { height: this.args.height } + }); + }); }
84-94: 🛠️ Refactor suggestionExtract common animation logic to reduce duplication.
Both mouseenter and mouseleave event handlers do the same thing - play 'State Machine 1'. This should be extracted to a helper method to reduce duplication.
+ private playStateMachine(name: string): void { + if (this.riveInstance) { + this.riveInstance.play(name); + } + } // Then in the onLoad callback: // Set up hover state machine canvas.addEventListener('mouseenter', () => { - if (this.riveInstance) { - this.riveInstance.play('State Machine 1'); - } + this.playStateMachine('State Machine 1'); }); canvas.addEventListener('mouseleave', () => { - if (this.riveInstance) { - this.riveInstance.play('State Machine 1'); - } + this.playStateMachine('State Machine 1'); });
🧹 Nitpick comments (3)
app/components/gleam-logo.ts (3)
6-16: Interface signature could be simplified.The component interface includes an explicit block definition that is empty (
default: []). Since your component doesn't appear to use block content, you could simplify the signature by removing the Blocks section entirely.interface GleamLogoSignature { Element: HTMLDivElement; Args: { height: number; }; - - Blocks: { - default: []; - }; }
23-25: Consider handling height:auto or responsive options.Currently, the component only supports a fixed pixel height. For better flexibility, consider supporting percentage-based heights or accepting an optional width parameter.
interface GleamLogoSignature { Element: HTMLDivElement; Args: { height: number; + width?: number | string; + responsive?: boolean; }; } // Then update the getter: get containerStyle(): string { - return `height: ${this.args.height}px; width: auto;`; + const height = this.args.height ? `height: ${this.args.height}px;` : ''; + const width = this.args.width ? `width: ${typeof this.args.width === 'number' ? `${this.args.width}px` : this.args.width};` : 'width: auto;'; + return `${height} ${width}`; }
66-66: Remove debug console log.This debug log statement should be removed before going to production.
onLoad: () => { - console.log('onLoad'); if (this.riveInstance) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
app/components/gleam-logo.hbs(1 hunks)app/components/gleam-logo.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/components/gleam-logo.hbs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (1)
app/components/gleam-logo.ts (1)
48-50: Canvas dimensions should respect aspect ratio of the animation.The hardcoded 400x400 dimensions assume a square animation. If the Gleam logo animation has a different aspect ratio, this could cause distortion.
Does the Gleam logo animation actually have a 1:1 aspect ratio? If not, consider adjusting the dimensions to match the native aspect ratio of the animation file.
app/components/gleam-logo.ts
Outdated
| canvas.addEventListener('mouseenter', () => { | ||
| if (this.riveInstance) { | ||
| this.riveInstance.play('State Machine 1'); | ||
| } | ||
| }); | ||
|
|
||
| canvas.addEventListener('mouseleave', () => { | ||
| if (this.riveInstance) { | ||
| this.riveInstance.play('State Machine 1'); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Clean up event listeners to prevent memory leaks.
Event listeners are added to the canvas but never removed, which could cause memory leaks if the component is repeatedly created and destroyed.
@tracked riveInstance: Rive | null = null;
container: HTMLElement | null = null;
animationInterval: number | null = null;
+canvasElement: HTMLCanvasElement | null = null;
@action
cleanupRive() {
// Existing cleanup code...
+
+ // Remove event listeners
+ if (this.canvasElement) {
+ this.canvasElement.removeEventListener('mouseenter', this.handleMouseEnter);
+ this.canvasElement.removeEventListener('mouseleave', this.handleMouseLeave);
+ this.canvasElement = null;
+ }
}
@action
handleDidInsert(element: HTMLDivElement) {
// Existing code...
try {
const canvas = document.createElement('canvas');
+ this.canvasElement = canvas;
// Existing setup code...
// Create bound event handlers
+ this.handleMouseEnter = this.handleMouseEnter.bind(this);
+ this.handleMouseLeave = this.handleMouseLeave.bind(this);
// Set up hover state machine
- canvas.addEventListener('mouseenter', () => {
- if (this.riveInstance) {
- this.riveInstance.play('State Machine 1');
- }
- });
+ canvas.addEventListener('mouseenter', this.handleMouseEnter);
- canvas.addEventListener('mouseleave', () => {
- if (this.riveInstance) {
- this.riveInstance.play('State Machine 1');
- }
- });
+ canvas.addEventListener('mouseleave', this.handleMouseLeave);
}
// Add the handler methods
+ @action
+ handleMouseEnter() {
+ this.playStateMachine('State Machine 1');
+ }
+
+ @action
+ handleMouseLeave() {
+ this.playStateMachine('State Machine 1');
+ }Committable suggestion skipped: line range outside the PR's diff.
app/components/gleam-logo.ts
Outdated
| setTimeout(() => { | ||
| if (this.riveInstance) { | ||
| this.riveInstance.play('State Machine 3'); | ||
| } | ||
| }, 1000); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for setTimeout cleanup.
The timeout for playing State Machine 3 is created but never stored or cleared, which could cause issues if the component is destroyed before the timeout fires.
@tracked riveInstance: Rive | null = null;
container: HTMLElement | null = null;
animationInterval: number | null = null;
+resetTimeout: number | null = null;
@action
cleanupRive() {
if (this.animationInterval) {
clearInterval(this.animationInterval);
this.animationInterval = null;
}
+
+ if (this.resetTimeout) {
+ clearTimeout(this.resetTimeout);
+ this.resetTimeout = null;
+ }
// Rest of the method...
}
// Then in the onLoad callback:
// Play State Machine 3 after 1 second to reset
-setTimeout(() => {
+this.resetTimeout = window.setTimeout(() => {
if (this.riveInstance) {
this.riveInstance.play('State Machine 3');
}
}, 1000);📝 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.
| setTimeout(() => { | |
| if (this.riveInstance) { | |
| this.riveInstance.play('State Machine 3'); | |
| } | |
| }, 1000); | |
| // app/components/gleam-logo.ts | |
| export default class GleamLogoComponent extends Component { | |
| @tracked riveInstance: Rive | null = null; | |
| container: HTMLElement | null = null; | |
| animationInterval: number | null = null; | |
| resetTimeout: number | null = null; | |
| @action | |
| cleanupRive() { | |
| if (this.animationInterval) { | |
| clearInterval(this.animationInterval); | |
| this.animationInterval = null; | |
| } | |
| if (this.resetTimeout) { | |
| clearTimeout(this.resetTimeout); | |
| this.resetTimeout = null; | |
| } | |
| // …any other teardown logic… | |
| } | |
| private initializeRive() { | |
| new Rive({ | |
| // …other options… | |
| onLoad: () => { | |
| // Play State Machine 3 after 1 second to reset | |
| this.resetTimeout = window.setTimeout(() => { | |
| if (this.riveInstance) { | |
| this.riveInstance.play('State Machine 3'); | |
| } | |
| }, 1000); | |
| } | |
| }); | |
| } | |
| } |
app/components/gleam-logo.ts
Outdated
| export default class GleamLogoComponent extends Component<GleamLogoSignature> { | ||
| @tracked riveInstance: Rive | null = null; | ||
| container: HTMLElement | null = null; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add unit tests for this component.
The component is completely missing test coverage according to static analysis. Consider adding unit tests that verify the initialization, animation behaviors, and cleanup processes.
I can help you implement comprehensive tests for this component that verify:
- Component rendering
- Animation initialization and event handling
- Cleanup processes
Would you like me to provide a test implementation?
🌐 Web query:
How to write unit tests for Ember components with Rive animations?
💡 Result:
Writing Unit Tests for Ember Components with Rive Animations
Overview
Testing Ember components that embed Rive animations requires a combination of standard Ember component testing strategies and considerations specific to third-party rendering handled by Rive. Because Rive animations are rendered in a <canvas> element (or similar), and their side effects aren't always visible in the DOM as text or attributes, you must use a mix of rendering tests and DOM assertions, and potentially mock or spy on Rive's APIs.
Recommended Approach
1. Use Rendering or Integration Tests
While "unit tests" in Ember often refer to logic-only tests, components—especially those outputting dynamic UI like Rive animations—are best tested with rendering (also known as integration) tests. These let you render the component, interact with it, and assert on the DOM or called behaviors[1][5][7][8].
- Setup using
setupRenderingTest(hooks)in your test module. - Use the
renderhelper to instantiate your component in the test environment.
import { module, test } from 'qunit';
import { setupRenderingTest } from 'ember-qunit';
import { render } from '@ember/test-helpers';
import hbs from 'htmlbars-inline-precompile';
module('Integration | Component | my-rive-component', function(hooks) {
setupRenderingTest(hooks);
test('it renders the canvas for Rive', async function(assert) {
await render(hbs`<MyRiveComponent />`);
assert.dom('canvas').exists('Rive canvas is rendered');
});
});[1][5][7]
2. Mock Rive Animation Logic
External libraries like Rive often require DOM interaction and asynchronous file loading. To keep your tests fast and reliable:
-
Stub or Mock Rive: Mock the global Rive constructor or inject a mock via Ember's dependency injection. This way, you can spy on initialization or event handlers without actually loading Rive files.
- For example, if your component instantiates
new Rive({ ... }), mock this in your test so you can assert it was called with the expected props.
- For example, if your component instantiates
// In your test setup
let riveInstanceMock = {
on: sinon.spy(),
play: sinon.spy(),
pause: sinon.spy()
};
window.Rive = sinon.stub().returns(riveInstanceMock);- Assert event handlers: If your component listens for Rive events (like
on(EventType.RiveEvent, handler)), you can simulate these events in your test by calling the handler directly and asserting component behavior[6].
3. Assert DOM and Behavioral Effects
- Check that the correct canvas or container is rendered.
- If your UI reacts to Rive events (for example, on animation completion), simulate those events and verify the correct DOM changes or actions are triggered.
test('it reacts to Rive animation event', async function(assert) {
await render(hbs`<MyRiveComponent />`);
// Simulate event
riveInstanceMock.on.withArgs('riveevent').callArgWith(1, { data: { name: 'someEvent' } });
// Assert component behavior
assert.dom('.some-result').hasText('Expected Result');
});4. Handle Asynchronous Behavior
Many Rive interactions (loading, animation events) are async. Use async/await in your test functions and await any helpers or simulated event triggers[7].
5. Tips and Good Practices
- Only assert what your component is responsible for (e.g., the canvas is rendered, specific event handlers are registered, or the component reacts to Rive events).
- Avoid dependence on actual Rive files or network requests.
- Use Ember’s
pauseTest()helper if you need to manually inspect DOM output during test development[7].
Example Workflow
| Purpose | Test Method |
|---|---|
| Canvas is rendered | Assert with assert.dom('canvas').exists() |
| Rive is initialized | Mock Rive and assert initialization parameters |
| Animation event fires | Simulate event, assert component reaction |
| UI updates after event | Assert DOM updates or action calls |
Summary
- Use rendering (integration) tests for Ember components with Rive.
- Mock Rive APIs to avoid real animation/network dependencies.
- Simulate Rive events and assert component reactions.
- Focus assertions on rendered elements and component behavior, not Rive’s internal rendering[1][5][6][7].
This test strategy ensures that your components work as intended, even when integrating complex, third-party animation libraries like Rive.
Citations:
- 1: https://guides.emberjs.com/v5.8.0/testing/testing-components/
- 2: https://guides.emberjs.com/v2.14.0/testing/unit-testing-basics/
- 3: https://timgthomas.com/2019/11/unit-testing-glimmer-components/
- 4: https://www.youtube.com/watch?v=GAkd0Hk9fbw
- 5: https://github.com/PoslinskiNet/ember-testing-guide
- 6: https://rive.app/docs/runtimes/web/rive-parameters
- 7: https://www.youtube.com/watch?v=nIxSdOA_aho
- 8: https://alisdair.mcdiarmid.org/ember-component-integration-tests/
- 9: https://surgestudio.gumroad.com/l/ember
- 10: https://guides.emberjs.com/v1.10.0/testing/testing-components/
Add integration tests for GleamLogoComponent
The GleamLogoComponent currently has no test coverage. To ensure its Rive‑based animation logic works as intended, add an integration (rendering) test that:
- Sets up the rendering test environment (
setupRenderingTest(hooks)) - Renders the component via
await render(hbs) - Asserts that a
<canvas>(or designated container) is inserted into the DOM - Stubs or mocks the global
Riveconstructor:- e.g.
let riveMock = { on: sinon.spy(), play: sinon.spy(), pause: sinon.spy() }; window.Rive = sinon.stub().returns(riveMock);
- e.g.
- Verifies
new Rive({...})is invoked with the expected init options (canvas element, autoplay, etc.) - Simulates a Rive event via
riveMock.on.callArgWith(1, { data: { name: '…' } })and asserts the component updates accordingly (DOM changes or tracked‐property updates) - Confirms cleanup: after destroying the component, ensure any Rive playback was stopped or the instance reference cleared
Feel free to follow Ember’s testing guide for rendering tests and let me know if you’d like a full example test implementation.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
app/components/gleam-logo.ts (4)
6-16: Consider simplifying the component interface.The interface includes a default block, but there's no use of yielded content in this component. This could be confusing for developers looking to use this component.
interface GleamLogoSignature { Element: HTMLDivElement; Args: { height: number; }; - - Blocks: { - default: []; - }; }
50-57: Consider making canvas dimensions dynamic and responsive.Currently, the canvas is created with a fixed base size of 400x400px regardless of the container size. This might lead to unnecessary memory usage on smaller screens or insufficient quality on larger screens.
- const baseSize = 400; // Base size for quality - canvas.width = baseSize; - canvas.height = baseSize; + // Set canvas size based on container or device pixel ratio + const pixelRatio = window.devicePixelRatio || 1; + const size = Math.min(this.args.height, 200) * pixelRatio; + canvas.width = size; + canvas.height = size; // Let the canvas scale naturally within its container canvas.style.width = '100%'; canvas.style.height = '100%';
79-92: Extract hover simulation logic to a separate method.The hover state simulation logic is embedded in the onLoad callback, making it harder to read and maintain.
+ @action + simulateHoverEffect(stateMachineName: string): void { + if (!this.riveInstance) return; + + const inputs = this.riveInstance.stateMachineInputs(stateMachineName); + + if (inputs) { + inputs.forEach(input => { + if (input.name.toLowerCase().includes('hover')) { + input.value = true; + + // Set timeout to trigger hover out after 1 second + setTimeout(() => { + if (this.riveInstance) { + input.value = false; + } + }, 1000); + } + }); + } + } // Then in the onLoad callback: onLoad: () => { if (this.riveInstance) { // Initial resize this.riveInstance.resizeDrawingSurfaceToCanvas(); - const inputs = this.riveInstance.stateMachineInputs('State Machine 1'); - - // Try to trigger hover state - if (inputs) { - inputs.forEach(input => { - if (input.name.toLowerCase().includes('hover')) { - input.value = true; - - // Set timeout to trigger hover out after 1 second - setTimeout(() => { - if (this.riveInstance) { - input.value = false; - } - }, 1000); - } - }); - } + this.simulateHoverEffect('State Machine 1'); } },🧰 Tools
🪛 ESLint
[error] 80-80: Replace
inputwith(input)(prettier/prettier)
[error] 83-83: Delete
··················(prettier/prettier)
62-71: Make animation file path configurable.The animation file path is hardcoded, making it less flexible for future use cases. Consider making it configurable through component arguments.
interface GleamLogoSignature { Element: HTMLDivElement; Args: { height: number; + animationPath?: string; }; } // ... this.riveInstance = new Rive({ - src: '/assets/animations/gleam_logo_animation.riv', + src: this.args.animationPath || '/assets/animations/gleam_logo_animation.riv', canvas: canvas, // ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
app/components/gleam-logo.ts(1 hunks)
🧰 Additional context used
🪛 ESLint
app/components/gleam-logo.ts
[error] 26-26: Expected blank line before this statement.
(padding-line-between-statements)
[error] 80-80: Replace input with (input)
(prettier/prettier)
[error] 83-83: Delete ··················
(prettier/prettier)
🔇 Additional comments (1)
app/components/gleam-logo.ts (1)
23-27:❓ Verification inconclusive
Ensure aspect ratio preservation is intentional.
The container style sets both width and height to the same value, enforcing a square aspect ratio. Make sure this matches the aspect ratio of the animation file to avoid distortion.
🏁 Script executed:
#!/bin/bash # Check if the animation file is square or if it has a different aspect ratio curl -sI https://github.com/codecrafters-io/frontend/raw/main/public/assets/animations/gleam_logo_animation.riv | grep -i content-length echo "Note: This checks the file size, but we'd need additional tooling to inspect the Rive file dimensions"Length of output: 354
Verify Rive animation matches square container
ThecontainerStylegetter forces a 1:1 aspect ratio by setting equal width and height. Confirm thegleam_logo_animation.rivasset is square to prevent distortion. If it’s not, consider computing width based on its actual aspect ratio or removing the square constraint.• File:
app/components/gleam-logo.ts
Lines 23–27🧰 Tools
🪛 ESLint
[error] 26-26: Expected blank line before this statement.
(padding-line-between-statements)
| setTimeout(() => { | ||
| if (this.riveInstance) { | ||
| input.value = false; | ||
| } | ||
| }, 1000); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Store and clean up the hover timeout.
The hover timeout is not stored or cleared when the component is destroyed, which could lead to errors if the component is removed before the timeout completes.
@tracked riveInstance: Rive | null = null;
container: HTMLElement | null = null;
animationInterval: number | null = null;
+ hoverTimeout: number | null = null;
@action
cleanupRive() {
if (this.animationInterval) {
clearInterval(this.animationInterval);
this.animationInterval = null;
}
+ if (this.hoverTimeout) {
+ clearTimeout(this.hoverTimeout);
+ this.hoverTimeout = null;
+ }
if (this.riveInstance) {
this.riveInstance.stop();
this.riveInstance = null;
}
}
// Then when setting the timeout:
- setTimeout(() => {
+ this.hoverTimeout = window.setTimeout(() => {
if (this.riveInstance) {
input.value = false;
}
}, 1000);📝 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.
| setTimeout(() => { | |
| if (this.riveInstance) { | |
| input.value = false; | |
| } | |
| }, 1000); | |
| } | |
| // In app/components/gleam-logo.ts | |
| // Add a tracked property to hold the timeout ID | |
| @tracked riveInstance: Rive | null = null; | |
| container: HTMLElement | null = null; | |
| animationInterval: number | null = null; | |
| @tracked hoverTimeout: number | null = null; | |
| @action | |
| cleanupRive() { | |
| if (this.animationInterval) { | |
| clearInterval(this.animationInterval); | |
| this.animationInterval = null; | |
| } | |
| if (this.hoverTimeout) { | |
| clearTimeout(this.hoverTimeout); | |
| this.hoverTimeout = null; | |
| } | |
| if (this.riveInstance) { | |
| this.riveInstance.stop(); | |
| this.riveInstance = null; | |
| } | |
| } | |
| // … elsewhere in your hover‐leave handler … | |
| // Replace the bare setTimeout call with a stored timeout | |
| this.hoverTimeout = window.setTimeout(() => { | |
| if (this.riveInstance) { | |
| input.value = false; | |
| } | |
| }, 1000); |
app/components/gleam-logo.ts
Outdated
| } catch (error: unknown) { | ||
| console.error('Error setting up Rive:', error); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Implement better error handling for Rive initialization failures.
Currently, errors are only logged to the console, which isn't sufficient for production environments. Consider sending these errors to a monitoring service or providing a fallback UI.
🌐 Web query:
Does Ember have a built-in error reporting service or recommended error handling pattern for components?
💡 Result:
Ember Error Reporting and Error Handling Patterns
Ember does not provide a built-in error reporting service directly integrated into the framework, but it does offer several built-in hooks and patterns for handling errors in components and across the application.
Global Error Handling
Ember provides a global error handling mechanism through the Ember.onerror hook. This function catches uncaught errors thrown anywhere within your Ember application, allowing you to implement custom logic for logging, user notification, or reporting to external services. For promise-related errors, Ember exposes the Ember.RSVP.on('error', handler) hook, which allows you to catch unhandled promise rejections globally[2][6].
Recommended Error Handling Patterns
- Ember.onerror: Define a global error handler to catch and process uncaught exceptions. This can be used for logging and reporting errors.
- RSVP Promise Errors: Use
Ember.RSVP.configure('onerror', handler)to capture errors from unhandled promise rejections[2][6]. - Route-Specific Errors: Handle errors in route actions such as the
erroraction within a route, which will be triggered when errors occur during transitions. - Try-Catch Blocks: Use try-catch blocks in your methods or actions for more granular error handling, especially in asynchronous code[6].
- Display User-Friendly Messages: Normalize errors and display clear, actionable error messages to users.
Component Error Handling
While Ember does not offer component-level error “boundaries” like React, you can use try-catch logic within component actions or lifecycle hooks. For more advanced use, some community addons and patterns facilitate error propagation and management within components[6].
Error Reporting
For reporting errors to external services, Ember supports integration with many third-party error monitoring platforms, such as Sentry, Rollbar, and Airbrake:
- Sentry: Official Sentry SDK for Ember (
@sentry/ember) automatically captures uncaught errors and exceptions. After installing the addon, you initialize Sentry early in your application’s startup file, and errors will be reported automatically[3][8]. You can also manually report errors usingSentry.captureException()within your code[4]. - Other Services: Platforms like Rollbar, Errorception, and ExceptionHub are also compatible with Ember via global hooks[2].
Community Addons
There are several Ember addons to enhance error handling and reporting. For example, ember-error-handler centralizes error capturing and lets you route errors to logging services, custom error pages, or whatever logic you need[1][9].
Summary Table: Ember Error Handling Approaches
| Pattern/Tool | Purpose | Built-in or Addon | Usage |
|---|---|---|---|
| Ember.onerror | Catch uncaught errors app-wide | Built-in | Global handler for logging/reporting |
| RSVP.on('error') | Handle uncaught promise rejections | Built-in | Promise rejection handling |
| Route error action | Catch errors in specific route contexts | Built-in | Show route-level error UI |
| Sentry/Rollbar | Report errors to external services | Third-party addon | Error monitoring and alerting |
| ember-error-handler | Centralize and customize error handling | Community addon | Custom logic, UI, or remote logging |
Conclusion
Ember does not have an all-in-one built-in error reporting service, but it provides robust primitives (Ember.onerror, RSVP hooks, and route error actions) for handling and customizing error behavior. For error reporting, the Ember ecosystem supports seamless integration with tools like Sentry via official and community-maintained addons, and there are established patterns for routing errors and providing user feedback in components and across the app[2][6][8][9].
Citations:
- 1: https://emberobserver.com/categories/error-handling-and-reporting
- 2: https://discuss.emberjs.com/t/capturing-and-reporting-errors-in-production/2647
- 3: https://sentry.io/for/ember/
- 4: https://mainmatter.com/blog/2019/07/15/sentry-and-ember/
- 5: https://discuss.emberjs.com/t/app-wide-error-handling/9212
- 6: https://moldstud.com/articles/p-handling-errors-and-exceptions-in-emberjs-applications
- 7: https://discuss.emberjs.com/t/handling-errors-with-the-now-default-ember-data-json-api-adapter/8542
- 8: https://docs.sentry.io/platforms/javascript/guides/ember/
- 9: https://github.com/janmisek/ember-error-handler
- 10: https://www.zipy.ai/blog/debug-ember-container-not-found-error
Enhance Rive Initialization Error Handling
Ember doesn’t include a turnkey error-reporting service, but you can hook into its global error APIs (or integrate Sentry/Rollbar) and fall back to a safe UI when initialization fails.
• Location to address:
– app/components/gleam-logo.ts (lines 96–98)
• Suggested changes:
} catch (error: unknown) {
- console.error('Error setting up Rive:', error);
+ // 1) Report the error globally or to an external service
+ Ember.onerror?.(error);
+ // If using Sentry add-on:
+ // import { captureException } from '@sentry/ember';
+ // captureException(error);
+
+ // 2) Trigger a fallback UI state
+ this.hasLoadError = true;
}• Next steps:
- Add
@tracked hasLoadError = false;in the component. - Update the template to render a static logo (or message) when
hasLoadErroris true. - Initialise and configure your chosen monitoring addon (e.g.
@sentry/ember) and/or implement anEmber.onerrorhook inapp/app.jsfor centralized reporting.
📝 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.
| } catch (error: unknown) { | |
| console.error('Error setting up Rive:', error); | |
| } | |
| } catch (error: unknown) { | |
| // 1) Report the error globally or to an external service | |
| Ember.onerror?.(error); | |
| // If using Sentry add-on: | |
| // import { captureException } from '@sentry/ember'; | |
| // captureException(error); | |
| // 2) Trigger a fallback UI state | |
| this.hasLoadError = true; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
app/components/gleam-logo.ts(1 hunks)tests/integration/components/gleam-logo-test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/components/gleam-logo.ts
🧰 Additional context used
🪛 Biome (1.9.4)
tests/integration/components/gleam-logo-test.js
[error] 77-77: The constructor should not return a value.
The constructor is here:
Returning a value from a constructor is ignored.
(lint/correctness/noConstructorReturn)
🔇 Additional comments (2)
tests/integration/components/gleam-logo-test.js (2)
86-104: Good implementation of the rendering testThe rendering test is thorough and properly checks all the expected properties of both the container and canvas elements. It verifies dimensions, styles, and the existence of elements, which are all important aspects of the component's visual representation.
7-65: Well-structured MockRive implementationThe mock implementation of the Rive animation library provides all the necessary methods for testing while maintaining a clean separation of concerns. The event handling methods, state machine inputs, and load simulation are implemented appropriately.
| window.Rive = class MockRiveConstructor { | ||
| constructor(options) { | ||
| const instance = new MockRive(options); | ||
| this._onLoadCallback = options.onLoad; | ||
|
|
||
| return instance; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Fix the constructor return pattern in MockRiveConstructor
The constructor is returning a value, which is problematic in JavaScript. Constructors should initialize the object being created, not return a different object.
window.Rive = class MockRiveConstructor {
constructor(options) {
- const instance = new MockRive(options);
this._onLoadCallback = options.onLoad;
-
- return instance;
+ Object.assign(this, new MockRive(options));
}
};This approach ensures that this is properly initialized with the properties from MockRive while maintaining the constructor pattern's integrity.
📝 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.
| window.Rive = class MockRiveConstructor { | |
| constructor(options) { | |
| const instance = new MockRive(options); | |
| this._onLoadCallback = options.onLoad; | |
| return instance; | |
| } | |
| }; | |
| // tests/integration/components/gleam-logo-test.js | |
| window.Rive = class MockRiveConstructor { | |
| constructor(options) { | |
| this._onLoadCallback = options.onLoad; | |
| Object.assign(this, new MockRive(options)); | |
| } | |
| }; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 77-77: The constructor should not return a value.
The constructor is here:
Returning a value from a constructor is ignored.
(lint/correctness/noConstructorReturn)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/components/gleam-logo.ts (1)
87-92: 🛠️ Refactor suggestionStore and clean up the hover timeout to prevent memory leaks.
The timeout created to reset the hover state isn't tracked or cleaned up if the component is destroyed before the timeout completes. This could lead to memory leaks or errors accessing destroyed objects.
export default class GleamLogoComponent extends Component<GleamLogoSignature> { animationInterval: number | null = null; container: HTMLElement | null = null; @tracked riveInstance: Rive | null = null; + hoverTimeout: number | null = null; @action cleanupRive() { if (this.animationInterval) { clearInterval(this.animationInterval); this.animationInterval = null; } + if (this.hoverTimeout) { + clearTimeout(this.hoverTimeout); + this.hoverTimeout = null; + } if (this.riveInstance) { // ...And in the hover reset code:
// Set timeout to trigger hover out after 1 second - setTimeout(() => { + this.hoverTimeout = window.setTimeout(() => { if (this.riveInstance) { input.value = false; } }, 1000);
🧹 Nitpick comments (3)
app/components/gleam-logo.ts (3)
73-77: Add window resize handling to ensure proper animation display.The animation is sized correctly on initial load but won't adjust if the window or container is resized after initialization. This could lead to display issues on responsive layouts.
onLoad: () => { if (this.riveInstance) { // Initial resize this.riveInstance.resizeDrawingSurfaceToCanvas(); + // Handle window resize events + const handleResize = () => { + if (this.riveInstance) { + this.riveInstance.resizeDrawingSurfaceToCanvas(); + } + }; + + window.addEventListener('resize', handleResize); + + // Store resize handler for cleanup + this._resizeHandler = handleResize; }And add cleanup to the
cleanupRivemethod:@action cleanupRive() { // Existing cleanup... + // Remove resize event listener + if (this._resizeHandler) { + window.removeEventListener('resize', this._resizeHandler); + this._resizeHandler = null; + } }Don't forget to add the property declaration:
export default class GleamLogoComponent extends Component<GleamLogoSignature> { animationInterval: number | null = null; container: HTMLElement | null = null; @tracked riveInstance: Rive | null = null; + _resizeHandler: (() => void) | null = null;
78-94: Improve input validation for hover state handling.The current implementation doesn't validate if a suitable hover input exists before trying to manipulate it. Without proper validation, this could lead to runtime errors if the animation file changes.
const inputs = this.riveInstance.stateMachineInputs('State Machine 1'); // Try to trigger hover state if (inputs) { + // Find a hover input if available + const hoverInput = inputs.find(input => + input.name.toLowerCase().includes('hover') && + typeof input.value === 'boolean' + ); + + if (hoverInput) { + // Set hover state and reset after delay + hoverInput.value = true; + + // Set timeout to trigger hover out after 1 second + this.hoverTimeout = window.setTimeout(() => { + if (this.riveInstance) { + hoverInput.value = false; + } + }, 1000); + } - inputs.forEach((input) => { - if (input.name.toLowerCase().includes('hover')) { - input.value = true; - - // Set timeout to trigger hover out after 1 second - setTimeout(() => { - if (this.riveInstance) { - input.value = false; - } - }, 1000); - } - }); }
24-29: Consider using more explicit responsive sizing approach.The current container sizing logic uses inline styles with a fixed aspect ratio. Consider using CSS class-based approaches for more maintainable responsive sizing.
You could define a CSS class for the container in a separate stylesheet and use standard responsive approaches instead of computing inline styles. This would make the styling more maintainable and consistent with other components.
Example of using CSS classes instead:
get containerStyle(): string { // Ensure minimum size on mobile while maintaining aspect ratio const minSize = Math.min(this.args.height, 200); return `height: ${minSize}px; width: ${minSize}px; max-width: 100%;`; }Could become:
With CSS:
.gleam-logo-container { height: var(--logo-size, 144px); width: var(--logo-size, 144px); max-width: 100%; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
app/components/gleam-logo.ts(1 hunks)
🔇 Additional comments (4)
app/components/gleam-logo.ts (4)
7-17: LGTM: Well-structured component signature.The component signature is well-defined with appropriate types for the element, arguments, and blocks. This provides good TypeScript support and documentation.
31-42: LGTM: Good resource cleanup implementation.The
cleanupRiveaction properly handles resource cleanup by clearing intervals and stopping the Rive instance, which helps prevent memory leaks.
64-72: LGTM: Appropriate Rive initialization with good configuration options.The Rive instance is initialized with proper options for the animation source, canvas, state machines, and layout. The use of
Fit.Containensures the animation scales properly while maintaining its aspect ratio.
110-114: LGTM: Proper Glint registry registration.The component is correctly registered in the Glint registry, which ensures proper TypeScript type checking and autocompletion when using this component in templates.
| } catch (error: unknown) { | ||
| console.error('Error setting up Rive:', error); | ||
| Sentry.captureException(error, { | ||
| tags: { | ||
| component: 'GleamLogo', | ||
| action: 'handleDidInsert', | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling with fallback UI.
The error handling captures and reports errors but doesn't provide any visual feedback if the animation fails to load. Users will see an empty space rather than a fallback logo.
export default class GleamLogoComponent extends Component<GleamLogoSignature> {
animationInterval: number | null = null;
container: HTMLElement | null = null;
@tracked riveInstance: Rive | null = null;
+ @tracked hasLoadError: boolean = false;
// ...existing code...
@action
handleDidInsert(element: HTMLDivElement) {
this.container = element;
try {
// ...existing initialization code...
} catch (error: unknown) {
console.error('Error setting up Rive:', error);
+ this.hasLoadError = true;
Sentry.captureException(error, {
tags: {
component: 'GleamLogo',
action: 'handleDidInsert',
},
});
}
}
}Then in the template file (app/components/gleam-logo.hbs), add a fallback:
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/integration/components/gleam-logo-test.js (1)
72-79:⚠️ Potential issueFix the constructor return pattern in MockRiveConstructor
The constructor is returning a value, which is problematic in JavaScript. Constructors should initialize the object being created, not return a different object.
window.Rive = class MockRiveConstructor { constructor(options) { - const instance = new MockRive(options); this._onLoadCallback = options.onLoad; - - return instance; + Object.assign(this, new MockRive(options)); } };This approach ensures that
thisis properly initialized with the properties fromMockRivewhile maintaining the constructor pattern's integrity.🧰 Tools
🪛 Biome (1.9.4)
[error] 77-77: The constructor should not return a value.
The constructor is here:
Returning a value from a constructor is ignored.
(lint/correctness/noConstructorReturn)
🧹 Nitpick comments (1)
tests/integration/components/gleam-logo-test.js (1)
141-141: Update assertion message to match actual testThe assertion message refers to "after timeout" but the test is now using a mouseleave event instead of a timeout.
- assert.false(hoverInput.value, 'Hover input is set to false after timeout'); + assert.false(hoverInput.value, 'Hover input is set to false after mouseleave');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
tests/integration/components/gleam-logo-test.js(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
tests/integration/components/gleam-logo-test.js
[error] 77-77: The constructor should not return a value.
The constructor is here:
Returning a value from a constructor is ignored.
(lint/correctness/noConstructorReturn)
🪛 ESLint
tests/integration/components/gleam-logo-test.js
[error] 108-108: Do not call settled() right after a test helper that already calls it internally.
(ember/no-settled-after-test-helper)
🪛 GitHub Actions: Test
tests/integration/components/gleam-logo-test.js
[error] 108-108: ESLint: Do not call settled() right after a test helper that already calls it internally (ember/no-settled-after-test-helper)
🔇 Additional comments (1)
tests/integration/components/gleam-logo-test.js (1)
1-143: Good job implementing the Gleam logo animation testsOverall, the test implementation for the Gleam logo animation component looks well-structured and comprehensive. The tests verify both the rendering functionality and the hover interaction behavior. The mock implementation of the Rive library is well designed to simulate the necessary behaviors for testing.
🧰 Tools
🪛 Biome (1.9.4)
[error] 77-77: The constructor should not return a value.
The constructor is here:
Returning a value from a constructor is ignored.
(lint/correctness/noConstructorReturn)
🪛 ESLint
[error] 108-108: Do not call
settled()right after a test helper that already calls it internally.(ember/no-settled-after-test-helper)
🪛 GitHub Actions: Test
[error] 108-108: ESLint: Do not call
settled()right after a test helper that already calls it internally (ember/no-settled-after-test-helper)
|
|
||
| test('it handles hover state correctly', async function (assert) { | ||
| await render(hbs`<GleamLogo />`); | ||
| await settled(); |
There was a problem hiding this comment.
Remove unnecessary settled() call after render()
The render() helper already calls settled() internally, so this additional call is redundant and violates the ember/no-settled-after-test-helper ESLint rule.
await render(hbs`<GleamLogo />`);
- await settled();Removing this line will fix the pipeline failure while maintaining the same test behavior.
📝 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.
| await settled(); | |
| await render(hbs`<GleamLogo />`); |
🧰 Tools
🪛 ESLint
[error] 108-108: Do not call settled() right after a test helper that already calls it internally.
(ember/no-settled-after-test-helper)
🪛 GitHub Actions: Test
[error] 108-108: ESLint: Do not call settled() right after a test helper that already calls it internally (ember/no-settled-after-test-helper)
Arpan-206
force-pushed
the
arpan/cc-1698-add-rive-animation-for-gleam-icon
branch
from
490791e to
8603f7e
Compare
| @@ -0,0 +1,3 @@ | |||
| <div {{did-insert this.handleDidInsert}} {{will-destroy this.cleanupRive}} style={{this.containerStyle}} ...attributes> | |||
There was a problem hiding this comment.
@Arpan-206 Why is the naming convention inconsistent here? did-insert uses this.handleDidInsert (correct), and will-destroy uses this.cleanupRive instead of this.handleWillDestroy?
| Element: HTMLDivElement; | ||
|
|
||
| Args: { | ||
| height: number; |
There was a problem hiding this comment.
Let's try to avoid needing a number to be passed in and see if we can just have it fit the container / parent element
|
|
||
| get containerStyle(): string { | ||
| // Ensure minimum size on mobile while maintaining aspect ratio | ||
| const minSize = Math.min(this.args.height, 200); |
There was a problem hiding this comment.
@Arpan-206 where does the "200" number come from?
| const canvas = document.createElement('canvas'); | ||
|
|
||
| // Set initial canvas size for high quality | ||
| const baseSize = 400; // Base size for quality |
There was a problem hiding this comment.
@Arpan-206 let's see if we can avoid this magic number too and just have it pick a sensible value (like based on pixel density & container height)
| this.riveInstance = new Rive({ | ||
| src: '/assets/animations/gleam_logo_animation.riv', | ||
| canvas: canvas, | ||
| stateMachines: 'State Machine 1', |
There was a problem hiding this comment.
It is mandatory to pass in state machine names? Will this work without it?
| const inputs = this.riveInstance.stateMachineInputs('State Machine 1'); | ||
|
|
||
| // Try to trigger hover state | ||
| if (inputs) { |
There was a problem hiding this comment.
Let's move most logic to the animatino file itself, and maybe name the animation / state machine onLoad
| @@ -1,4 +1,6 @@ | |||
| {{#if (eq @variant "color")}} | |||
| {{#if (and (eq @language.slug "gleam") (eq @variant "color"))}} | |||
| <GleamLogo @height={{144}} ...attributes /> | |||
There was a problem hiding this comment.
@Arpan-206 I mentioned this before, let's move this out to the tracks page
Checklist:
[percy]in the message to trigger)Summary by CodeRabbit