Sync back to main #108

SergeiGolos · 2025-05-23T11:10:20Z

No description provided.

…pport) (#28) * Initial plan for issue * Fix SetClockAction and CompleteTimerAction to properly start and stop timers Co-authored-by: SergeiGolos <660992+SergeiGolos@users.noreply.github.com> * Implement timer state management and UI integration Co-authored-by: SergeiGolos <660992+SergeiGolos@users.noreply.github.com> * Add tests for timer functionality Co-authored-by: SergeiGolos <660992+SergeiGolos@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: SergeiGolos <660992+SergeiGolos@users.noreply.github.com>

* Initial plan for issue * Add fade-in animation for new effort rows Co-authored-by: SergeiGolos <660992+SergeiGolos@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: SergeiGolos <660992+SergeiGolos@users.noreply.github.com>

gemini-code-assist

Hello @SergeiGolos, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

Summary of Changes

Hello team, gemini-code-assist here with a summary of this pull request. Based on the code changes, as the PR title and description are minimal, the primary intent of this pull request is to introduce a comprehensive timer state management system. This system defines distinct states like RUNNING_COUNTDOWN, RUNNING_COUNTUP, PAUSED, and STOPPED to provide more granular control over how timers behave and are displayed in the UI. The changes involve updating the core runtime logic to dispatch these state changes, modifying UI components and context to consume the state, and enhancing the clock registry hook to track both timer durations and states. Additionally, this PR includes a migration of the testing framework from Jest to Vitest and a minor UI enhancement for event list items.

Highlights

Timer State Management: Introduced a new TimerState enum and SetTimerStateAction to explicitly manage the state of UI timers (countdown, countup, paused, stopped).
Runtime Block Integration: Updated EffortBlock and TimedGroupBlock lifecycle methods (onEnter, onLeave, onBlockStart, onNext, onBlockStop) to dispatch SetTimerStateAction based on block logic and duration availability.
UI Component & Context Updates: Modified the WodTimer component and ClockContext to track and expose the timer state, allowing child components to react to whether the timer is running or in countdown mode. The useClockRegistry hook was updated to manage both durations and states.
Testing Framework Migration: Initiated a migration from Jest to Vitest, updating test files and configuration accordingly.
Timer Completion Logic: Ensured that CompleteTimerAction explicitly stops the timer before popping the block, guaranteeing a final timespan is recorded.

Changelog

Click here to see the changelog

docs/TimerStateManagement.md
- Added new documentation detailing the timer state system, including the TimerState enum, SetTimerStateAction, and integration points in runtime blocks and UI components.
src/components/clock/WodTimer.tsx
- Imported TimerState, getClockDuration, and getClockState.
- Updated ClockContext.Provider to include isRunning and isCountdown derived from the primary timer's state (lines 66-69).
- Adjusted the useEffect hook to initialize durations using the new registry structure (lines 36-54).
src/components/clock/tests/WodTimer.test.tsx
- Added new test file with tests for the WodTimer component, specifically verifying its behavior and context values (isRunning, isCountdown) based on different SET_TIMER_STATE events (lines 54-164).
- Mocked ClockContext and useClockRegistry to facilitate testing.
src/components/metrics/EventsView.tsx
- Added the fade-in CSS class to table rows for unknown events (line 61) and displayable metrics (lines 95, 134).
src/contexts/ClockContext.tsx
- Updated ClockContextType to include separate maps for durations and states, plus isRunning and isCountdown booleans (lines 7-13).
- Updated the default context value to match the new structure (lines 17-23).
- Renamed getClockByName to getClockDuration and updated its signature (lines 31-36).
- Added a new helper function getClockStateName to retrieve timer state by name (lines 39-44).
src/core/runtime/tests/TimerRuntime.spec.ts
- Replaced jest imports and calls with vitest equivalents (describe, it, expect, vi, beforeEach, afterEach, vi.mock, vi.fn, vi.Mocked) (lines 1, 14-17, 19, 24, 39-45, 53-55, 63-65, 68-69, 117-118, 135-136, 156-157, 165-166, 194-195, 220-223, 232-235).
src/core/runtime/actions/CompleteTimerAction.ts
- Modified the applyBlock method to explicitly create and apply a StopTimerAction before popping the block, ensuring the timer is stopped correctly (lines 20-28).
src/core/runtime/actions/tests/ActionOverhaul.test.ts
- Replaced jest imports and calls with vitest equivalents (describe, it, expect, vi, vi.fn) (lines 1, 12-19, 36, 39, 46, 53, 56).
src/core/runtime/actions/tests/SetTimerStateAction.test.ts
- Added new test file with tests for the SetTimerStateAction, verifying event creation with correct state and target (lines 5-35).
src/core/runtime/blocks/EffortBlock.ts
- Imported SetTimerStateAction, TimerState, and getDuration (lines 15-16).
- In onEnter, added logic to determine initial TimerState (countdown or countup) based on duration and dispatched a SetTimerStateAction (lines 33-43).
- Changed the target of SetClockAction from "runtime" to "primary" (line 42).
- In onLeave, added a SetTimerStateAction to set the timer state to STOPPED (line 55).
src/core/runtime/blocks/TimedGroupBlock.ts
- Imported SetTimerStateAction and TimerState (line 19).
- In onBlockStart, added a SetTimerStateAction to set the timer state to RUNNING_COUNTDOWN if a duration exists (lines 112-116).
- In onNext, added a SetTimerStateAction to reset the timer state to RUNNING_COUNTDOWN if a duration exists (lines 193-196).
- In onBlockStop, added a SetTimerStateAction to set the timer state to STOPPED (line 244).
src/core/runtime/blocks/tests/RuntimeBlock.spec.ts
- Replaced jest imports and calls with vitest equivalents (describe, it, expect, vi, beforeEach, vi.fn) (lines 1, 15, 20, 24, 30-31, 227-228, 260, 273).
src/core/runtime/outputs/SetClockAction.ts
- Added a null check for the current block in write (lines 16-18).
- Updated write to retrieve timeSpans from the latest RuntimeSpan and include them in the TimeSpanDuration object (lines 22-24, 29).
- Changed the target of SetClockAction from "runtime" to "primary" (line 30).
src/core/runtime/outputs/SetTimerStateAction.ts
- Added new file defining the TimerState enum and the SetTimerStateAction class, which emits SET_TIMER_STATE output events (lines 1-35).
src/hooks/useClockRegistry.ts
- Defined ClockRegistryState interface to hold both durations and states (lines 10-13).
- Updated state to clockRegistryState holding both maps (lines 24-27).
- Modified useEffect to process SET_TIMER_STATE events and update the states map (lines 56-64).
- Updated the interval logic to create new maps for both durations and states to force re-render (lines 80-83).
- Changed the return value to an object containing both durations and states maps (lines 90-93).
- Added getClockState helper function (lines 107-116).
- Updated getClockDuration signature to use the new registry structure (lines 103-104).
src/index.css
- Added a @keyframes fadeIn animation and a .fade-in CSS class (lines 19-27).
vitest.config.ts
- Added new file configuring Vitest for the project, including plugins, test environment, and path aliases (lines 1-21).

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

A timer ticks, state shifts,
From countup high to countdown gifts.
Running, paused, or stopped,
UI logic's hopped,
On states the runtime lifts.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request introduces timer state management to the wod.wiki project, enhancing the control and display of UI timers. The changes include a new TimerState enum, a SetTimerStateAction class, and integration with the WodTimer component. Additionally, the pull request includes updates to the EffortBlock and TimedGroupBlock to manage timer states during their lifecycles. Overall, the changes seem well-structured and address the intended functionality. However, there are a few areas that could benefit from further review and refinement.

Summary of Findings

Potential Null/Undefined Errors: Several instances exist where object properties are accessed without prior checks for null or undefined values. This could lead to runtime errors if the objects are not properly initialized or if data is missing.
Timer State Determination Logic: The logic for determining the timer state (countdown vs. countup) is repeated in multiple blocks. Extracting this logic into a separate function would improve code reuse and maintainability.
Mocking Library Consistency: The test files use a mix of jest.fn and vi.fn for creating mocks. Using a consistent mocking library (preferably vi.fn as it seems to be the more prevalent choice) would improve code clarity and maintainability.

Merge Readiness

The pull request introduces valuable timer state management features. However, addressing the potential null/undefined errors and improving code consistency would enhance the overall quality and robustness of the code. I recommend addressing these points before merging. I am unable to approve this pull request, and recommend that others review and approve this code before merging.

gemini-code-assist · 2025-05-23T11:14:23Z

src/components/clock/WodTimer.tsx

  useEffect(() => {
-    if (!clockRegistry || !primary || !total) return;
+    if (!clockRegistry) return;


Consider adding a check for primary and total before accessing clockRegistry.durations to avoid potential errors if clockRegistry is not yet initialized or if these values are undefined. This could prevent unexpected behavior or crashes.

if (!clockRegistry || !primary || !total) return; const newDurations = new Map(clockRegistry.durations);

gemini-code-assist · 2025-05-23T11:14:24Z

src/core/runtime/actions/CompleteTimerAction.ts

+    // First, stop the timer properly before completing
+    const stopEvent = new StopEvent(new Date());
+    const stopAction = new StopTimerAction(stopEvent);
+
+    // Apply the stop timer action to close the current timespan
+    stopAction.apply(runtime, block);


It's good that you're stopping the timer before popping the block. However, consider whether the StopEvent should be created with the current time or the time when the timer is actually completed. Using the completion time might provide more accurate data for analysis.

Suggested change

// First, stop the timer properly before completing

const stopEvent = new StopEvent(new Date());

const stopAction = new StopTimerAction(stopEvent);

// Apply the stop timer action to close the current timespan

stopAction.apply(runtime, block);

// First, stop the timer properly before completing

const stopEvent = new StopEvent(new Date()); // Consider using completion time here

const stopAction = new StopTimerAction(stopEvent);

// Apply the stop timer action to close the current timespan

stopAction.apply(runtime, block);

gemini-code-assist · 2025-05-23T11:14:24Z

src/core/runtime/blocks/EffortBlock.ts

+    const duration = this.selectMany(getDuration)[0];
+    const timerState = duration?.original 
+      ? TimerState.RUNNING_COUNTDOWN 
+      : TimerState.RUNNING_COUNTUP;


Consider extracting the timer state determination logic into a separate, well-named function to improve readability and maintainability. This would encapsulate the conditional logic and make the onEnter method easier to understand at a glance.

// Determine if this effort block has a specific duration const duration = this.selectMany(getDuration)[0]; const timerState = duration?.original ? TimerState.RUNNING_COUNTDOWN : TimerState.RUNNING_COUNTUP; // Extract this logic into a separate function, e.g., determineTimerState(duration)

gemini-code-assist · 2025-05-23T11:14:24Z

src/core/runtime/blocks/TimedGroupBlock.ts

+    // Add the SetTimerStateAction to set the timer to RUNNING_COUNTDOWN
+    const duration = this.selectMany(getDuration)[0];
+    if (duration?.original) {
+      actions.push(new SetTimerStateAction(TimerState.RUNNING_COUNTDOWN, "primary"));
+    }


Similar to the EffortBlock, consider extracting the timer state determination logic into a separate function to improve readability and maintainability. This would encapsulate the conditional logic and make the onBlockStart method easier to understand at a glance.

Suggested change

// Add the SetTimerStateAction to set the timer to RUNNING_COUNTDOWN

const duration = this.selectMany(getDuration)[0];

if (duration?.original) {

actions.push(new SetTimerStateAction(TimerState.RUNNING_COUNTDOWN, "primary"));

}

// Add the SetTimerStateAction to set the timer to RUNNING_COUNTDOWN

const duration = this.selectMany(getDuration)[0];

if (duration?.original) {

actions.push(new SetTimerStateAction(TimerState.RUNNING_COUNTDOWN, "primary"));

}

// Extract this logic into a separate function, e.g., determineTimerState(duration)

gemini-code-assist · 2025-05-23T11:14:24Z

src/core/runtime/outputs/SetClockAction.ts

+        // Get the current RuntimeSpan's timeSpans to ensure the timer shows as running
+        const latestRuntimeSpan = block.spans().length > 0 ? block.spans()[block.spans().length - 1] : undefined;
+        const timeSpans = latestRuntimeSpan?.timeSpans ?? [];


Consider adding a check to ensure latestRuntimeSpan is defined before accessing its timeSpans property. This would prevent potential errors if there are no spans available.

// Get the current RuntimeSpan's timeSpans to ensure the timer shows as running const latestRuntimeSpan = block.spans().length > 0 ? block.spans()[block.spans().length - 1] : undefined; const timeSpans = latestRuntimeSpan?.timeSpans ?? []; // Add check for latestRuntimeSpan before accessing timeSpans const timeSpans = latestRuntimeSpan ? latestRuntimeSpan.timeSpans ?? [] : [];

gemini-code-assist · 2025-05-23T11:14:24Z

src/hooks/useClockRegistry.ts

          event.bag?.duration) {

        // Update the registry with the new duration
-        newRegistry.set(event.bag.target, event.bag.duration);
+        newDurations.set(event.bag.target, event.bag.duration);
        hasChanges = true;


Consider adding a check to ensure that event.bag is defined before accessing its properties to prevent potential errors if the event is malformed or incomplete.

Suggested change

event.bag?.duration) {

// Update the registry with the new duration

newRegistry.set(event.bag.target, e 9E4A vent.bag.duration);

newDurations.set(event.bag.target, event.bag.duration);

hasChanges = true;

event.bag?.duration) {

// Update the registry with the new duration

newDurations.set(event.bag.target, event.bag.duration);

// Add check for event.bag before accessing its properties

if (event.bag && event.bag.target && event.bag.duration) {

newDurations.set(event.bag.target, event.bag.duration);

Copilot AI and others added 3 commits May 23, 2025 02:29

gemini-code-assist bot reviewed May 23, 2025

View reviewed changes

gemini-code-assist bot suggested changes May 23, 2025

View reviewed changes

SergeiGolos merged commit 0b34c32 into bitcobblers:main May 23, 2025
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Sync back to main #108

Sync back to main #108

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Sync back to main #108

Sync back to main #108

Uh oh!

Conversation

Uh oh!

Choose a reason for hiding this comment

Summary of Changes

Highlights

Changelog

Footnotes

Uh oh!

Choose a reason for hiding this comment

Code Review

Summary of Findings

Merge Readiness

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!