Sparkles

[DXCanas]

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.

[rtibblesbot]

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

[rtibblesbot]

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;
});

[rtibblesbot]

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.

[rtibblesbot]

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);
  }
}

[rtibblesbot]

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.

[github-actions]

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.19.5.dev85_g61e67b7b6.d20260605.pex
Windows Installer (EXE)	kolibri-0.19.5.dev85+g61e67b7b6.d20260605-windows-setup-unsigned.exe
Debian Package	kolibri_0.19.5.dev85+g61e67b7b6-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.19.5.dev85+g61e67b7b6.d20260605.dmg
Android Package (APK)	kolibri-0.19.5.dev85+g61e67b7b6.d20260605-0.1.10-debug.apk
Raspberry Pi Image	kolibri-pi-image-0.19.5.dev85+g61e67b7b6.zip
TAR file	kolibri-0.19.5.dev85+g61e67b7b6.d20260605.tar.gz
WHL file	kolibri-0.19.5.dev85+g61e67b7b6.d20260605-py3-none-any.whl

Smoke test screenshot

Download screenshot

[rtibblesbot]

Prior findings

Resolved:

• player.stop() instead of player.destruct() (blocking) — onBeforeUnmount now calls player.destruct() with a defensive type-check
• Blanket <!-- eslint-disable --> at file level (nitpick) — replaced with targeted max-len, vue/max-len and scoped /* eslint-disable */ around the minified block
• No reduced-motion guard for the burst (suggestion) — burst is skipped entirely when reduce is true
• .submit-burst missing left coordinate (nitpick) — now has left: 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

[rtibblesbot]

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.

[rtibblesbot]

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.

[rtibblesbot]

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.

[rtibbles]

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.

[DXCanas]

Ah, yeah, this was one of the AI-Assisted changes from the test-writing phase. I will double check, but I think it was having issues with some of the lint-ignore comments for the svgator-vue-component file

[DXCanas]

AI's defense:

We only bumped ecmaVersion for the animation files because the SVGator-generated player code embedded inside those .vue files uses newer syntax (class fields + ??=) that Kolibri’s base ESLint config didn’t understand. Without the override, every lint run failed before we could even touch the file. That’s why the block at the bottom of eslint.config.mjs pins languageOptions.ecmaVersion = 2022 for /animations//*.vue only — it was a surgical workaround so we didn’t change the language target for the entire repo.

I'll look into alternatives!

[rtibblesbot]

Understood — @rtibbles' suggestion to set ecmaVersion: 'latest' in the base kolibri-format configuration is the right approach. That would cover the SVGator syntax needs globally without a file-specific override. Deferring to @rtibbles' assessment here.

[AlexVelezLl]

Just some thoughts!

[AlexVelezLl]

Are these lines actually needed? In theory, the comment above says the previous blob code already plays the animation, right? Asked Claude to interpret the code above, and it says it does, but I'm not completely sure 😅. Just double-checking :). In any case, I'd say the comment above should explain a bit more where all this bundled code is coming from and what it does.

[AlexVelezLl]

We don't really need a player global variable right? we can just query the element and extract the player directly here.

[AlexVelezLl]

Wonder if it'd be better to move this to its own file just to keep the code a bit more readable, as in theory is the "general" tool (in contrast to the code for the specific animation below).

[DXCanas]

I'd considered this, but could not find a precedent, and figured "if it ain't broke" after the mess I made prior to getting @LianaHarris360's code in 😅

[AlexVelezLl]

Yeah, there isn't really any practical motivation other than abstracting this piece of code because we won't be mounting this component more than once at any given time, and we don't use this in other places (and if we do in the future, we can then figure it out 😅)

[AlexVelezLl]

Could we define all of this in a more imperative way? I think defining that many setTimeOuts having to manually compute how much time will it take and having to check an isLast to know if the animation burst needs to be triggered is too complex.

I think we can just create a wait(time)=> new Promise(resolve => setTimeout(resolve, time)) helper, and then just call await wait(STAGGER) after every sequence item but the last one would make all of this much more readable.

something like

for (i in ..) {
  playAnimationFor(i);
  if (!isLast) {
    await wait(STAGGER)
  }
}

playBurstAnimation();
await wait(BURST_DURATION);
resolve();

Then the resolve call is called idiomatically after all animations succeeded instead of computing what the wait time should be and modify it every time we add something new here.

Relatedly, if reduce is true, should we just call the resolve() without playing any animation and avoid the reduce ? 0 : ... computations?

[DXCanas]

I have not ignored this, and am investigating! The general complication with the timings has to do with the orchestration of the various animations throughout picture login, so I'm trying to see what's compatible and how we might be able to streamline the JS/CSS pieces in a more legible format :)

That being said, I admittedly just sort of let the AI go wild here; there's a lot here 😬

[AlexVelezLl]

Ah, my other high-level comment: would it be easier if we just played a .gif here? Not sure what the complexities of using a .gif would be, but bumping the build size with the bundled svgator for a tiny animation that happens only on a very specific path feels a bit too much? 😅. (I know it may have some resolution issues, but the animation height and width are very small, so I don't think this should be too noticeable)

[DXCanas]

Ah, my other high-level comment: would it be easier if we just played a .gif here? Not sure what the complexities of using a .gif would be, but bumping the build size with the bundled svgator for a tiny animation that happens only on a very specific path feels a bit too much? 😅. (I know it may have some resolution issues, but the animation height and width are very small, so I don't think this should be too noticeable)

This was not really my call, and therefore more a question for @LianaHarris360, I think?

[LianaHarris360]

I'm not against giving it a try, especially if there's a decrease in the build size with the .gif and it displays the same output. I've asked Jessica for the converted animation and will share it through Slack. I'm not sure of the complexities involved for using it either, but I will research it and share what I find.