feat: various updates and improvements#135
Merged
Merged
Conversation
… placeholder text
…nd modal for workout selection
…UI for minutes and seconds input
…ple entries are available
…lement setupAppCheck function
Reviewer's GuideImplements weekly plan scheduling with rest-day awareness and schedule persistence, enriches workout stats and session UX (time input, continuous weight/reps controls, selection flows), links quick workouts to saved templates, fixes timer/labeling issues, and integrates Firebase App Check plus related build/config updates. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
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 persistent weekly plan scheduling (DB schema, store, hooks, editor), integrates schedule-aware Home logic (rest days, per-workout targets), refactors FAB/header actions to bottom/header buttons, improves session save/linking and continuous adjustments, updates components, and initialises Firebase App Check. ChangesPlan Scheduling Feature
Action Button UI Refactoring
Workout Session Enhancements
Component Refinements & UI Updates
Stats & Settings
Infrastructure & Configuration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 8 issues, and left some high level feedback:
- In _layout.tsx you call setupAppCheck() but never import it (e.g. from utils/initAppCheck), which will throw at runtime; add the missing import.
- In the stats summary,
volumeUnitis set to "st" for lbs but the code is actually computing tons (totalVolume / 2000), which is confusing; consider renaming the label to something like "tons"/"t" or similar to match the calculation. - The plan schedule in the editor uses workout array indices, and you only sync it on workout removal; if the user reorders workouts, the schedule will silently point at the wrong workouts, so you likely need a corresponding sync-on-reorder path to remap indices.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In _layout.tsx you call setupAppCheck() but never import it (e.g. from utils/initAppCheck), which will throw at runtime; add the missing import.
- In the stats summary, `volumeUnit` is set to "st" for lbs but the code is actually computing tons (`totalVolume / 2000`), which is confusing; consider renaming the label to something like "tons"/"t" or similar to match the calculation.
- The plan schedule in the editor uses workout array indices, and you only sync it on workout removal; if the user reorders workouts, the schedule will silently point at the wrong workouts, so you likely need a corresponding sync-on-reorder path to remap indices.
## Individual Comments
### Comment 1
<location path="components/TimeInput.tsx" line_range="76-77" />
<code_context>
+ }, 50);
+ };
+
+ const handleMinutesChange = (text: string) => {
+ const digit = text.replace(/[^0-9]/g, "").slice(0, 1);
+ setMinutes(digit);
+ onChange(emitValue(digit, seconds));
</code_context>
<issue_to_address>
**issue (bug_risk):** Minutes input is limited to a single digit, preventing times above 9 minutes.
Because the input is sliced to one character and `maxLength` is 1, users can’t enter values like 12 or 15 minutes. If durations above 9 minutes are required, increase this to 2 digits (e.g., `.slice(0, 2)` and `maxLength={2}`) and update any code that assumes minutes are a single digit.
</issue_to_address>
### Comment 2
<location path="app/(app)/(tabs)/index.tsx" line_range="131-140" />
<code_context>
if (activePlan) {
const sortedWorkouts = activePlan.workouts;
+ const weeklyGoal = Number(settings?.weeklyGoal);
+ const numWorkouts = activePlan.workouts.length;
+
+ // Build per-workout targets
+ if (planScheduleEntries && planScheduleEntries.length > 0) {
+ // Count how many days each workout is scheduled this week
+ for (const entry of planScheduleEntries) {
+ const current = perWorkoutTarget.get(entry.workout_id) ?? 0;
+ perWorkoutTarget.set(entry.workout_id, current + 1);
+ }
+ // Determine if today is a rest day
+ const todayEntry = planScheduleEntries.find(
+ (e) => e.day_of_week === todayDow,
+ );
+ isRestDay = todayEntry == null;
+ } else {
+ // No schedule: distribute weeklyGoal evenly across workouts
+ // Use floor/ceil distribution so the total exactly equals weeklyGoal
+ sortedWorkouts.forEach((workout, i) => {
</code_context>
<issue_to_address>
**issue (bug_risk):** Handling of invalid or zero `weeklyGoal` can lead to `NaN` per-workout targets.
If `settings?.weeklyGoal` is missing, empty, or non-numeric, `Number(settings?.weeklyGoal)` becomes `NaN`, which then propagates into `base`/`extra` and `perWorkoutTarget`. That makes comparisons like `completedTimes >= target` unreliable. Consider normalizing to a sane default (e.g. `const weeklyGoal = Math.max(1, Number(settings?.weeklyGoal) || 0);`) or skipping the distribution logic when `!Number.isFinite(weeklyGoal)`.
</issue_to_address>
### Comment 3
<location path="app/(app)/(tabs)/(stats)/index.tsx" line_range="99-104" />
<code_context>
+ );
+ }, 0)
+ : 0;
+ const tonDivisor = weightUnit === "lbs" ? 2000 : 1000;
+ const formattedVolume = (totalVolume / tonDivisor).toLocaleString(undefined, {
+ minimumFractionDigits: 2,
+ maximumFractionDigits: 2,
+ });
+ const volumeUnit = weightUnit === "lbs" ? "st" : "t";
+
+ const avgSetsPerWorkout =
</code_context>
<issue_to_address>
**issue (bug_risk):** Volume unit label "st" conflicts with the divisor used (tons vs stones).
`tonDivisor` uses 2000 (short tons) for lbs and 1000 (metric tons) for kg, but the lbs label is `
</issue_to_address>
### Comment 4
<location path="utils/initAppCheck.ts" line_range="10-19" />
<code_context>
+ const isDevBuild = extra?.appVariant === "development";
+ const debugToken: string | undefined = extra?.appCheckDebugToken ?? undefined;
+
+ await initializeAppCheck(getApp(), {
+ provider: {
+ providerOptions: {
+ android: {
+ provider: isDevBuild ? "debug" : "playIntegrity",
+ debugToken,
+ },
+ apple: {
+ provider: isDevBuild ? "debug" : "appAttest",
+ debugToken,
+ },
+ },
+ },
+ isTokenAutoRefreshEnabled: true,
+ });
+}
</code_context>
<issue_to_address>
**issue (bug_risk):** App Check initialization doesn’t follow the standard provider API and may fail at runtime.
The React Native Firebase App Check API expects the built-in provider helpers (e.g., `playIntegrityProvider`, `appAttestProvider`, `debugProvider`) rather than a custom `provider` object with `providerOptions`. This structure is unlikely to be recognized by the SDK and could cause initialization to fail or behave unexpectedly. Please cross-check with the `@react-native-firebase/app-check` documentation and use the documented provider helper functions instead.
</issue_to_address>
### Comment 5
<location path="components/TimeInput.tsx" line_range="30" />
<code_context>
+ const { minutes: initM, seconds: initS } = parseValue(value);
+ const [minutes, setMinutes] = useState(initM === "0" ? "" : initM);
+ const [seconds, setSeconds] = useState(initS === "00" ? "" : initS);
+ const secondsRef = useRef<{ focus: () => void }>(null);
+ const isEditingRef = useRef(false);
+ const blurTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the TimeInput component by replacing the ref/timer editing lock with a boolean state and extracting parsing/normalization helpers so the handlers and JSX stay focused and easier to reason about.
You can keep all the current behavior while trimming a lot of incidental complexity by:
1. **Remove refs/timers; use a simple `isEditing` state**
You don’t need `isEditingRef` + `blurTimerRef` with a magic 50 ms delay. A boolean state plus focus/blur handlers is enough:
```ts
export const TimeInput = ({ value, onChange, style }: TimeInputProps) => {
const [minutes, setMinutes] = useState("");
const [seconds, setSeconds] = useState("");
const [isEditing, setIsEditing] = useState(false);
const secondsRef = useRef<TextInput | null>(null);
useEffect(() => {
if (isEditing) return; // don't override while user is editing
const { minutes: m, seconds: s } = parseValue(value);
setMinutes(m === "0" ? "" : m);
setSeconds(s === "00" ? "" : s);
}, [value, isEditing]);
const handleFocus = () => setIsEditing(true);
const handleBlur = () => setIsEditing(false);
// ...
};
```
Then use `handleFocus` / `handleBlur` on both inputs instead of the timer-based lock.
2. **Extract parsing/formatting/normalization into pure helpers**
Move all pure logic out of the component so the handlers stay trivial:
```ts
// timeInputUtils.ts
export function parseValue(value: string): { minutes: string; seconds: string } {
const [m = "0", s = "00"] = value.split(":");
return { minutes: m, seconds: s };
}
export function emitValue(minutes: string, seconds: string): string {
const m = parseInt(minutes || "0", 10);
const s = parseInt(seconds || "0", 10);
return `${m}:${String(s).padStart(2, "0")}`;
}
export function normalizeSeconds(seconds: string): string {
const s = Math.min(parseInt(seconds || "0", 10), 59);
return String(s).padStart(2, "0");
}
```
Then the blur handler becomes much simpler and focused:
```ts
const handleSecondsBlur = () => {
const normalized = normalizeSeconds(seconds);
setSeconds(normalized);
onChange(emitValue(minutes, normalized));
setIsEditing(false);
};
```
3. **Keep input handlers focused on sanitizing and emitting**
Minutes change handler can handle auto-focus + emit without mixing other concerns:
```ts
const handleMinutesChange = (text: string) => {
const digit = text.replace(/\D/g, "").slice(0, 1);
setMinutes(digit);
onChange(emitValue(digit, seconds));
if (digit.length === 1) {
secondsRef.current?.focus();
}
};
const handleSecondsChange = (text: string) => {
const digits = text.replace(/\D/g, "").slice(0, 2);
setSeconds(digits);
onChange(emitValue(minutes, digits));
};
```
Altogether this removes refs + timers, centralizes formatting/normalization, and keeps JSX event handlers short and easy to reason about while preserving all current behavior.
</issue_to_address>
### Comment 6
<location path="app/(app)/(tabs)/index.tsx" line_range="123" />
<code_context>
let completedWorkoutsThisPlanThisWeek: CompletedWorkout[] = [];
let workoutsToDisplay: Workout[] = [];
+ let isRestDay = false;
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the schedule/target calculation, workout ordering, and delayed resume-card logic into small helpers or hooks so the HomeScreen component reads more declaratively.
You can keep all of this behavior while cutting the cognitive load by extracting two small pieces:
1. **Schedule/targets + ordering** into pure helpers (or a small hook).
2. **Delayed `showResumeCard`** into a tiny reusable hook.
### 1. Extract schedule → targets + ordering
All of this:
- building `perWorkoutTarget`
- detecting `isRestDay`
- reordering `uncompletedWorkouts` based on schedule (today vs next scheduled)
can live in a helper that takes plain data and returns plain data.
Example (placed near the file bottom or in a small `homeSchedule.ts`):
```ts
type ScheduleEntry = {
workout_id: number;
day_of_week: number; // 0=Mon..6=Sun
};
function buildPerWorkoutTarget(
workouts: Workout[],
planScheduleEntries: ScheduleEntry[] | undefined,
weeklyGoal: number,
): { perWorkoutTarget: Map<number, number>; isRestDay: boolean } {
const perWorkoutTarget = new Map<number, number>();
let isRestDay = false;
if (planScheduleEntries && planScheduleEntries.length > 0) {
for (const entry of planScheduleEntries) {
const current = perWorkoutTarget.get(entry.workout_id) ?? 0;
perWorkoutTarget.set(entry.workout_id, current + 1);
}
const todayDow = (getDay(new Date()) + 6) % 7;
const todayEntry = planScheduleEntries.find((e) => e.day_of_week === todayDow);
isRestDay = todayEntry == null;
} else {
const numWorkouts = workouts.length;
workouts.forEach((workout, i) => {
if (workout.id == null) return;
const base = Math.floor(weeklyGoal / numWorkouts);
const extra = i < weeklyGoal % numWorkouts ? 1 : 0;
perWorkoutTarget.set(workout.id, Math.max(1, base + extra));
});
}
return { perWorkoutTarget, isRestDay };
}
```
…and the schedule-driven reordering can be pulled out similarly:
```ts
function orderUncompletedWorkoutsBySchedule(
uncompleted: Workout[],
planScheduleEntries: ScheduleEntry[] | undefined,
todayDow: number,
isRestDay: boolean,
): Workout[] {
if (!planScheduleEntries?.length) return uncompleted;
const result = [...uncompleted];
const pinWorkoutFirst = (workoutId: number | undefined) => {
if (!workoutId) return;
const idx = result.findIndex((w) => w.id === workoutId);
if (idx > 0) {
const [w] = result.splice(idx, 1);
result.unshift(w);
}
};
if (!isRestDay) {
const todayEntry = planScheduleEntries.find((e) => e.day_of_week === todayDow);
pinWorkoutFirst(todayEntry?.workout_id);
} else {
for (let i = 1; i <= 6; i++) {
const nextDow = (todayDow + i) % 7;
const nextEntry = planScheduleEntries.find((e) => e.day_of_week === nextDow);
if (nextEntry) {
pinWorkoutFirst(nextEntry.workout_id);
break;
}
}
}
return result;
}
```
Then your component body becomes much flatter:
```ts
const today = new Date();
const todayDow = (getDay(today) + 6) % 7;
let completedWorkoutsThisPlanThisWeek: CompletedWorkout[] = [];
let workoutsToDisplay: Workout[] = [];
let isRestDay = false;
let perWorkoutTarget = new Map<number, number>();
if (activePlan && settings) {
const sortedWorkouts = activePlan.workouts;
const weeklyGoal = Number(settings.weeklyGoal);
const targetResult = buildPerWorkoutTarget(
sortedWorkouts,
planScheduleEntries,
weeklyGoal,
);
perWorkoutTarget = targetResult.perWorkoutTarget;
isRestDay = targetResult.isRestDay;
completedWorkoutsThisPlanThisWeek =
completedWorkoutsThisWeek?.filter(
(workout) => String(workout.plan_id) === String(activePlan.id),
) ?? [];
const completedWorkoutCounts = new Map<number, number>();
completedWorkoutsThisPlanThisWeek.forEach((completedWorkout) => {
const count = completedWorkoutCounts.get(completedWorkout.workout_id) || 0;
completedWorkoutCounts.set(completedWorkout.workout_id, count + 1);
});
const uncompleted: Workout[] = [];
const completedList: Workout[] = [];
sortedWorkouts.forEach((workout) => {
const target = perWorkoutTarget.get(workout.id!) ?? 1;
const completedTimes = completedWorkoutCounts.get(workout.id!) || 0;
const workoutCompleted = completedTimes >= target;
(workoutCompleted ? completedList : uncompleted).push(workout);
});
const orderedUncompleted = orderUncompletedWorkoutsBySchedule(
uncompleted,
planScheduleEntries,
todayDow,
isRestDay,
);
workoutsToDisplay = [...orderedUncompleted, ...completedList];
}
```
This keeps all the behavior but makes the screen read as “what happens” rather than “how it’s computed”.
You could also wrap the above into a hook (e.g. `usePlanScheduleViewModel`) if you prefer:
```ts
const {
workoutsToDisplay,
perWorkoutTarget,
isRestDay,
completedWorkoutsThisPlanThisWeek,
} = usePlanScheduleViewModel({
activePlan,
settings,
completedWorkoutsThisWeek,
planScheduleEntries,
});
```
### 2. Extract delayed `showResumeCard` into a tiny hook
The `useEffect` + timeout for `showResumeCard` is a reusable pattern and adds noise inline. A tiny hook keeps the HomeScreen cleaner:
```ts
function useDelayedFlag(active: boolean, delayMs: number): boolean {
const [flag, setFlag] = useState(active);
useEffect(() => {
if (!active) {
setFlag(false);
return;
}
const timer = setTimeout(() => setFlag(true), delayMs);
return () => clearTimeout(timer);
}, [active, delayMs]);
return flag;
}
```
Usage in `HomeScreen`:
```ts
const workoutInProgress = useActiveWorkoutStore((state) =>
Boolean(state.activeWorkout && state.workout),
);
const showResumeCard = useDelayedFlag(workoutInProgress, 1000);
```
That lets you drop the inline `useEffect` and `useState` and keeps the timing behavior self-contained and testable.
</issue_to_address>
### Comment 7
<location path="components/SessionSetInfo.tsx" line_range="98" />
<code_context>
const [menuVisible, setMenuVisible] = useState(false);
const [timeInput, setTimeInput] = useState(time);
+ const pressTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
+ const pressIntervalRef = useRef<ReturnType<typeof setInterval> | null>(null);
+ const isHoldingRef = useRef(false);
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the new press-and-hold logic into a reusable `useContinuousPress` hook to keep SessionSetInfo focused and avoid duplicated timer/ref handling code.
The new continuous-press behavior is generic and duplicated; it increases this component’s responsibilities (refs, timers, effects, and event wiring) in a way that can be cleanly abstracted.
You can reduce complexity by extracting a `useContinuousPress` hook that encapsulates the timers, refs, and cleanup, and then reuse it for weight and reps.
### 1. Extract `useContinuousPress` hook
```ts
// useContinuousPress.ts
import { useRef, useCallback, useEffect } from "react";
type Options = {
delay?: number;
interval?: number;
};
export function useContinuousPress(
step: () => void,
{ delay = 400, interval = 150 }: Options = {}
) {
const pressTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
const pressIntervalRef = useRef<ReturnType<typeof setInterval> | null>(null);
const isHoldingRef = useRef(false);
const onPressIn = useCallback(() => {
pressTimerRef.current = setTimeout(() => {
isHoldingRef.current = true;
step();
pressIntervalRef.current = setInterval(step, interval);
}, delay);
}, [step, delay, interval]);
const onPressOut = useCallback(() => {
if (pressTimerRef.current) {
clearTimeout(pressTimerRef.current);
pressTimerRef.current = null;
}
if (pressIntervalRef.current) {
clearInterval(pressIntervalRef.current);
pressIntervalRef.current = null;
}
setTimeout(() => {
isHoldingRef.current = false;
}, 50);
}, []);
const onPress = useCallback(() => {
if (!isHoldingRef.current) {
step();
}
}, [step]);
useEffect(() => {
return () => {
if (pressTimerRef.current) clearTimeout(pressTimerRef.current);
if (pressIntervalRef.current) clearInterval(pressIntervalRef.current);
};
}, []);
return { onPress, onPressIn, onPressOut };
}
```
### 2. Use it in `SessionSetInfo`
```ts
// inside SessionSetInfo
import { useContinuousPress } from "./useContinuousPress"; // adjust path
const weightDecPress = useContinuousPress(() =>
handleWeightChange(-weightIncrement)
);
const weightIncPress = useContinuousPress(() =>
handleWeightChange(weightIncrement)
);
const repsDecPress = useContinuousPress(() => handleRepsChange(-1));
const repsIncPress = useContinuousPress(() => handleRepsChange(1));
```
Then simplify the buttons:
```tsx
<IconButton
icon="minus"
{...weightDecPress}
size={buttonSize}
iconColor={Colors.dark.text}
style={styles.iconButton}
/>
<IconButton
icon="plus"
{...weightIncPress}
size={buttonSize}
iconColor={Colors.dark.text}
style={styles.iconButton}
/>
<IconButton
icon="minus"
{...repsDecPress}
size={buttonSize}
iconColor={Colors.dark.text}
style={styles.iconButton}
/>
<IconButton
icon="plus"
{...repsIncPress}
size={buttonSize}
iconColor={Colors.dark.text}
style={styles.iconButton}
/>
```
This keeps all current behavior (single tap vs. press-and-hold, delay, repeat interval, cleanup) but:
- Removes three refs and two callbacks from `SessionSetInfo`.
- Eliminates the `isHoldingRef` checks from the JSX.
- Centralizes the timer/gesture logic in one reusable hook.
</issue_to_address>
### Comment 8
<location path="hooks/useCreatePlan.ts" line_range="52" />
<code_context>
try {
let newPlanId: number | null = null;
+ let savedPlanId: number;
+
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the schedule and weeklyGoal persistence into a dedicated helper so `handleSavePlan` only orchestrates saving and invalidations instead of doing low-level DB work inline.
You can keep all functionality but reduce `handleSavePlan`’s responsibility by extracting the schedule/weeklyGoal logic into a helper. That keeps the hook focused on “save plan” orchestration instead of doing low-level DB + settings work inline.
For example, move the schedule + weeklyGoal logic into a small helper (either in this file or a `utils/database` module):
```ts
// utils/database/planSchedule.ts (new small module)
import { openDatabase, savePlanSchedule, updateSettings } from "@/utils/database";
export type PlanScheduleMap = Record<string, number>;
export async function persistPlanScheduleAndWeeklyGoal(
planId: number,
planSchedule: PlanScheduleMap,
) {
const db = await openDatabase("userData.db");
if (Object.keys(planSchedule).length === 0) {
await savePlanSchedule(planId, []);
return { entriesCount: 0 };
}
const rows = await db.getAllAsync<{ id: number }>(
`SELECT id FROM user_workouts
WHERE plan_id = ? AND is_deleted = FALSE
ORDER BY workout_order ASC`,
[planId],
);
const workoutIds = rows.map((r) => r.id);
const entries = Object.entries(planSchedule)
.map(([day, idx]) => ({
day_of_week: Number(day),
workout_id: workoutIds[idx],
}))
.filter((e) => e.workout_id != null);
await savePlanSchedule(planId, entries);
const activePlan = await db.getFirstAsync<{ id: number }>(
`SELECT id FROM user_plans WHERE is_active = TRUE LIMIT 1`,
);
if (activePlan?.id === planId) {
await updateSettings("weeklyGoal", String(entries.length));
}
return { entriesCount: entries.length };
}
```
Then `handleSavePlan` becomes simpler and easier to follow:
```ts
import { persistPlanScheduleAndWeeklyGoal } from "@/utils/database/planSchedule";
const handleSavePlan = async (planId: number | null, appPlanId?: number | null) => {
// ...validation, insert/update plan as you already do...
const savedPlanId = newPlanId ?? planId!;
try {
const { entriesCount } = await persistPlanScheduleAndWeeklyGoal(
savedPlanId,
planSchedule,
);
queryClient.invalidateQueries({ queryKey: ["planSchedule", savedPlanId] });
if (entriesCount > 0) {
queryClient.invalidateQueries({ queryKey: ["settings"] });
}
} catch (scheduleError: any) {
console.error("Error saving plan schedule:", scheduleError);
Bugsnag.notify(scheduleError);
}
setPlanSaved(true);
return newPlanId ?? undefined;
};
```
This keeps the same behavior (including weeklyGoal syncing and schedule clearing) but:
- Removes direct `openDatabase`/raw SQL from the hook.
- Collapses the schedule/weeklyGoal control flow into one testable helper.
- Leaves `useCreatePlan` primarily orchestrating “save plan” + query invalidations.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -38,12 +50,61 @@ export const useCreatePlan = (existingPlan?: Plan) => { | |||
|
|
|||
| try { | |||
| let newPlanId: number | null = null; | |||
There was a problem hiding this comment.
issue (complexity): Consider extracting the schedule and weeklyGoal persistence into a dedicated helper so handleSavePlan only orchestrates saving and invalidations instead of doing low-level DB work inline.
You can keep all functionality but reduce handleSavePlan’s responsibility by extracting the schedule/weeklyGoal logic into a helper. That keeps the hook focused on “save plan” orchestration instead of doing low-level DB + settings work inline.
For example, move the schedule + weeklyGoal logic into a small helper (either in this file or a utils/database module):
// utils/database/planSchedule.ts (new small module)
import { openDatabase, savePlanSchedule, updateSettings } from "@/utils/database";
export type PlanScheduleMap = Record<string, number>;
export async function persistPlanScheduleAndWeeklyGoal(
planId: number,
planSchedule: PlanScheduleMap,
) {
const db = await openDatabase("userData.db");
if (Object.keys(planSchedule).length === 0) {
await savePlanSchedule(planId, []);
return { entriesCount: 0 };
}
const rows = await db.getAllAsync<{ id: number }>(
`SELECT id FROM user_workouts
WHERE plan_id = ? AND is_deleted = FALSE
ORDER BY workout_order ASC`,
[planId],
);
const workoutIds = rows.map((r) => r.id);
const entries = Object.entries(planSchedule)
.map(([day, idx]) => ({
day_of_week: Number(day),
workout_id: workoutIds[idx],
}))
.filter((e) => e.workout_id != null);
await savePlanSchedule(planId, entries);
const activePlan = await db.getFirstAsync<{ id: number }>(
`SELECT id FROM user_plans WHERE is_active = TRUE LIMIT 1`,
);
if (activePlan?.id === planId) {
await updateSettings("weeklyGoal", String(entries.length));
}
return { entriesCount: entries.length };
}Then handleSavePlan becomes simpler and easier to follow:
import { persistPlanScheduleAndWeeklyGoal } from "@/utils/database/planSchedule";
const handleSavePlan = async (planId: number | null, appPlanId?: number | null) => {
// ...validation, insert/update plan as you already do...
const savedPlanId = newPlanId ?? planId!;
try {
const { entriesCount } = await persistPlanScheduleAndWeeklyGoal(
savedPlanId,
planSchedule,
);
queryClient.invalidateQueries({ queryKey: ["planSchedule", savedPlanId] });
if (entriesCount > 0) {
queryClient.invalidateQueries({ queryKey: ["settings"] });
}
} catch (scheduleError: any) {
console.error("Error saving plan schedule:", scheduleError);
Bugsnag.notify(scheduleError);
}
setPlanSaved(true);
return newPlanId ?? undefined;
};This keeps the same behavior (including weeklyGoal syncing and schedule clearing) but:
- Removes direct
openDatabase/raw SQL from the hook. - Collapses the schedule/weeklyGoal control flow into one testable helper.
- Leaves
useCreatePlanprimarily orchestrating “save plan” + query invalidations.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hooks/useCreatePlan.ts (1)
110-121:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
finallyblock never runs the cleanup on success — stale closure values forisError/planSaved.
setIsErrorandsetPlanSavedqueue React state updates; they do not mutate the local variables captured by this async function. By the timefinallyexecutes,planSavedis stillfalse(its value whenhandleSavePlanwas called), soclearWorkouts(), the newclearPlanSchedule()and theplans/activePlaninvalidations are all skipped on the happy path. The screen-level effect increate.tsxonly callsclearPlanSchedule()from thebeforeRemovelistener, so after a successful save the editor’s schedule state lingers in the Zustand store until the user navigates back.The new
clearPlanSchedule()call now exacerbates the pre-existing bug.🔧 Suggested fix
Track success/error with locals instead of state:
const handleSavePlan = async ( planId: number | null, appPlanId?: number | null, ): Promise<number | void> => { + let saved = false; + let errored = false; ... try { ... setPlanSaved(true); + saved = true; return newPlanId ?? undefined; } catch (error: any) { console.error("Error inserting/updating plan data:", error); Bugsnag.notify(error); setIsError(true); + errored = true; } finally { - if (!isError && planSaved) { + if (!errored && saved) { clearWorkouts(); clearPlanSchedule(); queryClient.invalidateQueries({ queryKey: ["plans"] }); queryClient.invalidateQueries({ queryKey: ["activePlan"] }); } } };🤖 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 `@hooks/useCreatePlan.ts` around lines 110 - 121, The finally block uses React state variables (isError, planSaved) which are stale due to async setState; change handleSavePlan to track success with local booleans (e.g., let localError = false, let localPlanSaved = false) and set those inside try/catch rather than relying on setIsError/setPlanSaved, then use localPlanSaved && !localError in the finally to call clearWorkouts(), clearPlanSchedule() and queryClient.invalidateQueries(["plans"]) / invalidateQueries(["activePlan"]); still call the React setters for UI state, but base the cleanup logic on the local flags to ensure cleanup runs on success.
🧹 Nitpick comments (15)
components/WorkoutCard.tsx (1)
172-177: ⚡ Quick winConsider adding an accessibility label.
The IconButton serving as the menu anchor lacks an
accessibilityLabelprop, making it difficult for screen reader users to understand its purpose.♿ Proposed fix to improve accessibility
<IconButton icon="dots-vertical" size={24} onPress={() => openMenu(item.exercise_id)} iconColor={Colors.dark.text} + accessibilityLabel="Exercise options menu" />🤖 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/WorkoutCard.tsx` around lines 172 - 177, The IconButton in WorkoutCard that calls openMenu(item.exercise_id) is missing an accessibilityLabel; update the IconButton JSX to include an accessibilityLabel (and optionally accessibilityHint) that clearly describes its action (e.g., "Open exercise menu" or "Open menu for exercise {exercise name/id}") so screen readers understand its purpose—add the prop to the IconButton instance that uses openMenu and use item properties (exercise_id or exercise name) to make the label specific.components/TimeInput.tsx (2)
20-24: ⚡ Quick winPass an explicit radix to
parseInt.
parseInthere is invoked without a radix at lines 21, 22, 61, and 66. While modern engines default to base 10 for non-0xinputs, omitting the radix is flagged by the standardradixESLint rule and is fragile if input ever contains leading zeros or whitespace prefixes.♻️ Proposed fix
function emitValue(minutes: string, seconds: string): string { - const m = parseInt(minutes || "0"); - const s = parseInt(seconds || "0"); + const m = parseInt(minutes || "0", 10); + const s = parseInt(seconds || "0", 10); return `${m}:${String(s).padStart(2, "0")}`; }const handleSecondsBlur = () => { // Clamp and pad on blur, before unblocking external sync - const s = Math.min(parseInt(seconds || "0"), 59); + const s = Math.min(parseInt(seconds || "0", 10), 59); if (seconds.length === 1) { const padded = seconds.padStart(2, "0"); setSeconds(padded); onChange(emitValue(minutes, padded)); - } else if (s < parseInt(seconds || "0")) { + } else if (s < parseInt(seconds || "0", 10)) { const clamped = String(s);Also applies to: 59-70
🤖 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/TimeInput.tsx` around lines 20 - 24, The parseInt calls in TimeInput.tsx (notably inside emitValue and the input parsing handlers that convert minutes/seconds strings to numbers) are missing an explicit radix; update each parseInt(minutes || "0") and parseInt(seconds || "0") (and any other parseInt usages in this component) to pass radix 10 (e.g., parseInt(..., 10)) so numeric parsing is deterministic for decimals; ensure you update every occurrence referenced in the component (emitValue and the input change/submit handlers) to include the radix.
9-9: 💤 Low valueTighten the typings for
styleandsecondsRef.
style?: anydefeats type checking at the call sites, anduseRef<{ focus: () => void }>(null)re-declares a structural subset ofTextInput. Both can be expressed precisely with the types already exported byreact-native.♻️ Proposed fix
-import { StyleSheet, Text, TextInput, View } from "react-native"; +import { + StyleProp, + StyleSheet, + Text, + TextInput, + TextStyle, + View, +} from "react-native";interface TimeInputProps { value: string; onChange: (newValue: string) => void; - style?: any; + style?: StyleProp<TextStyle>; }- const secondsRef = useRef<{ focus: () => void }>(null); + const secondsRef = useRef<TextInput>(null);Also applies to: 30-30
🤖 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/TimeInput.tsx` at line 9, The props on TimeInput are too loose: replace style?: any with a proper react-native StyleProp type (e.g. style?: StyleProp<TextStyle> or StyleProp<ViewStyle> depending on whether the component styles a TextInput or wrapping View) by importing StyleProp and TextStyle/ViewStyle from 'react-native', and change secondsRef's type from useRef<{ focus: () => void }>(null) to a proper TextInput ref type such as useRef<TextInput | null>(null) or React.RefObject<TextInput> (import TextInput from 'react-native') so callers get accurate typings and IDE support.components/FilterRow.tsx (1)
200-255: ⚡ Quick winConsider extracting the ItemSeparatorComponent to eliminate duplication.
The
ItemSeparatorComponentwith identical inline styling is repeated three times. Extracting it improves maintainability and ensures consistency.♻️ Proposed refactor
Define the separator component once before the return statement:
+ const renderSeparator = () => ( + <View style={styles.separator} /> + ); + return ( <View style={styles.row}>Then reference it in each dropdown:
flatListProps={{ nestedScrollEnabled: true, - ItemSeparatorComponent: () => ( - <View - style={{ height: 1, backgroundColor: Colors.dark.subText }} - /> - ), + ItemSeparatorComponent: renderSeparator, }}Add the style to the StyleSheet:
checkmark: { marginLeft: 10, }, + separator: { + height: 1, + backgroundColor: Colors.dark.subText, + }, });🤖 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/FilterRow.tsx` around lines 200 - 255, Extract the repeated inline ItemSeparatorComponent into a single reusable component (e.g., ItemSeparator) defined in FilterRow.tsx before the component's return, replace the three inline ItemSeparatorComponent anonymous functions in each Dropdown's flatListProps with that ItemSeparator, and add a new style entry (e.g., itemSeparator) to the existing StyleSheet containing height: 1 and backgroundColor: Colors.dark.subText so all Dropdowns reference the same styled separator.components/SettingsModal.tsx (1)
154-196: ⚡ Quick winConsider extracting
ItemSeparatorComponentandrenderItemfor better performance and readability.The inline definitions of
ItemSeparatorComponentandrenderItemare recreated on every render. Extracting them outside the component or usinguseCallbackprevents unnecessary re-renders of dropdown items.♻️ Proposed refactor
Define the separator and item renderer before the return statement:
+ const renderSeparator = () => ( + <View style={styles.dropdownSeparator} /> + ); + + const renderDropdownItem = (item: { label: string; value: string }) => ( + <View + style={[ + styles.dropdownItem, + item.value === inputValue && styles.dropdownItemSelected, + ]} + > + <Text style={styles.dropdownItemText}>{item.label}</Text> + {item.value === inputValue && ( + <MaterialCommunityIcons + name="check" + size={18} + color={Colors.dark.text} + /> + )} + </View> + ); + return (Then reference them in the Dropdown:
flatListProps={{ - ItemSeparatorComponent: () => ( - <View - style={{ - height: 1, - backgroundColor: Colors.dark.subText, - }} - /> - ), + ItemSeparatorComponent: renderSeparator, }} - renderItem={(item) => ( - <View - style={[ - styles.dropdownItem, - item.value === inputValue && - styles.dropdownItemSelected, - ]} - > - <Text style={styles.dropdownItemText}> - {item.label} - </Text> - {item.value === inputValue && ( - <MaterialCommunityIcons - name="check" - size={18} - color={Colors.dark.text} - /> - )} - </View> - )} + renderItem={renderDropdownItem}Add the separator style to StyleSheet:
dropdownItemText: { color: Colors.dark.text, fontSize: 16, }, + dropdownSeparator: { + height: 1, + backgroundColor: Colors.dark.subText, + }, });🤖 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/SettingsModal.tsx` around lines 154 - 196, The inline ItemSeparatorComponent and renderItem passed into the Dropdown are recreated on every render; extract them (e.g., const renderSeparator = () => <View ... /> and const renderDropdownItem = useCallback((item) => { ... }, [inputValue]) or move them outside the component) and reference those functions in the Dropdown props (ItemSeparatorComponent and renderItem) to avoid re-creating closures each render; keep usage of inputValue and onChangeValue in the render callback's dependency list and add any new separator style to the StyleSheet (refer to Dropdown, ItemSeparatorComponent, renderItem, inputValue, onChangeValue, styles.dropdownItemSelected).components/SessionSetInfo.tsx (1)
98-122: 💤 Low valueConsider the race condition mitigation approach.
The 50ms
setTimeouton line 119 is a timing-based workaround to prevent theonPresshandler from firing after a hold gesture. Whilst this pattern works in practice, it relies on assumptions about event timing and could theoretically fail if React's event processing is delayed or batching changes.A more robust approach would be to track whether the initial timeout fired (indicating a hold started) and skip the
onPressbased on that flag rather than introducing an arbitrary delay.♻️ Alternative implementation without timing dependency
const pressTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null); const pressIntervalRef = useRef<ReturnType<typeof setInterval> | null>(null); - const isHoldingRef = useRef(false); + const didHoldRef = useRef(false); const startContinuousPress = useCallback((handler: () => void) => { pressTimerRef.current = setTimeout(() => { - isHoldingRef.current = true; + didHoldRef.current = true; handler(); pressIntervalRef.current = setInterval(handler, 150); }, 400); }, []); const stopContinuousPress = useCallback(() => { if (pressTimerRef.current !== null) { clearTimeout(pressTimerRef.current); pressTimerRef.current = null; } if (pressIntervalRef.current !== null) { clearInterval(pressIntervalRef.current); pressIntervalRef.current = null; } - setTimeout(() => { - isHoldingRef.current = false; - }, 50); }, []); + + const resetHoldFlag = useCallback(() => { + didHoldRef.current = false; + }, []); // Then in the IconButton: - onPress={() => { - if (!isHoldingRef.current) handleWeightChange(-weightIncrement); - }} + onPress={() => { + if (!didHoldRef.current) handleWeightChange(-weightIncrement); + resetHoldFlag(); + }} onPressIn={() => startContinuousPress(() => handleWeightChange(-weightIncrement)) } - onPressOut={stopContinuousPress} + onPressOut={() => { + stopContinuousPress(); + // Reset flag after a microtask to let onPress check the flag first + queueMicrotask(resetHoldFlag); + }}Note: This still uses
queueMicrotaskfor ordering, but makes the intent clearer.🤖 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/SessionSetInfo.tsx` around lines 98 - 122, The current 50ms setTimeout in stopContinuousPress is a fragile timing hack; instead make the hold state explicit by tracking whether the initial hold timeout fired and using that flag to suppress onPress. Concretely: in startContinuousPress set isHoldingRef.current = true at the start of the timeout (as you already do) and also set a new ref flag like holdStartedRef.current = true when the timeout runs; remove the 50ms delayed clear in stopContinuousPress and clear holdStartedRef.current and isHoldingRef.current synchronously when stopping (and clear timers in pressTimerRef/pressIntervalRef as you already do). Update the onPress handler to check holdStartedRef.current (or isHoldingRef.current) and return early if a hold was detected so you no longer rely on a timing delay to prevent onPress from firing.hooks/usePlanScheduleQuery.ts (1)
4-11: 💤 Low valueConsider using strict equality for clarity.
The hook implementation is correct, but consider using strict equality in the
enabledcheck for consistency and clarity:export const usePlanScheduleQuery = (planId: number | null) => { return useQuery<PlanScheduleEntry[]>({ queryKey: ["planSchedule", planId], queryFn: () => fetchPlanSchedule(planId!), - enabled: planId != null, + enabled: planId !== null, staleTime: 5 * 60 * 1000, }); };Whilst
!= nullcorrectly checks for bothnullandundefined, using!==is more explicit and aligns with modern TypeScript/JavaScript conventions. SinceplanIdis typed asnumber | null(notundefined), both are functionally equivalent here.🤖 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 `@hooks/usePlanScheduleQuery.ts` around lines 4 - 11, The enabled check in usePlanScheduleQuery should use strict equality for clarity: change the enabled condition from `planId != null` to `planId !== null` inside the useQuery call in usePlanScheduleQuery (keep the rest, including the existing `queryFn: () => fetchPlanSchedule(planId!)` and types, unchanged).utils/initAppCheck.ts (1)
5-25: ⚡ Quick winConsider adding error handling context.
The function currently lets errors propagate to the caller. Consider wrapping the initialisation in a try-catch block to provide more context about App Check setup failures:
export async function setupAppCheck(): Promise<void> { const extra = Constants.expoConfig?.extra; const isDevBuild = extra?.appVariant === "development"; const debugToken: string | undefined = extra?.appCheckDebugToken ?? undefined; + try { await initializeAppCheck(getApp(), { provider: { providerOptions: { android: { provider: isDevBuild ? "debug" : "playIntegrity", debugToken, }, apple: { provider: isDevBuild ? "debug" : "appAttest", debugToken, }, }, }, isTokenAutoRefreshEnabled: true, }); + } catch (error) { + throw new Error( + `Failed to initialize App Check: ${error instanceof Error ? error.message : String(error)}` + ); + } }This would make debugging initialisation failures easier whilst still propagating errors to the Bugsnag handler in
app/_layout.tsx.🤖 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 `@utils/initAppCheck.ts` around lines 5 - 25, Wrap the initializeAppCheck call inside setupAppCheck in a try-catch: call initializeAppCheck(getApp(), {...}) inside try, and in catch log contextual error information (e.g., console.error or a logger) like "Failed to initialize App Check" along with the caught error and then rethrow the error so it still propagates to the existing Bugsnag handler; keep function name setupAppCheck and the initializeAppCheck/getApp usage unchanged.hooks/useSetActivePlanMutation.ts (1)
12-22: ⚡ Quick winSurface the swallowed schedule-sync error to Bugsnag.
Catching but completely discarding
_errmeans a regression infetchPlanSchedule(or in the resultingupdateSettingscall) would silently leaveweeklyGoalout of sync with the newly-activated plan and you’d have no telemetry to diagnose it. Reporting without rethrowing keeps the non-critical UX while preserving observability.🔧 Suggested fix
+import Bugsnag from "@bugsnag/expo"; import { fetchPlanSchedule, updateActivePlan, updateSettings, } from "@/utils/database"; @@ - } catch (_err) { - // Non-critical: don't block UI if schedule fetch fails + } catch (err) { + // Non-critical: don't block UI if schedule fetch fails + Bugsnag.notify(err as Error); }🤖 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 `@hooks/useSetActivePlanMutation.ts` around lines 12 - 22, The catch block in the onSuccess handler for useSetActivePlanMutation swallows errors from fetchPlanSchedule/updateSettings; change it to log/report the caught error to your telemetry (e.g. Bugsnag) while still not rethrowing so UX remains non-blocking: capture the error in the catch (replace _err) and call your Bugsnag notify/report function with context (e.g. "fetchPlanSchedule/updateSettings failed in useSetActivePlanMutation", include planId and error), then continue to silently ignore the failure and leave queryClient.invalidateQueries as-is.hooks/useCreatePlan.ts (1)
53-62: ⚡ Quick winAvoid the non-null assertion on
newPlanId.
savedPlanId = newPlanId!will happily produceundefined(orNaNvia downstreamNumber()conversions) ifinsertWorkoutPlanever resolves without an id — for instance if its internal error handling decides to returnnull. The schedule write that follows uses this id, so a silent miss here would orphan or misroute schedule entries. Either narrow with an explicit guard or haveinsertWorkoutPlan’s return type rule outnull/undefined.🔧 Suggested fix
- if (appPlanId || !planId) { - newPlanId = await insertWorkoutPlan(planName, planImageUrl, workouts); - savedPlanId = newPlanId!; - } else { + if (appPlanId || !planId) { + newPlanId = await insertWorkoutPlan(planName, planImageUrl, workouts); + if (newPlanId == null) { + throw new Error("insertWorkoutPlan did not return a plan id"); + } + savedPlanId = newPlanId; + } else { await updateWorkoutPlan(planId, planName, planImageUrl, workouts); savedPlanId = planId; queryClient.invalidateQueries({ queryKey: ["plan", planId] }); }🤖 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 `@hooks/useCreatePlan.ts` around lines 53 - 62, The code uses a non-null assertion when assigning savedPlanId from newPlanId after calling insertWorkoutPlan; instead check the resolved value explicitly and handle the failure path: call insertWorkoutPlan(...) into newPlanId, if newPlanId is null/undefined throw or return an error (or surface it via setError) so you never assign an invalid id to savedPlanId, otherwise set savedPlanId = newPlanId; keep the updateWorkoutPlan branch as-is and continue to call queryClient.invalidateQueries(["plan", planId]) only when planId is valid.hooks/useCompletedWorkoutsQuery.ts (1)
57-57: 💤 Low valueConsider moving the "Quick Workout" fallback out of SQL.
The user-facing string is now duplicated in three SQL sites (here twice, plus
fetchCompletedWorkoutByIdper the PR summary). Pushing the COALESCE into the data layer makes future localisation or copy changes a multi-file sweep. ReturningNULLfrom SQL and resolving the label once in the mapping layer (e.g. insidefetchAndOrganize/fetchWorkoutHistoryForSession) keeps the display string in a single place.Also applies to: 209-209
🤖 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 `@hooks/useCompletedWorkoutsQuery.ts` at line 57, SQL currently uses COALESCE(user_workouts.name, 'Quick Workout') AS workout_name which hardcodes the display string in multiple query sites; remove the COALESCE so the query returns NULL for missing names and instead map NULL -> localized "Quick Workout" in the data/mapping layer (e.g. inside fetchAndOrganize or fetchWorkoutHistoryForSession), centralizing the fallback text (preferably as a single constant or i18n key) and update the other two SQL sites to match.app/(app)/(workout)/index.tsx (1)
395-462: ⚡ Quick winSave-modal dismiss/discard paths should also cancel pending rest notifications.
handleCancelWorkoutnow callscancelRestNotifications()before clearing state, but the equivalent exit paths from the quick-workout save modal (theonDismisshandler on Line 398 and the “Discard” button on Line 422) only callclearPersistedStore()and navigate away. A user who completes a quick workout, sees the save modal, and dismisses it can leave a scheduled rest notification queued. Consider extracting a small helper that doescancelRestNotifications+clearPersistedStore+ navigate, and reuse it across all three exit points.🤖 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)/index.tsx around lines 395 - 462, The dismiss/discard handlers in the save modal (the onDismiss callback and the Discard button) fail to call cancelRestNotifications(), so pending rest notifications can remain scheduled; extract a small helper (e.g., handleExitSaveModal or closeAndCancelRest) that calls cancelRestNotifications(), clearPersistedStore(), sets setShowSaveModal(false) and navigates via router.push("/(app)/(tabs)"), then replace the onDismiss, the Discard button onPress, and the successful Save completion path with calls to that helper to ensure all three exit points cancel rest notifications and clear state consistently.app/(app)/(create-plan)/create.tsx (1)
80-100: 💤 Low valueOptional: report schedule-load failure to Bugsnag.
For parity with the rest of the codebase, consider piping the silent
catchhere throughBugsnag.notifyso a regression infetchPlanScheduleis observable instead of just degrading the editor to an empty schedule.🤖 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)/(create-plan)/create.tsx around lines 80 - 100, The silent catch in the fetchPlanSchedule block suppresses failures; update the catch to notify Bugsnag with the caught error and context (e.g., include existingPlan.id and a descriptive message) so failures in fetchPlanSchedule are reported; modify the promise chain around fetchPlanSchedule(existingPlan.id).then(...).catch(...) to call Bugsnag.notify(err, {metaData: {planId: existingPlan.id}}) (or the project's equivalent Bugsnag API) while preserving the current non-fatal behavior and still leaving setPlanSchedule as-is when entries are absent.utils/database.ts (2)
756-765: ⚡ Quick winAdd error handling for consistency and observability.
This function lacks the try-catch wrapper with Bugsnag notification that most other database functions in this file use (e.g.,
fetchNote,saveNote,fetchCompletedWorkoutById,fetchPlanSchedule). WhilstrunAsyncwill throw on error, wrapping it ensures errors are logged to Bugsnag and the console for better observability.♻️ Proposed fix to add error handling
export const linkCompletedWorkoutToWorkout = async ( completedWorkoutId: number, workoutId: number, ): Promise<void> => { - const db = await openDatabase("userData.db"); - await db.runAsync( - `UPDATE completed_workouts SET workout_id = ? WHERE id = ?`, - [workoutId, completedWorkoutId], - ); + try { + const db = await openDatabase("userData.db"); + await db.runAsync( + `UPDATE completed_workouts SET workout_id = ? WHERE id = ?`, + [workoutId, completedWorkoutId], + ); + } catch (error: any) { + console.error("Error linking completed workout to workout:", error); + Bugsnag.notify(error); + throw error; + } };🤖 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 `@utils/database.ts` around lines 756 - 765, Wrap the body of linkCompletedWorkoutToWorkout in a try-catch: call openDatabase and await db.runAsync inside the try, and in the catch call Bugsnag.notify(error) and console.error with a descriptive message including completedWorkoutId and workoutId, then rethrow the error so callers still receive it; reference the existing openDatabase and db.runAsync usage and mirror the error-handling pattern used by fetchNote/saveNote/fetchCompletedWorkoutById.
1331-1345: ⚡ Quick winAdd error handling wrapper for consistency and observability.
Whilst
withExclusiveTransactionAsyncautomatically rolls back on error, the lack of an explicit try-catch wrapper means errors won't be logged to Bugsnag or the console. This is inconsistent withfetchPlanSchedule(lines 1315-1329) and other database functions in this file. Adding error handling improves observability and debugging.♻️ Proposed fix to add error handling
export const savePlanSchedule = async ( planId: number, entries: PlanScheduleEntry[], ): Promise<void> => { - const db = await openDatabase("userData.db"); - await db.withExclusiveTransactionAsync(async (txn) => { - await txn.runAsync(`DELETE FROM plan_schedule WHERE plan_id = ?`, [planId]); - for (const entry of entries) { - await txn.runAsync( - `INSERT INTO plan_schedule (plan_id, day_of_week, workout_id) VALUES (?, ?, ?)`, - [planId, entry.day_of_week, entry.workout_id], - ); - } - }); + try { + const db = await openDatabase("userData.db"); + await db.withExclusiveTransactionAsync(async (txn) => { + await txn.runAsync(`DELETE FROM plan_schedule WHERE plan_id = ?`, [planId]); + for (const entry of entries) { + await txn.runAsync( + `INSERT INTO plan_schedule (plan_id, day_of_week, workout_id) VALUES (?, ?, ?)`, + [planId, entry.day_of_week, entry.workout_id], + ); + } + }); + } catch (error: any) { + console.error("Error saving plan schedule:", error); + Bugsnag.notify(error); + throw error; + } };🤖 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 `@utils/database.ts` around lines 1331 - 1345, The savePlanSchedule function lacks an explicit try/catch for error logging; wrap the existing body of savePlanSchedule in a try/catch that catches any error thrown by openDatabase or db.withExclusiveTransactionAsync, call Bugsnag.notify(error) and console.error with a clear message including "savePlanSchedule" and the planId, then rethrow the error so callers see the failure; mirror the error-handling pattern used by fetchPlanSchedule to preserve consistency and observability.
🤖 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)/(tabs)/(stats)/index.tsx:
- Around line 99-104: The volumeUnit value currently uses "st" which conflicts
with the common "stone" abbreviation; update the logic that sets volumeUnit
(near symbols weightUnit, tonDivisor, formattedVolume, totalVolume) so that when
weightUnit === "lbs" it uses a clear short-ton label such as "tn" or "ton"
(e.g., set volumeUnit = weightUnit === "lbs" ? "tn" : "t") and keep the rest of
the formatting logic unchanged.
In `@components/TimeInput.tsx`:
- Around line 26-46: The minutes field is being truncated because the minutes
TextInput is limited to one char and handleMinutesChange slices to 1; update the
TimeInput component to allow two-digit minutes by changing the minutes input
maxLength to 2 and updating handleMinutesChange (and any .slice calls) to allow
up to 2 characters, while keeping the existing initialization logic in TimeInput
(parseValue, setMinutes) intact so values like "10" or "15" are preserved and
not lost when the user focuses/edits.
In `@utils/initAppCheck.ts`:
- Around line 10-24: The current initializeAppCheck call uses an invalid nested
providerOptions shape; instead create the RNFirebase provider instance via
appCheck.newReactNativeFirebaseAppCheckProvider, call its .configure() with
platform options (use android: provider = isDevBuild ? "debug" : "playIntegrity"
and apple: provider = isDevBuild ? "debug" : "appAttestWithDeviceCheckFallback",
passing debugToken), then pass that configured provider to
initializeAppCheck(getApp(), { provider: rnfbProvider,
isTokenAutoRefreshEnabled: true }); update references around initializeAppCheck,
appCheck.newReactNativeFirebaseAppCheckProvider, and .configure() accordingly.
In `@utils/initUserDataDB.ts`:
- Around line 156-165: The plan_schedule table creation in the db.execAsync call
should add ON DELETE CASCADE to both foreign key clauses to prevent orphaned
rows when a user_plan or user_workout is removed; update the FOREIGN KEY
(plan_id) REFERENCES user_plans(id) and FOREIGN KEY (workout_id) REFERENCES
user_workouts(id) in the CREATE TABLE statement inside initUserDataDB (the
db.execAsync block that creates plan_schedule) to include ON DELETE CASCADE for
each foreign key.
---
Outside diff comments:
In `@hooks/useCreatePlan.ts`:
- Around line 110-121: The finally block uses React state variables (isError,
planSaved) which are stale due to async setState; change handleSavePlan to track
success with local booleans (e.g., let localError = false, let localPlanSaved =
false) and set those inside try/catch rather than relying on
setIsError/setPlanSaved, then use localPlanSaved && !localError in the finally
to call clearWorkouts(), clearPlanSchedule() and
queryClient.invalidateQueries(["plans"]) / invalidateQueries(["activePlan"]);
still call the React setters for UI state, but base the cleanup logic on the
local flags to ensure cleanup runs on success.
---
Nitpick comments:
In `@app/`(app)/(create-plan)/create.tsx:
- Around line 80-100: The silent catch in the fetchPlanSchedule block suppresses
failures; update the catch to notify Bugsnag with the caught error and context
(e.g., include existingPlan.id and a descriptive message) so failures in
fetchPlanSchedule are reported; modify the promise chain around
fetchPlanSchedule(existingPlan.id).then(...).catch(...) to call
Bugsnag.notify(err, {metaData: {planId: existingPlan.id}}) (or the project's
equivalent Bugsnag API) while preserving the current non-fatal behavior and
still leaving setPlanSchedule as-is when entries are absent.
In `@app/`(app)/(workout)/index.tsx:
- Around line 395-462: The dismiss/discard handlers in the save modal (the
onDismiss callback and the Discard button) fail to call
cancelRestNotifications(), so pending rest notifications can remain scheduled;
extract a small helper (e.g., handleExitSaveModal or closeAndCancelRest) that
calls cancelRestNotifications(), clearPersistedStore(), sets
setShowSaveModal(false) and navigates via router.push("/(app)/(tabs)"), then
replace the onDismiss, the Discard button onPress, and the successful Save
completion path with calls to that helper to ensure all three exit points cancel
rest notifications and clear state consistently.
In `@components/FilterRow.tsx`:
- Around line 200-255: Extract the repeated inline ItemSeparatorComponent into a
single reusable component (e.g., ItemSeparator) defined in FilterRow.tsx before
the component's return, replace the three inline ItemSeparatorComponent
anonymous functions in each Dropdown's flatListProps with that ItemSeparator,
and add a new style entry (e.g., itemSeparator) to the existing StyleSheet
containing height: 1 and backgroundColor: Colors.dark.subText so all Dropdowns
reference the same styled separator.
In `@components/SessionSetInfo.tsx`:
- Around line 98-122: The current 50ms setTimeout in stopContinuousPress is a
fragile timing hack; instead make the hold state explicit by tracking whether
the initial hold timeout fired and using that flag to suppress onPress.
Concretely: in startContinuousPress set isHoldingRef.current = true at the start
of the timeout (as you already do) and also set a new ref flag like
holdStartedRef.current = true when the timeout runs; remove the 50ms delayed
clear in stopContinuousPress and clear holdStartedRef.current and
isHoldingRef.current synchronously when stopping (and clear timers in
pressTimerRef/pressIntervalRef as you already do). Update the onPress handler to
check holdStartedRef.current (or isHoldingRef.current) and return early if a
hold was detected so you no longer rely on a timing delay to prevent onPress
from firing.
In `@components/SettingsModal.tsx`:
- Around line 154-196: The inline ItemSeparatorComponent and renderItem passed
into the Dropdown are recreated on every render; extract them (e.g., const
renderSeparator = () => <View ... /> and const renderDropdownItem =
useCallback((item) => { ... }, [inputValue]) or move them outside the component)
and reference those functions in the Dropdown props (ItemSeparatorComponent and
renderItem) to avoid re-creating closures each render; keep usage of inputValue
and onChangeValue in the render callback's dependency list and add any new
separator style to the StyleSheet (refer to Dropdown, ItemSeparatorComponent,
renderItem, inputValue, onChangeValue, styles.dropdownItemSelected).
In `@components/TimeInput.tsx`:
- Around line 20-24: The parseInt calls in TimeInput.tsx (notably inside
emitValue and the input parsing handlers that convert minutes/seconds strings to
numbers) are missing an explicit radix; update each parseInt(minutes || "0") and
parseInt(seconds || "0") (and any other parseInt usages in this component) to
pass radix 10 (e.g., parseInt(..., 10)) so numeric parsing is deterministic for
decimals; ensure you update every occurrence referenced in the component
(emitValue and the input change/submit handlers) to include the radix.
- Line 9: The props on TimeInput are too loose: replace style?: any with a
proper react-native StyleProp type (e.g. style?: StyleProp<TextStyle> or
StyleProp<ViewStyle> depending on whether the component styles a TextInput or
wrapping View) by importing StyleProp and TextStyle/ViewStyle from
'react-native', and change secondsRef's type from useRef<{ focus: () => void
}>(null) to a proper TextInput ref type such as useRef<TextInput | null>(null)
or React.RefObject<TextInput> (import TextInput from 'react-native') so callers
get accurate typings and IDE support.
In `@components/WorkoutCard.tsx`:
- Around line 172-177: The IconButton in WorkoutCard that calls
openMenu(item.exercise_id) is missing an accessibilityLabel; update the
IconButton JSX to include an accessibilityLabel (and optionally
accessibilityHint) that clearly describes its action (e.g., "Open exercise menu"
or "Open menu for exercise {exercise name/id}") so screen readers understand its
purpose—add the prop to the IconButton instance that uses openMenu and use item
properties (exercise_id or exercise name) to make the label specific.
In `@hooks/useCompletedWorkoutsQuery.ts`:
- Line 57: SQL currently uses COALESCE(user_workouts.name, 'Quick Workout') AS
workout_name which hardcodes the display string in multiple query sites; remove
the COALESCE so the query returns NULL for missing names and instead map NULL ->
localized "Quick Workout" in the data/mapping layer (e.g. inside
fetchAndOrganize or fetchWorkoutHistoryForSession), centralizing the fallback
text (preferably as a single constant or i18n key) and update the other two SQL
sites to match.
In `@hooks/useCreatePlan.ts`:
- Around line 53-62: The code uses a non-null assertion when assigning
savedPlanId from newPlanId after calling insertWorkoutPlan; instead check the
resolved value explicitly and handle the failure path: call
insertWorkoutPlan(...) into newPlanId, if newPlanId is null/undefined throw or
return an error (or surface it via setError) so you never assign an invalid id
to savedPlanId, otherwise set savedPlanId = newPlanId; keep the
updateWorkoutPlan branch as-is and continue to call
queryClient.invalidateQueries(["plan", planId]) only when planId is valid.
In `@hooks/usePlanScheduleQuery.ts`:
- Around line 4-11: The enabled check in usePlanScheduleQuery should use strict
equality for clarity: change the enabled condition from `planId != null` to
`planId !== null` inside the useQuery call in usePlanScheduleQuery (keep the
rest, including the existing `queryFn: () => fetchPlanSchedule(planId!)` and
types, unchanged).
In `@hooks/useSetActivePlanMutation.ts`:
- Around line 12-22: The catch block in the onSuccess handler for
useSetActivePlanMutation swallows errors from fetchPlanSchedule/updateSettings;
change it to log/report the caught error to your telemetry (e.g. Bugsnag) while
still not rethrowing so UX remains non-blocking: capture the error in the catch
(replace _err) and call your Bugsnag notify/report function with context (e.g.
"fetchPlanSchedule/updateSettings failed in useSetActivePlanMutation", include
planId and error), then continue to silently ignore the failure and leave
queryClient.invalidateQueries as-is.
In `@utils/database.ts`:
- Around line 756-765: Wrap the body of linkCompletedWorkoutToWorkout in a
try-catch: call openDatabase and await db.runAsync inside the try, and in the
catch call Bugsnag.notify(error) and console.error with a descriptive message
including completedWorkoutId and workoutId, then rethrow the error so callers
still receive it; reference the existing openDatabase and db.runAsync usage and
mirror the error-handling pattern used by
fetchNote/saveNote/fetchCompletedWorkoutById.
- Around line 1331-1345: The savePlanSchedule function lacks an explicit
try/catch for error logging; wrap the existing body of savePlanSchedule in a
try/catch that catches any error thrown by openDatabase or
db.withExclusiveTransactionAsync, call Bugsnag.notify(error) and console.error
with a clear message including "savePlanSchedule" and the planId, then rethrow
the error so callers see the failure; mirror the error-handling pattern used by
fetchPlanSchedule to preserve consistency and observability.
In `@utils/initAppCheck.ts`:
- Around line 5-25: Wrap the initializeAppCheck call inside setupAppCheck in a
try-catch: call initializeAppCheck(getApp(), {...}) inside try, and in catch log
contextual error information (e.g., console.error or a logger) like "Failed to
initialize App Check" along with the caught error and then rethrow the error so
it still propagates to the existing Bugsnag handler; keep function name
setupAppCheck and the initializeAppCheck/getApp usage unchanged.
🪄 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: 677e68fc-f151-49d0-971f-310db0010fd6
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (35)
.claude/settings.jsonapp.config.jsapp/(app)/(create-plan)/create.tsxapp/(app)/(create-plan)/exercises.tsxapp/(app)/(tabs)/(plans)/index.tsxapp/(app)/(tabs)/(plans)/overview.tsxapp/(app)/(tabs)/(stats)/exercises.tsxapp/(app)/(tabs)/(stats)/index.tsxapp/(app)/(tabs)/index.tsxapp/(app)/(tabs)/settings.tsxapp/(app)/(workout)/exercises.tsxapp/(app)/(workout)/index.tsxapp/(app)/(workout)/workout-session.tsxapp/_layout.tsxcomponents/EditSetModal.tsxcomponents/FilterRow.tsxcomponents/PlanScheduleEditor.tsxcomponents/RestDayCard.tsxcomponents/SessionSetInfo.tsxcomponents/SettingsModal.tsxcomponents/StandaloneWorkoutListItem.tsxcomponents/TimeInput.tsxcomponents/WhatsNewModal.tsxcomponents/WorkoutCard.tsxconstants/WhatsNew.tshooks/useCompletedWorkoutsQuery.tshooks/useCreatePlan.tshooks/usePlanScheduleQuery.tshooks/useSetActivePlanMutation.tspackage.jsonstore/activeWorkoutStore.tsstore/workoutStore.tsutils/database.tsutils/initAppCheck.tsutils/initUserDataDB.ts
💤 Files with no reviewable changes (2)
- .claude/settings.json
- components/EditSetModal.tsx
Comment on lines
+26
to
+46
| export const TimeInput = ({ value, onChange, style }: TimeInputProps) => { | ||
| const { minutes: initM, seconds: initS } = parseValue(value); | ||
| const [minutes, setMinutes] = useState(initM === "0" ? "" : initM); | ||
| const [seconds, setSeconds] = useState(initS === "00" ? "" : initS); | ||
| const secondsRef = useRef<{ focus: () => void }>(null); | ||
| const isEditingRef = useRef(false); | ||
| const blurTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null); | ||
|
|
||
| useEffect(() => { | ||
| return () => { | ||
| if (blurTimerRef.current) clearTimeout(blurTimerRef.current); | ||
| }; | ||
| }, []); | ||
|
|
||
| // Only sync external value when the user is not actively editing | ||
| useEffect(() => { | ||
| if (isEditingRef.current) return; | ||
| const { minutes: m, seconds: s } = parseValue(value); | ||
| setMinutes(m === "0" ? "" : m); | ||
| setSeconds(s === "00" ? "" : s); | ||
| }, [value]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find callers of TimeInput to determine the maximum minute values flowing in/out.
rg -nP -C4 '\bTimeInput\b' --type=ts --type=tsxRepository: isotronic/MuscleQuest
Length of output: 93
🏁 Script executed:
rg -nP -C4 'TimeInput' --type=tsRepository: isotronic/MuscleQuest
Length of output: 16019
🏁 Script executed:
# Also search without type restriction to catch tsx files
rg -nP -C4 'TimeInput' | head -100Repository: isotronic/MuscleQuest
Length of output: 6127
🏁 Script executed:
# Find the TimeInput component definition and check file structure
fd -i TimeInputRepository: isotronic/MuscleQuest
Length of output: 90
🏁 Script executed:
cat -n components/TimeInput.tsxRepository: isotronic/MuscleQuest
Length of output: 5111
🏁 Script executed:
# Also check the formatTimeInput implementation to understand what it supports
cat -n utils/utility.ts | head -60Repository: isotronic/MuscleQuest
Length of output: 2344
🏁 Script executed:
# Search for time-related constants, settings, or validation
rg -i "rest.*time|time.*limit|max.*minute" --type=ts --type=js -A 2 -B 2Repository: isotronic/MuscleQuest
Length of output: 20454
🏁 Script executed:
# Check SettingsModal to understand the expected range for rest time
sed -n '50,90p' components/SettingsModal.tsxRepository: isotronic/MuscleQuest
Length of output: 1383
Two-digit minutes are silently truncated to one digit when edited.
parseValue accepts any number of digits, and the initial state holds e.g. "10" if value is "10:30". However, the minutes TextInput has maxLength={1} and handleMinutesChange slices to a single digit with .slice(0, 1). As soon as the user focuses and edits the minutes field, the value collapses to a single digit—this is a data loss risk.
The app allows setting custom rest times with no documented upper limit. If a user sets a rest period to 15 minutes or more (realistic for extended recovery or warm-up phases), the next time they edit that field, it silently truncates to 0–9 minutes.
Proposed fix: allow 2-digit minutes
const handleMinutesChange = (text: string) => {
- const digit = text.replace(/[^0-9]/g, "").slice(0, 1);
- setMinutes(digit);
- onChange(emitValue(digit, seconds));
- if (digit.length >= 1) {
+ const digits = text.replace(/[^0-9]/g, "").slice(0, 2);
+ setMinutes(digits);
+ onChange(emitValue(digits, seconds));
+ if (digits.length >= 2) {
secondsRef.current?.focus();
}
};- maxLength={1}
+ maxLength={2}
style={[style, styles.minutesField]}🤖 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/TimeInput.tsx` around lines 26 - 46, The minutes field is being
truncated because the minutes TextInput is limited to one char and
handleMinutesChange slices to 1; update the TimeInput component to allow
two-digit minutes by changing the minutes input maxLength to 2 and updating
handleMinutesChange (and any .slice calls) to allow up to 2 characters, while
keeping the existing initialization logic in TimeInput (parseValue, setMinutes)
intact so values like "10" or "15" are preserved and not lost when the user
focuses/edits.
Owner
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- The continuous press/hold logic for weight and reps in
SessionSetInfois fairly involved; consider extracting it into a reusable hook (e.g.,useContinuousPress) so the component stays easier to read and you avoid duplicating timers/cleanup logic if you need similar behavior elsewhere. - The weekly goal ↔ plan schedule synchronization is implemented in both
useCreatePlananduseSetActivePlanMutationwith raw SQL calls; it may be worth centralizing this into a dedicated helper indatabase.tsso the behavior stays consistent and easier to evolve (e.g., if the schedule schema or weekly goal logic changes).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The continuous press/hold logic for weight and reps in `SessionSetInfo` is fairly involved; consider extracting it into a reusable hook (e.g., `useContinuousPress`) so the component stays easier to read and you avoid duplicating timers/cleanup logic if you need similar behavior elsewhere.
- The weekly goal ↔ plan schedule synchronization is implemented in both `useCreatePlan` and `useSetActivePlanMutation` with raw SQL calls; it may be worth centralizing this into a dedicated helper in `database.ts` so the behavior stays consistent and easier to evolve (e.g., if the schedule schema or weekly goal logic changes).
## Individual Comments
### Comment 1
<location path="app/(app)/(tabs)/index.tsx" line_range="131-140" />
<code_context>
+ const weeklyGoal = Number(settings?.weeklyGoal);
</code_context>
<issue_to_address>
**issue:** Guard against NaN weeklyGoal when computing per-workout targets
If `settings?.weeklyGoal` is missing or non-numeric, `Number(settings?.weeklyGoal)` becomes `NaN`, which then propagates through the calculations. Because `perWorkoutTarget` ends up storing `NaN`, `perWorkoutTarget.get(workout.id!) ?? 1` also yields `NaN` (not nullish) and `completedTimes >= target` is always false, breaking completion and UI when no weekly goal is set. Consider defaulting `weeklyGoal` (e.g. `Number(settings?.weeklyGoal ?? 0)`) and/or skipping the map write when the computed target is not finite, falling back to `1` instead.
</issue_to_address>
### Comment 2
<location path="store/workoutStore.ts" line_range="289" />
<code_context>
+ syncScheduleOnRemoveWorkout: (removedIndex: number) =>
</code_context>
<issue_to_address>
**issue (bug_risk):** Plan schedule is not kept in sync when workouts are reordered
`planSchedule` maps `day_of_week -> workout array index` and `syncScheduleOnRemoveWorkout` keeps it consistent only on deletion. When workouts are reordered via `reorderWorkouts`, these indices are left stale, so days can point to the wrong workouts (e.g., after save/reload or on the home screen). Please either update `planSchedule` inside `reorderWorkouts` to remap indices to the new order, or change the schedule to store workout IDs instead of indices and resolve them only when needed.
</issue_to_address>
### Comment 3
<location path="components/TimeInput.tsx" line_range="30" />
<code_context>
+ const { minutes: initM, seconds: initS } = parseValue(value);
+ const [minutes, setMinutes] = useState(initM === "0" ? "" : initM);
+ const [seconds, setSeconds] = useState(initS === "00" ? "" : initS);
+ const secondsRef = useRef<{ focus: () => void } | null>(null);
+ const isEditingRef = useRef(false);
+ const blurTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the TimeInput component by replacing the timer-based editing state and inline seconds logic with a focus-driven flag and pure helpers to keep synchronization and normalization easier to reason about.
You can simplify this without losing the two‑field UX or behavior.
### 1. Replace `isEditingRef` + `blurTimerRef` with focus state
You don’t need a timer; you just need “any field focused?” as a boolean. That removes the timeout, cleanup effect, and the ref logic.
```ts
export const TimeInput = ({ value, onChange, style }: TimeInputProps) => {
const { minutes: initM, seconds: initS } = parseValue(value);
const [minutes, setMinutes] = useState(initM === "0" ? "" : initM);
const [seconds, setSeconds] = useState(initS === "00" ? "" : initS);
const secondsRef = useRef<{ focus: () => void } | null>(null);
const [isFocused, setIsFocused] = useState(false);
useEffect(() => {
if (isFocused) return;
const { minutes: m, seconds: s } = parseValue(value);
setMinutes(m === "0" ? "" : m);
setSeconds(s === "00" ? "" : s);
}, [value, isFocused]);
const handleFocus = () => setIsFocused(true);
const handleMinutesBlur = () => {
// keep any minutes-specific onBlur work here if needed
setIsFocused(false);
};
const handleSecondsBlur = () => {
const normalized = normalizeSeconds(seconds);
if (normalized !== seconds) {
setSeconds(normalized);
onChange(emitValue(minutes, normalized));
}
setIsFocused(false);
};
// ...
};
```
This keeps the “block prop sync while editing” behavior but makes it purely focus‑driven and easier to reason about.
### 2. Extract seconds normalization into a pure helper
`handleSecondsBlur` currently mixes:
- parsing / clamping / padding
- deciding when to emit `onChange`
- editing state toggling
Move the parsing/clamping/padding out into a pure function:
```ts
function normalizeSeconds(raw: string): string {
const numeric = raw.replace(/[^0-9]/g, "");
if (!numeric) return "00";
const s = Math.min(parseInt(numeric, 10), 59);
return String(s).padStart(2, "0");
}
```
Then use it from both change and blur handlers:
```ts
const handleSecondsChange = (text: string) => {
const digits = text.replace(/[^0-9]/g, "").slice(0, 2);
setSeconds(digits);
onChange(emitValue(minutes, digits));
};
const handleSecondsBlur = () => {
const normalized = normalizeSeconds(seconds);
if (normalized !== seconds) {
setSeconds(normalized);
onChange(emitValue(minutes, normalized));
}
setIsFocused(false);
};
```
This removes branching from the component, makes the blur logic testable in isolation, and preserves the current clamping/padding behavior.
### 3. Keep the parsing effect focused on sync policy
You already have `parseValue` and `emitValue`. With `normalizeSeconds` in place and `isFocused` handling the gate, the sync effect only needs to:
- parse `value`
- translate `"0"`/`"00"` into empty fields
You’ve largely done this; after extracting helpers and dropping the timer, the effect becomes straightforward and single‑purpose, which makes future changes safer.
</issue_to_address>
### Comment 4
<location path="app/(app)/(tabs)/index.tsx" line_range="125" />
<code_context>
let completedWorkoutsThisPlanThisWeek: CompletedWorkout[] = [];
let workoutsToDisplay: Workout[] = [];
+ let isRestDay = false;
+ // Map of workoutId -> required completions per week
+ const perWorkoutTarget = new Map<number, number>();
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the new scheduling and per-workout target logic into small pure helpers (and optionally a hook) to simplify HomeScreen and make the behavior more testable.
You can reduce the new complexity without changing behavior by extracting the dense scheduling/target logic into small pure helpers and using them in `HomeScreen`.
### 1. Extract `perWorkoutTarget` + `isRestDay` computation
Right now this is inline in the component, mutating a `Map` and a boolean. You can move it into a pure helper to separate business logic from view wiring and make it testable.
```ts
// helpers/planTargets.ts
import { getDay } from "date-fns";
import type { Workout } from "@/store/workoutStore";
import type { PlanScheduleEntry } from "@/hooks/usePlanScheduleQuery";
export function computeWeeklyTargets(options: {
activePlan: { workouts: Workout[] };
weeklyGoal: number;
schedule?: PlanScheduleEntry[] | null;
today: Date;
}) {
const { activePlan, weeklyGoal, schedule, today } = options;
const perWorkoutTarget = new Map<number, number>();
let isRestDay = false;
const numWorkouts = activePlan.workouts.length;
// day_of_week: 0=Mon…6=Sun
const todayDow = (getDay(today) + 6) % 7;
if (schedule && schedule.length > 0) {
for (const entry of schedule) {
const current = perWorkoutTarget.get(entry.workout_id) ?? 0;
perWorkoutTarget.set(entry.workout_id, current + 1);
}
const todayEntry = schedule.find((e) => e.day_of_week === todayDow);
isRestDay = todayEntry == null;
} else {
activePlan.workouts.forEach((workout, i) => {
if (workout.id == null) return;
const base = Math.floor(weeklyGoal / numWorkouts);
const extra = i < weeklyGoal % numWorkouts ? 1 : 0;
perWorkoutTarget.set(workout.id, Math.max(1, base + extra));
});
}
return { perWorkoutTarget, isRestDay, todayDow };
}
```
Usage in `HomeScreen`:
```ts
let isRestDay = false;
let todayDow = (getDay(today) + 6) % 7;
const perWorkoutTarget = new Map<number, number>();
if (activePlan && settings) {
const weeklyGoal = Number(settings.weeklyGoal);
const result = computeWeeklyTargets({
activePlan,
weeklyGoal,
schedule: planScheduleEntries,
today,
});
isRestDay = result.isRestDay;
todayDow = result.todayDow;
result.perWorkoutTarget.forEach((v, k) => perWorkoutTarget.set(k, v));
// ...rest of the logic stays the same
}
```
This keeps behavior identical while cutting a big chunk of logic out of the component body.
### 2. Extract “prioritize scheduled workout” logic
The reordering logic for `uncompletedWorkouts` is also inline and imperative. A small utility makes it much easier to follow:
```ts
// helpers/planScheduling.ts
import type { Workout } from "@/store/workoutStore";
import type { PlanScheduleEntry } from "@/hooks/usePlanScheduleQuery";
export function prioritizeScheduledWorkout(options: {
uncompleted: Workout[];
schedule?: PlanScheduleEntry[] | null;
todayDow: number;
isRestDay: boolean;
}) {
const { uncompleted, schedule, todayDow, isRestDay } = options;
if (!schedule || schedule.length === 0) return uncompleted;
const result = [...uncompleted];
const pinWorkout = (workoutId: number | undefined) => {
if (!workoutId) return;
const idx = result.findIndex((w) => w.id === workoutId);
if (idx > 0) {
const [w] = result.splice(idx, 1);
result.unshift(w);
}
};
if (!isRestDay) {
const todayEntry = schedule.find((e) => e.day_of_week === todayDow);
pinWorkout(todayEntry?.workout_id);
} else {
for (let i = 1; i <= 6; i++) {
const nextDow = (todayDow + i) % 7;
const nextEntry = schedule.find((e) => e.day_of_week === nextDow);
if (nextEntry) {
pinWorkout(nextEntry.workout_id);
break;
}
}
}
return result;
}
```
Usage in `HomeScreen`:
```ts
// After filling uncompletedWorkouts/completedWorkoutsList
const orderedUncompleted =
prioritizeScheduledWorkout({
uncompleted: uncompletedWorkouts,
schedule: planScheduleEntries,
todayDow,
isRestDay,
});
workoutsToDisplay = [...orderedUncompleted, ...completedWorkoutsList];
```
### 3. Optional: small hook for summary data
If you see this pattern growing, you can wrap the above helpers in a hook (without moving rendering) to further isolate “plan week summary” logic, but even just the two helpers above substantially reduce the cognitive load in `HomeScreen` while preserving all behavior.
</issue_to_address>
### Comment 5
<location path="components/SessionSetInfo.tsx" line_range="98" />
<code_context>
const [menuVisible, setMenuVisible] = useState(false);
const [timeInput, setTimeInput] = useState(time);
+ const pressTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
+ const pressIntervalRef = useRef<ReturnType<typeof setInterval> | null>(null);
+ const isHoldingRef = useRef(false);
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the continuous-press logic into a reusable hook and dropping unused refs to simplify the component and eliminate duplicated handlers.
You can keep the continuous‑press behavior while cutting a lot of duplication and local state by extracting the “press state machine” into a small hook and removing unused refs.
### 1. Remove unused / redundant state
`isHoldingRef` is never read, only written, so it can be dropped. Cleanup can also just call `stopContinuousPress` instead of duplicating the logic.
```ts
// before
const pressTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
const pressIntervalRef = useRef<ReturnType<typeof setInterval> | null>(null);
const isHoldingRef = useRef(false);
const holdStartedRef = useRef(false);
// ...
useEffect(() => {
return () => {
if (pressTimerRef.current !== null) clearTimeout(pressTimerRef.current);
if (pressIntervalRef.current !== null)
clearInterval(pressIntervalRef.current);
};
}, []);
```
```ts
// after
const pressTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
const pressIntervalRef = useRef<ReturnType<typeof setInterval> | null>(null);
const holdStartedRef = useRef(false);
// ...
useEffect(() => () => stopContinuousPress(), [stopContinuousPress]);
```
### 2. Extract a reusable continuous‑press hook
You can encapsulate the refs, timers, and “ignore onPress after long‑press” logic into a hook used by all four buttons:
```ts
// in this file or a small helper module
function useContinuousPress(
action: () => void,
delay = 400,
interval = 150
) {
const pressTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
const pressIntervalRef = useRef<ReturnType<typeof setInterval> | null>(null);
const holdStartedRef = useRef(false);
const stopContinuousPress = useCallback(() => {
if (pressTimerRef.current !== null) {
clearTimeout(pressTimerRef.current);
pressTimerRef.current = null;
}
if (pressIntervalRef.current !== null) {
clearInterval(pressIntervalRef.current);
pressIntervalRef.current = null;
}
}, []);
const onPressIn = useCallback(() => {
holdStartedRef.current = false;
pressTimerRef.current = setTimeout(() => {
holdStartedRef.current = true;
action();
pressIntervalRef.current = setInterval(action, interval);
}, delay);
}, [action, delay, interval]);
const onPressOut = useCallback(() => {
stopContinuousPress();
}, [stopContinuousPress]);
const onPress = useCallback(() => {
if (holdStartedRef.current) {
holdStartedRef.current = false;
return; // ignore tap after long press, matches current behavior
}
action();
}, [action]);
useEffect(() => stopContinuousPress, [stopContinuousPress]);
return { onPress, onPressIn, onPressOut };
}
```
### 3. Use the hook for each button instead of duplicating handlers
Then wire each button via the hook, which removes the per‑button state/timer wiring and keeps `SessionSetInfo` focused on domain logic:
```tsx
// inside SessionSetInfo
const weightMinusPress = useContinuousPress(
() => handleWeightChange(-weightIncrement)
);
const weightPlusPress = useContinuousPress(
() => handleWeightChange(weightIncrement)
);
const repsMinusPress = useContinuousPress(
() => handleRepsChange(-1)
);
const repsPlusPress = useContinuousPress(
() => handleRepsChange(1)
);
// ...
<IconButton
icon="minus"
{...weightMinusPress}
size={buttonSize}
iconColor={Colors.dark.text}
style={styles.iconButton}
/>
<IconButton
icon="plus"
{...weightPlusPress}
size={buttonSize}
iconColor={Colors.dark.text}
style={styles.iconButton}
/>
// same pattern for reps +/- buttons
```
This keeps the behavior identical (including the tap vs long‑press distinction and timing) while noticeably reducing complexity and duplication in the main component.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…ets and prioritizing scheduled workouts
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/(app)/(tabs)/index.tsx (2)
336-359:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe rest-day restriction is cosmetic here.
Line 341 only changes the button style.
onPressstill starts the workout anddisableddoes not includeisRestDay, so users can still begin a scheduled workout on a rest day.Suggested fix
<Button mode={ !workoutInProgress && index === 0 && !weeklyGoalReached && !isRestDay ? "contained" : "outlined" } onPress={() => { - if (isStartingWorkout) return; + if (isStartingWorkout || isRestDay) return; confirmStartWorkout(setIsStartingWorkout, () => { useActiveWorkoutStore .getState() .setWorkout( JSON.parse(JSON.stringify(workout)), activePlan.id!, workout.id!, workout.name || `Workout ${index + 1}`, ); }); }} labelStyle={styles.smallButtonLabel} - disabled={isStartingWorkout} + disabled={isStartingWorkout || isRestDay} >🤖 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)/(tabs)/index.tsx around lines 336 - 359, The button currently only uses isRestDay to choose its visual mode but still allows starting the workout; update the Button so it cannot start workouts on rest days by adding isRestDay to the disabled prop (e.g., disabled={isStartingWorkout || isRestDay}) and adding a guard at the top of the onPress handler to return early when isRestDay is true (before calling confirmStartWorkout/useActiveWorkoutStore.getState().setWorkout); keep the existing visual mode logic but ensure behavior and disabled state are consistent with isRestDay.
347-355:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the stable plan index for the fallback workout name.
indexis the reordered display position, so an unnamed workout can be started asWorkout 1,Workout 2, etc. depending on the current schedule ordering.originalIndexkeeps that fallback consistent.Suggested fix
- workout.name || `Workout ${index + 1}`, + workout.name || + `Workout ${(originalIndex >= 0 ? originalIndex : index) + 1}`,🤖 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)/(tabs)/index.tsx around lines 347 - 355, The fallback workout name uses the reordered display index; update the call inside confirmStartWorkout that invokes useActiveWorkoutStore.getState().setWorkout to use originalIndex for a stable name: replace the expression workout.name || `Workout ${index + 1}` with workout.name || `Workout ${originalIndex + 1}` so unnamed workouts keep a consistent fallback across reordering (leave other arguments like activePlan.id!, workout.id! unchanged).
♻️ Duplicate comments (1)
components/TimeInput.tsx (1)
61-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMinutes are still truncated to one digit during edit
Line 62 (
.slice(0, 1)) plus Line 87 (maxLength={1}) still collapse minute input to a single digit, so values like10/15are lost when edited.Proposed minimal fix
- const digit = text.replace(/[^0-9]/g, "").slice(0, 1); - setMinutes(digit); - onChange(emitValue(digit, seconds)); - if (digit.length >= 1) { + const digits = text.replace(/[^0-9]/g, "").slice(0, 2); + setMinutes(digits); + onChange(emitValue(digits, seconds)); + if (digits.length >= 2) { secondsRef.current?.focus(); }- maxLength={1} + maxLength={2}Also applies to: 87-87
🤖 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/TimeInput.tsx` around lines 61 - 63, The minute input is being limited to one digit in the TimeInput component: update handleMinutesChange to allow up to two numeric characters (remove or change .slice(0, 1) to allow two chars) and adjust the input prop (remove or change maxLength={1}) so minutes like "10" or "15" are preserved; ensure setMinutes receives the full sanitized two-digit string and any related validation (e.g., clamp to 0-59) is applied where appropriate.
🧹 Nitpick comments (3)
hooks/useContinuousPress.ts (1)
29-36: ⚡ Quick winDefensively reset timers at the start of
onPressIn.On Line 29, a quick
stop()first would prevent overlapping timers if a re-entrant press sequence firesonPressIntwice beforeonPressOut, which can otherwise duplicate actions.Suggested diff
const onPressIn = useCallback(() => { + stop(); holdStartedRef.current = false; pressTimerRef.current = setTimeout(() => { holdStartedRef.current = true; action(); pressIntervalRef.current = setInterval(action, interval); }, delay); - }, [action, delay, interval]); + }, [action, delay, interval, stop]);🤖 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 `@hooks/useContinuousPress.ts` around lines 29 - 36, The onPressIn handler can start overlapping timers if invoked re-entrantly, so modify useContinuousPress's onPressIn to defensively call the existing stop() function at the very start to clear any existing pressTimerRef/pressIntervalRef and reset holdStartedRef before scheduling the new timeout; specifically, in the onPressIn callback (which sets holdStartedRef, pressTimerRef, and pressIntervalRef) invoke stop() first, then proceed with the existing setTimeout logic so duplicate actions are prevented.store/workoutStore.ts (2)
302-304: 💤 Low valueConsider adding validation to
setPlanSchedule.The current implementation accepts any
Record<number, number>without validation. Consider adding checks to ensure day keys are within 0–6 and workout indices are within the bounds of theworkoutsarray to prevent invalid state.🛡️ Optional validation example
- setPlanSchedule: (schedule) => set({ planSchedule: schedule }), + setPlanSchedule: (schedule) => + set((state) => { + const validated: Record<number, number> = {}; + for (const [day, idx] of Object.entries(schedule)) { + const dayNum = Number(day); + const idxNum = Number(idx); + if (dayNum >= 0 && dayNum <= 6 && idxNum >= 0 && idxNum < state.workouts.length) { + validated[dayNum] = idxNum; + } + } + return { planSchedule: validated }; + }),🤖 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/workoutStore.ts` around lines 302 - 304, setPlanSchedule currently accepts any Record<number, number> into planSchedule without checks; update the setPlanSchedule(schedule) implementation to validate keys and values before calling set: ensure each key is an integer 0–6 (weekday), each value is an integer within 0..workouts.length-1 (or null/undefined if you allow empty), and ignore or reject entries that fail validation (or log an error) rather than writing invalid state; reference the setPlanSchedule setter and the workouts array when performing these bounds and type checks so only valid day→workout-index mappings are stored.
305-315: ⚡ Quick winConsider integrating schedule sync with workout removal.
The current design requires callers to explicitly invoke
syncScheduleOnRemoveWorkout(index)when callingremoveWorkout(index). Whilst this follows a valid "primitive operations" pattern, it creates a maintenance risk—future callers might forget to sync the schedule, leading to stale indices.Consider one of these approaches:
- Have
removeWorkoutautomatically callsyncScheduleOnRemoveWorkoutinternally- Create a combined method
removeWorkoutWithScheduleSync(index)that performs both operations- Add clear documentation (JSDoc) on
removeWorkoutnoting the requirement♻️ Option 1: Auto-sync in removeWorkout
removeWorkout: (index) => set((state) => { + // Sync schedule first + const updatedSchedule: Record<number, number> = {}; + for (const [day, idx] of Object.entries(state.planSchedule)) { + const workoutIdx = Number(idx); + if (workoutIdx === index) continue; + updatedSchedule[Number(day)] = + workoutIdx > index ? workoutIdx - 1 : workoutIdx; + } + return { ...state, workouts: state.workouts.filter((_, i) => i !== index), + planSchedule: updatedSchedule, }; }),🤖 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/workoutStore.ts` around lines 305 - 315, Make removal atomic by having removeWorkout automatically adjust planSchedule: update the removeWorkout implementation so that after removing the workout at index (or within the same set update) you also update planSchedule the same way syncScheduleOnRemoveWorkout(removedIndex) does (drop days that referenced the removed index and decrement indices > removedIndex); update/remove the separate syncScheduleOnRemoveWorkout callers accordingly or keep it private, and ensure you operate on state.planSchedule and state.workouts in the same state mutation to avoid stale state inconsistencies.
🤖 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)/(tabs)/index.tsx:
- Around line 166-170: The code uses a fallback of 1 for missing schedule
entries which incorrectly marks unscheduled workouts as due; update the fallback
in the two spots (the sortedWorkouts.forEach block that reads const target =
perWorkoutTarget.get(workout.id!) ?? 1 and the similar logic around the other
block at lines ~285-296) to use ?? 0 instead so
perWorkoutTarget.get(workout.id!) defaults to 0 for unscheduled workouts; ensure
you update both occurrences (references: sortedWorkouts.forEach,
perWorkoutTarget, completedWorkoutCounts, workout.id) so unscheduled workouts
are not considered required.
- Around line 57-59: The code calls computeWeeklyTargets while
planScheduleEntries can be undefined during usePlanScheduleQuery() loading,
causing a temporary fallback to the no-schedule path; change the logic so the
component does not treat the plan as unscheduled while the query is loading: use
the hook's loading/defined value (planScheduleEntries !== undefined or the
query's isLoading flag) and defer calling computeWeeklyTargets (or render the
schedule-dependent UI) until planScheduleEntries is defined; update the
references around usePlanScheduleQuery(), computeWeeklyTargets(), and the blocks
noted (including the similar logic at the later 137-146 area) to guard against
undefined schedule data and only compute/apply the no-schedule path after the
query has resolved.
In `@utils/planHelpers.ts`:
- Around line 53-70: The current pinFirst(workoutId) is a no-op when the workout
is already at index 0, but the calling code stops searching after the first
match; change pinFirst to return a boolean indicating whether it actually moved
an item (true when splice+unshift happened, false otherwise), then update the
callers: for the non-rest-day branch, if todayEntry exists call
pinFirst(todayEntry.workout_id) and if it returns false continue scanning
subsequent days the same way as the rest-day loop; for the rest-day loop only
break when pinFirst(nextEntry.workout_id) returns true, otherwise keep iterating
up to 6 days. Ensure references to result, schedule, todayDow, isRestDay,
todayEntry and nextEntry are used to locate and modify the logic.
---
Outside diff comments:
In `@app/`(app)/(tabs)/index.tsx:
- Around line 336-359: The button currently only uses isRestDay to choose its
visual mode but still allows starting the workout; update the Button so it
cannot start workouts on rest days by adding isRestDay to the disabled prop
(e.g., disabled={isStartingWorkout || isRestDay}) and adding a guard at the top
of the onPress handler to return early when isRestDay is true (before calling
confirmStartWorkout/useActiveWorkoutStore.getState().setWorkout); keep the
existing visual mode logic but ensure behavior and disabled state are consistent
with isRestDay.
- Around line 347-355: The fallback workout name uses the reordered display
index; update the call inside confirmStartWorkout that invokes
useActiveWorkoutStore.getState().setWorkout to use originalIndex for a stable
name: replace the expression workout.name || `Workout ${index + 1}` with
workout.name || `Workout ${originalIndex + 1}` so unnamed workouts keep a
consistent fallback across reordering (leave other arguments like
activePlan.id!, workout.id! unchanged).
---
Duplicate comments:
In `@components/TimeInput.tsx`:
- Around line 61-63: The minute input is being limited to one digit in the
TimeInput component: update handleMinutesChange to allow up to two numeric
characters (remove or change .slice(0, 1) to allow two chars) and adjust the
input prop (remove or change maxLength={1}) so minutes like "10" or "15" are
preserved; ensure setMinutes receives the full sanitized two-digit string and
any related validation (e.g., clamp to 0-59) is applied where appropriate.
---
Nitpick comments:
In `@hooks/useContinuousPress.ts`:
- Around line 29-36: The onPressIn handler can start overlapping timers if
invoked re-entrantly, so modify useContinuousPress's onPressIn to defensively
call the existing stop() function at the very start to clear any existing
pressTimerRef/pressIntervalRef and reset holdStartedRef before scheduling the
new timeout; specifically, in the onPressIn callback (which sets holdStartedRef,
pressTimerRef, and pressIntervalRef) invoke stop() first, then proceed with the
existing setTimeout logic so duplicate actions are prevented.
In `@store/workoutStore.ts`:
- Around line 302-304: setPlanSchedule currently accepts any Record<number,
number> into planSchedule without checks; update the setPlanSchedule(schedule)
implementation to validate keys and values before calling set: ensure each key
is an integer 0–6 (weekday), each value is an integer within
0..workouts.length-1 (or null/undefined if you allow empty), and ignore or
reject entries that fail validation (or log an error) rather than writing
invalid state; reference the setPlanSchedule setter and the workouts array when
performing these bounds and type checks so only valid day→workout-index mappings
are stored.
- Around line 305-315: Make removal atomic by having removeWorkout automatically
adjust planSchedule: update the removeWorkout implementation so that after
removing the workout at index (or within the same set update) you also update
planSchedule the same way syncScheduleOnRemoveWorkout(removedIndex) does (drop
days that referenced the removed index and decrement indices > removedIndex);
update/remove the separate syncScheduleOnRemoveWorkout callers accordingly or
keep it private, and ensure you operate on state.planSchedule and state.workouts
in the same state mutation to avoid stale state inconsistencies.
🪄 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: 27619cac-4d29-415c-849e-0d55bf8d8ce7
📒 Files selected for processing (6)
app/(app)/(tabs)/index.tsxcomponents/SessionSetInfo.tsxcomponents/TimeInput.tsxhooks/useContinuousPress.tsstore/workoutStore.tsutils/planHelpers.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/(app)/(create-plan)/create.tsx (1)
79-104: ⚡ Quick winConsider adding cleanup for the async schedule fetch.
The
fetchPlanSchedulepromise isn't cancelled if the component unmounts or ifexistingPlanchanges whilst the fetch is pending. This could result in callingsetPlanSchedulewith stale data or on an unmounted component.Whilst React 19 handles this gracefully, it's still best practice to clean up async operations.
🔄 Suggested refactor using an abort flag
// Load existing schedule and convert workout_id → array index if (existingPlan.id) { + let isMounted = true; fetchPlanSchedule(existingPlan.id) .then((entries) => { - if (entries.length > 0 && existingPlan.workouts) { + if (isMounted && entries.length > 0 && existingPlan.workouts) { const schedule: Record<number, number> = {}; for (const entry of entries) { const idx = existingPlan.workouts.findIndex( (w) => w.id === entry.workout_id, ); if (idx !== -1) { schedule[entry.day_of_week] = idx; } } setPlanSchedule(schedule); } }) .catch((err) => { - Bugsnag.notify(err, (event) => { - event.addMetadata("fetchPlanSchedule", { - planId: existingPlan.id, - }); - }); + if (isMounted) { + Bugsnag.notify(err, (event) => { + event.addMetadata("fetchPlanSchedule", { + planId: existingPlan.id, + }); + }); + } // Non-critical: schedule defaults to empty }); + return () => { + isMounted = false; + }; }🤖 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)/(create-plan)/create.tsx around lines 79 - 104, The async fetchPlanSchedule call can update state after unmount or when existingPlan changes; wrap the fetch in the same useEffect and add a cancellation guard (e.g., a local "cancelled" boolean or an AbortController if fetchPlanSchedule supports it) and check that guard before calling setPlanSchedule or Bugsnag.notify; also return a cleanup function from the useEffect that flips the cancelled flag (or aborts the controller) so pending promises don't update state or log with stale context for existingPlan.id.
🤖 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 `@app/`(app)/(create-plan)/create.tsx:
- Around line 79-104: The async fetchPlanSchedule call can update state after
unmount or when existingPlan changes; wrap the fetch in the same useEffect and
add a cancellation guard (e.g., a local "cancelled" boolean or an
AbortController if fetchPlanSchedule supports it) and check that guard before
calling setPlanSchedule or Bugsnag.notify; also return a cleanup function from
the useEffect that flips the cancelled flag (or aborts the controller) so
pending promises don't update state or log with stale context for
existingPlan.id.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 16e344bd-c48f-4d67-9eb9-0ee57466024d
📒 Files selected for processing (6)
app/(app)/(create-plan)/create.tsxapp/(app)/(tabs)/index.tsxcomponents/TimeInput.tsxhooks/useContinuousPress.tsstore/workoutStore.tsutils/planHelpers.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- utils/planHelpers.ts
- store/workoutStore.ts
- hooks/useContinuousPress.ts
- components/TimeInput.tsx
- app/(app)/(tabs)/index.tsx
…Targets functions
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
app/(app)/(tabs)/settings.tsx (1)
354-354: ⚡ Quick winConfirm weekly goal slider range defaults are appropriate.
The weekly goal slider doesn't specify
sliderMin,sliderMax, orsliderStepprops, so it relies on the defaults fromSettingsModal(1, 7, 1). Whilst 1-7 days per week is a reasonable range, this implicit dependency isn't documented. If the defaults inSettingsModalchange, this setting's behaviour would change unexpectedly.Consider explicitly passing the slider configuration for clarity:
- showOverlay("weeklyGoal", settings?.weeklyGoal || "1", "slider") + showOverlay("weeklyGoal", settings?.weeklyGoal || "1", "slider")Or update the
SettingsModalcall to accept these props:<SettingsModal // ... existing props sliderMin={1} sliderMax={7} sliderStep={1} />Actually, I notice
SettingsModalon line 310 doesn't pass these props. You may want to extend the modal invocation to support per-setting slider configuration.🤖 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)/(tabs)/settings.tsx at line 354, The weekly goal slider call currently relies on implicit defaults (showOverlay("weeklyGoal", settings?.weeklyGoal || "1", "slider")) which couples this usage to SettingsModal's defaults; explicitly pass the slider configuration (sliderMin/sliderMax/sliderStep) so behavior is obvious and stable, either by changing the showOverlay invocation for "weeklyGoal" to include those three values or by extending SettingsModal (and its showOverlay consumer) to accept per-setting slider props and then supply sliderMin=1, sliderMax=7, sliderStep=1 for the weeklyGoal setting; update any related prop types/signatures for SettingsModal and the showOverlay handler so the modal reads and uses these props instead of relying on internal defaults.components/SettingsModal.tsx (1)
75-77: ⚡ Quick winConsider validating slider configuration.
The default values for
sliderMin,sliderMax, andsliderStepare defined, but there's no validation thatsliderMin < sliderMaxor thatstepis positive. Invalid configurations could result in confusing UI behaviour.🛡️ Proposed validation
Add a validation check after the props destructuring:
}) => { const [timeInput, setTimeInput] = useState(""); + + // Validate slider configuration + if (settingType === "slider") { + if (sliderMin >= sliderMax) { + console.error("Invalid slider config: sliderMin must be less than sliderMax"); + } + if (sliderStep <= 0) { + console.error("Invalid slider config: sliderStep must be positive"); + } + } // Initialize timeInput when modal opens🤖 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/SettingsModal.tsx` around lines 75 - 77, Validate the slider props in the SettingsModal component after props destructuring: check that sliderMin < sliderMax and sliderStep > 0 (and optionally that (sliderMax - sliderMin) is divisible or compatible with sliderStep), and if the checks fail either clamp to safe defaults (e.g., swap min/max or set step = 1) or throw/log a clear error so the UI isn't given invalid values; reference the sliderMin, sliderMax, sliderStep symbols and perform this guard inside the SettingsModal function initialization before using them to render the slider.
🤖 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.config.js`:
- Around line 110-115: The navigation bar config for the "expo-navigation-bar"
plugin uses barStyle: "dark" with a dark backgroundColor "#22222d", causing poor
contrast; update the plugin configuration by setting barStyle to "light" (leave
enforceContrast and backgroundColor as-is) so navigation buttons use light icons
on the dark background—locate the "expo-navigation-bar" config block and change
the barStyle value accordingly.
In `@components/SettingsModal.tsx`:
- Around line 20-31: The current code defines a custom SliderProps interface and
force-casts SliderLib via "const Slider = SliderLib as unknown as
React.ComponentType<SliderProps>"; remove that custom interface and the double
type assertion and instead import the component and its types directly from the
package by replacing the SliderLib import and the custom SliderProps/interface
with a direct import like "import Slider, { SliderProps } from
'@react-native-community/slider'" and use Slider normally in the file (remove
the const Slider assignment and redundant type declaration).
In `@components/WorkoutDoneCard.tsx`:
- Line 52: WorkoutDoneCard currently hardcodes Colors.dark.* for the icon,
border and subtitle (references in the JSX around the Icon, container border and
ThemedText subtitle) which breaks light mode; update the component to derive
theme-aware colours instead (e.g., call useColorScheme() or useThemeColor() at
the top of WorkoutDoneCard, get colors = Colors[theme] or
useThemeColor('completed'/'icon'), and replace Colors.dark.completed and
Colors.dark.icon with the theme-derived values for the Icon color prop, the
container borderColor, and the subtitle text color so the component respects
light/dark themes).
---
Nitpick comments:
In `@app/`(app)/(tabs)/settings.tsx:
- Line 354: The weekly goal slider call currently relies on implicit defaults
(showOverlay("weeklyGoal", settings?.weeklyGoal || "1", "slider")) which couples
this usage to SettingsModal's defaults; explicitly pass the slider configuration
(sliderMin/sliderMax/sliderStep) so behavior is obvious and stable, either by
changing the showOverlay invocation for "weeklyGoal" to include those three
values or by extending SettingsModal (and its showOverlay consumer) to accept
per-setting slider props and then supply sliderMin=1, sliderMax=7, sliderStep=1
for the weeklyGoal setting; update any related prop types/signatures for
SettingsModal and the showOverlay handler so the modal reads and uses these
props instead of relying on internal defaults.
In `@components/SettingsModal.tsx`:
- Around line 75-77: Validate the slider props in the SettingsModal component
after props destructuring: check that sliderMin < sliderMax and sliderStep > 0
(and optionally that (sliderMax - sliderMin) is divisible or compatible with
sliderStep), and if the checks fail either clamp to safe defaults (e.g., swap
min/max or set step = 1) or throw/log a clear error so the UI isn't given
invalid values; reference the sliderMin, sliderMax, sliderStep symbols and
perform this guard inside the SettingsModal function initialization before using
them to render the slider.
🪄 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: e1896700-c6f1-4e29-97ce-227311c1b60e
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
app.config.jsapp/(app)/(create-plan)/create.tsxapp/(app)/(tabs)/index.tsxapp/(app)/(tabs)/settings.tsxapp/(app)/(workout)/workout-session.tsxcomponents/SettingsModal.tsxcomponents/WorkoutDoneCard.tsxpackage.jsonutils/__tests__/planHelpers.test.tsutils/initAppCheck.tsutils/notificationSetup.tsutils/planHelpers.ts
✅ Files skipped from review due to trivial changes (1)
- utils/tests/planHelpers.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- app/(app)/(workout)/workout-session.tsx
- utils/planHelpers.ts
- app/(app)/(tabs)/index.tsx
- package.json
- utils/initAppCheck.ts
- app/(app)/(create-plan)/create.tsx
This was referenced May 11, 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
Add weekly plan scheduling, rest-day awareness, and workout/quick-workout linkage, enhance workout/session UX, and integrate Firebase App Check and navigation bar configuration.
New Features:
Bug Fixes:
Enhancements:
Build:
Chores:
Summary by CodeRabbit
New Features
Improvements