Conversation
Summary of ChangesHello @CodeWithCJ, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's external integration capabilities by adding support for Hevy, a popular workout tracking app. Users can now seamlessly bring their workout data, including detailed set information and RPE, into Sparky Fitness. This integration provides a more comprehensive view of a user's fitness journey by consolidating data from multiple sources, improving the overall utility and data richness of the platform. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces Hevy integration, allowing users to sync workouts and body measurements via API keys. It also adds RPE (Rate of Perceived Exertion) tracking to exercise sets. While the integration is comprehensive, there are several critical issues regarding API endpoint paths in the frontend, duration unit mismatches (seconds vs. minutes), and logic that causes exercise duration inflation in reports. These should be addressed to ensure data accuracy and functional connectivity.
| export const connectHevyAccount = async (data: { | ||
| apiKey: string; | ||
| }): Promise<void> => { | ||
| return apiCall('/hevy/connect', { |
There was a problem hiding this comment.
The API path is missing the /integrations prefix. The backend routes for Hevy are mounted at /api/integrations/hevy in SparkyFitnessServer.js. Calling /hevy/connect will result in a 404 error.
| return apiCall('/hevy/connect', { | |
| return apiCall('/integrations/hevy/connect', { |
| export const syncHevyData = async ( | ||
| fullSync: boolean = false | ||
| ): Promise<void> => { | ||
| return apiCall(`/hevy/sync${fullSync ? '?fullSync=true' : ''}`, { |
There was a problem hiding this comment.
The API path is missing the /integrations prefix. This request will fail with a 404.
| return apiCall(`/hevy/sync${fullSync ? '?fullSync=true' : ''}`, { | |
| return apiCall(`/integrations/hevy/sync${fullSync ? '?fullSync=true' : ''}`, { |
| }; | ||
|
|
||
| export const disconnectHevyAccount = async (): Promise<void> => { | ||
| return apiCall('/hevy/disconnect', { |
There was a problem hiding this comment.
The API path is missing the /integrations prefix. This request will fail with a 404.
| return apiCall('/hevy/disconnect', { | |
| return apiCall('/integrations/hevy/disconnect', { |
| if (hevyProviders.length > 0) { | ||
| for (const provider of hevyProviders) { | ||
| try { | ||
| const hevyStatus = await apiCall(`/hevy/status`); |
There was a problem hiding this comment.
The API path is missing the /integrations prefix. This will cause the status check to fail with a 404.
| const hevyStatus = await apiCall(`/hevy/status`); | |
| const hevyStatus = await apiCall(`/integrations/hevy/status`); |
| set_type: mapSetType(set.type), | ||
| weight: set.weight_kg, | ||
| reps: set.reps, | ||
| duration: set.duration_seconds, |
There was a problem hiding this comment.
Hevy provides duration_seconds, but the Sparky Fitness system (as seen in WorkoutPresetSet) expects the duration in minutes. Passing raw seconds will cause durations to be displayed 60x larger than they are (e.g., a 60-second set will appear as 60 minutes).
| duration: set.duration_seconds, | |
| duration: set.duration_seconds ? Math.round(set.duration_seconds / 60) : 0, |
| const entryData = { | ||
| exercise_id: exercise.id, | ||
| entry_date: startTime.toISOString().split('T')[0], | ||
| duration_minutes: durationMinutes, // Note: Hevy provides total workout duration, not per-exercise |
There was a problem hiding this comment.
Setting duration_minutes to the total workout duration for every exercise entry will cause significant data inflation in daily reports. For example, a 60-minute workout with 5 exercises would be recorded as 300 minutes of total exercise. It is better to set this to 0 (relying on set-level duration) or calculate the sum of set durations for this specific exercise.
| duration_minutes: durationMinutes, // Note: Hevy provides total workout duration, not per-exercise | |
| duration_minutes: Math.round(hevyExercise.sets.reduce((sum, s) => sum + (s.duration_seconds || 0), 0) / 60), |
| try { | ||
| const userId = req.userId; | ||
| const createdByUserId = req.userId; // Currently same as userId | ||
| const result = await hevyService.syncHevyData(userId, createdByUserId); |
There was a problem hiding this comment.
The fullSync query parameter from the request is ignored. This prevents the 'Sync All History' functionality from working as intended.
| const result = await hevyService.syncHevyData(userId, createdByUserId); | |
| const { fullSync } = req.query; | |
| const result = await hevyService.syncHevyData(userId, createdByUserId, fullSync === 'true'); |
|
@CodeWithCJ the connectHeavyAccount function is not used |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant new feature: integration with the Hevy workout tracking app. The changes are comprehensive, spanning the frontend for configuration and display, the backend for API communication and data processing, and the database with necessary schema updates. The implementation is well-structured, including new API routes, services, and data processors for Hevy. Additionally, the concept of Rate of Perceived Exertion (RPE) has been added to exercise sets throughout the application, which is a valuable enhancement for strength training tracking. Overall, this is a solid contribution. I've included a few suggestions to improve robustness and code clarity.
| if (oldestTime < sevenDaysAgo) { | ||
| log('debug', `Stopping incremental sync: workout date ${oldestTime.toISOString()} is older than 7 days.`); | ||
| hasMore = false; | ||
| } else { |
There was a problem hiding this comment.
The incremental sync logic assumes that workouts from the Hevy API are always sorted in descending chronological order. The comment on line 255, "Hevy workouts are usually sorted by date desc," suggests this order is not guaranteed. If the API response is not strictly sorted, this logic could cause the sync to terminate prematurely, potentially missing recent workouts that appear on later pages.
To improve robustness, consider fetching pages until a workout's start_time is older than the sevenDaysAgo threshold, but ensure you still process all workouts on that final page to catch any out-of-order entries. An even safer, though less efficient, approach would be to continue paginating as long as any workout on the page is within the 7-day window.
| reps: set.reps, | ||
| duration: set.duration_seconds ? set.duration_seconds / 60 : null, | ||
| rpe: set.rpe, // This requires the migration to be applied | ||
| notes: set.rpe ? `RPE: ${set.rpe}` : null, // Fallback: store RPE in notes if column doesn't exist yet |
There was a problem hiding this comment.
The rpe value is being stored in its dedicated column, but this line also adds it to the notes field as a fallback. Since the database migration adds the rpe column and the model has been updated to use it, storing it in the notes as well creates redundant data. It's better to rely on the rpe column exclusively to avoid data duplication and potential inconsistencies. The deployment process should ensure migrations are run before the new code is active.
| notes: set.rpe ? `RPE: ${set.rpe}` : null, // Fallback: store RPE in notes if column doesn't exist yet | |
| notes: null, |
| } | ||
| }; | ||
|
|
||
| const handleDisconnectHevy = async (providerId: string) => { |
There was a problem hiding this comment.
The handleDisconnectHevy function is defined to accept a providerId, but this parameter is not used within the function body. The disconnectHevyAccount API call it invokes operates on the current user's context without needing an ID.
To improve code clarity and remove the unused variable, you could update the function signature here and in the corresponding prop types. You would also need to adjust the call site in ExternalProviderList.tsx.
For example:
- Here: Change to
const handleDisconnectHevy = async () => { ... } - In
ExternalProviderList.tsxprops: Change tohandleDisconnectHevy: () => Promise<void>; - In
ExternalProviderList.tsxcall site: ChangeonClick={() => handleDisconnectHevy(provider.id)}toonClick={handleDisconnectHevy}
| const handleDisconnectHevy = async (providerId: string) => { | |
| const handleDisconnectHevy = async () => { |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new integration with Hevy, a workout tracking app. The changes are extensive, covering the backend service, API endpoints, database schema, and frontend UI for configuration and data display. The implementation is solid, adding RPE tracking and sync capabilities. I've identified a few areas for improvement regarding code duplication, efficiency, and a potential bug in the mock data handling logic. Overall, great work on adding this significant feature.
| await apiCall( | ||
| `/integrations/hevy/sync${fullSyncOnConnect ? '?fullSync=true' : ''}`, | ||
| { | ||
| method: 'POST', | ||
| body: JSON.stringify({ providerId: data.id }), | ||
| } | ||
| ); |
There was a problem hiding this comment.
This block for triggering the initial Hevy sync duplicates the logic from the syncHevyData helper function. To follow the DRY (Don't Repeat Yourself) principle and improve maintainability, you should reuse the existing syncHevyData function here.
await syncHevyData(fullSyncOnConnect, data.id);
| if (hevyProviders.length > 0) { | ||
| for (const provider of hevyProviders) { | ||
| try { | ||
| const hevyStatus = await apiCall(`/integrations/hevy/status`); | ||
| setProviders((prev) => | ||
| prev.map((p) => | ||
| p.id === provider.id | ||
| ? { | ||
| ...p, | ||
| hevy_last_sync_at: hevyStatus.lastSyncAt, | ||
| hevy_connect_status: hevyStatus.connected | ||
| ? 'connected' | ||
| : 'disconnected', | ||
| } | ||
| : p | ||
| ) | ||
| ); | ||
| } catch (hevyError) { | ||
| console.error( | ||
| 'Failed to fetch Hevy specific status for provider:', | ||
| provider.id, | ||
| hevyError | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
This loop makes a redundant API call to /integrations/hevy/status for each Hevy provider. Since the backend endpoint is not provider-specific and returns a general status for the user, you should fetch the status only once before mapping over the providers. This will improve performance and simplify the code.
if (hevyProviders.length > 0) {
try {
const hevyStatus = await apiCall(`/integrations/hevy/status`);
setProviders((prev) =>
prev.map((p) =>
p.provider_type === 'hevy'
? {
...p,
hevy_last_sync_at: hevyStatus.lastSyncAt,
hevy_connect_status: hevyStatus.connected
? 'connected'
: 'disconnected',
}
: p
)
);
} catch (hevyError) {
console.error(
'Failed to fetch Hevy status for providers:',
hevyProviders.map((p) => p.id),
hevyError
);
}
}
| <TooltipProvider> | ||
| <Tooltip> | ||
| <TooltipTrigger asChild> | ||
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| onClick={() => handleManualSyncHevy(provider.id)} | ||
| disabled={loading} | ||
| className="ml-2 text-blue-500" | ||
| > | ||
| <RefreshCw className="h-4 w-4" /> | ||
| </Button> | ||
| </TooltipTrigger> | ||
| <TooltipContent> | ||
| <p>Sync Recently (7 Days)</p> | ||
| </TooltipContent> | ||
| </Tooltip> | ||
| </TooltipProvider> | ||
| <TooltipProvider> | ||
| <Tooltip> | ||
| <TooltipTrigger asChild> | ||
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| onClick={() => | ||
| handleManualSyncHevy(provider.id, true) | ||
| } | ||
| disabled={loading} | ||
| className="ml-2 text-cyan-500" | ||
| > | ||
| <RefreshCw className="h-4 w-4" /> | ||
| </Button> | ||
| </TooltipTrigger> | ||
| <TooltipContent> | ||
| <p>Sync All History</p> | ||
| </TooltipContent> | ||
| </Tooltip> | ||
| </TooltipProvider> |
There was a problem hiding this comment.
You are using two separate TooltipProvider components for the two sync buttons. A single TooltipProvider can wrap multiple Tooltip components. Consolidating this into one provider would make the code slightly cleaner and more efficient.
<TooltipProvider>
<Tooltip>
<TooltipTrigger asChild>
<Button
variant="ghost"
size="icon"
onClick={() => handleManualSyncHevy(provider.id)}
disabled={loading}
className="ml-2 text-blue-500"
>
<RefreshCw className="h-4 w-4" />
</Button>
</TooltipTrigger>
<TooltipContent>
<p>Sync Recently (7 Days)</p>
</TooltipContent>
</Tooltip>
<Tooltip>
<TooltipTrigger asChild>
<Button
variant="ghost"
size="icon"
onClick={() =>
handleManualSyncHevy(provider.id, true)
}
disabled={loading}
className="ml-2 text-cyan-500"
>
<RefreshCw className="h-4 w-4" />
</Button>
</TooltipTrigger>
<TooltipContent>
<p>Sync All History</p>
</TooltipContent>
</Tooltip>
</TooltipProvider>
| await client.query( | ||
| `UPDATE external_data_providers | ||
| SET last_sync_at = NOW(), updated_at = NOW() | ||
| WHERE user_id = $1 AND provider_type = 'hevy'`, | ||
| [userId], | ||
| ); |
There was a problem hiding this comment.
The query to update last_sync_at for local mock data sources targets all 'hevy' providers for a user, which could lead to incorrect data if a user has multiple Hevy providers configured. This should be scoped to a specific providerId to be consistent with the live API sync logic and prevent potential bugs.
const targetProviderId = providerId || (await getHevyProviderId(userId));
if (targetProviderId) {
await client.query(
`UPDATE external_data_providers
SET last_sync_at = NOW(), updated_at = NOW()
WHERE id = $1`,
[targetProviderId],
);
} else {
log('warn', `[hevyService] Could not determine a provider ID to update sync time for user ${userId}.`);
}
2 participants
No description provided.