Conversation
Add ORDER BY te.sort_order ASC to the allTrackedRows query and replace Object.values(groupedExercises) with an allTrackedRows-ordered map so the returned array respects sort_order rather than numeric exercise_id order.
…yle to stylesheet
…orderTrackedExercises, atomicize tracked exercise save
…omExercisesForSharing
…etchAllCustomExercisesForSharing
…PublishAllCustomExercises
…d bulkPublishAllCustomExercises
…pushCustomExercise error handling
…viewScreen and WorkoutSessionScreen
…t entry and adjust text
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @isotronic, your pull request is larger than the review limit of 150000 diff characters
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds bulk-sharing and a startup sync, extracts PrivacySettings and integrates it into Friends/Settings, refactors workout timers and immersive mode, records feedback-submitted IDs to suppress progression suggestions, implements tracked-exercise reorder with DB migration, refactors progression logic, updates queries/tests, and refreshes localisation files. ChangesSharing, Feedback, Workout UX, Stats, and Progression
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
hooks/useCreatePlan.ts (1)
64-66: 💤 Low valueConsider invalidating the published plans query to keep UI consistent.
The fire-and-forget
publishPlancall doesn't invalidate["publishedPlanIds"]or["planPublished", user.uid, newPlanId]queries. While startup sync will eventually reconcile state, matching the pattern inusePlanPublishMutation.ts:22-23would keep the UI more consistent. However, the current approach is acceptable since the publish is async and the user may not immediately see the published indicator.♻️ Optional refinement to align with mutation pattern
After the
publishPlancall resolves, invalidate the relevant queries:if (user && privacySettings?.sharePlans) { - publishPlan(user.uid, newPlanId).catch((err) => Bugsnag.notify(err)); + publishPlan(user.uid, newPlanId) + .then(() => { + queryClient.setQueryData(["planPublished", user.uid, newPlanId], true); + queryClient.invalidateQueries({ queryKey: ["publishedPlanIds"] }); + }) + .catch((err) => Bugsnag.notify(err)); }🤖 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 64 - 66, The fire-and-forget publishPlan call in useCreatePlan.ts should invalidate the cached queries for ["publishedPlanIds"] and ["planPublished", user.uid, newPlanId] after it completes to keep the UI consistent; update the publishPlan invocation to chain a .then(() => { queryClient.invalidateQueries(["publishedPlanIds"]); queryClient.invalidateQueries(["planPublished", user.uid, newPlanId]); }) and retain the existing .catch(err => Bugsnag.notify(err)), mirroring the invalidation pattern used in usePlanPublishMutation.ts (lines ~22-23) and referencing publishPlan, ["publishedPlanIds"], and ["planPublished", user.uid, newPlanId].hooks/useCreateStandaloneWorkout.ts (1)
28-35: 💤 Low valueConsider invalidating the published workouts query to keep UI consistent.
Similar to the plan creation flow, this fire-and-forget
publishStandaloneWorkoutcall doesn't invalidate the published workout queries. While functionally correct, matching the pattern in publish mutations would provide more immediate UI feedback. The current approach relies on startup sync for eventual consistency.♻️ Optional refinement to align with mutation pattern
After the
publishStandaloneWorkoutcall resolves, invalidate the relevant queries:onSuccess: (workoutId) => { queryClient.invalidateQueries({ queryKey: ["standaloneWorkouts"] }); if (user && privacySettings?.shareStandaloneWorkouts) { - publishStandaloneWorkout(user.uid, workoutId).catch((err) => - Bugsnag.notify(err), - ); + publishStandaloneWorkout(user.uid, workoutId) + .then(() => { + queryClient.setQueryData(["workoutPublished", user.uid, workoutId], true); + queryClient.invalidateQueries({ queryKey: ["publishedWorkoutIds"] }); + }) + .catch((err) => Bugsnag.notify(err)); } }🤖 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/useCreateStandaloneWorkout.ts` around lines 28 - 35, The onSuccess handler for the create standalone workout mutation calls publishStandaloneWorkout fire-and-forget but doesn't invalidate the published-workouts cache; update the onSuccess in useCreateStandaloneWorkout so that when user && privacySettings?.shareStandaloneWorkouts you await publishStandaloneWorkout(user.uid, workoutId).then(() => queryClient.invalidateQueries({ queryKey: ["publishedWorkouts"] })).catch(err => Bugsnag.notify(err)); in short, call publishStandaloneWorkout and on its resolution invalidate the publishedWorkouts query via queryClient.invalidateQueries instead of only notifying on error.hooks/useSocialSyncOnStartup.ts (1)
39-80: 💤 Low valueRemove redundant type annotations in map callbacks.
The type annotations
(d: QDocSnap)on lines 46, 57, and 70 are unnecessary—TypeScript infers the type fromsnap.docs.map(...).♻️ Simplify by removing explicit type annotations
- const published = new Set(snap.docs.map((d: QDocSnap) => d.id)); + const published = new Set(snap.docs.map((d) => d.id));Apply the same change on lines 57 and 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 `@hooks/useSocialSyncOnStartup.ts` around lines 39 - 80, The map callback type annotations like (d: QDocSnap) and (ex: Exercise) are redundant because TypeScript infers types from snap.docs.map(...) and exercises arrays; remove the explicit annotations in the three map callbacks (the snap.docs.map(...) usages and the exercises.filter(...) and missing.map(...) callbacks used alongside publishPlan, publishStandaloneWorkout and pushCustomExercise) so the callbacks become d => d.id and ex => ex.exercise_id checks (and ex => pushCustomExercise(uid, ex)), letting the compiler infer types.utils/__tests__/progressionEngine.test.ts (1)
624-648: ⚡ Quick winAdd a reps-only ceiling regression test for easy sessions.
The updated suite validates reps-only progression when below max, but it still misses the “already at
repsMax” case fortrackingType: "reps". Adding that case will lock behaviour and prevent no-opincrease_repsregressions.✅ Suggested test addition
+ it("easy reps-only: holds when all sets are already at repsMax", () => { + const result = evaluateProgression( + makeInputs({ + trackingType: "reps", + equipment: "body weight", + latestFeedback: makeFeedback({ + effortRating: "easy", + performanceRatio: 1.0, + }), + recentWorkingWeight: null, + currentSets: [WORKING_SET], + completedRepsPerSet: [12], + }), + ); + expect(result.action).toBe("hold"); + expect(result.ruleKey).toBe("DEFAULT"); + });🤖 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/__tests__/progressionEngine.test.ts` around lines 624 - 648, The test suite is missing a reps-only ceiling regression case: add a new unit test that calls evaluateProgression with makeInputs using trackingType: "reps", a set whose repsMax equals the completed reps (e.g., repsMin/repsMax both 10 or completedRepsPerSet: [10] matching repsMax), and latestFeedback easy, then assert the returned action is NOT "increase_reps" (expect no change or appropriate ceiling action) and suggestedRepsPerSet either equals the current completed reps or is undefined; reference the existing test that uses evaluateProgression, makeInputs and makeFeedback to model the new test and ensure it prevents regressions when at repsMax.
🤖 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)/(plans)/index.tsx:
- Around line 164-167: The isPublished check is using the wrong privacy flag:
replace privacySettings?.sharePlans with
privacySettings?.shareStandaloneWorkouts in the isPublished expression (the
block where isPublished is set using
publishedWorkoutIds?.includes(String(item.id))). Update the conditional that
computes isPublished (referencing privacySettings, publishedWorkoutIds, and
item.id) so it uses shareStandaloneWorkouts to correctly reflect standalone
workout publishing (consistent with usePublishedWorkoutIdsQuery which reads
sharedStandaloneWorkouts).
In `@components/PrivacySettings.tsx`:
- Around line 45-63: The function handlePrivacyToggle updates local state and
calls updatePrivacy before verifying the user, which can cause updatePrivacy
(from usePrivacySettingsMutation) to throw when user is null; move the user
existence check (user) to the top of handlePrivacyToggle so you return early if
no user, then perform setLocalPrivacySettings and updatePrivacy only after
confirming user is defined, and keep the subsequent conditional
bulkPublishAllPlans, bulkPublishAllStandaloneWorkouts, and
bulkPublishAllCustomExercises calls unchanged but also guarded by the confirmed
user.
In `@components/RestTimerOverlay.tsx`:
- Around line 90-106: The style in createStyles currently uses the web-only
boxShadow property; replace it with React Native shadow props for iOS and keep
elevation for Android: remove boxShadow and add shadowColor (use a color from
colors, e.g., colors.shadow or colors.card with alpha), shadowOffset (width: 0,
height: -2), shadowOpacity (e.g., 0.3), and shadowRadius (e.g., 4) inside the
container style so the overlay shows a cross-platform shadow while preserving
elevation.
- Around line 15-20: Replace the incorrect use of "typeof LayoutChangeEvent"
with the LayoutChangeEvent type itself in the AnimatedView prop typing and the
related interface (remove the typeof operator), and ensure LayoutChangeEvent is
imported from 'react-native' so the signatures read onLayout?: (event:
LayoutChangeEvent) => void; (do this for both the AnimatedView declaration and
the interface at the other occurrence).
In `@locales/es/messages.po`:
- Around line 1716-1724: Replace the current translations for the
"Dismiss"/"DISMISS" msgid entries so they use an action verb that conveys
rejection rather than closing: change msgstr "Cerrar"/"CERRAR" to
"Descartar"/"DESCARTAR" for the entries used by ProgressionSummaryCard.tsx,
UpdateModal.tsx and app/(app)/(tabs)/(plans)/overview.tsx so the UI indicates
rejecting a suggestion rather than merely closing a modal.
In `@utils/progressionEngine.ts`:
- Around line 274-285: The branch handling trackingType === "reps" must avoid
emitting an "increase_reps" suggestion when computePerSetRepTargets(...) returns
targets identical to the current set reps (i.e., at the rep ceiling); change the
logic in that branch (where perSetTargets is computed) to compare perSetTargets
against the current workingSets reps and only return the increase_reps response
(with ruleKey "EASY_TARGET_REPS") if at least one suggested rep is greater than
the corresponding workingSets rep; otherwise return hold("DEFAULT"). Use the
existing symbols computePerSetRepTargets, perSetTargets, workingSets, and hold
to locate and implement this check.
---
Nitpick comments:
In `@hooks/useCreatePlan.ts`:
- Around line 64-66: The fire-and-forget publishPlan call in useCreatePlan.ts
should invalidate the cached queries for ["publishedPlanIds"] and
["planPublished", user.uid, newPlanId] after it completes to keep the UI
consistent; update the publishPlan invocation to chain a .then(() => {
queryClient.invalidateQueries(["publishedPlanIds"]);
queryClient.invalidateQueries(["planPublished", user.uid, newPlanId]); }) and
retain the existing .catch(err => Bugsnag.notify(err)), mirroring the
invalidation pattern used in usePlanPublishMutation.ts (lines ~22-23) and
referencing publishPlan, ["publishedPlanIds"], and ["planPublished", user.uid,
newPlanId].
In `@hooks/useCreateStandaloneWorkout.ts`:
- Around line 28-35: The onSuccess handler for the create standalone workout
mutation calls publishStandaloneWorkout fire-and-forget but doesn't invalidate
the published-workouts cache; update the onSuccess in useCreateStandaloneWorkout
so that when user && privacySettings?.shareStandaloneWorkouts you await
publishStandaloneWorkout(user.uid, workoutId).then(() =>
queryClient.invalidateQueries({ queryKey: ["publishedWorkouts"] })).catch(err =>
Bugsnag.notify(err)); in short, call publishStandaloneWorkout and on its
resolution invalidate the publishedWorkouts query via
queryClient.invalidateQueries instead of only notifying on error.
In `@hooks/useSocialSyncOnStartup.ts`:
- Around line 39-80: The map callback type annotations like (d: QDocSnap) and
(ex: Exercise) are redundant because TypeScript infers types from
snap.docs.map(...) and exercises arrays; remove the explicit annotations in the
three map callbacks (the snap.docs.map(...) usages and the exercises.filter(...)
and missing.map(...) callbacks used alongside publishPlan,
publishStandaloneWorkout and pushCustomExercise) so the callbacks become d =>
d.id and ex => ex.exercise_id checks (and ex => pushCustomExercise(uid, ex)),
letting the compiler infer types.
In `@utils/__tests__/progressionEngine.test.ts`:
- Around line 624-648: The test suite is missing a reps-only ceiling regression
case: add a new unit test that calls evaluateProgression with makeInputs using
trackingType: "reps", a set whose repsMax equals the completed reps (e.g.,
repsMin/repsMax both 10 or completedRepsPerSet: [10] matching repsMax), and
latestFeedback easy, then assert the returned action is NOT "increase_reps"
(expect no change or appropriate ceiling action) and suggestedRepsPerSet either
equals the current completed reps or is undefined; reference the existing test
that uses evaluateProgression, makeInputs and makeFeedback to model the new test
and ensure it prevents regressions when at repsMax.
🪄 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: 4b756ad2-c4c9-4489-9a59-bc8ed97ce494
📒 Files selected for processing (49)
app/(app)/(tabs)/(plans)/index.tsxapp/(app)/(tabs)/(plans)/overview.tsxapp/(app)/(tabs)/(plans)/standalone-workout.tsxapp/(app)/(tabs)/(stats)/exercises.tsxapp/(app)/(tabs)/(stats)/index.tsxapp/(app)/(tabs)/_layout.tsxapp/(app)/(workout)/index.tsxapp/(app)/(workout)/workout-session.tsxapp/(app)/_layout.tsxapp/(app)/friend-profile.tsxapp/(app)/friends.tsxapp/(app)/settings.tsxcomponents/ExerciseFeedbackSheet.tsxcomponents/PrivacySettings.tsxcomponents/ProgressionSuggestionChip.tsxcomponents/ProgressionSummaryCard.tsxcomponents/RestTimerOverlay.tsxcomponents/SessionSetInfo.tsxcomponents/StandaloneWorkoutListItem.tsxcomponents/TrainingPlanCard.tsxcomponents/stats/ExerciseCompactCard.tsxfirestore.ruleshooks/__tests__/useReorderTrackedExercisesMutation.test.tshooks/__tests__/useTrackedExercisesQuery.test.tshooks/useCreatePlan.tshooks/useCreateStandaloneWorkout.tshooks/useExerciseDetailQuery.tshooks/usePlanPublishMutation.tshooks/useReorderTrackedExercisesMutation.tshooks/useSocialSyncOnStartup.tshooks/useTrackedExercisesQuery.tshooks/useWorkoutImmersiveMode.tshooks/useWorkoutPublishMutation.tslocales/de/messages.jslocales/de/messages.polocales/en/messages.jslocales/en/messages.polocales/es/messages.jslocales/es/messages.polocales/fr/messages.jslocales/fr/messages.postore/activeWorkoutStore.tsutils/__tests__/database.test.tsutils/__tests__/progressionEngine.test.tsutils/__tests__/sharing.test.tsutils/database.tsutils/initUserDataDB.tsutils/progressionEngine.tsutils/sharing.ts
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 CodeRabbit
Release Notes
New Features
Improvements