feat: workout insights, draft persistence, and plan editing improvements#155
Merged
Conversation
…ndling and compact formatting
…cross plans, home, workouts
…ding persistence and user prompts
- Add "Add Warm-up" button that inserts at the warmup/working boundary, copying values from the last warmup set - Scope "apply to all" in edit modal to only affect sets of the same type (warmup or working), with a dynamic label reflecting the scope - Add blue left border and group divider to visually distinguish warmup sets from working sets - Move Details button to the navigation header - Add insertSetAtExercise store action for index-based set insertion
![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 found 5 issues, and left some high level feedback:
- The draft persistence/continuation logic in
CreatePlanScreenandCreateWorkoutScreenis very similar (hydrate check, Alert, draftDecision, startFreshSession) and could be extracted into a shared hook or utility to reduce duplication and keep the flows in sync more easily. - In
InsightsStrip, pills are keyed by their array index in both the flat and grid layouts; consider using a more stable key such aspill.label(or an explicitid) to avoid unnecessary re-renders or potential issues if the pills array is reordered. - The additional
useCompletedWorkoutsQueryin the stats screen to derive streaks introduces a second fetch alongside the existinguseCompletedWorkoutsQueryfor the selected range; it may be worth checking whether a single query (with client-side filtering) could serve both to avoid redundant network calls.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The draft persistence/continuation logic in `CreatePlanScreen` and `CreateWorkoutScreen` is very similar (hydrate check, Alert, draftDecision, startFreshSession) and could be extracted into a shared hook or utility to reduce duplication and keep the flows in sync more easily.
- In `InsightsStrip`, pills are keyed by their array index in both the flat and grid layouts; consider using a more stable key such as `pill.label` (or an explicit `id`) to avoid unnecessary re-renders or potential issues if the pills array is reordered.
- The additional `useCompletedWorkoutsQuery` in the stats screen to derive streaks introduces a second fetch alongside the existing `useCompletedWorkoutsQuery` for the selected range; it may be worth checking whether a single query (with client-side filtering) could serve both to avoid redundant network calls.
## Individual Comments
### Comment 1
<location path="components/stats/InsightsStrip.tsx" line_range="85-92" />
<code_context>
if (pills.length === 0) return null;
+ const renderPill = (pill: InsightPill, i: number) => (
+ <Pressable
+ key={i}
+ ref={pill.tooltip ? gainPillRef : undefined}
+ style={styles.pill}
+ onPress={pill.tooltip ? () => showTooltip(pill.tooltip!) : undefined}
+ >
+ <View style={styles.pillLabelRow}>
+ <ThemedText style={styles.pillLabel}>{pill.label}</ThemedText>
+ {pill.tooltip ? (
</code_context>
<issue_to_address>
**issue (bug_risk):** Tooltip anchor ref is shared across all pills, so tooltips may not appear next to the pressed pill.
Because `gainPillRef` is reused, it always points to the last pill with a tooltip, so earlier pills will show a tooltip anchored to the wrong pill. Use a distinct ref per pill (e.g. an array of refs) or capture the pressed pill’s ref in state on `onPress` so the tooltip anchors to the correct pill.
</issue_to_address>
### Comment 2
<location path="app/(app)/(tabs)/(stats)/index.tsx" line_range="137-146" />
<code_context>
parseInt(selectedTimeRange),
);
+ const { data: allWorkouts } = useCompletedWorkoutsQuery(
+ weightUnit,
+ distanceUnit,
+ );
+
+ const thisWeekStart = startOfWeek(new Date(), { weekStartsOn: 1 });
+ const thisWeekEnd = endOfWeek(new Date(), { weekStartsOn: 1 });
+ const thisWeekWorkouts = allWorkouts?.filter((w) => {
+ const d = new Date(w.date_completed);
+ return d >= thisWeekStart && d <= thisWeekEnd;
+ });
+ const uniqueWorkoutDaysCount = new Set(
+ thisWeekWorkouts?.map((w) => new Date(w.date_completed).toDateString()),
+ ).size;
+ const weeklyGoal = Number(settings?.weeklyGoal ?? 0);
+ const weeklyGoalReached =
+ uniqueWorkoutDaysCount >= weeklyGoal && weeklyGoal > 0;
+ const { streak } = useWeeklyStreak(
+ allWorkouts,
+ weeklyGoal,
+ uniqueWorkoutDaysCount,
+ weeklyGoalReached,
+ );
+
</code_context>
<issue_to_address>
**suggestion (performance):** Completed workouts are fetched twice, which is redundant and may add unnecessary load.
This screen now calls `useCompletedWorkoutsQuery` twice with identical `weightUnit`/`distanceUnit` args: once for `completedWorkouts` (insights) and once for `allWorkouts` (streak). Consider using a single query result for both, or deriving one from the other, to avoid duplicate network/cache work and reduce complexity.
Suggested implementation:
```typescript
import { startOfWeek, endOfWeek } from "date-fns";
import { useWeeklyStreak } from "@/hooks/useWeeklyStreak";
import { useExercisesQuery } from "@/hooks/useExercisesQuery";
import { Colors } from "@/constants/Colors";
import { WorkoutHistorySection } from "@/components/stats/WorkoutHistorySection";
parseInt(selectedTimeRange),
);
const thisWeekStart = startOfWeek(new Date(), { weekStartsOn: 1 });
const thisWeekEnd = endOfWeek(new Date(), { weekStartsOn: 1 });
const thisWeekWorkouts = completedWorkouts?.filter((w) => {
```
```typescript
const thisWeekWorkouts = completedWorkouts?.filter((w) => {
const d = new Date(w.date_completed);
return d >= thisWeekStart && d <= thisWeekEnd;
});
const uniqueWorkoutDaysCount = new Set(
thisWeekWorkouts?.map((w) => new Date(w.date_completed).toDateString()),
).size;
const weeklyGoal = Number(settings?.weeklyGoal ?? 0);
const weeklyGoalReached =
uniqueWorkoutDaysCount >= weeklyGoal && weeklyGoal > 0;
const { streak } = useWeeklyStreak(
completedWorkouts,
```
These edits assume there is already a `const { data: completedWorkouts } = useCompletedWorkoutsQuery(weightUnit, distanceUnit, ...)` (or similar) earlier in this file. If the variable name is different, update `completedWorkouts` in the replacements above to match the existing data variable from `useCompletedWorkoutsQuery`. Also ensure any type expectations in `useWeeklyStreak` still align with the `completedWorkouts` data shape.
</issue_to_address>
### Comment 3
<location path="store/workoutStore.ts" line_range="259-268" />
<code_context>
+ addSetToExercise: (
</code_context>
<issue_to_address>
**suggestion:** Superset set-mirroring logic is duplicated across `addSetToExercise`, `insertSetAtExercise`, and `removeSetFromExercise`.
This duplication makes superset behavior harder to maintain and easy to desynchronize if only one call site is updated. Consider extracting a shared helper (e.g. `mapExercisesWithSupersetMirror(...)`) so superset handling is centralized and consistently applied.
Suggested implementation:
```typescript
const mapExercisesWithSupersetMirror = (
exercises: Exercise[],
targetExerciseId: number,
updateSets: (sets: Set[]) => Set[],
): Exercise[] => {
// Find the primary exercise; if it has no superset, only update that exercise.
const target = exercises.find((exercise) => exercise.id === targetExerciseId);
if (!target || target.supersetId == null) {
return exercises.map((exercise) =>
exercise.id === targetExerciseId
? { ...exercise, sets: updateSets(exercise.sets) }
: exercise,
);
}
const supersetId = target.supersetId;
// Mirror the set updates to all exercises in the same superset group.
return exercises.map((exercise) =>
exercise.supersetId === supersetId
? { ...exercise, sets: updateSets(exercise.sets) }
: exercise,
);
};
export const useWorkoutStore = create<WorkoutState>()(
```
```typescript
addSetToExercise: (
workoutIndex: number,
exerciseId: number,
newSet: Set,
) => {
set((state) => {
const workout = state.workouts[workoutIndex];
if (!workout) return state;
const updatedExercises = mapExercisesWithSupersetMirror(
workout.exercises,
exerciseId,
(sets) => [...sets, newSet],
);
const updatedWorkouts = [...state.workouts];
updatedWorkouts[workoutIndex] = {
...workout,
exercises: updatedExercises,
};
return { workouts: updatedWorkouts };
});
},
```
To fully centralize superset set-mirroring logic as suggested in your review comment, you should:
1. Update `insertSetAtExercise` to also use `mapExercisesWithSupersetMirror`, e.g.:
- Replace any manual superset/set-mirroring logic with:
```ts
const updatedExercises = mapExercisesWithSupersetMirror(
workout.exercises,
exerciseId,
(sets) => {
const next = [...sets];
next.splice(insertIndex, 0, newSet);
return next;
},
);
```
- Then reassign `workout.exercises` (or the cloned workout) with `updatedExercises` similarly to `addSetToExercise`.
2. Update `removeSetFromExercise` to use the helper:
- Replace duplicated superset handling with:
```ts
const updatedExercises = mapExercisesWithSupersetMirror(
workout.exercises,
exerciseId,
(sets) => sets.filter((_, index) => index !== setIndexToRemove),
);
```
- Again, write the updated workout back into `state.workouts`.
3. Align field names if your `Exercise` type differs:
- If your schema does not use `id`, `supersetId`, or `sets`, update `mapExercisesWithSupersetMirror` accordingly:
- `exercise.id` → the exercise identifier you are using.
- `exercise.supersetId` → the field that groups exercises into a superset.
- `exercise.sets` → the property that stores the array of `Set`s.
4. If `supersetId` semantics are different (e.g. leader vs group id):
- Adjust the helper to use your actual superset grouping logic (for example, match on `exercise.supersetRootId` or a `supersetGroupId` instead of `supersetId`), ensuring that all members of the superset group are updated together.
Once `insertSetAtExercise` and `removeSetFromExercise` also call `mapExercisesWithSupersetMirror`, all superset behavior will be centralized in one place and easier to maintain.
</issue_to_address>
### Comment 4
<location path="app/(app)/(create-plan)/create.tsx" line_range="71" />
<code_context>
const createMutation = useCreateStandaloneWorkout();
const updateMutation = useUpdateStandaloneWorkout();
+ // 'pending' until draft check resolves; then 'continue' or 'fresh'
+ const [draftDecision, setDraftDecision] = useState<
+ "pending" | "continue" | "fresh"
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the new draft-session state machine and hydration logic into a dedicated hook (e.g., usePlanDraftSession) so the screen only consumes a simple draft API instead of managing store details directly.
You can keep the new draft feature but significantly reduce screen complexity by extracting the “draft session” state machine + hydration wiring into a dedicated hook and hiding the store shape from the screen.
### 1. Extract draft / hydration logic into a hook
Right now, the screen is doing:
- hydration subscription
- draft lookup (`draftContext`, `draftId`, `draftName`, `workouts`)
- alert branching
- deciding `"pending" | "fresh" | "continue"`
- syncing `planName` into `draftName`
- managing when `dataLoaded` flips
All in `useEffect`s.
You can move this into something like `usePlanDraftSession(planId)` that exposes a small API:
```ts
// hooks/usePlanDraftSession.ts
export function usePlanDraftSession(planId?: string | string[]) {
const {
setDraftContext,
setDraftId,
setDraftName,
clearDraft,
} = useWorkoutStore();
const [status, setStatus] = useState<"pending" | "fresh" | "continue">(
"pending",
);
const [draftName, setLocalDraftName] = useState("");
const startFreshSession = useCallback(() => {
const currentPlanId = planId ? Number(planId) : null;
setDraftContext("plan");
setDraftId(currentPlanId);
setStatus("fresh");
}, [planId, setDraftContext, setDraftId]);
// run once after hydration
useEffect(() => {
const checkDraft = () => {
const {
workouts: storedWorkouts,
draftContext,
draftId,
draftName: storedDraftName,
} = useWorkoutStore.getState();
const currentPlanId = planId ? Number(planId) : null;
const hasDraft =
draftContext === "plan" &&
draftId === currentPlanId &&
(storedWorkouts.length > 0 || storedDraftName.trim() !== "");
if (hasDraft) {
Alert.alert(
"Continue Editing?",
"You have unsaved changes from your last session. Would you like to continue?",
[
{
text: "Discard Changes",
style: "destructive",
onPress: () => {
clearDraft();
startFreshSession();
},
},
{
text: "Continue",
onPress: () => {
setLocalDraftName(storedDraftName);
setStatus("continue");
},
},
],
);
} else {
if (draftContext !== null) clearDraft();
startFreshSession();
}
};
if (useWorkoutStore.persist.hasHydrated()) {
checkDraft();
} else {
return useWorkoutStore.persist.onFinishHydration(checkDraft);
}
}, [planId, clearDraft, startFreshSession]);
// public helpers for the screen
const syncDraftName = useCallback(
(name: string) => {
if (status === "pending") return;
setDraftName(name);
},
[status, setDraftName],
);
return { status, draftName, syncDraftName, clearDraft };
}
```
Then `CreatePlanScreen` becomes simpler and only works with a higher-level contract:
```ts
export default function CreatePlanScreen() {
const { planId } = useLocalSearchParams();
const [dataLoaded, setDataLoaded] = useState(!planId);
const { status: draftDecision, syncDraftName, clearDraft } =
usePlanDraftSession(planId);
// ...
// Sync plan name to draft
useEffect(() => {
syncDraftName(planName);
}, [planName, syncDraftName]);
// Existing plan loading becomes only “draft-aware”
useEffect(() => {
if (!existingPlan || draftDecision === "pending") return;
if (draftDecision === "fresh") {
// existing logic: setPlanName, setPlanImageUrl, setWorkouts, schedule...
}
setDataLoaded(true);
}, [existingPlan, draftDecision, setPlanName, setWorkouts, setPlanImageUrl, setPlanSchedule]);
}
```
This keeps all current behavior (including Alerts, hydration, draft persistence), but:
- the screen no longer reaches into `useWorkoutStore.getState()` or knows about `draftContext` / `draftId` internals;
- the main `useEffect`s are now “load plan when `draftDecision` is ready” + “sync name to draft”, which are easier to reason about;
- the draft state machine (`pending/fresh/continue`) is localized inside the hook.
### 2. Reuse for other screens
You mentioned similar logic in `create-workout.tsx`. The same `usePlanDraftSession` pattern can be duplicated/parametrized (or a generic `useDraftSession("plan", planId)` if you want) and shared, so both screens stay thin and aligned without duplicating the hydration / alert / store-peeking logic.
</issue_to_address>
### Comment 5
<location path="app/(app)/(create-plan)/create-workout.tsx" line_range="57" />
<code_context>
const createMutation = useCreateStandaloneWorkout();
const updateMutation = useUpdateStandaloneWorkout();
+ // 'pending' until draft check resolves; then 'continue' or 'fresh'
+ const [draftDecision, setDraftDecision] = useState<
+ "pending" | "continue" | "fresh"
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the draft-session handling and unsaved-changes navigation guard into shared hooks to avoid duplicating this state machine and make the control flow easier to reason about across screens.
You’ve effectively reimplemented the draft-session state machine inline here; pulling that into a shared hook would reduce complexity and keep the behavior consistent with `create.tsx`.
### 1. Extract draft-session handling into a shared hook
Right now this screen owns:
- `draftDecision` state
- `startFreshSession`
- the hydration / draft-checking `useEffect`
- direct calls to `setDraftContext`, `setDraftId`, `clearDraft`, `setWorkouts`, `initializedWorkoutId`
You can centralize this into something like `useDraftedWorkoutSession`, used both here and in `create.tsx`. The screen only needs to know “what’s the draft state?” and “when can I initialize workouts?”, not *how* the draft is stored.
Example (kept minimal and self‑contained):
```ts
// useDraftedWorkoutSession.ts
type DraftDecision = "pending" | "continue" | "fresh";
export function useDraftedWorkoutSession(
context: "standalone" | "plan",
existingWorkoutId?: number,
) {
const { setDraftContext, setDraftId, clearDraft } = useWorkoutStore();
const initializedWorkoutId = useRef<number | null | undefined>(undefined);
const [decision, setDecision] = useState<DraftDecision>("pending");
const startFreshSession = useCallback(() => {
setDraftContext(context);
setDraftId(existingWorkoutId);
}, [context, existingWorkoutId, setDraftContext, setDraftId]);
useEffect(() => {
const checkDraft = () => {
const { workouts, draftContext, draftId } = useWorkoutStore.getState();
const hasDraft =
draftContext === context &&
draftId === existingWorkoutId &&
workouts.length > 0 &&
(workouts[0].exercises.length > 0 || workouts[0].name.trim() !== "");
if (hasDraft) {
Alert.alert(
"Continue Editing?",
"You have unsaved changes from your last session. Would you like to continue?",
[
{
text: "Discard Changes",
style: "destructive",
onPress: () => {
clearDraft();
startFreshSession();
setDecision("fresh");
},
},
{
text: "Continue",
onPress: () => {
initializedWorkoutId.current = existingWorkoutId ?? null;
setDecision("continue");
},
},
],
);
} else {
if (draftContext !== null) clearDraft();
startFreshSession();
setDecision("fresh");
}
};
if (useWorkoutStore.persist.hasHydrated()) {
checkDraft();
} else {
return useWorkoutStore.persist.onFinishHydration(checkDraft);
}
}, [context, existingWorkoutId, clearDraft, startFreshSession]);
return { decision, initializedWorkoutId, clearDraft };
}
```
Then this screen becomes simpler and symmetric with `create.tsx`:
```ts
// in standalone screen
const { decision: draftDecision, initializedWorkoutId, clearDraft } =
useDraftedWorkoutSession("standalone", existingWorkoutId);
// Initialise workoutStore for this screen — runs once per workout id
useEffect(() => {
if (draftDecision !== "fresh") return; // only run when we know we should initialize fresh
const sentinel = existingWorkoutId ?? null;
if (initializedWorkoutId.current === sentinel) return;
if (existingWorkoutId && standaloneWorkouts) {
const existing = standaloneWorkouts.find((w) => w.id === existingWorkoutId);
if (existing) {
initializedWorkoutId.current = sentinel;
setWorkouts([existing]);
return;
}
} else if (existingWorkoutId && !standaloneWorkouts) {
return;
}
initializedWorkoutId.current = sentinel;
clearWorkouts();
addWorkout({ name: "", exercises: [], id: -Date.now() });
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [draftDecision, existingWorkoutId, standaloneWorkouts]);
```
This keeps the behavior but:
- removes local `startFreshSession` / draft-checking effect duplication here and in `create.tsx`
- decouples initialization logic from the draft state machine (it only cares about `draftDecision`)
- gives you a single place to change draft behavior for both flows.
### 2. Use a shared navigation guard helper (optional, but similar win)
You also duplicated the “unsaved changes + clearDraft” guard. A small helper hook would keep this in sync across screens:
```ts
// useUnsavedWorkoutGuard.ts
export function useUnsavedWorkoutGuard({ navigation, clearDraft }: {
navigation: any;
clearDraft: () => void;
}) {
const workouts = useWorkoutStore((s) => s.workouts);
const savedRef = useRef(false); // or inject as param if already owned
useEffect(() => {
const unsubscribe = navigation.addListener("beforeRemove", (e) => {
e.preventDefault();
if (savedRef.current) {
savedRef.current = false;
clearDraft();
return navigation.dispatch(e.data.action);
}
const workout = workouts[0];
if (!workout || (!workout.exercises.length && !workout.name.trim())) {
clearDraft();
return navigation.dispatch(e.data.action);
}
Alert.alert("Discard Changes?", "You have unsaved changes. Are you sure you want to discard them?", [
{ text: "Cancel", style: "cancel" },
{
text: "Discard",
style: "destructive",
onPress: () => {
clearDraft();
navigation.dispatch(e.data.action);
},
},
]);
});
return unsubscribe;
}, [navigation, workouts, clearDraft]);
}
```
Usage in this screen and `create.tsx`:
```ts
useUnsavedWorkoutGuard({ navigation, clearDraft });
```
Both changes preserve the new draft behavior but significantly reduce duplication and make the control flow easier to reason about.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds AsyncStorage-backed draft persistence and continue/discard flows to plan/workout creation; introduces warm‑up set insertion and grouped rendering with scoped apply‑to‑all; threads unilateral counting into duration estimates and surfaces compact estimates across multiple UI lists/cards; computes weekly streaks and updates Insights layout and stats tiles. ChangesDraft persistence and decision flows
Warm‑up set management and selective apply‑to‑all
Workout duration estimation and display
Weekly streak computation and stats UI layout
Active workout and history UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 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. 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: 7
🤖 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 `@app/`(app)/(create-plan)/create.tsx:
- Around line 123-126: The code is unconditionally clearing any non-null draft
via clearDraft() when opening the editor; change this to only clear a draft when
it belongs to the same context (i.e., compare draftContext to the current
plan/workout id) or, if they mismatch, prompt the user for confirmation before
replacing it. Concretely, update the block that currently does "if (draftContext
!== null) clearDraft(); startFreshSession(); setDraftDecision('fresh');" to: 1)
check if draftContext is null (safe to clear), 2) if draftContext equals the
current context id then clearDraft() and proceed, 3) if draftContext differs
then show a confirmation dialog and only call clearDraft(), startFreshSession(),
and setDraftDecision('fresh') when the user confirms; apply the same
conditional/prompt logic to the corresponding block in create-workout.tsx.
- Around line 147-149: The early-return currently using existingPlan and
draftDecision prevents the UI from showing a hydrated draft; change the guard so
it does not return when the user chose to continue a draft — i.e., update the
condition around existingPlan and draftDecision so that continuing (the
"continue" draftDecision value) bypasses the return and allows the editor to use
the hydrated store (so dataLoaded can become true immediately); locate and
modify the block that checks existingPlan and draftDecision (and the subsequent
if (draftDecision === "fresh") branch) to allow the continue path through.
In `@app/`(app)/(tabs)/(stats)/index.tsx:
- Around line 137-140: The new useCompletedWorkoutsQuery call only reads data
(allWorkouts) so its loading/error are ignored; update the hook invocation to
also destructure isLoading and error (e.g., const { data: allWorkouts,
isLoading: isLoadingAllWorkouts, error: allWorkoutsError } =
useCompletedWorkoutsQuery(...)) and then fold those into the component's
existing loading and error flows (combine isLoading flags with logical OR and
include allWorkoutsError in the same error handling/reporting path) so failures
or loading from allWorkouts are surfaced just like the other queries.
In `@app/`(app)/(tabs)/index.tsx:
- Around line 218-220: The completion logic is inconsistent: the sorting uses
const target = perWorkoutTarget.get(workout.id!); const completedTimes =
completedWorkoutCounts.get(workout.id!) || 0; const workoutCompleted = target
!== undefined && completedTimes >= target, but the card status path still uses
completedWorkoutCounts.get(... ) ?? 0 which treats missing target as completed;
unify by centralizing the predicate—create/replace uses with the same check
(target !== undefined && completedTimes >= target) when rendering card status
(and any other place that checks completion) so workouts without a target are
treated as not completed; reference perWorkoutTarget, completedWorkoutCounts,
and workoutCompleted when updating the card status rendering to use the same
logic.
In `@components/EditSetModal.tsx`:
- Around line 210-214: The apply-to-all loop filters sets by their current
(s.isWarmup) so when the user toggles the warmup state the edited set can be
omitted; change the logic in the exercise?.sets.forEach block to compare against
the original warmup state (capture the pre-toggle value, e.g. originalIsWarmup)
or always include the currently edited index (check sIndex === editingSetIndex)
so updateSetInExercise(workoutIndex, exerciseId, sIndex, updatedSet) is called
for the edited set even if its isWarmup value changed.
In `@components/StandaloneWorkoutListItem.tsx`:
- Line 42: The subtitle is adding an extra tilde because
formatDurationEstimate(estimate) already includes the approximation prefix for
single-value estimates; remove the manual " ~" prefix here and render the
formatted string directly (e.g., replace `·
~${formatDurationEstimate(estimate)}` with `·
${formatDurationEstimate(estimate)}`) so the displayed duration never gets a
double "~~". Use the formatDurationEstimate symbol to locate the change in
StandaloneWorkoutListItem.
In `@store/workoutStore.ts`:
- Around line 325-329: When mirroring an inserted set into a superset partner
(the branch using partner, lastSet, newSet, sets and atIndex), the code
currently clones lastSet with { ...lastSet } which can lose warm-up/apply-to-all
flags; change the cloned object to merge the flag fields from newSet into the
clone (e.g. const clone = { ...lastSet, ...(pick warmUp/apply-to-all flags from
newSet such as isWarmUp or applyToAll) } ) before splicing so the partner
receives the same warm-up/apply-to-all markers as the original insert.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7cbbccf1-dd09-4012-b03f-21467e48b921
📒 Files selected for processing (16)
app/(app)/(create-plan)/create-workout.tsxapp/(app)/(create-plan)/create.tsxapp/(app)/(create-plan)/sets-overview.tsxapp/(app)/(tabs)/(plans)/index.tsxapp/(app)/(tabs)/(plans)/overview.tsxapp/(app)/(tabs)/(stats)/index.tsxapp/(app)/(tabs)/index.tsxcomponents/EditSetModal.tsxcomponents/StandaloneWorkoutListItem.tsxcomponents/WorkoutCard.tsxcomponents/stats/InsightsStrip.tsxcomponents/stats/StatsTile.tsxhooks/useCreatePlan.tshooks/useWorkoutDurationEstimate.tsstore/workoutStore.tsutils/estimateWorkoutDuration.ts
…een for improved persistence and clarity
There was a problem hiding this comment.
🧹 Nitpick comments (1)
components/stats/InsightsStrip.tsx (1)
54-54: 💤 Low valueHardcoded " 1RM" suffix may be incorrect for future tooltip types.
The tooltip text unconditionally appends
" 1RM"regardless of the pill's context. Currently only the "Best gain" pill has a tooltip, so this works, but if additional tooltip-enabled pills are added, the suffix will be incorrect. Consider moving this suffix into thetooltipvalue itself or conditionally appending it based on the pill label.🤖 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 `@components/stats/InsightsStrip.tsx` at line 54, The tooltip text currently appends a hardcoded " 1RM" in setTooltipText regardless of context; update the logic so the suffix is provided by the tooltip data itself or only appended when appropriate (e.g., when the pill label === "Best gain"). Locate the setTooltipText call in InsightsStrip.tsx and either use the existing tooltip prop/value (preferable) as-is or add a conditional that appends " 1RM" only for the specific pill(s) that require it (reference the pill label and the tooltip value to implement this).
🤖 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.
Nitpick comments:
In `@components/stats/InsightsStrip.tsx`:
- Line 54: The tooltip text currently appends a hardcoded " 1RM" in
setTooltipText regardless of context; update the logic so the suffix is provided
by the tooltip data itself or only appended when appropriate (e.g., when the
pill label === "Best gain"). Locate the setTooltipText call in InsightsStrip.tsx
and either use the existing tooltip prop/value (preferable) as-is or add a
conditional that appends " 1RM" only for the specific pill(s) that require it
(reference the pill label and the tooltip value to implement this).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e3915e2d-a5d5-4885-b60c-0cfefc8e215d
📒 Files selected for processing (8)
app/(app)/(create-plan)/create-workout.tsxapp/(app)/(create-plan)/create.tsxapp/(app)/(tabs)/(stats)/index.tsxapp/(app)/(tabs)/index.tsxcomponents/EditSetModal.tsxcomponents/stats/InsightsStrip.tsxstore/workoutStore.tsutils/estimateWorkoutDuration.ts
…tion for better readability
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/(app)/(workout)/workout-session.tsx (1)
973-981:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not start a rest timer in the branch that is about to leave the session.
shouldAnimateis false exactly whennextSet()will not keep this screen alive. UsinghasNextSethere can still be true for a sequential-but-already-completed exercise, so this path can fire a rest timer/notification right beforerouter.back().Suggested fix
if (!shouldAnimate) { - if (isFirstInSuperset) { - stopTimer(); - void cancelRestNotifications(); - } else if (hasNextSet) { - void startRestTimer(currentSet.restMinutes, currentSet.restSeconds); - } else { - void cancelRestNotifications(); - } + stopTimer(); + void cancelRestNotifications(); lastCompletedSetRef.current = { exerciseIndex: currentExerciseIndex, setIndex: currentSetIndex, };🤖 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 `@app/`(app)/(workout)/workout-session.tsx around lines 973 - 981, When shouldAnimate is false we must not schedule a rest timer/notification because the screen is about to be popped; update the branch that checks shouldAnimate so that when shouldAnimate === false you never call startRestTimer—call stopTimer() and/or cancelRestNotifications() as appropriate (e.g., if isFirstInSuperset call stopTimer() and cancelRestNotifications(), otherwise just cancelRestNotifications()), and move the hasNextSet + startRestTimer(currentSet.restMinutes, currentSet.restSeconds) logic into the shouldAnimate === true branch so startRestTimer only runs when the session will remain active.store/activeWorkoutStore.ts (1)
390-418:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCount only
trueentries when deciding an exercise is complete.These carry-forward branches still rely on
Object.keys(completedSets[...]).length === totalSets. That is wrong once the map contains explicitfalseentries —addSet()andreplaceExercise()both create them — sonextSet()can skip an unfinished exercise or close the session early.Suggested fix
+ const isExerciseComplete = ( + exerciseIndex: number, + totalSets: number, + ) => + Array.from({ length: totalSets }, (_, s) => s).every( + (s) => updatedCompletedSets[exerciseIndex]?.[s] === true, + ); + while (nextExerciseIndex < workout.exercises.length) { const totalSets = workout.exercises[nextExerciseIndex].sets.length; - const isComplete = - updatedCompletedSets[nextExerciseIndex] && - Object.keys(updatedCompletedSets[nextExerciseIndex]) - .length === totalSets; + const isComplete = isExerciseComplete( + nextExerciseIndex, + totalSets, + ); if (!isComplete) break; nextExerciseIndex++; } ... while (nextExerciseIndex < workout.exercises.length) { const totalSets = workout.exercises[nextExerciseIndex].sets.length; - const isExerciseCompleted = - updatedCompletedSets[nextExerciseIndex] && - Object.keys(updatedCompletedSets[nextExerciseIndex]).length === - totalSets; + const isExerciseCompleted = isExerciseComplete( + nextExerciseIndex, + totalSets, + );Also applies to: 528-550
🤖 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 `@store/activeWorkoutStore.ts` around lines 390 - 418, The completion check currently uses Object.keys(updatedCompletedSets[nextExerciseIndex]).length === totalSets which incorrectly counts false entries; change both occurrences (the one using updatedCompletedSets[...] and the similar one around 528-550) to count only truthy/completed entries, e.g. compute completedCount = Object.values(updatedCompletedSets[nextExerciseIndex] || {}).filter(v => v === true).length and compare completedCount === totalSets (apply the same change where completedSets[...] is used), so only true entries mark an exercise complete.
🤖 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.
Outside diff comments:
In `@app/`(app)/(workout)/workout-session.tsx:
- Around line 973-981: When shouldAnimate is false we must not schedule a rest
timer/notification because the screen is about to be popped; update the branch
that checks shouldAnimate so that when shouldAnimate === false you never call
startRestTimer—call stopTimer() and/or cancelRestNotifications() as appropriate
(e.g., if isFirstInSuperset call stopTimer() and cancelRestNotifications(),
otherwise just cancelRestNotifications()), and move the hasNextSet +
startRestTimer(currentSet.restMinutes, currentSet.restSeconds) logic into the
shouldAnimate === true branch so startRestTimer only runs when the session will
remain active.
In `@store/activeWorkoutStore.ts`:
- Around line 390-418: The completion check currently uses
Object.keys(updatedCompletedSets[nextExerciseIndex]).length === totalSets which
incorrectly counts false entries; change both occurrences (the one using
updatedCompletedSets[...] and the similar one around 528-550) to count only
truthy/completed entries, e.g. compute completedCount =
Object.values(updatedCompletedSets[nextExerciseIndex] || {}).filter(v => v ===
true).length and compare completedCount === totalSets (apply the same change
where completedSets[...] is used), so only true entries mark an exercise
complete.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 564b7330-b85c-4daa-b75e-c08cab6ccf9d
📒 Files selected for processing (2)
app/(app)/(workout)/workout-session.tsxstore/activeWorkoutStore.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@utils/loadPremadePlans.ts`:
- Around line 110-115: Replace the current non-fatal handling of missing
exercises in loadPremadePlans.ts (the block checking localExercise for each
exercise) with a hard failure so the migration transaction can roll back:
instead of console.warn(...) and continue when localExercise is falsy for
exercise.exercise_id, throw a descriptive Error including the
exercise.exercise_id (and optionally context like plan id) so the caller can
abort/retry; update any surrounding try/catch or transaction handling if needed
to ensure the throw triggers a rollback.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0f5fe6fe-821d-4d4c-a244-428647290cec
📒 Files selected for processing (3)
app/(app)/(workout)/workout-session.tsxstore/activeWorkoutStore.tsutils/loadPremadePlans.ts
This was referenced May 23, 2026
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.
Summary by Sourcery
Persist workout and plan editing drafts across sessions, enhance workout duration insights and stats display, and improve set and warm-up management in workouts.
New Features:
Bug Fixes:
Enhancements:
Summary by CodeRabbit
New Features
Improvements
UX