Feat/spinner pulse 1739#1944
Merged
Karanjot786 merged 5 commits intoJul 3, 2026
Merged
Conversation
Animate Checkbox checkmark with fadeIn/fadeOut using terminal dim attribute. Animate Switch knob sliding through intermediate positions (●── → ─●─ → ──●). Respect prefersReducedMotion() — skip animation when caps.motion is false. Add @termuijs/motion dependency to @termuijs/ui package. Closes Karanjot786#1738
github-actions
Bot
added
area:widgets
📝 WalkthroughWalkthroughThis PR adds motion-based animations to Switch, Checkbox, Tooltip, and Spinner widgets using a new ChangesWidget Animation Cohort
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Widget as Widget (Switch/Checkbox/Tooltip/Spinner)
participant Motion as `@termuijs/motion`
User->>Widget: state change (setValue/setChecked/setVisible/setActive)
Widget->>Widget: cancel in-flight animation
alt prefersReducedMotion
Widget->>Widget: snap progress/opacity to end state
else animate
Widget->>Motion: fadeIn/fadeOut(150ms)
Motion-->>Widget: progress/opacity updates
Widget->>Widget: markDirty()
end
Widget->>Widget: _renderSelf uses animation state
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
packages/widgets/src/display/Tooltip.test.ts (1)
113-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest doesn't actually exercise "cancels previous animation".
The title claims cancellation of an in-progress fade-in, but the test never starts a fade-in before calling
setVisible(false); it only assertsfadeOutwas called once, which would pass even without any cancellation logic. Consider driving an actual in-flightfadeInfirst (e.g. mockfadeInto capture its cancel function) and asserting that cancel function is invoked whensetVisible(false)runs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/widgets/src/display/Tooltip.test.ts` around lines 113 - 127, The Tooltip test currently only checks that Tooltip.setVisible(false) calls motion.fadeOut, so it never proves an in-progress fade-in is canceled. Update the test in Tooltip.test.ts to first trigger a real fade-in path by constructing Tooltip with visible false and then calling setVisible(true) while mocking motion.fadeIn to return a cancel function, then call setVisible(false) and assert that the captured cancel function was invoked before fadeOut. Use the existing Tooltip, setVisible, motion.fadeIn, and motion.fadeOut symbols to keep the test focused on the cancellation behavior.packages/widgets/src/feedback/Spinner.ts (1)
243-255: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
mount()doesn't clean up existing timers/animations before starting new ones.
setActive()cancels_timerUnsub/_animCancelbefore starting a new one (lines 176-181), butmount()doesn't apply the same guard. Ifmount()were ever called again without an interveningunmount()(e.g. re-parenting), this would leak a duplicatesetIntervalsubscription or a duplicate pulse fade loop running in parallel. MirroringsetActive()'s cleanup here would make the lifecycle robust regardless of caller assumptions, and would also remove the visible jump from the constructor's initial_animProgress = 0.5straight to0when_startPulse()kicks in.🔧 Proposed fix
mount(): void { super.mount(); + this._timerUnsub?.(); + this._animCancel?.(); if (prefersReducedMotion() || !this._active) return; if (this._animation === 'pulse') { this._startPulse();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/widgets/src/feedback/Spinner.ts` around lines 243 - 255, The Spinner.mount() lifecycle method starts a new timer or pulse animation without clearing any existing _timerUnsub/_animCancel state first, unlike setActive(); update mount() to mirror the same cleanup guard before calling _startPulse() or timerPoolSubscribe so repeated mounts cannot leak duplicate animation loops or intervals. Use the Spinner class methods mount(), setActive(), _startPulse(), and the _timerUnsub/_animCancel fields to locate and apply the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/demo-animations.ts`:
- Around line 38-52: The global key handler in the demo is toggling all widgets
on space/enter, which can conflict with focused widgets’ own key handling and
cause double-toggles. Update the key listener in the demo’s event handler to
avoid manually calling toggle() on cb1/cb2/cb3/sw1/sw2/sw3 for those keys, and
rely on each widget’s built-in handleKey behavior or add explicit focus
navigation if you want to demonstrate interaction. Use the app.events.on('key',
...) handler and the widget instances in this demo to locate the change.
In `@packages/ui/src/Switch.ts`:
- Around line 80-86: The Switch mount/unmount lifecycle leaves _animProgress
stale for the off state, so a remounted switch can stay mid-transition after a
cancelled fadeOut. Update Switch.mount() (and, if needed, the unmount/reset
path) to resync _animProgress for both this._value true and false so the visual
state always matches the current value after remounting, then markDirty() when
the value-based progress is corrected.
In `@packages/widgets/src/display/Tooltip.test.ts`:
- Line 99: The Tooltip tests are mocking the plain boolean caps.motion
incorrectly with a getter spy, which will not work reliably. Update the
reduced-motion test setup in Tooltip.test.ts to override caps.motion via
Object.defineProperty with a configurable value, and make sure the original
value is restored after each test. Use the caps.motion reference in the affected
describe/test block to keep the mock localized and repeatable.
In `@packages/widgets/src/display/Tooltip.ts`:
- Around line 42-48: The Tooltip remount path is leaving a stale `_animOpacity`
behind after an interrupted fade-out, so a hidden tooltip can still render on
remount. Update `Tooltip.mount()` to also normalize the hidden case by resetting
`_animOpacity` when `_visible` is false (or otherwise ensuring hidden state
cannot keep a positive opacity), alongside the existing visible-state
correction. Make sure the fix works with `setVisible(false)`, `unmount()`, and
`_renderSelf()` so remounting a hidden tooltip cannot show border/content until
visibility changes again.
In `@packages/widgets/src/feedback/Spinner.test.ts`:
- Around line 153-160: The Spinner test setup is using getter spies on
caps.motion and caps.unicode, but those env capability fields are plain values,
not accessors. Update the beforeEach in Spinner.test.ts to override caps.motion
and caps.unicode directly, and make sure the original values are restored
explicitly in afterEach; keep the change localized to the Spinner — Pulse
animation block and reference the caps object usage there.
In `@packages/widgets/src/input/Checkbox.ts`:
- Around line 145-151: The Checkbox mount() logic only restores animation
progress for the checked state, so interrupted fadeOut transitions can leave
_animProgress mid-transition on remount. Update Checkbox.mount() to mirror
Switch.mount() by correcting both branches: when _checked is true clamp
_animProgress to 1, and when _checked is false restore it to 0 if it was left
partially transitioned, then call markDirty() so the visual state is reset.
---
Nitpick comments:
In `@packages/widgets/src/display/Tooltip.test.ts`:
- Around line 113-127: The Tooltip test currently only checks that
Tooltip.setVisible(false) calls motion.fadeOut, so it never proves an
in-progress fade-in is canceled. Update the test in Tooltip.test.ts to first
trigger a real fade-in path by constructing Tooltip with visible false and then
calling setVisible(true) while mocking motion.fadeIn to return a cancel
function, then call setVisible(false) and assert that the captured cancel
function was invoked before fadeOut. Use the existing Tooltip, setVisible,
motion.fadeIn, and motion.fadeOut symbols to keep the test focused on the
cancellation behavior.
In `@packages/widgets/src/feedback/Spinner.ts`:
- Around line 243-255: The Spinner.mount() lifecycle method starts a new timer
or pulse animation without clearing any existing _timerUnsub/_animCancel state
first, unlike setActive(); update mount() to mirror the same cleanup guard
before calling _startPulse() or timerPoolSubscribe so repeated mounts cannot
leak duplicate animation loops or intervals. Use the Spinner class methods
mount(), setActive(), _startPulse(), and the _timerUnsub/_animCancel fields to
locate and apply the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f3459d0e-21fb-466c-b368-9577e0dffbf6
📒 Files selected for processing (8)
examples/demo-animations.tspackages/ui/package.jsonpackages/ui/src/Switch.tspackages/widgets/src/display/Tooltip.test.tspackages/widgets/src/display/Tooltip.tspackages/widgets/src/feedback/Spinner.test.tspackages/widgets/src/feedback/Spinner.tspackages/widgets/src/input/Checkbox.ts
Karanjot786
added
gssoc:approved
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
2 participants
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.
Description
Adds an optional
animation: 'pulse'mode to Spinner that smoothly oscillates a single character between dim and bold usingfadeIn/fadeOutfrom@termuijs/motion. Existing'spin'default (frame cycling) is unchanged.Related Issue
Closes #1739
Which package(s)?
@termuijs/widgets
Type of Change
type:bug)type:feature)type:docs)type:testing)type:refactor)type:design)type:accessibility)type:performance)type:devops)type:security)Checklist
needs-starcheck blocks your merge otherwise.bun vitest runbun run buildbun run typecheckCONTRIBUTING.md.type: short description.markDirty()(if your change affects rendering).anytypes without an inline comment explaining why.GSSoC 2026 Participation
https://gssoc.girlscript.org/profile/ac85deec-3598-47ef-a9e8-f39ae0af5147Screenshots / Recordings (UI changes)
Notes for the Reviewer
'spin'mode (default, unchanged) cycles through frame characters (⠋⠙⠹...) — this is the terminal equivalent of rotation and was already present pre-PR. This PR adds'pulse'as an additional animation style, not a replacement.small/medium/largeconcept (it renders a single character atheight: 1). The requirement from the issue doesn't map onto the actual API — no changes needed.prefersReducedMotion()is true, consistent with other animated widgets.