refactor + new account sync metrics + isolating health status inside folder admin-panel > health-status #10314
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR introduces significant changes to the health status monitoring system, focusing on account synchronization and component organization.
- Replaces message sync with comprehensive account sync monitoring in
/packages/twenty-server/src/engine/core-modules/health/indicators/account-sync.health.ts, adding calendar sync metrics - Reorganizes health status components into dedicated
/health-statusfolder structure for better code organization - Simplifies
SystemHealthDTO in/packages/twenty-server/src/engine/core-modules/admin-panel/dtos/system-health.dto.tsto use direct status enums - Adds calendar sync health monitoring with 20% failure rate threshold and timeout handling
- Integrates health monitoring into calendar event import system via
HealthModulein related modules
Note: The PR description references issues about keyboard shortcuts and soft focus mode, but the actual changes are unrelated to those issues.
39 file(s) reviewed, 14 comment(s)
Edit PR Review Bot Settings | Greptile
| the service is down | ||
| </StyledErrorMessage> | ||
| ) : ( | ||
| <StyledDetailsContainer>{formattedDetails}</StyledDetailsContainer> |
There was a problem hiding this comment.
style: formattedDetails could be null here. Consider adding a fallback message when no details are available.
...t/src/modules/settings/admin-panel/health-status/components/DatabaseAndRedisHealthStatus.tsx
Outdated
Show resolved
Hide resolved
...ettings/admin-panel/health-status/components/SettingsAdminHealthAccountSyncCountersTable.tsx
Outdated
Show resolved
Hide resolved
| details, | ||
| title, | ||
| }: { | ||
| details: Record<string, any> | null; |
There was a problem hiding this comment.
style: Using Record<string, any> is too permissive. Consider creating a specific type for the details object structure
...wenty-front/src/modules/settings/admin-panel/health-status/components/AccountSyncMetrics.tsx
Outdated
Show resolved
Hide resolved
| accountSyncHealth.isHealthy.mockRejectedValue( | ||
| new Error(HEALTH_ERROR_MESSAGES.MESSAGE_SYNC_CHECK_FAILED), | ||
| ); |
There was a problem hiding this comment.
logic: Using MESSAGE_SYNC_CHECK_FAILED error for accountSync is inconsistent with the service name change. Should use a new ACCOUNT_SYNC_CHECK_FAILED error message.
| const currentCounter = | ||
| await this.cacheStorage.get<AccountSyncJobByStatusCounter>(cacheKey); | ||
|
|
||
| const updatedCounter = { | ||
| ...(currentCounter || {}), | ||
| [status]: (currentCounter?.[status] || 0) + increment, | ||
| }; |
There was a problem hiding this comment.
style: Duplicate code pattern between message and calendar sync counters. Consider extracting to a shared method to reduce duplication.
| expect(result.accountSync.status).toBe('up'); | ||
| expect(result.accountSync.details.messageSync.status).toBe('up'); | ||
| expect(result.accountSync.details.calendarSync.status).toBe('down'); |
There was a problem hiding this comment.
logic: Test case may be misleading - overall status is 'up' despite calendar sync being 'down'. Consider verifying if this is the intended behavior
| async isHealthy(): Promise<HealthIndicatorResult> { | ||
| const [messageResult, calendarResult] = await Promise.all([ | ||
| this.checkMessageSyncHealth(), | ||
| this.checkCalendarSyncHealth(), | ||
| ]); | ||
|
|
||
| const indicator = this.healthIndicatorService.check('accountSync'); | ||
| const details = { | ||
| messageSync: messageResult.messageSync, | ||
| calendarSync: calendarResult.calendarSync, | ||
| }; | ||
|
|
||
| return indicator.up({ details }); | ||
| } |
There was a problem hiding this comment.
logic: isHealthy() always returns 'up' status regardless of message/calendar sync health states. Should return 'down' if either sync check fails.
| async isHealthy(): Promise<HealthIndicatorResult> { | |
| const [messageResult, calendarResult] = await Promise.all([ | |
| this.checkMessageSyncHealth(), | |
| this.checkCalendarSyncHealth(), | |
| ]); | |
| const indicator = this.healthIndicatorService.check('accountSync'); | |
| const details = { | |
| messageSync: messageResult.messageSync, | |
| calendarSync: calendarResult.calendarSync, | |
| }; | |
| return indicator.up({ details }); | |
| } | |
| async isHealthy(): Promise<HealthIndicatorResult> { | |
| const [messageResult, calendarResult] = await Promise.all([ | |
| this.checkMessageSyncHealth(), | |
| this.checkCalendarSyncHealth(), | |
| ]); | |
| const indicator = this.healthIndicatorService.check('accountSync'); | |
| const details = { | |
| messageSync: messageResult.messageSync, | |
| calendarSync: calendarResult.calendarSync, | |
| }; | |
| if (messageResult.status === 'down' || calendarResult.status === 'down') { | |
| return indicator.down({ details }); | |
| } | |
| return indicator.up({ details }); | |
| } |
There was a problem hiding this comment.
these are just metrics though
There was a problem hiding this comment.
I agree with Greptile's comment. If we use an indicator to wrap thing then it should be a real indicator. Also true for the UI. What is the point of setting a failure rate threshold otherwise?
...r/src/modules/calendar/calendar-event-import-manager/calendar-event-import-manager.module.ts
Outdated
Show resolved
Hide resolved
| failureRate, | ||
| }; | ||
|
|
||
| if (totalJobs === 0 || failureRate < 20) { |
There was a problem hiding this comment.
Extract failure rate threshold as a const
| @@ -4,7 +4,7 @@ export enum AdminPanelIndicatorHealthStatusInputEnum { | |||
| DATABASE = 'database', | |||
| REDIS = 'redis', | |||
| WORKER = 'worker', | |||
| MESSAGE_SYNC = 'messageSync', | |||
| ACCOUNT_SYNC = 'accountSync', | |||
There was a problem hiding this comment.
I think we used "connectedAccount" in the codebase so I probably would have stuck to that, "Connected Account Status" in the admin panel , etc
| isLoading={loading} | ||
| RowRightComponent={({ item: service }) => ( | ||
| <> | ||
| {service.id === 'ACCOUNT_SYNC' ? ( |
There was a problem hiding this comment.
Let's make it a status like others
| const services = [ | ||
| { | ||
| id: 'DATABASE', | ||
| name: 'Database Status', |
There was a problem hiding this comment.
Also use this in breadcrumb?
closes twentyhq/core-team-issues#444
twentyhq/core-team-issues#443
twentyhq/core-team-issues#442