Skip to content

feat(mobile): Add Per Set Rest Periods Editing and Functionality - #2071

Merged
CodeWithCJ merged 8 commits into
CodeWithCJ:mainfrom
Gtt1229:main
Aug 15, 2026
Merged

feat(mobile): Add Per Set Rest Periods Editing and Functionality#2071
CodeWithCJ merged 8 commits into
CodeWithCJ:mainfrom
Gtt1229:main

Conversation

@Gtt1229

@Gtt1229 Gtt1229 commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Tip

Help us review and merge your PR faster!
Please ensure you have completed the Checklist below.
For Frontend changes, please run pnpm run validate to check for any errors.
PRs that include tests and clear screenshots are highly preferred!
Note: AI-generated descriptions must be manually edited for conciseness. Do not paste raw AI summaries.

Description

Added functionality to support per set rest duration and the ability to edit it.

What problem does this PR solve?
Per set rest functionality

How did you implement the solution?
Via Android Studio with AI Assistance for React elements

Linked Issue: Closes #
#1977

How to Test

  1. Check out this branch and build mobile
  2. Edit or Activate a workout
  3. Press the "Rest .." time to view edit
  4. Complete a few sets to verify rest times are honored.

PR Type

  • Issue (bug fix)
  • New Feature
  • Refactor
  • Documentation

Checklist

All PRs:

  • [MANDATORY - ALL] Integrity & License: I certify this is my own work, free of malicious code, and I agree to the License terms.

New features only:

  • [MANDATORY for new feature] Alignment: I have raised a GitHub issue and it was reviewed/approved by maintainers or it was approved on Discord.

Frontend changes (SparkyFitnessFrontend/):

  • [MANDATORY for Frontend changes] Quality: I have run pnpm run validate and it passes.
  • [MANDATORY for Frontend changes] Translations: I have only updated the English (en) translation file.

Backend changes (SparkyFitnessServer/):

  • [MANDATORY for Backend changes] Code Quality: I have run typecheck, lint, and tests. New files use TypeScript, new endpoints have Zod schemas, and new endpoints include tests.
  • [MANDATORY for Backend changes] Database Security: I have updated rls_policies.sql for any new user-specific tables.

UI changes (components, screens, pages):

  • [MANDATORY for UI changes] Screenshots: I have attached Before/After screenshots below.

Mobile changes (SparkyFitnessMobile/):

  • [MANDATORY for Mobile changes] Tested on device or emulator: I have verified the changes work on iOS or Android.

Screenshots

Click to expand

Before

image

After

image image

Notes for Reviewers

I would like a review of AI's implementation of the duration wheel aspects.

Optional — use this for anything that doesn't fit above: known tradeoffs, areas you'd like specific feedback on, questions you have or context that helps reviewers.

Summary by CodeRabbit

  • New Features

    • Added per-set and per-round rest-time editing through a new bottom sheet.
    • Added a minute-and-second duration picker for rest periods.
    • Rest displays now show ranges when sets have different durations.
    • Added support for editing rest times across superset rounds.
  • Bug Fixes

    • Rest periods now use each set’s configured duration instead of incorrectly reusing the first set’s value.
    • Improved handling of rest-time updates and workout session reconciliation.

@github-actions github-actions Bot added enhancement New feature or request mobile labels Aug 7, 2026
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

PR Validation Results

Change Detection

  • 📱 Mobile changes detected

