feat: add MyFitnessPal integration and external provider settings UI#1088
feat: add MyFitnessPal integration and external provider settings UI#1088serjsv87 wants to merge 2 commits intoCodeWithCJ:mainfrom
Conversation
![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
This pull request introduces a MyFitnessPal integration, allowing users to sync nutrition data by providing session cookies and CSRF tokens. It includes new frontend components for credential management, backend services for data synchronization with idempotency logic, and a manual sync trigger. Additionally, the PR adds Ukrainian language support, a new custom logging utility, and a Telegram bot integration with a webhook handler. Feedback focuses on critical security and reliability concerns: the Telegram webhook lacks authentication, sensitive credential inputs should be masked as passwords, and the MFP sync logic needs better error resilience, cookie persistence, and distributed locking. There are also concerns regarding potential data loss in the MFP deletion logic and request timeouts during multi-day syncs.
| app.post('/api/telegram/webhook', (req, res) => { | ||
| telegramBotService.handleUpdate(req.body); | ||
| res.sendStatus(200); | ||
| }); |
There was a problem hiding this comment.
The Telegram webhook endpoint is publicly accessible and lacks any verification. An attacker could send forged updates to this endpoint to manipulate the bot's state or trigger unauthorized actions. You should implement a secret token check (e.g., via the X-Telegram-Bot-Api-Secret-Token header) as recommended by the Telegram Bot API documentation.
| for (const item of items) { | ||
| // IMPORTANT: Some items have ID under 'id', others under 'item_id' or inside 'links' | ||
| const itemId = item.id || item.item_id; | ||
| const itemType = item.type; | ||
| const itemName = item.food_name || item.diary_meal || 'Unnamed Entry'; | ||
|
|
||
| log('info', `pushNutritionToMFP: [DISCOVERY] Found item: ID=${itemId}, Type=${itemType}, Name="${itemName}"`); | ||
|
|
||
| if (itemId) { | ||
| try { | ||
| log('info', `pushNutritionToMFP: Attempting to DELETE item ${itemId}...`); | ||
| const delResp = await axios.delete(`https://api.myfitnesspal.com/v2/diary/${itemId}`, { | ||
| headers: authHeaders as any | ||
| }); | ||
| log('info', `pushNutritionToMFP: Successfully deleted item ${itemId}. Status: ${delResp.status}`); | ||
| } catch (e: any) { | ||
| log('warn', `pushNutritionToMFP: Delete failed for item ${itemId}: ${e.message} (Payload: ${JSON.stringify(e.response?.data || {})})`); | ||
| } | ||
| } else { | ||
| log('info', `pushNutritionToMFP: [DEBUG] Item has NO ID at top level. Full structure: ${JSON.stringify(item)}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
This idempotency logic is potentially destructive. It deletes all items returned by the discovery API for the target date without verifying if they were originally created by SparkyFitness. This could lead to data loss if the user has manually entered items or used other integrations on the same day. It is safer to filter items by a specific name or metadata before deletion.
| <> | ||
| <div> | ||
| <Label>MFP CSRF Token (x-csrf-token)</Label> | ||
| <Input |
There was a problem hiding this comment.
The input field for the MyFitnessPal CSRF token is using type="text", which leaves sensitive session-level credentials visible on the screen. It is recommended to use type="password" to prevent shoulder surfing and improve security.
| <Input | |
| type="password" |
| </div> | ||
| <div> | ||
| <Label>MFP Session Cookies</Label> | ||
| <Input |
There was a problem hiding this comment.
The input field for MyFitnessPal session cookies is using type="text". Since these cookies grant full access to the user's account, they should be treated as sensitive credentials and masked using type="password".
| <Input | |
| type="password" |
| <Label htmlFor="add-mfp-csrf">MFP CSRF Token (x-csrf-token)</Label> | ||
| <Input | ||
| id="add-mfp-csrf" | ||
| type="text" |
There was a problem hiding this comment.
Sensitive credentials like the MFP CSRF token should not be displayed in plain text. Please change the input type to password.
| type="text" | |
| type="password" |
| const truncateStrings = (obj: any, maxLength: number = 1000): any => { | ||
| if (typeof obj === 'string') { | ||
| return obj.length > maxLength ? obj.substring(0, maxLength) + '... [truncated]' : obj; | ||
| } | ||
| if (Array.isArray(obj)) { | ||
| return obj.map(item => truncateStrings(item, maxLength)); | ||
| } | ||
| if (typeof obj === 'object' && obj !== null) { | ||
| const newObj: any = {}; | ||
| for (const key in obj) { | ||
| if (Object.prototype.hasOwnProperty.call(obj, key)) { | ||
| newObj[key] = truncateStrings(obj[key], maxLength); | ||
| } | ||
| } | ||
| return newObj; | ||
| } | ||
| return obj; | ||
| }; |
There was a problem hiding this comment.
The truncateStrings function is recursive and does not handle circular references. If an object with circular dependencies is passed to the logger, it will result in a stack overflow. Consider using a library like json-stringify-safe or implementing a cycle-detection mechanism.
| const setCookie = csrfResp.headers['set-cookie']; | ||
| if (setCookie) { | ||
| const newCookies = setCookie.map(c => c.split(';')[0]).join('; '); | ||
| currentCookies = `${currentCookies}; ${newCookies}`; | ||
| } |
There was a problem hiding this comment.
The function captures new cookies from the set-cookie header but does not persist them back to the database. If MyFitnessPal rotates session cookies or updates them, subsequent sync attempts will continue using the old, potentially invalid cookies stored in the app_key field. You should update the provider record in the database whenever new cookies are received.
| for (const [categoryName, categoryData] of Object.entries(data.categories)) { | ||
| if (categoryData.calories <= 0) continue; // Skip empty categories | ||
|
|
||
| const mfpMealName = mealMapping[categoryName.toLowerCase()] || 'Snacks'; | ||
|
|
||
| const payload = { | ||
| items: [ | ||
| { | ||
| type: 'quick_add', | ||
| date: data.date, | ||
| meal_name: mfpMealName, | ||
| nutritional_contents: { | ||
| energy: { unit: 'calories', value: Math.round(categoryData.calories) }, | ||
| carbohydrates: Number(categoryData.carbohydrate), | ||
| fat: Number(categoryData.fat), | ||
| protein: Number(categoryData.protein) | ||
| } | ||
| } | ||
| ] | ||
| }; | ||
|
|
||
| log('info', `pushNutritionToMFP: Pushing ${mfpMealName} total for ${data.date}: ${categoryData.calories} kcal`); | ||
| const finalResp = await axios.post('https://api.myfitnesspal.com/v2/diary', payload, { | ||
| headers: authHeaders as any | ||
| }); | ||
|
|
||
| if (finalResp.status >= 400) { | ||
| log('error', `pushNutritionToMFP: MFP API rejected ${mfpMealName} sync: ${JSON.stringify(finalResp.data)}`); | ||
| } else { | ||
| responses.push(finalResp.data); | ||
| } | ||
| } |
There was a problem hiding this comment.
The category push loop is wrapped in a single try-catch block. If one category (e.g., Breakfast) fails to sync, the entire function throws, and all subsequent categories (Lunch, Dinner, etc.) are skipped. Wrapping each individual push in its own try-catch would make the sync process more resilient.
| for (const date of dates) { | ||
| await syncDailyTotals(userId, date); | ||
| } |
There was a problem hiding this comment.
Processing up to 31 days of sync sequentially within a single HTTP request is risky. Each day involves multiple external network calls (auth, discovery, deletions, pushes). This could easily exceed the timeout limits of reverse proxies or gateways. Consider offloading this to a background worker or processing days in smaller batches.
| import { pushNutritionToMFP } from '../integrations/myfitnesspal/myFitnessPalService'; | ||
|
|
||
| // In-memory lock to prevent multiple concurrent syncs for the same user and date. | ||
| const activeSyncs = new Set<string>(); |
There was a problem hiding this comment.
Using an in-memory Set for locking is not resilient to server restarts and will not work in a distributed environment with multiple server instances. If the application scales, concurrent syncs for the same user/date could still occur. A database-backed lock or a distributed lock manager (like Redis) would be more appropriate for production environments.
|
Could you explain more on MFP? Is it only write back to MFP or importing into SparkyFitness as well? We never did any write-back so far not because we couldn't but simply we want to be caution if anything goes wrong. How does the deletion in MFP performed only for the SparkyFitness written data and have you make sure it doesn't delete any other sources in MFP? How are the custom meal types (say late night snacks, morning snack that are different than breakfast, lunch etc.) are handled? |
|
Mfp - MyFitnessPal |
Hi @CodeWithCJ! To answer your questions about the MFP integration:
I'll attach a screenshot of the new UI warning shortly! |
serjsv87
force-pushed
the
feature/fitnesspal
branch
from
365b16b to
fdf39ba
Compare
serjsv87
force-pushed
the
feature/fitnesspal
branch
from
fdf39ba to
9980d65
Compare
I've added a strong warning in the UI about the data overwrite risk. To clarify: the sync works as a write-back (push) to MFP. It deletes existing entries for the day to ensure idempotency. Custom meal types are mapped to standard MFP categories (Breakfast, Lunch, Dinner, Snacks)
|
|
CI test failure is still there |
2 participants
Comprehensive UI for managing external data providers and MyFitnessPal sync.
New Settings page: ExternalProviderSettings.tsx.
Form components for adding/editing providers (MFP, Edamam, FatSecret, Garmin).
Implemented mfpSyncService for data synchronization.
Centralized external provider service and hooks.
✅ Checklist
pnpm run typecheckandpnpm run validatepass.external_data_providersschema.