fix: resolve race condition in auth bento password animation#2759
Open
myusubov wants to merge 5 commits intoappwrite:mainfrom
Open
fix: resolve race condition in auth bento password animation#2759myusubov wants to merge 5 commits intoappwrite:mainfrom
myusubov wants to merge 5 commits intoappwrite:mainfrom
Conversation
Contributor
WalkthroughThe PR adds AbortSignal support and a startIndex option to the animations module’s write and unwrite functions. Both now accept an options object ({ signal?, startIndex? }), initialize their index from startIndex, listen for abort events to clear intervals, and reject with 'Aborted' when signaled. A Svelte component was updated to create and manage an AbortController per interaction, pass its signal and startIndex to write/unwrite calls, abort any existing controller before starting a new sequence, handle aborted rejections, and abort on component unmount. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/(marketing)/(components)/bento/(animations)/auth.svelte (1)
19-73:⚠️ Potential issue | 🟡 Minor
controlleris never aborted when the component is destroyedThe
$effectbody does not return a cleanup function, socontroller?.abort()is never called on component teardown. Any in-flightsetIntervalfromwrite/unwritewill keep firing and attempt to mutatepasswordon a destroyed component.🛡️ Proposed fix — return a teardown from `$effect`
$effect(() => { inView( ... ); hover( ... ); + + return () => { + controller?.abort(); + }; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(marketing)/(components)/bento/(animations)/auth.svelte around lines 19 - 73, The $effect currently doesn't return a teardown, so the AbortController and the inView/hover listeners are never cleaned up; update the $effect to return a cleanup function that (1) calls controller?.abort() to stop any in-flight write/unwrite, (2) invokes any cleanup functions returned by inView(...) and hover(...) (store their return values when you call them), and (3) reset controller to a fresh AbortController if needed—refer to the existing symbols controller, $effect, inView, hover, write, unwrite, and password to locate and wire up this teardown logic.
🧹 Nitpick comments (1)
src/lib/animations/index.ts (1)
28-31: Abort event listener is never removed on normal completionThe
'abort'listener on lines 28–31 (and the mirror on lines 62–65) persists after the animation resolves. In this codebase the signal is never reused, so the stale listener is harmless, but{ once: true }is the standard way to self-remove it.♻️ Proposed change — apply `{ once: true }` to both listeners
- signal?.addEventListener('abort', () => { + signal?.addEventListener('abort', () => { clearInterval(interval); reject(new Error('Aborted')); - }); + }, { once: true });Apply the same change to the identical listener in
unwrite(lines 62–65).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/animations/index.ts` around lines 28 - 31, The abort event listeners added via signal?.addEventListener('abort', ...) in the write and unwrite functions should be made self-removing by passing the options object { once: true } so the handler is removed after it fires; update the listener calls in the write function (the listener that clears interval and rejects with Error('Aborted')) and the mirror listener in unwrite to include { once: true } as the third argument.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/animations/index.ts`:
- Around line 7-26: In the write function, guard against startIndex >=
text.length to avoid the ++i pre-increment skipping the terminating condition
and causing an infinite interval; before creating the Promise (after the
existing empty-text early return) check if startIndex >= text.length (or clamp
startIndex to text.length-1) and immediately invoke cb(text) and return a
resolved Promise so no interval is started; reference the write function, the
startIndex/i variables and the ++i termination check when locating where to add
the guard.
- Around line 41-60: In unwrite, i is initialized with startIndex ?? text.length
which treats 0 as valid and causes a pre-decrement to -1; change the i
initialization in the unwrite implementation so it is clamped/guarded (e.g. if
typeof startIndex === 'number' and startIndex > 0 use that value, otherwise
default to text.length or clamp into [1, text.length]) to prevent i from ever
being 0 or negative before the pre-decrement; update the i assignment in the
unwrite function in src/lib/animations/index.ts accordingly so the interval
callback never sees negative slice indices or an infinite loop.
---
Outside diff comments:
In `@src/routes/`(marketing)/(components)/bento/(animations)/auth.svelte:
- Around line 19-73: The $effect currently doesn't return a teardown, so the
AbortController and the inView/hover listeners are never cleaned up; update the
$effect to return a cleanup function that (1) calls controller?.abort() to stop
any in-flight write/unwrite, (2) invokes any cleanup functions returned by
inView(...) and hover(...) (store their return values when you call them), and
(3) reset controller to a fresh AbortController if needed—refer to the existing
symbols controller, $effect, inView, hover, write, unwrite, and password to
locate and wire up this teardown logic.
---
Nitpick comments:
In `@src/lib/animations/index.ts`:
- Around line 28-31: The abort event listeners added via
signal?.addEventListener('abort', ...) in the write and unwrite functions should
be made self-removing by passing the options object { once: true } so the
handler is removed after it fires; update the listener calls in the write
function (the listener that clears interval and rejects with Error('Aborted'))
and the mirror listener in unwrite to include { once: true } as the third
argument.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/routes/(marketing)/(components)/bento/(animations)/auth.svelte (1)
22-72: Duplicate animation setup logic betweeninViewandhoverhandlersThe bodies of the
inViewcallback (lines 22–35) and its cleanup (lines 37–44) are byte-for-byte identical to thehovercallback (lines 50–62) and its cleanup (lines 64–71), differing only in the guard (!isMobile()vsisMobile()). Extracting shared helpers would reduce surface area for future divergence.♻️ Example refactor
+ function startWrite() { + controller?.abort(); + controller = new AbortController(); + write('•••••••••••••', (v) => (password = v), 1000, { + signal: controller.signal, + startIndex: password.length + }) + .then(() => { animate(button, { scale: [1, 0.95, 1] }, { duration: 0.25 }); }) + .catch((err) => { if (err !== 'Aborted') console.error(err); }); + } + + function startUnwrite() { + controller?.abort(); + controller = new AbortController(); + unwrite('•••••••••••••', (v) => (password = v), 500, { + signal: controller.signal, + startIndex: password.length + }).catch((err) => { if (err !== 'Aborted') console.error(err); }); + } $effect(() => { inView(container, () => { if (!isMobile()) return; - controller?.abort(); - controller = new AbortController(); - write('•••••••••••••', (v) => (password = v), 1000, { - signal: controller.signal, - startIndex: password.length - }) - .then(() => { animate(button, { scale: [1, 0.95, 1] }, { duration: 0.25 }); }) - .catch(() => {}); - return () => { - controller?.abort(); - controller = new AbortController(); - unwrite('•••••••••••••', (v) => (password = v), 500, { - signal: controller.signal, - startIndex: password.length - }).catch(() => {}); - }; + startWrite(); + return startUnwrite; }, { amount: 'all' }); hover(container, () => { if (isMobile()) return; - controller?.abort(); - controller = new AbortController(); - write('•••••••••••••', (v) => (password = v), 1000, { - signal: controller.signal, - startIndex: password.length - }) - .then(() => { animate(button, { scale: [1, 0.95, 1] }, { duration: 0.25 }); }) - .catch(() => {}); - return () => { - controller?.abort(); - controller = new AbortController(); - unwrite('•••••••••••••', (v) => (password = v), 500, { - signal: controller.signal, - startIndex: password.length - }).catch(() => {}); - }; + startWrite(); + return startUnwrite; }); return () => { controller?.abort(); }; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(marketing)/(components)/bento/(animations)/auth.svelte around lines 22 - 72, Extract the duplicated animation logic used in the inView callback and the hover callback into a shared helper function (e.g., runMaskedPasswordAnimation) that accepts the guard check (or a predicate like shouldRun = () => !isMobile()/isMobile()), the AbortController reference (controller), element refs (button, container), and state setter (password); move the common sequence (controller?.abort(), controller = new AbortController(), write(...).then(animate...).catch(), and cleanup that aborts and calls unwrite(...).catch()) into this helper, then call that helper from both the inView handler and the hover handler, preserving the different guard conditions and startIndex usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/routes/`(marketing)/(components)/bento/(animations)/auth.svelte:
- Line 35: Replace the silent .catch(() => {}) handlers on the write/unwrite
promise chains with logic that only swallows the expected cancellation error
(the string 'Aborted') and rethrows any other rejection; update all four .catch
call-sites associated with the write/unwrite calls (the promises that follow the
(v) => (password = v) and similar callbacks) so that runtime errors are not
silently lost but cancellations are suppressed.
---
Nitpick comments:
In `@src/routes/`(marketing)/(components)/bento/(animations)/auth.svelte:
- Around line 22-72: Extract the duplicated animation logic used in the inView
callback and the hover callback into a shared helper function (e.g.,
runMaskedPasswordAnimation) that accepts the guard check (or a predicate like
shouldRun = () => !isMobile()/isMobile()), the AbortController reference
(controller), element refs (button, container), and state setter (password);
move the common sequence (controller?.abort(), controller = new
AbortController(), write(...).then(animate...).catch(), and cleanup that aborts
and calls unwrite(...).catch()) into this helper, then call that helper from
both the inView handler and the hover handler, preserving the different guard
conditions and startIndex usage.
src/routes/(marketing)/(components)/bento/(animations)/auth.svelte
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/routes/(marketing)/(components)/bento/(animations)/auth.svelte (1)
19-80: Consider extracting shared animation logic to reduce duplication.The
inViewandhoverhandlers share identical write/unwrite calls, signal wiring, and error handling — only theisMobile()guard differs. A small helper (e.g.,startWriteAnimation/startUnwriteAnimation) would consolidate the four near-identical call sites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(marketing)/(components)/bento/(animations)/auth.svelte around lines 19 - 80, Extract the repeated animation setup into a small helper (e.g., startWriteAnimation and stopWriteAnimation) that accepts the controller reference, text, callbacks to set password, the button element for animate, and the signal options; move the shared logic from both inView and hover (the controller?.abort(); controller = new AbortController(); call to write(...).then(...).catch(...), and the teardown that aborts, reassigns controller and calls unwrite(...).catch(...)) into these helpers and call them from the inView and hover handlers while keeping the differing isMobile() guards; reference existing symbols controller, write, unwrite, animate, isMobile, container, button and ensure error handling still checks for Error && err.message !== 'Aborted'.src/lib/animations/index.ts (1)
18-40: Already-aborted signal: one tick leaks before detection.If
writeis called with a signal that is already aborted, theaddEventListener('abort', …)on line 32 is a no-op (event already dispatched). The abort is only detected on the first interval tick (line 20), so onecb()invocation with++istill fires before rejection.A pre-flight guard eliminates the leak:
Suggested improvement
let i = startIndex; return new Promise<void>((resolve, reject) => { + if (signal?.aborted) { + return reject(new Error('Aborted')); + } + const interval = setInterval(() => {Same pattern applies to
unwrite(line 61).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/animations/index.ts` around lines 18 - 40, The write function currently sets up an interval and only checks signal.aborted inside the first tick, allowing one cb(text.slice(0, ++i)) to run when the provided AbortSignal is already aborted; add a pre-flight guard in write that checks if signal?.aborted and immediately reject(new Error('Aborted')) (and avoid starting the interval) before creating interval/adding the listener, and mirror the same pre-flight check in unwrite; reference the write/unwrite functions, the signal/abort check, interval, cb and i so you can place the guard before setInterval and before attaching the 'abort' listener.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/routes/`(marketing)/(components)/bento/(animations)/auth.svelte:
- Around line 82-84: The cleanup currently only aborts controller; capture and
store the cancel/stop functions returned from motion.inView(...) and
motion.hover(...) (e.g., const stopInView = inView(...), const stopHover =
hover(...)) when you call them, and then call those stop functions inside the
existing return cleanup alongside controller?.abort() so both the
IntersectionObserver and mouse event listeners are removed on unmount (ensure
you call stopInView() and stopHover() if they are defined).
---
Nitpick comments:
In `@src/lib/animations/index.ts`:
- Around line 18-40: The write function currently sets up an interval and only
checks signal.aborted inside the first tick, allowing one cb(text.slice(0, ++i))
to run when the provided AbortSignal is already aborted; add a pre-flight guard
in write that checks if signal?.aborted and immediately reject(new
Error('Aborted')) (and avoid starting the interval) before creating
interval/adding the listener, and mirror the same pre-flight check in unwrite;
reference the write/unwrite functions, the signal/abort check, interval, cb and
i so you can place the guard before setInterval and before attaching the 'abort'
listener.
In `@src/routes/`(marketing)/(components)/bento/(animations)/auth.svelte:
- Around line 19-80: Extract the repeated animation setup into a small helper
(e.g., startWriteAnimation and stopWriteAnimation) that accepts the controller
reference, text, callbacks to set password, the button element for animate, and
the signal options; move the shared logic from both inView and hover (the
controller?.abort(); controller = new AbortController(); call to
write(...).then(...).catch(...), and the teardown that aborts, reassigns
controller and calls unwrite(...).catch(...)) into these helpers and call them
from the inView and hover handlers while keeping the differing isMobile()
guards; reference existing symbols controller, write, unwrite, animate,
isMobile, container, button and ensure error handling still checks for Error &&
err.message !== 'Aborted'.
Comment on lines
+82
to
+84
| return () => { | ||
| controller?.abort(); | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find and examine the auth.svelte file
fd -t f "auth.svelte" -x cat -n {}Repository: appwrite/website
Length of output: 8980
🏁 Script executed:
#!/bin/bash
# Search for inView and hover usage in the codebase
rg -n "inView\|hover" src/routes/\(marketing\)/\(components\)/bento/\(animations\)/ -A 3 -B 1Repository: appwrite/website
Length of output: 42
🌐 Web query:
motion library svelte inView hover return stop cleanup function
💡 Result:
If you mean Motion (the motion package from motion.dev), both inView and hover follow the same pattern:
- You return a function from the “start” callback to run on leave/unhover (good place to
.stop()an animation). - The gesture function itself returns a “stop/cancel” function you should call during Svelte teardown (eg, in
onDestroy/onMountcleanup).
Svelte example (with proper cleanup)
<script>
import { onMount } from "svelte";
import { animate, hover, inView } from "motion";
let el;
onMount(() => {
// hover: return runs on hover end
const cancelHover = hover(el, (element) => {
const a = animate(element, { scale: 1.05 }, { duration: 0.15 });
return () => a.stop(); // hover end
});
// inView: return runs when leaving viewport
const stopInView = inView(el, (element) => {
const a = animate(element, { opacity: 1, y: 0 }, { duration: 0.25 });
return () => a.stop(); // left viewport
});
// Svelte teardown
return () => {
cancelHover();
stopInView();
};
});
</script>
<div bind:this={el} style="opacity:0; transform: translateY(12px);">
Watch me
</div>Notes that often trip people up
- In Motion 12+, the callback signature is
(element, eventOrEntry)(element is first). [1] hover(...)returns a function that cancels gesture detection, and your callback may return an “hover end” cleanup. [2]inView(...)returns a function that stops viewport detection, and your callback may return a “leave viewport” cleanup. [3]
Sources: Motion hover docs [2], Motion inView docs [3], Motion upgrade guide (v12 signature change) [1].
Call the stop functions returned by inView and hover in the $effect cleanup to prevent memory leaks.
The motion library's inView() and hover() functions return cancel functions that must be called to remove their underlying event listeners (IntersectionObserver for inView, MouseEvent listeners for hover). Without calling these, the listeners persist even after component unmount, causing memory and resource leaks.
Required fix
$effect(() => {
- inView(
+ const stopInView = inView(
container,
() => {
// ...
},
{ amount: 'all' }
);
- hover(container, () => {
+ const stopHover = hover(container, () => {
// ...
});
return () => {
controller?.abort();
+ stopInView();
+ stopHover();
};
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/`(marketing)/(components)/bento/(animations)/auth.svelte around
lines 82 - 84, The cleanup currently only aborts controller; capture and store
the cancel/stop functions returned from motion.inView(...) and motion.hover(...)
(e.g., const stopInView = inView(...), const stopHover = hover(...)) when you
call them, and then call those stop functions inside the existing return cleanup
alongside controller?.abort() so both the IntersectionObserver and mouse event
listeners are removed on unmount (ensure you call stopInView() and stopHover()
if they are defined).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes a race condition in the Auth bento card password animation.
When hovering in and out rapidly on desktop, the
writeandunwriteanimation utilities both ran simultaneously, causing the password dots
to flicker and jump erratically.
Changes:
AbortSignalsupport towriteandunwriteinsrc/lib/animations/index.tsso animations can be cleanly cancelled mid-run.
startIndexparameter so animations resume from thecurrent string length instead of jumping to the start/end.
auth.svelteto use a sharedAbortControllerthat cancelsthe previous animation before starting a new one on hover in/out.
Test Plan
Related PRs and Issues
Resolves #2758
Have you read the Contributing Guidelines on issues?
Yes
Summary by CodeRabbit
New Features
Bug Fixes