feat(FlipClock): add Reset instance method #7657
Conversation
Reviewer's GuideAdds a reusable Reset method to FlipClock and refactors the JavaScript initialization logic to make the countdown/counter state part of the options object, updating invocation wiring between .NET and JS accordingly. Sequence diagram for FlipClock Reset and JS init interactionsequenceDiagram
actor User
participant FlipClock
participant JSInterop
participant FlipClock_js
participant Browser
User->>FlipClock: Reset()
activate FlipClock
FlipClock->>JSInterop: InvokeVoidAsync init(Id, Interop, options)
activate JSInterop
JSInterop->>FlipClock_js: init(id, invoke, options)
deactivate JSInterop
activate FlipClock_js
FlipClock_js->>Browser: requestAnimationFrame(flip)
activate Browser
loop every_1000ms
Browser->>FlipClock_js: flip(timestamp)
FlipClock_js->>FlipClock_js: go(el, options)
FlipClock_js->>FlipClock_js: getDate(options)
FlipClock_js->>FlipClock_js: getConfig(el)
FlipClock_js->>FlipClock_js: setDigits(list, value, digits, options.countDown)
alt countdown_completed
FlipClock_js->>JSInterop: invoke.invokeMethodAsync(OnCompleted)
deactivate FlipClock_js
JSInterop->>FlipClock: OnCompleted()
deactivate Browser
else not_completed
Browser-->>Browser: schedule next frame
end
end
deactivate FlipClock
Class diagram for updated FlipClock componentclassDiagram
class FlipClock {
+Task InvokeInitAsync()
+Task Reset()
-double GetTicks()
+Task OnCompleted()
}
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Calling the new
Resetmethod will invokeinitagain and schedule a newrequestAnimationFrameloop without cancelling the previous one; consider tracking and cancelling the existing animation frame (e.g., storing the id and usingcancelAnimationFrame) to avoid multiple overlapping update loops per clock instance.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Calling the new `Reset` method will invoke `init` again and schedule a new `requestAnimationFrame` loop without cancelling the previous one; consider tracking and cancelling the existing animation frame (e.g., storing the id and using `cancelAnimationFrame`) to avoid multiple overlapping update loops per clock instance.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7657 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 750 750
Lines 33185 33191 +6
Branches 4605 4605
=========================================
+ Hits 33185 33191 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds a new instance-level reset capability to the FlipClock component so consumers can restart counting (especially after a countdown completes), aligning with issue #7640’s requested behavior.
Changes:
- Introduces a new public
Reset()method onFlipClockand uses it for initial JS initialization. - Refactors
FlipClock.razor.jsto move timing state into theoptionsobject and changes JS init signature to accept a separate .NET interop reference.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/BootstrapBlazor/Components/FlipClock/FlipClock.razor.js | Updates init signature/state handling and refactors tick/render helpers. |
| src/BootstrapBlazor/Components/FlipClock/FlipClock.razor.cs | Adds Reset() API and routes initial initialization through it. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -104,6 +39,72 @@ | |||
| requestAnimationFrame(flip); | |||
There was a problem hiding this comment.
init() starts a new requestAnimationFrame loop every time it’s called, but there’s no cancellation/guard for an existing loop on the same element. With the new Reset() API (which re-invokes init), repeated resets will leave prior loops running concurrently (double updates + potential multiple onCompleted invocations). Consider tracking the RAF handle/token per id (e.g., in a module-level map or on the element) and cancel/stop the previous loop before starting a new one; also consider exporting a dispose(id) to stop the loop when the component is removed.
|
|
||
| if (countDown && current.hours === 0 && current.minutes === 0 && current.seconds === 0) { | ||
| options.invoke.invokeMethodAsync(options.onCompleted); | ||
| if (options.countDown && current.hours === 0 && current.minutes === 0 && current.seconds === 0) { |
There was a problem hiding this comment.
Countdown completion detection should not rely only on hours/minutes/seconds == 0. For durations >= 24h, when totalMilliseconds lands exactly on a day boundary (e.g., 86400000ms), hours/minutes/seconds become 0 while days is still > 0, causing premature completion. Use options.totalMilliseconds <= 0 (or include days/months/years in the check) to decide completion reliably.
| if (options.countDown && current.hours === 0 && current.minutes === 0 && current.seconds === 0) { | |
| if (options.countDown && options.totalMilliseconds <= 0) { |
| counter: 0, | ||
| totalMilliseconds : 0, | ||
| countDown: false |
There was a problem hiding this comment.
totalMilliseconds : 0 has inconsistent spacing before the colon compared to typical object literal formatting in this file (startValue: 0, onCompleted: null). Consider normalizing to totalMilliseconds: 0 for consistency.
| /// <para lang="zh">重置方法</para> | ||
| /// <para lang="en">Reset method</para> | ||
| /// </summary> | ||
| /// <returns></returns> |
There was a problem hiding this comment.
XML doc has an empty <returns> tag on a new public API (Reset). Please either remove the tag or fill it with the standard Task description (e.g., “A Task representing the asynchronous operation.”) to match other public methods’ docs.
| /// <returns></returns> | |
| /// <returns>A Task representing the asynchronous operation.</returns> |
1 participant
Link issues
fixes #7640
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add a resettable FlipClock initialization flow and expose it via a new instance Reset API.
New Features:
Enhancements: