Fix caldav issues #16297
Fix caldav issues #16297
Conversation
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
| return null; | ||
| } | ||
|
|
||
| return ( | ||
| <SettingsAccountsConfigurationStepCalendar | ||
| calendarChannel={calendarChannel} | ||
| messageChannel={messageChannel} | ||
| isSubmitting={isSubmitting} | ||
| onAddAccount={handleAddAccount} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
Bug: SettingsAccountsConfiguration renders SettingsAccountsConfigurationStepCalendar when currentStep is Email for CALDAV-only accounts due to missing messageChannel check.
Severity: HIGH | Confidence: High
🔍 Detailed Analysis
When currentStep is SettingsAccountsConfigurationStep.Email for an account that only has CALDAV configured (i.e., messageChannel is undefined and calendarChannel is defined), the component incorrectly renders SettingsAccountsConfigurationStepCalendar. This occurs because the showEmailStep condition currentStep === SettingsAccountsConfigurationStep.Email && isDefined(messageChannel) evaluates to false, and the subsequent check !isDefined(calendarChannel) also evaluates to false, causing the SettingsAccountsConfigurationStepCalendar component to be rendered by default. This leads to a confusing user experience where the Calendar configuration UI is shown instead of the expected Email configuration UI.
💡 Suggested Fix
Reintroduce an explicit check for isDefined(messageChannel) when currentStep is SettingsAccountsConfigurationStep.Email to ensure the correct step is rendered or null is returned if messageChannel is not present.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location:
packages/twenty-front/src/pages/settings/accounts/SettingsAccountsConfiguration.tsx#L102-L129
Potential issue: When `currentStep` is `SettingsAccountsConfigurationStep.Email` for an
account that only has CALDAV configured (i.e., `messageChannel` is `undefined` and
`calendarChannel` is `defined`), the component incorrectly renders
`SettingsAccountsConfigurationStepCalendar`. This occurs because the `showEmailStep`
condition `currentStep === SettingsAccountsConfigurationStep.Email &&
isDefined(messageChannel)` evaluates to `false`, and the subsequent check
`!isDefined(calendarChannel)` also evaluates to `false`, causing the
`SettingsAccountsConfigurationStepCalendar` component to be rendered by default. This
leads to a confusing user experience where the Calendar configuration UI is shown
instead of the expected Email configuration UI.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 5265647
Greptile OverviewGreptile SummaryThis PR fixes several CalDAV-related issues:
Confidence Score: 4/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant User
participant Frontend
participant AccountAPI
participant CalendarService
participant CalDAVDriver
User->>Frontend: Configure CalDAV account
Frontend->>AccountAPI: Process account with parameters
AccountAPI->>AccountAPI: Check CALDAV in connection params
alt CALDAV exists
AccountAPI->>AccountAPI: Create calendar channel
end
alt IMAP exists
AccountAPI->>AccountAPI: Create message channel
end
AccountAPI-->>Frontend: Return account ID
User->>Frontend: Start calendar sync
Frontend->>CalendarService: Validate connection
CalendarService->>CalendarService: Check CALDAV field (bug fix)
alt CALDAV present
CalendarService->>CalendarService: Verify credentials
end
CalendarService-->>Frontend: Connection validated
Frontend->>CalDAVDriver: Fetch calendar events
CalDAVDriver->>CalDAVDriver: List available calendars
CalDAVDriver->>CalDAVDriver: Normalize file extensions to lowercase
CalDAVDriver->>CalDAVDriver: Filter valid formats
CalDAVDriver-->>Frontend: Return calendar events
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const showEmailStep = | ||
| currentStep === SettingsAccountsConfigurationStep.Email && | ||
| isDefined(messageChannel); | ||
|
|
||
| if (showEmailStep) { | ||
| return ( | ||
| <SettingsAccountsConfigurationStepEmail | ||
| messageChannel={messageChannel} | ||
| hasNextStep={isDefined(calendarChannel)} | ||
| isSubmitting={isSubmitting} | ||
| onNext={handleNext} | ||
| onAddAccount={handleAddAccount} | ||
| /> | ||
| ); | ||
| } | ||
|
|
||
| if (!isDefined(calendarChannel)) { | ||
| return null; | ||
| } | ||
|
|
||
| return ( | ||
| <SettingsAccountsConfigurationStepCalendar | ||
| calendarChannel={calendarChannel} | ||
| messageChannel={messageChannel} | ||
| isSubmitting={isSubmitting} | ||
| onAddAccount={handleAddAccount} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
logic: Potential logic change: the new code will always check currentStep for the email step but will render the calendar step for ANY other step value (not just SettingsAccountsConfigurationStep.Calendar). The old switch statement would implicitly return undefined for unknown step values.
If currentStep can only be Email or Calendar, this is fine. Otherwise, you may want to add an explicit check:
| const showEmailStep = | |
| currentStep === SettingsAccountsConfigurationStep.Email && | |
| isDefined(messageChannel); | |
| if (showEmailStep) { | |
| return ( | |
| <SettingsAccountsConfigurationStepEmail | |
| messageChannel={messageChannel} | |
| hasNextStep={isDefined(calendarChannel)} | |
| isSubmitting={isSubmitting} | |
| onNext={handleNext} | |
| onAddAccount={handleAddAccount} | |
| /> | |
| ); | |
| } | |
| if (!isDefined(calendarChannel)) { | |
| return null; | |
| } | |
| return ( | |
| <SettingsAccountsConfigurationStepCalendar | |
| calendarChannel={calendarChannel} | |
| messageChannel={messageChannel} | |
| isSubmitting={isSubmitting} | |
| onAddAccount={handleAddAccount} | |
| /> | |
| ); | |
| if (showEmailStep) { | |
| return ( | |
| <SettingsAccountsConfigurationStepEmail | |
| messageChannel={messageChannel} | |
| hasNextStep={isDefined(calendarChannel)} | |
| isSubmitting={isSubmitting} | |
| onNext={handleNext} | |
| onAddAccount={handleAddAccount} | |
| /> | |
| ); | |
| } | |
| if (currentStep !== SettingsAccountsConfigurationStep.Calendar || !isDefined(calendarChannel)) { | |
| return null; | |
| } | |
| return ( | |
| <SettingsAccountsConfigurationStepCalendar | |
| calendarChannel={calendarChannel} | |
| messageChannel={messageChannel} | |
| isSubmitting={isSubmitting} | |
| onAddAccount={handleAddAccount} | |
| /> | |
| ); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/twenty-front/src/pages/settings/accounts/SettingsAccountsConfiguration.tsx
Line: 102:129
Comment:
**logic:** Potential logic change: the new code will always check `currentStep` for the email step but will render the calendar step for ANY other step value (not just `SettingsAccountsConfigurationStep.Calendar`). The old switch statement would implicitly return `undefined` for unknown step values.
If `currentStep` can only be `Email` or `Calendar`, this is fine. Otherwise, you may want to add an explicit check:
```suggestion
if (showEmailStep) {
return (
<SettingsAccountsConfigurationStepEmail
messageChannel={messageChannel}
hasNextStep={isDefined(calendarChannel)}
isSubmitting={isSubmitting}
onNext={handleNext}
onAddAccount={handleAddAccount}
/>
);
}
if (currentStep !== SettingsAccountsConfigurationStep.Calendar || !isDefined(calendarChannel)) {
return null;
}
return (
<SettingsAccountsConfigurationStepCalendar
calendarChannel={calendarChannel}
messageChannel={messageChannel}
isSubmitting={isSubmitting}
onAddAccount={handleAddAccount}
/>
);
```
How can I resolve this? If you propose a fix, please make it concise.
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:2076 This environment will automatically shut down when the PR is closed or after 5 hours. |
|
Hey @charlesBochet! After you've done the QA of your Pull Request, you can mark it as done here. Thank you! |
No description provided.