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: Play "State Machine 2"
GleamLogoComponent->>RiveLibrary: Play "State Machine 3" after 1s (reset)
User->>GleamLogoComponent: Mouse enters logo
GleamLogoComponent->>RiveLibrary: Play "State Machine 1"
User->>GleamLogoComponent: Mouse leaves logo
GleamLogoComponent->>RiveLibrary: Play "State Machine 1"
User->>GleamLogoComponent: Component removed from DOM
GleamLogoComponent->>RiveLibrary: Stop animation and cleanup
Poem
🪧 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 128.46kB (0.35%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: client-array-pushAssets Changed:
|
Test Results 1 files ±0 1 suites ±0 7m 50s ⏱️ - 1m 31s For more details on these errors, see this check. Results for commit d007423. ± Comparison against base commit 803a7fe. This pull request removes 16 and adds 20 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
❌ 3 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
![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
🔭 Outside diff range comments (1)
tests/integration/components/gleam-logo-test.js (1)
1-216: 💡 Verification agent🧩 Analysis chain
Overall excellent test coverage
This test file provides comprehensive coverage of the Gleam logo component's functionality, including rendering, initialization, hover behavior, and cleanup. The MockRive implementation is thorough and enables effective testing of the component's interaction with the Rive library.
Since this is a visual component, please ensure visual verification has been done:
🌐 Web query:
Does Ember.js have a visual testing tool like Percy or Storybook that could be used to verify this animation?💡 Result:
Ember.js supports several visual testing tools that are comparable to Percy and Storybook and can be used to verify animations in your application.
Visual Testing in Ember.js
Percy for Ember.js
- Percy is a popular visual regression testing tool that integrates with Ember through the
percy-emberaddon. This enables developers to capture screenshots of application states (including animations) and compare them over time to catch visual regressions. Percy supports Ember directly, making it an excellent fit for visually verifying UI changes, including those involving animation[4][10].Storybook for Ember.js
- Storybook, a widely used tool for developing and testing UI components in isolation, has official support for Ember. It allows you to render components and manually verify animations interactively. For automated visual regression, Storybook can be paired with Chromatic, a cloud-based service designed by Storybook’s maintainers, to run visual regression tests on your Storybook stories[7]. This setup can help catch unintended changes in heavily animated or interactive components.
Other Visual Testing Tools
- Ember Observer and GitHub list
ember-visual-test, an Ember-specific addon that allows testing screens for visual changes during acceptance and integration tests. This can also be useful for verifying that animations render as expected across test runs[8].Comparison Table
Tool Ember.js Support Visual Regression Animation Verification Integration Ease Percy Yes (addon) Yes Yes High Storybook Yes (official) Manual, Chromatic Yes (w/ Chromatic) High ember-visual-test Yes Yes Yes Medium Summary
- Percy: Provides visual regression testing for Ember, capturing and comparing animated states.
- Storybook: Great for manual animation verification; can be extended with Chromatic for automated visual tests.
- ember-visual-test: Ember-specific visual regression addon for acceptance and integration tests.
Ember.js is well served by modern visual testing solutions, allowing you to rigorously verify animations and other dynamic visual changes with tools similar to (and including) Percy and Storybook[4][7][8][10].
Citations:
- 1: https://guides.emberjs.com/v5.5.0/testing/testing-tools/
- 2: https://guides.emberjs.com/v4.3.0/testing/
- 3: https://discuss.emberjs.com/t/ember-automated-testing-tools/15787
- 4: https://github.com/percy/percy-ember
- 5: https://yehudakatz.com/2024/09/12/why-ember-is-different-running-browser-tests-in-a-browser/
- 6: https://guides.emberjs.com/v3.2.0/testing/
- 7: https://storybook.js.org/tutorials/intro-to-storybook/ember/en/test/
- 8: https://github.com/Cropster/ember-visual-test
- 9: https://guides.emberjs.com/v1.10.0/testing/test-runners/
- 10: https://www.browserstack.com/guide/visual-testing-tools
Add Visual Regression Tests for GleamLogo Component
The existing tests cover rendering, initialization, hover behavior, and cleanup thoroughly, but they don’t verify the animation visually. Ember.js supports several visual testing solutions that you can integrate to catch unintended changes in the GleamLogo animation:
• Percy (percy-ember addon): capture and compare screenshots of
<GleamLogo />states during your Ember tests.
• Storybook (official Ember support) + Chromatic: author a GleamLogo story and enable automated visual diffs.
• ember-visual-test addon: include visual regression checks directly in your integration or acceptance test suites.Consider adding one of these tools to ensure your Rive‑driven animation renders correctly across future changes.
🧹 Nitpick comments (3)
tests/integration/components/gleam-logo-test.js (3)
8-82: Excellent mock implementation, but consider adding JSDoc commentsThe
MockRiveclass is a thorough and well-designed mock of the Rive animation library, with appropriate tracked properties and methods. This allows for effective testing without the actual library dependency.Consider adding JSDoc comments to document the purpose of each method and property, particularly for developers unfamiliar with the Rive API. For example:
// Mock Rive class for testing +/** + * Mock implementation of the Rive animation library for testing. + * Simulates the behavior of the real Rive instance. + */ class MockRive { // Private properties _interval = null; _listeners = new Map(); // Public properties @tracked canvas; @tracked isLoaded = false; @tracked stateMachineNames = ['State Machine 1', 'State Machine 2']; @tracked lastPlayedStateMachine = null; @tracked lastInputsStateMachine = null; // Constructor + /** + * Creates a new MockRive instance. + * @param {Object} options - The initialization options. + * @param {HTMLCanvasElement} options.canvas - The canvas element to render to. + */ constructor(options) { this.canvas = options.canvas; }
51-62: Consider testing failure scenariosYour mock currently only simulates successful loading, but it would be valuable to also test how the component handles loading failures.
Add a method to simulate load failure:
simulateLoad() { this.isLoaded = true; this.triggerEvent('load'); } + + simulateLoadError(error) { + this.isLoaded = false; + this.triggerEvent('error', error); + }Then add a test case that verifies the component handles errors gracefully.
16-17: Use constants for state machine namesThe state machine names are hardcoded in multiple places, which makes the tests brittle if the actual state machine names change.
Define constants for the state machine names:
+ // Constants for state machine names + const PRIMARY_STATE_MACHINE = 'State Machine 1'; + const SECONDARY_STATE_MACHINE = 'State Machine 2'; class MockRive { // Private properties _interval = null; _listeners = new Map(); // Public properties @tracked canvas; @tracked isLoaded = false; - @tracked stateMachineNames = ['State Machine 1', 'State Machine 2']; + @tracked stateMachineNames = [PRIMARY_STATE_MACHINE, SECONDARY_STATE_MACHINE]; @tracked lastPlayedStateMachine = null; @tracked lastInputsStateMachine = null;Then use these constants throughout the test file instead of hardcoded strings.
Also applies to: 138-138, 153-153
📜 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)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (3)
tests/integration/components/gleam-logo-test.js (3)
1-6: Well-structured test setup!Good job using QUnit and Ember testing helpers for these integration tests. The imports are appropriate for the testing needs.
84-98: Good test setup with proper hooksExcellent use of QUnit hooks for managing the Rive mock. Storing and restoring the original Rive constructor ensures tests don't interfere with each other.
99-120: Thorough rendering testsThe rendering tests appropriately verify that the canvas element is created and that Rive is initialized with the correct parameters.
app/components/gleam-logo.hbs
Outdated
| @@ -0,0 +1,9 @@ | |||
| <div | |||
| class="gleam-logo-container" | |||
| {{did-insert this.setupRive}} | |||
There was a problem hiding this comment.
Our convention for handlers is to always name them after the event (i.e. something like this.handleDidInsert in this case). Let's us now have to worry about naming each of these + allows us to easily edit handlers to add more functionality without having to rename them.
app/components/gleam-logo.ts
Outdated
| Args: { | ||
| class?: string; | ||
| style?: string; | ||
| [key: string]: unknown; |
There was a problem hiding this comment.
@Arpan-206 what is this used for?
There was a problem hiding this comment.
well the first one, forgot to remove the signature
There was a problem hiding this comment.
We don't have to do this anywhere else in the repository, try figuring out why?
app/components/gleam-logo.ts
Outdated
| interface GleamLogoSignature { | ||
| Element: HTMLDivElement; | ||
| Args: { | ||
| class?: string; |
There was a problem hiding this comment.
@Arpan-206 in Ember you don't need to accept DOM-level attributes like class/style, these just get passed along with ...attributes. You only need to use Args for things that you'll explicitly pass in
app/components/language-logo.ts
Outdated
| @@ -15,5 +15,6 @@ export default class LanguageLogoComponent extends Component<Signature> {} | |||
| declare module '@glint/environment-ember-loose/registry' { | |||
| export default interface Registry { | |||
| LanguageLogo: typeof LanguageLogoComponent; | |||
| GleamLogo: typeof import('./gleam-logo').default; | |||
There was a problem hiding this comment.
@Arpan-206 why is this registered here and not in gleam-logo.ts (like every other component is)?
app/components/language-logo.hbs
Outdated
| @@ -1,4 +1,6 @@ | |||
| {{#if (eq @variant "color")}} | |||
| {{#if (and (eq @language.slug "gleam") (eq @variant "color"))}} | |||
| <GleamLogo ...attributes /> | |||
There was a problem hiding this comment.
@Arpan-206 let's not assume that every color language logo usage of Gleam is going to be animated for now - let's keep this explicit (use GleamLogo instead of LanguageLogo in the tracks template).
You can imagine using a color logo in things like user profile pages where we might not want this animation to occur
app/components/gleam-logo.ts
Outdated
| const canvas = document.createElement('canvas'); | ||
|
|
||
| // Set the canvas size to be 4x the original size for better quality | ||
| canvas.width = 141 * 4; |
There was a problem hiding this comment.
@Arpan-206 where do these numbers like 141 and 144 come from? Our goal is to be able to use Rive animations as a "component" in the future, without having to hardcode values specific to each animation file. Is there a way to do that here? Something like saying we want the height to fit the container and the width to just adjust accordingly? Similar to how we handle SVG images
app/components/gleam-logo.ts
Outdated
| }, 2000); | ||
| }, 15000); | ||
| // Play initial animation with State Machine 2 | ||
| setTimeout(() => { |
There was a problem hiding this comment.
Does autoplay not cover this? And it not, what does autoplay do?
| }, | ||
| }); | ||
| } catch (error: unknown) { | ||
| console.error('Error setting up Rive:', error); |
There was a problem hiding this comment.
Let's report this to sentry, don't just swallow and ignore
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.
| 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.
| 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); | |
| } | |
| }); | |
| } | |
| } |
| 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.
Checklist:
[percy]in the message to trigger)Summary by CodeRabbit