⚠️ Recommendations (1)

  • Please link a related GitHub issue (Linked Issue: Closes #123).

✅ All required checks passed.

@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 40413bb3-bb14-4110-9ea2-2e9a5e1d6df7

📥 Commits

Reviewing files that changed from the base of the PR and between 1bf5771 and 2e4c5e6.

📒 Files selected for processing (5)
  • SparkyFitnessMobile/__tests__/screens/WorkoutPresetDetailScreen.test.tsx
  • SparkyFitnessMobile/src/components/DurationWheel.tsx
  • SparkyFitnessMobile/src/components/ExerciseSetRestSheet.tsx
  • SparkyFitnessMobile/src/components/ui/wheel-picker/wheel-picker.tsx
  • SparkyFitnessMobile/src/screens/ActiveWorkoutScreen.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
  • SparkyFitnessMobile/src/components/DurationWheel.tsx
  • SparkyFitnessMobile/src/screens/ActiveWorkoutScreen.tsx
  • SparkyFitnessMobile/src/components/ui/wheel-picker/wheel-picker.tsx
  • SparkyFitnessMobile/src/components/ExerciseSetRestSheet.tsx

📝 Walkthrough

Walkthrough

The mobile workout flow now edits rest durations per set or superset round. It displays rest ranges, stores set-level rest metadata, uses set-specific durations during progression, and adds regression coverage.

Changes

Per-set rest duration flow

Layer / File(s) Summary
Rest contracts and controls
SparkyFitnessMobile/src/types/drafts.ts, SparkyFitnessMobile/src/components/ui/wheel-picker/*, SparkyFitnessMobile/src/components/DurationWheel.tsx, SparkyFitnessMobile/src/components/RestPeriodChip.tsx, SparkyFitnessMobile/__tests__/screens/WorkoutPresetDetailScreen.test.tsx
Adds nullable set rest metadata, wheel-picker controls, bounded duration selection, and single-value or range rest labels.
Per-set rest editor
SparkyFitnessMobile/src/components/ExerciseSetRestSheet.tsx
Adds a ref-controlled bottom sheet that edits all sets or one set, confirms mixed-value overwrites, and emits changed rest values.
Workout rest workflow integration
SparkyFitnessMobile/src/components/WorkoutFormExerciseList.tsx, SparkyFitnessMobile/src/screens/ActiveWorkoutScreen.tsx, SparkyFitnessMobile/src/components/ActiveWorkoutExerciseCard.tsx
Replaces the exercise-level sheet, applies solo and superset updates, and passes all set rest values to the display chip.
Rest progression and validation
SparkyFitnessMobile/src/stores/activeWorkoutStore.ts, SparkyFitnessMobile/__tests__/stores/activeWorkoutStore.test.ts, SparkyFitnessMobile/__tests__/components/WorkoutFormExerciseList.test.tsx
Uses set-specific or round-specific rest during step generation, completion, fallback, and reconciliation. Tests cover these behaviors and the new sheet contract.

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: 🔵 Low · up to 2e4c5

The PR adds per-set rest editing and timing behavior. It is mergeable with owner awareness to rerun the focused mobile test and required validation because a CI failure was reported.

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant ActiveWorkoutScreen
  participant ExerciseSetRestSheet
  participant DurationWheel
  participant activeWorkoutStore
  User->>ActiveWorkoutScreen: Open rest editor
  ActiveWorkoutScreen->>ExerciseSetRestSheet: present exercise and set rest values
  ExerciseSetRestSheet->>DurationWheel: Display selected duration
  User->>DurationWheel: Select duration
  DurationWheel->>ExerciseSetRestSheet: onChangeSec(seconds)
  ExerciseSetRestSheet->>ActiveWorkoutScreen: onApply(changed set values)
  ActiveWorkoutScreen->>activeWorkoutStore: Update set rest metadata
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the primary mobile feature: editing and supporting per-set rest periods.
Description check ✅ Passed The description covers the problem, implementation, testing steps, issue, feature type, required checklists, screenshots, and mobile validation.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (2)
SparkyFitnessMobile/src/components/RestPeriodChip.tsx (1)

26-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Simplify the range computation.

Line 28 tests normalized.length after the map. The map preserves length, so test values.length first and return early. You can also replace the manual loop with Math.min/Math.max over the normalized array.

♻️ Optional refactor
 ): string {
-  const normalized = values.map((v) => (v ?? defaultRestSec));
-  if (normalized.length === 0) return formatRestLabel(defaultRestSec);
-  let min = normalized[0];
-  let max = normalized[0];
-  for (const value of normalized) {
-    if (value < min) min = value;
-    if (value > max) max = value;
-  }
+  if (values.length === 0) return formatRestLabel(defaultRestSec);
+  const normalized = values.map((v) => v ?? defaultRestSec);
+  const min = Math.min(...normalized);
+  const max = Math.max(...normalized);
🤖 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 `@SparkyFitnessMobile/src/components/RestPeriodChip.tsx` around lines 26 - 34,
In the range computation function containing normalized, check values.length
before mapping and return formatRestLabel(defaultRestSec) for an empty input.
Then replace the manual min/max loop with Math.min and Math.max applied to the
normalized array, preserving the existing range-label behavior.
SparkyFitnessMobile/__tests__/stores/activeWorkoutStore.test.ts (1)

1257-1279: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extend the superset coverage and drop the unused setup.

The beforeEach on lines 1259-1261 starts a 2-round session. The test on line 1268 immediately replaces it with a 3-round session, so the setup has no effect. Remove it or make the test use it.

The test also sets the same rest_time on both members for round 1. That hides the divergence between buildStepsFromSession, which reads the anchor member's set, and restSecBeforeNextSet, which reads the completed set. Add a case where the two members carry different rest_time values in the same round, and a case where one member has fewer sets than the other. Both relate to the issue raised on SparkyFitnessMobile/src/stores/activeWorkoutStore.ts lines 462-467.

🤖 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 `@SparkyFitnessMobile/__tests__/stores/activeWorkoutStore.test.ts` around lines
1257 - 1279, Remove the unused beforeEach setup in the completeSet rest
supersets tests, then extend coverage with separate cases for differing
rest_time values between superset members in the same round and for one member
having fewer sets than the other. Anchor the assertions to completeSet and the
resulting rest.durationSec, covering both buildStepsFromSession and
restSecBeforeNextSet behavior.
🤖 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 `@SparkyFitnessMobile/src/components/DurationWheel.tsx`:
- Around line 3-9: Remove the unused eslint-disable directive above the
WheelPicker import while preserving the explanatory comment and the import
itself.
- Around line 60-62: Update the seconds selection state in DurationWheel so
handleSecondChange preserves the raw wheel index across the 359-to-360 wrap
instead of recomputing it from currentSec. Reconcile that local index only when
the external valueSec prop changes, and ensure secondsWheelValue uses the
preserved index so WheelPicker does not jump.

In `@SparkyFitnessMobile/src/components/ExerciseSetRestSheet.tsx`:
- Around line 113-122: Update handleDone in ExerciseSetRestSheet so selecting
ALL_KEY commits selectedSeconds to every set, even when no wheel movement
changes draftBySetId; build updates for all sets in that mode, while preserving
the existing changed-only behavior for individual selections.
- Around line 81-96: Update selectedSeconds in ExerciseSetRestSheet so the
ALL_KEY branch uses the existing highestSetRest value instead of the first set’s
draft, ensuring the “All” chip and wheel display the same maximum rest duration;
move the highestSetRest memo above selectedSeconds if needed to satisfy
declaration ordering.

In `@SparkyFitnessMobile/src/components/RestPeriodChip.tsx`:
- Around line 22-44: Update the array type annotations in formatRestRangeLabel
and RestPeriodChipProps to use T[] syntax instead of Array<T>, preserving the
existing element types and optionality.

In `@SparkyFitnessMobile/src/stores/activeWorkoutStore.ts`:
- Around line 462-467: Update the round-rest calculation in the superset
grouping logic to select the first member with a set at the current round,
rather than always using members[0], while retaining the default only when no
member has a set. Apply the identical first-member-with-a-set rule in
restSecBeforeNextSet for the step that closes a round, so steps[].restSec and
the live countdown use the same duration.

---

Nitpick comments:
In `@SparkyFitnessMobile/__tests__/stores/activeWorkoutStore.test.ts`:
- Around line 1257-1279: Remove the unused beforeEach setup in the completeSet
rest supersets tests, then extend coverage with separate cases for differing
rest_time values between superset members in the same round and for one member
having fewer sets than the other. Anchor the assertions to completeSet and the
resulting rest.durationSec, covering both buildStepsFromSession and
restSecBeforeNextSet behavior.

In `@SparkyFitnessMobile/src/components/RestPeriodChip.tsx`:
- Around line 26-34: In the range computation function containing normalized,
check values.length before mapping and return formatRestLabel(defaultRestSec)
for an empty input. Then replace the manual min/max loop with Math.min and
Math.max applied to the normalized array, preserving the existing range-label
behavior.
🪄 Autofix

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: 07a19e95-b274-4f65-aef8-84e026e5861e

📥 Commits

Reviewing files that changed from the base of the PR and between 2241581 and 2e463ea.

📒 Files selected for processing (10)
  • SparkyFitnessMobile/__tests__/stores/activeWorkoutStore.test.ts
  • SparkyFitnessMobile/src/components/ActiveWorkoutExerciseCard.tsx
  • SparkyFitnessMobile/src/components/DurationWheel.tsx
  • SparkyFitnessMobile/src/components/ExerciseSetRestSheet.tsx
  • SparkyFitnessMobile/src/components/RestPeriodChip.tsx
  • SparkyFitnessMobile/src/components/WorkoutFormExerciseList.tsx
  • SparkyFitnessMobile/src/screens/ActiveWorkoutScreen.tsx
  • SparkyFitnessMobile/src/stores/activeWorkoutStore.ts
  • SparkyFitnessMobile/src/types/assets.d.ts
  • SparkyFitnessMobile/src/types/drafts.ts

Comment thread SparkyFitnessMobile/src/components/DurationWheel.tsx Outdated
Comment thread SparkyFitnessMobile/src/components/DurationWheel.tsx Outdated
Comment thread SparkyFitnessMobile/src/components/ExerciseSetRestSheet.tsx
Comment on lines +113 to +122
const handleDone = useCallback(() => {
const updates: ExerciseSetRestUpdate[] = [];
for (const set of sets) {
const next = draftBySetId[set.setId];
const initial = initialBySetId[set.setId];
if (next !== initial) updates.push({ setId: set.setId, seconds: next });
}
if (updates.length > 0) onApply(updates);
sheetRef.current?.dismiss();
}, [draftBySetId, initialBySetId, onApply, sets]);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

"All" emits no update when the user does not move the wheel.

handleDone emits only sets whose draft differs from the initial value. If the user selects "All" and presses Done without moving the wheel, the drafts stay unequal and no harmonization occurs. The user selected "All" to make every set match. Consider committing every set to selectedSeconds when selectedKey === ALL_KEY, or apply the value to all sets at the moment "All" is selected.

🤖 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 `@SparkyFitnessMobile/src/components/ExerciseSetRestSheet.tsx` around lines 113
- 122, Update handleDone in ExerciseSetRestSheet so selecting ALL_KEY commits
selectedSeconds to every set, even when no wheel movement changes draftBySetId;
build updates for all sets in that mode, while preserving the existing
changed-only behavior for individual selections.

Comment thread SparkyFitnessMobile/src/components/RestPeriodChip.tsx
Comment thread SparkyFitnessMobile/src/stores/activeWorkoutStore.ts

@apedley apedley left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for handling this. The store stuff is well done and I appreciate the new regression tests. Some problems:

  1. I think I may have given you bad advice on extracting the wheel from react-native-ui-datepicker. CI is failing because jest enforces strict exports. Rather than working around that and the other inevitable problems, lets just copy paste it into our own components/ui directory with the MIT license and attribution in the file as a comment. This lets us get rid of the shim at assets.d.ts as well.
  2. If we're vendoring the component, there's also a bug in the wheel that we should fix that seems to only show up on iOS. The seconds column recenters to the middle repetition after every change but it also has a useEffect that calls scrollToIndex. The result is scrolling across 59-00 gives an index > 60 which then animates the wheel rotating back a full rotation. It took me a while to figure out what was happening on this bug - its subtle unless you slow down animations on the simulator.
  3. The failing test in WorkoutPresetDetailScreen.test.tsx is just stale and needs updating
  4. WorkoutFormExerciseList.test.tsx "targets the rest sheet at the pressed exercise" still mocks RestPeriodSheet which the component doesn't import anymore. Needs rewriting against ExerciseSetRestSheet present(name, set)/onApply
  5. Supersets..the store's setExerciseRest was the thing harmonizing rest across superset members and nothing calls it now. buildStepsFromSession takes each round's rest from the anchor member's set, so editing rest on a non-anchor member's chip does nothing when sets are completed in order. Rest in a superset belongs to the round not to the member sets so let's display it like that. That would include relabeling the chips.

Small things:

  • the "All" chip in the sheet shows the max rest when formatRestRangeLabel is right there if you want it to show the range
  • the sheet opens with Set 1 selected when it should be All (the most common change)

Stuff we talked about before:

  • Add a dialog confirmation if changing all sets is going to overwrite a mixed set of times
  • Make the Set Name/Number/All text-secondary (second most important thing) and the time text-primary (most important thing). Border should be border-subtle.
  • Drop the "Selected: 1:30" text

Let me know if you have any questions or if you want me to take care of any part of it that's fine too.

Comment thread SparkyFitnessMobile/src/components/RestPeriodChip.tsx

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (5)
SparkyFitnessMobile/src/components/ui/wheel-picker/wheel-picker.tsx (3)

151-159: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Throwing from the effect crashes the sheet on an unmatched value.

selectedIndex is -1 when value is absent from options. The effect then throws, and React 19 reports it to the error boundary instead of degrading gracefully. The picker also seeds scrollY and initialScrollIndex with -1 before the effect runs.

DurationWheel clamps valueSec to maxSec and maps seconds through SEC_MID_OFFSET + currentSec, so every value resolves to an existing option today with maxSec = MAX_REST_SEC. The crash becomes reachable if maxSec or the option builders change.

Consider clamping the index instead of throwing:

♻️ Proposed refactor
-  const selectedIndex = options.findIndex((item) => item.value === value);
+  const foundIndex = options.findIndex((item) => item.value === value);
+  const selectedIndex = foundIndex >= 0 ? foundIndex : 0;
   useEffect(() => {
-    if (selectedIndex < 0 || selectedIndex >= options.length) {
-      throw new Error(
-        `Selected index ${selectedIndex} is out of bounds [0, ${
-          options.length - 1
-        }]`
-      );
+    if (foundIndex < 0 && __DEV__) {
+      console.warn(`WheelPicker: value ${value} is not in options; showing index 0.`);
     }
-  }, [selectedIndex, options]);
+  }, [foundIndex, value]);
🤖 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 `@SparkyFitnessMobile/src/components/ui/wheel-picker/wheel-picker.tsx` around
lines 151 - 159, Replace the throwing bounds validation in the wheel picker’s
selectedIndex useEffect with graceful clamping to the valid options range, so
unmatched values never propagate -1 into scrollY or initialScrollIndex. Preserve
valid indices unchanged and handle the options list safely when deriving the
fallback index.

110-115: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

|| 0 discards valid falsy option values and hides out-of-range indices.

options[index]?.value || 0 returns 0 in two different situations: the option value is genuinely falsy, and index is outside the array. For a picker with string option values, an empty string becomes the number 0, which never matches any option and leaves the wheel desynchronized from value.

DurationWheel uses numeric values only, so the current consumer is unaffected. Guard the lookup so a missing option emits nothing.

♻️ Proposed refactor
-    const nextValue = options[index]?.value || 0;
-    if (index !== selectedIndex && nextValue !== lastEmittedValueRef.current) {
+    const nextOption = options[index];
+    if (nextOption == null) return;
+    const nextValue = nextOption.value;
+    if (index !== selectedIndex && nextValue !== lastEmittedValueRef.current) {
🤖 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 `@SparkyFitnessMobile/src/components/ui/wheel-picker/wheel-picker.tsx` around
lines 110 - 115, Update the nextValue lookup in the wheel-picker scroll handling
to preserve valid falsy option values and distinguish missing options. Guard the
options[index] lookup, emit and update lastEmittedValueRef only when an option
exists, and emit its value unchanged; do not substitute 0 for an out-of-range
index.

141-147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Replace the as any cast with a typed partial event.

The repository guideline forbids any in new or edited code. Type the helper parameter to the fields it reads instead of casting the synthetic object.

♻️ Proposed refactor
-  const handleScrollEnd = (event: NativeSyntheticEvent<NativeScrollEvent>) => {
+  type ScrollOffsetEvent = {
+    nativeEvent: { contentOffset: { y: number } };
+  };
+
+  const handleScrollEnd = (event: ScrollOffsetEvent) => {
     const offsetY = Math.min(
-        handleScrollEnd(syntheticEvent as any);
+        handleScrollEnd(syntheticEvent);

NativeSyntheticEvent<NativeScrollEvent> structurally satisfies ScrollOffsetEvent, so the momentum and drag handlers keep working without a cast.

This follows the guideline "Never use any or // eslint-disable-next-line @typescript-eslint/no-explicit-any`` when creating functions or editing code; define explicit TypeScript types or import schemas from @workspace/shared."

🤖 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 `@SparkyFitnessMobile/src/components/ui/wheel-picker/wheel-picker.tsx` around
lines 141 - 147, Replace the as any cast in the synthetic event passed to
handleScrollEnd with an explicit partial event type containing the
nativeEvent.contentOffset.y fields that the helper reads. Update
handleScrollEnd’s parameter type as needed to accept this minimal shape while
remaining compatible with NativeSyntheticEvent<NativeScrollEvent> used by the
momentum and drag handlers.

Source: Coding guidelines

SparkyFitnessMobile/__tests__/components/WorkoutFormExerciseList.test.tsx (1)

746-762: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a case where a superset receives different values for two rounds.

The test applies a single update, so it cannot detect the value-collapsing behavior. Add a superset exercise with two sets and call onApply with two different seconds values. That test pins down the intended contract for the issue raised on SparkyFitnessMobile/src/screens/ActiveWorkoutScreen.tsx lines 520-537.

mockRestSheet.onApply?.([
  { setId: 'b-s1', seconds: 90 },
  { setId: 'b-s2', seconds: 150 },
]);
// Assert the intended contract: either both rounds keep their own value,
// or the sheet never emits differing values for a superset member.

This follows the guideline "Run focused tests for the touched surface, then lint and typecheck" for SparkyFitnessMobile/__tests__/**.

🤖 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 `@SparkyFitnessMobile/__tests__/components/WorkoutFormExerciseList.test.tsx`
around lines 746 - 762, Add a two-set superset exercise to the test around the
superset rest-sheet case, invoke mockRestSheet.onApply with distinct seconds for
each set, and assert the intended contract: each round retains its own value or
differing values are rejected before state updates. Keep the existing
setExerciseRest and isSupersetRound assertions intact, then run the focused
test, lint, and typecheck.

Source: Coding guidelines

SparkyFitnessMobile/src/components/ui/wheel-picker/wheel-picker-item.tsx (1)

143-153: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

The memo comparator ignores option, index, and height.

customComparator returns true whenever textClassName and textStyle are unchanged. React then skips the re-render even when option.text changes. This is safe only while every cell keeps a stable identity. wheel-picker.tsx builds keyExtractor from ${item.value}-${item.text}-${index}, so an option list change produces new keys and new mounts, which masks the risk today.

Add prevProps.option?.text === nextProps.option?.text && prevProps.index === nextProps.index && prevProps.height === nextProps.height if you want the component to stay correct under future option-list reuse. The current behavior matches upstream, so this is optional.

🤖 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 `@SparkyFitnessMobile/src/components/ui/wheel-picker/wheel-picker-item.tsx`
around lines 143 - 153, The customComparator for WheelPickerItem must also
compare option.text, index, and height before treating props as equal. Extend
the comparator alongside the existing textClassName and textStyle checks so
reused cells re-render when their displayed option or layout position changes.
🤖 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 `@SparkyFitnessMobile/src/components/ExerciseSetRestSheet.tsx`:
- Around line 64-70: Update the mixed-rest calculation used by
handleChangeSeconds to compare the current draft map via restTimesMixed instead
of the initial snapshot initialTimesMixed. Replace the references in the handler
and its dependency array, while preserving the existing allOverwriteConfirmed
session behavior.

In `@SparkyFitnessMobile/src/components/ui/wheel-picker/wheel-picker.tsx`:
- Around line 129-149: Update handleScrollEndDrag to store its 50 ms timeout in
a ref, clear any existing timer before scheduling a new one, and clear the timer
during component unmount cleanup so stale callbacks cannot invoke
handleScrollEnd or onChange after remounting.

In `@SparkyFitnessMobile/src/screens/ActiveWorkoutScreen.tsx`:
- Around line 520-537: Update the superset branch in the exercise-rest save flow
around getSupersetRuns and store.setExerciseRest so per-round values are not
discarded: either restrict ExerciseSetRestSheet to emitting only the “All” value
for superset members, or apply each update through store.updateSetField and
separately harmonize the value across run members. Remove the redundant
updates.length check because empty updates already return earlier, and ensure
the selected-round-only case does not overwrite other rounds.

---

Nitpick comments:
In `@SparkyFitnessMobile/__tests__/components/WorkoutFormExerciseList.test.tsx`:
- Around line 746-762: Add a two-set superset exercise to the test around the
superset rest-sheet case, invoke mockRestSheet.onApply with distinct seconds for
each set, and assert the intended contract: each round retains its own value or
differing values are rejected before state updates. Keep the existing
setExerciseRest and isSupersetRound assertions intact, then run the focused
test, lint, and typecheck.

In `@SparkyFitnessMobile/src/components/ui/wheel-picker/wheel-picker-item.tsx`:
- Around line 143-153: The customComparator for WheelPickerItem must also
compare option.text, index, and height before treating props as equal. Extend
the comparator alongside the existing textClassName and textStyle checks so
reused cells re-render when their displayed option or layout position changes.

In `@SparkyFitnessMobile/src/components/ui/wheel-picker/wheel-picker.tsx`:
- Around line 151-159: Replace the throwing bounds validation in the wheel
picker’s selectedIndex useEffect with graceful clamping to the valid options
range, so unmatched values never propagate -1 into scrollY or
initialScrollIndex. Preserve valid indices unchanged and handle the options list
safely when deriving the fallback index.
- Around line 110-115: Update the nextValue lookup in the wheel-picker scroll
handling to preserve valid falsy option values and distinguish missing options.
Guard the options[index] lookup, emit and update lastEmittedValueRef only when
an option exists, and emit its value unchanged; do not substitute 0 for an
out-of-range index.
- Around line 141-147: Replace the as any cast in the synthetic event passed to
handleScrollEnd with an explicit partial event type containing the
nativeEvent.contentOffset.y fields that the helper reads. Update
handleScrollEnd’s parameter type as needed to accept this minimal shape while
remaining compatible with NativeSyntheticEvent<NativeScrollEvent> used by the
momentum and drag handlers.
🪄 Autofix

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: 56728db6-c096-463e-bba3-d224be496ba7

📥 Commits

Reviewing files that changed from the base of the PR and between dd629d1 and 1bf5771.

📒 Files selected for processing (10)
  • SparkyFitnessMobile/__tests__/components/WorkoutFormExerciseList.test.tsx
  • SparkyFitnessMobile/src/components/DurationWheel.tsx
  • SparkyFitnessMobile/src/components/ExerciseSetRestSheet.tsx
  • SparkyFitnessMobile/src/components/WorkoutFormExerciseList.tsx
  • SparkyFitnessMobile/src/components/ui/wheel-picker/index.ts
  • SparkyFitnessMobile/src/components/ui/wheel-picker/types.ts
  • SparkyFitnessMobile/src/components/ui/wheel-picker/wheel-picker-item.tsx
  • SparkyFitnessMobile/src/components/ui/wheel-picker/wheel-picker.style.ts
  • SparkyFitnessMobile/src/components/ui/wheel-picker/wheel-picker.tsx
  • SparkyFitnessMobile/src/screens/ActiveWorkoutScreen.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
  • SparkyFitnessMobile/src/components/DurationWheel.tsx
  • SparkyFitnessMobile/src/components/WorkoutFormExerciseList.tsx

Comment thread SparkyFitnessMobile/src/components/ExerciseSetRestSheet.tsx Outdated
Comment thread SparkyFitnessMobile/src/components/ui/wheel-picker/wheel-picker.tsx
Comment thread SparkyFitnessMobile/src/screens/ActiveWorkoutScreen.tsx
@CodeWithCJ

Copy link
Copy Markdown
Owner

@Gtt1229 resolve all the conversations for us to merge. and also CI Test is in failed state.

@Gtt1229

Gtt1229 commented Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

Will need help with the CI test fail. The fail is due to a fix for the janky animation for the duration wheel's seconds wheel.

@CodeWithCJ
CodeWithCJ merged commit fa3fce6 into CodeWithCJ:main Aug 15, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request mobile

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants