Sparkles#14780
Conversation
github-actions
Bot
added
APP: User
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Summary
This PR adds a celebratory burst animation to the PicturePasswordGrid submit button. The core idea is good and the accessibility fundamentals are correct (aria-hidden="true" on the SVG, prefers-reduced-motion respected for the CSS animations). However, there are two blocking issues: a confirmed test regression from the DURATION change, and an incomplete resource cleanup in onBeforeUnmount.
Blocking
Test regression from DURATION change (__tests__/PicturePasswordGrid.spec.js:359-364)
The DURATION constant was changed from 380 to 1480, but the playSuccessAnimation test still advances timers by only 380ms at the final step and has a stale comment // After the final 380ms. With a 3-element sequence and STAGGER=150, the last icon starts bouncing at t=300ms and its cleanup timer fires at t=300+1480=1780ms. The test reads state at t=680ms total and expects bouncingId to be null, arrowBouncing to be false, and resolved to be true — none of which can be true at that point. The final jest.advanceTimersByTime call must be updated to 1480 and the comment corrected. (The spec file is not in this diff, so the inline comment appears in the review body.)
onBeforeUnmount calls player.stop() instead of player.destruct() — see inline comment on SubmitBurstAnimation.vue:195.
Suggestions
burstVisible remains true for 1100ms under prefers-reduced-motion — see inline comment on PicturePasswordGrid.vue:314.
SubmitBurstAnimation not stubbed in test mountComponent()
The test helper at __tests__/PicturePasswordGrid.spec.js:303-305 stubs PicturePasswordOption and KIcon but not SubmitBurstAnimation. When burstVisible becomes true during the animation test, the real SVGator component mounts and its embedded IIFE attempts to call document.getElementById, window.requestAnimationFrame, and register a resize listener. In a jsdom environment this may produce console errors caught by jest-fail-on-console. Adding 'SubmitBurstAnimation' to the stubs array would isolate the test correctly.
No unit test for SubmitBurstAnimation.vue
The AGENTS.md requirement states "Testing is Required." This new component has no test file. A minimal test verifying it renders without errors and that player.destruct is called on unmount would satisfy the requirement.
burstVisible toggle behaviour is untested
The new burstVisible ref — set to true when the last icon bounces, and auto-cleared after 1100ms — has no test coverage. A test checking burstVisible at the right time steps would close this gap.
Nitpicks
.submit-burst missing explicit horizontal centering — see inline comment on PicturePasswordGrid.vue:488.
Blanket <!-- eslint-disable --> at line 1 of SubmitBurstAnimation.vue — see inline comment.
submitBurst.svg is a design source artifact committed unnecessarily
This file is not imported anywhere in the codebase — the animation is fully embedded in SubmitBurstAnimation.vue. Committing SVG design source files to the component tree adds noise without benefit.
WIP/debug commit messages in branch history
The branch includes commits titled "WIP: Adding a container parent..." and "Testing burst animation display". These should be squashed or reworded before merge.
Praise
The ESLint flat config override in eslint.config.mjs is exactly the right mechanism — scoped to **/animations/**/*.vue, setting only languageOptions.ecmaVersion: 2022 without touching rules or globals, with a clear explanatory comment. This is a minimal and correct intervention that avoids disabling any actual lint rules.
The accessibility fundamentals are solid: aria-hidden="true" on the SVG correctly marks the animation as decorative, and the v-if-driven mount/unmount pattern for the SVGator player is the right approach — mounting equals play-start, and the onBeforeUnmount hook ensures cleanup on every hide cycle.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Reviewed the pull request diff checking for:
- Correctness: bugs, edge cases, undocumented behavior, resource leaks, hardcoded values
- Design: unnecessary complexity, naming, readability, comment accuracy, redundant state
- Architecture: duplicated concerns, minimal interfaces, composition over inheritance
- Testing: behavior-based assertions, mocks only at hard boundaries, accurate coverage
- Completeness: missing dependencies, unupdated usages, i18n, accessibility, security
- Principles: DRY (same reason to change), SRP, Rule of Three (no premature abstraction)
- Checked CI status and linked issue acceptance criteria
- For UI changes: inspected screenshots for layout, visual completeness, and consistency
| player.ready(player => player.play()); | ||
| }); | ||
|
|
||
| onBeforeUnmount(() => { |
There was a problem hiding this comment.
blocking: player.stop() only resets the playhead and cancels the pending requestAnimationFrame — it does not remove the global resize listener registered by the SVGator player IIFE. The SVGator player exposes player.destruct() specifically for full teardown, which removes all listeners and nulls internal state. Per the project convention "whoever allocates a resource releases it", this should call player.destruct() instead:
onBeforeUnmount(() => {
if (player && typeof player.destruct === 'function') {
player.destruct();
}
player = null;
});| @@ -0,0 +1,204 @@ | |||
| <!-- eslint-disable --> | |||
There was a problem hiding this comment.
nitpick: The file-level <!-- eslint-disable --> disables all ESLint rules for the entire file. The actual rule violation (import-x/no-import-module-exports from the UMD wrapper) is already handled by the targeted // eslint-disable-next-line comment on line 169. This blanket disable is redundant and will silently swallow any future lint issues introduced into the non-generated parts of this file. Consider removing it.
| @@ -305,7 +313,13 @@ | |||
| const isLast = i === sequence.value.length - 1; | |||
| window.setTimeout(() => { | |||
There was a problem hiding this comment.
suggestion: When prefers-reduced-motion: reduce is active, dur and stagger are zeroed (correct), but burstVisible is still set to true and the 1100ms teardown timer still runs. The SVGator player in SubmitBurstAnimation has no reduced-motion guard of its own, so the animation plays even for users who have opted into reduced motion.
Consider skipping the burst entirely when reduce is true:
if (isLast) {
arrowBouncing.value = true;
if (!reduce) {
burstVisible.value = true;
window.setTimeout(() => {
burstVisible.value = false;
}, 1100);
}
}| justify-content: center; | ||
| } | ||
|
|
||
| .submit-button { |
There was a problem hiding this comment.
nitpick: .submit-burst has position: absolute; top: 50%; z-index: 100 but no left coordinate. Without an explicit left: 50%; transform: translate(-50%, -50%), the horizontal position of the burst SVG depends on the flex container's current layout rather than being explicitly centered. This may work visually now but will be fragile if the container layout changes.
Build Artifacts
Smoke test screenshot |
…se as playground.
- Move flexbox centering from submit-button to submit-container - Remove redundant top positioning from submit-burst (centered via flex parent)
Align PicturePasswordGrid success animation timing with actual visual behavior so the promise resolves after the burst window, not just icon bounce. Keep reduced-motion users out of burst playback and make burst overlay positioning explicit. Update grid unit tests to validate burst visibility timing, reduced-motion behavior, and isolate animation tests by stubbing the burst component. Resolves #15097 AI-assisted-by: gpt-5
Use full SVGator teardown by calling player.destruct() on unmount, add focused unit coverage for burst render and teardown behavior, and remove the unused submitBurst.svg design-source artifact. Keep lint suppressions scoped to generated SVG constraints and the UMD wrapper import context. Resolves #15097 AI-assisted-by: gpt-5
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Prior findings
Resolved:
player.stop()instead ofplayer.destruct()(blocking) —onBeforeUnmountnow callsplayer.destruct()with a defensive type-check- Blanket
<!-- eslint-disable -->at file level (nitpick) — replaced with targetedmax-len, vue/max-lenand scoped/* eslint-disable */around the minified block - No reduced-motion guard for the burst (suggestion) — burst is skipped entirely when
reduceistrue .submit-burstmissingleftcoordinate (nitpick) — now hasleft: 50%; transform: translate(-50%, -50%)
4/4 prior findings resolved.
CI still running (Python/frontend jobs pending at review time). Static analysis (CodeQL, Analyze) passes.
Video shows the grid, error feedback, and confirmation dialog working correctly. The burst animation is too brief to appear in extracted frames, but code and lifecycle are sound.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews deterministically via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
- Checked CI status and linked issue acceptance criteria
| }); | ||
|
|
||
| onBeforeUnmount(() => { | ||
| if (player && typeof player.destruct === 'function') { |
There was a problem hiding this comment.
praise: player.destruct() is the correct full teardown, and the typeof player.destruct === 'function' guard handles the case where the SVGator runtime wasn't successfully installed. Good resource cleanup discipline — allocates in onMounted, releases in onBeforeUnmount.
|
|
||
| window.setTimeout(() => resolve(), (sequence.value.length - 1) * stagger + dur); | ||
| const lastStart = (iconCount - 1) * stagger; | ||
| const animationDuration = Math.max(lastStart + iconDuration, lastStart + burstDuration); |
There was a problem hiding this comment.
praise: Using Math.max(lastStart + iconDuration, lastStart + burstDuration) correctly gates the Promise on whichever animation takes longer — the burst outlasts the icon bounce, so the parent caller doesn't proceed while sparkles are still on screen.
| wrapper.destroy(); | ||
| }); | ||
|
|
||
| it('calls player.destruct on unmount when available', () => { |
There was a problem hiding this comment.
praise: Explicitly testing the teardown path (not just mount) closes the loop on the prior blocking finding — this test would have caught the original player.stop() bug.
| { | ||
| files: ['**/animations/**/*.vue'], | ||
| languageOptions: { | ||
| ecmaVersion: 2022, |
There was a problem hiding this comment.
I am not sure there's a good reason to pin the ecmaVersion - we can set it as 'latest' in the base kolibri-format configuration.
5 participants
Summary
Screen.Recording.2026-06-01.at.12.50.02.PM.mov
References
…
Reviewer guidance
…
AI usage
Actually, honestly, like... none 🤯 (up to 0e17493, the implementation)
Although it helped with some of the bullet points in the 0e17493 commit message.