refactor: migrate Garmin integration services to TypeScript#1089
refactor: migrate Garmin integration services to TypeScript#1089serjsv87 wants to merge 2 commits intoCodeWithCJ:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces Telegram bot integration, including a webhook handler and initialization logic, and migrates several core modules like the Garmin Connect service and exercise entry repository to TypeScript. It also implements a custom logging utility with level-based filtering and object sanitization. Key feedback includes a critical missing function definition for the new Garmin nutrition sync endpoint, potential unhandled promise rejections in the Telegram webhook, and a risk of stack overflow in the logger due to circular references. Additionally, there are opportunities to improve performance by resolving an N+1 query pattern and reducing redundant database client acquisitions.
| .json({ error: 'Valid date (YYYY-MM-DD) is required.' }); | ||
| } | ||
|
|
||
| const result = await garminService.syncDailyNutritionToGarmin(userId, date); |
There was a problem hiding this comment.
| app.post('/api/telegram/webhook', (req, res) => { | ||
| telegramBotService.handleUpdate(req.body); | ||
| res.sendStatus(200); | ||
| }); |
There was a problem hiding this comment.
The Telegram webhook handler calls telegramBotService.handleUpdate(req.body) without awaiting it or providing a catch block. If handleUpdate is an asynchronous function, any errors occurring during processing will result in unhandled promise rejections. It is safer to wrap this in a try-catch and ensure it is properly awaited.
app.post('/api/telegram/webhook', async (req, res) => {
try {
await telegramBotService.handleUpdate(req.body);
} catch (err) {
log('error', '[TELEGRAM BOT] Webhook handling failed:', err);
}
res.sendStatus(200);
});| 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. Passing an object with a circular reference (e.g., a request object or a complex model) will result in a stack overflow (Maximum call stack size exceeded). Since this is used in the global logger, it could crash the entire server when logging certain objects.
const truncateStrings = (obj: any, maxLength: number = 1000, visited = new WeakSet()): any => {
if (typeof obj === 'string') {
return obj.length > maxLength ? obj.substring(0, maxLength) + '... [truncated]' : obj;
}
if (Array.isArray(obj)) {
if (visited.has(obj)) return '[Circular]';
visited.add(obj);
return obj.map(item => truncateStrings(item, maxLength, visited));
}
if (typeof obj === 'object' && obj !== null) {
if (visited.has(obj)) return '[Circular]';
visited.add(obj);
const newObj: any = {};
for (const key in obj) {
if (Object.prototype.hasOwnProperty.call(obj, key)) {
newObj[key] = truncateStrings(obj[key], maxLength, visited);
}
}
return newObj;
}
return obj;
};| export async function upsertExerciseEntryData( | ||
| userId: string, | ||
| createdByUserId: string, | ||
| exerciseId: string, | ||
| caloriesBurned: number, | ||
| date: string | Date | ||
| ): Promise<any> { | ||
| log('info', 'upsertExerciseEntryData received date parameter:', date); | ||
| const client = await getClient(userId); | ||
| let existingEntry: any = null; | ||
| let exerciseName = 'Unknown Exercise'; | ||
|
|
||
| try { | ||
| const exercise = await exerciseRepository.getExerciseById(exerciseId, userId); | ||
| if (exercise) { | ||
| exerciseName = exercise.name; | ||
| log('info', `Fetched exercise name: ${exerciseName} for exerciseId: ${exerciseId}`); | ||
| } else { | ||
| log('warn', `Exercise with ID ${exerciseId} not found for user ${userId}. Using default name.`); | ||
| } | ||
|
|
||
| const result = await client.query( | ||
| 'SELECT id, calories_burned FROM exercise_entries WHERE user_id = $1 AND exercise_id = $2 AND entry_date = $3', | ||
| [userId, exerciseId, date] | ||
| ); | ||
| existingEntry = result.rows[0]; | ||
| } catch (error: any) { | ||
| log('error', 'Error checking for existing active calories exercise entry or fetching exercise name:', error); | ||
| throw new Error(`Failed to check existing active calories exercise entry or fetch exercise name: ${error.message}`); | ||
| } finally { | ||
| client.release(); | ||
| } | ||
|
|
||
| let finalResult: any; | ||
| if (existingEntry) { | ||
| log('info', `Existing active calories entry found for ${date}, updating calories from ${existingEntry.calories_burned} to ${caloriesBurned}.`); | ||
| const updateClient = await getClient(userId); | ||
| try { | ||
| const updateResult = await updateClient.query( | ||
| 'UPDATE exercise_entries SET calories_burned = $1, notes = $2, updated_by_user_id = $3, exercise_name = $4 WHERE id = $5 RETURNING *', | ||
| [ | ||
| caloriesBurned, | ||
| 'Active calories logged from Apple Health (updated).', | ||
| createdByUserId, | ||
| exerciseName, | ||
| existingEntry.id, | ||
| ] | ||
| ); | ||
| finalResult = updateResult.rows[0]; | ||
| } catch (error: any) { | ||
| log('error', 'Error updating active calories exercise entry:', error); | ||
| throw new Error(`Failed to update active calories exercise entry: ${error.message}`); | ||
| } finally { | ||
| updateClient.release(); | ||
| } | ||
| } else { | ||
| log('info', `No existing active calories entry found for ${date}, inserting new entry.`); | ||
| const insertClient = await getClient(userId); | ||
| try { | ||
| const insertResult = await insertClient.query( | ||
| `INSERT INTO exercise_entries (user_id, exercise_id, entry_date, calories_burned, duration_minutes, notes, created_by_user_id, exercise_name) | ||
| VALUES ($1, $2, $3, $4, $5, $6, $7, $8) RETURNING *`, | ||
| [ | ||
| userId, | ||
| exerciseId, | ||
| date, | ||
| caloriesBurned, | ||
| 0, | ||
| 'Active calories logged from Apple Health.', | ||
| createdByUserId, | ||
| exerciseName, | ||
| ] | ||
| ); | ||
| finalResult = insertResult.rows[0]; | ||
| } catch (error: any) { | ||
| log('error', 'Error inserting active calories exercise entry:', error); | ||
| throw new Error(`Failed to insert active calories exercise entry: ${error.message}`); | ||
| } finally { | ||
| insertClient.release(); | ||
| } | ||
| } | ||
| return finalResult; | ||
| } |
There was a problem hiding this comment.
The upsertExerciseEntryData function acquires and releases database clients multiple times redundantly. It acquires a client at the start, releases it, and then acquires another one (updateClient or insertClient) for the actual operation. This adds unnecessary overhead. You should reuse the initial client for all queries within the function.
export async function upsertExerciseEntryData(
userId: string,
createdByUserId: string,
exerciseId: string,
caloriesBurned: number,
date: string | Date
): Promise<any> {
log('info', 'upsertExerciseEntryData received date parameter:', date);
const client = await getClient(userId);
try {
let exerciseName = 'Unknown Exercise';
const exercise = await exerciseRepository.getExerciseById(exerciseId, userId);
if (exercise) {
exerciseName = exercise.name;
log('info', `Fetched exercise name: ${exerciseName} for exerciseId: ${exerciseId}`);
} else {
log('warn', `Exercise with ID ${exerciseId} not found for user ${userId}. Using default name.`);
}
const result = await client.query(
'SELECT id, calories_burned FROM exercise_entries WHERE user_id = $1 AND exercise_id = $2 AND entry_date = $3',
[userId, exerciseId, date]
);
const existingEntry = result.rows[0];
if (existingEntry) {
log('info', `Existing active calories entry found for ${date}, updating calories from ${existingEntry.calories_burned} to ${caloriesBurned}.`);
const updateResult = await client.query(
'UPDATE exercise_entries SET calories_burned = $1, notes = $2, updated_by_user_id = $3, exercise_name = $4 WHERE id = $5 RETURNING *',
[
caloriesBurned,
'Active calories logged from Apple Health (updated).',
createdByUserId,
exerciseName,
existingEntry.id,
]
);
return updateResult.rows[0];
} else {
log('info', `No existing active calories entry found for ${date}, inserting new entry.`);
const insertResult = await client.query(
`INSERT INTO exercise_entries (user_id, exercise_id, entry_date, calories_burned, duration_minutes, notes, created_by_user_id, exercise_name)
VALUES ($1, $2, $3, $4, $5, $6, $7, $8) RETURNING *`,
[
userId,
exerciseId,
date,
caloriesBurned,
0,
'Active calories logged from Apple Health.',
createdByUserId,
exerciseName,
]
);
return insertResult.rows[0];
}
} catch (error: any) {
log('error', 'Error in upsertExerciseEntryData:', error);
throw new Error(`Failed to upsert active calories exercise entry: ${error.message}`);
} finally {
client.release();
}
}| }); | ||
|
|
||
| const entriesWithDetails = await Promise.all(allExerciseEntries.map(async (row: any) => { | ||
| const activityDetails = await activityDetailsRepository.getActivityDetailsByEntryOrPresetId(userId, row.id, null); |
There was a problem hiding this comment.
This implementation introduces an N+1 query pattern. For every exercise entry found for a date, a separate database call is made to activityDetailsRepository.getActivityDetailsByEntryOrPresetId. This can significantly degrade performance as the number of entries grows. It is recommended to fetch all activity details in a single bulk query using the list of entry IDs.
| try { | ||
| calendarDate = moment(calendarDateRaw).format('YYYY-MM-DD'); | ||
| } catch (e) { | ||
| calendarDate = new Date(calendarDateRaw) | ||
| .toISOString() | ||
| .split('T')[0]; | ||
| } |
There was a problem hiding this comment.
The try-catch block around moment().format() is ineffective because moment does not throw an exception when parsing an invalid date; instead, it returns an "Invalid date" object, and format() returns the string "Invalid date". You should use moment(calendarDateRaw).isValid() to check for parsing success. Additionally, new Date(calendarDateRaw).toISOString() will throw a RangeError if the date is invalid, which could crash the process if not handled.
const m = moment(calendarDateRaw);
let calendarDate;
if (m.isValid()) {
calendarDate = m.format('YYYY-MM-DD');
} else {
try {
calendarDate = new Date(calendarDateRaw).toISOString().split('T')[0];
} catch (e) {
log('warn', `[garminService] Invalid date encountered: ${calendarDateRaw}`);
continue;
}
}|
Ci/test failed for all the CRs. have look at them and also the comments in my previous PR. I will merge them one by one in the same order you submitted, but not sure if it will throw any error as some PRs include same files being changed. Let me know if any questions. |
cc0bf05 to
b658ecf
Compare
b658ecf to
13c35c3
Compare
|
I have rebased this branch and fixed the formatting/type issues. It's now ready for review. |
As requested, migrating Garmin-related files to TypeScript to improve type safety and maintainability.
Migrated garminService.js to garminService.ts (implied by content).
Migrated garminRoutes.js to garminRoutes.ts (implied by content).
Fixed type definitions for exerciseEntry.
✅ Checklist