Enable editing for calendar event custom fields#17063
Enable editing for calendar event custom fields#17063charlesBochet merged 8 commits intotwentyhq:mainfrom
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:18291 This environment will automatically shut down when the PR is closed or after 5 hours. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-front/src/modules/activities/calendar/components/CalendarEventDetails.tsx">
<violation number="1" location="packages/twenty-front/src/modules/activities/calendar/components/CalendarEventDetails.tsx:136">
P2: The `loading` state is hardcoded to `false`, so the UI won't show loading feedback during updates. Consider using the actual loading state from `useUpdateOneRecord` if the mutation provides one, or document why loading state is intentionally disabled for calendar events.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| }); | ||
| }; | ||
|
|
||
| return [updateEntity, { loading: false }]; |
There was a problem hiding this comment.
P2: The loading state is hardcoded to false, so the UI won't show loading feedback during updates. Consider using the actual loading state from useUpdateOneRecord if the mutation provides one, or document why loading state is intentionally disabled for calendar events.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-front/src/modules/activities/calendar/components/CalendarEventDetails.tsx, line 136:
<comment>The `loading` state is hardcoded to `false`, so the UI won't show loading feedback during updates. Consider using the actual loading state from `useUpdateOneRecord` if the mutation provides one, or document why loading state is intentionally disabled for calendar events.</comment>
<file context>
@@ -113,11 +121,43 @@ export const CalendarEventDetails = ({
+ });
+ };
+
+ return [updateEntity, { loading: false }];
+ };
+
</file context>
✅ Addressed in fa40a7c
|
Updated the calendar event update hook to track the async update and surface the loading state instead of hardcoding false. |
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-front/src/modules/activities/calendar/components/CalendarEventDetails.tsx">
<violation number="1" location="packages/twenty-front/src/modules/activities/calendar/components/CalendarEventDetails.tsx:130">
P1: Using `useState` inside a function defined within a component body violates React's Rules of Hooks. The function `useUpdateOneCalendarEventRecordMutation` is recreated on every render and contains a `useState` call that executes when child components invoke it. Consider moving the loading state to the component level or using `useCallback` with state managed at the parent level.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| }); | ||
|
|
||
| const useUpdateOneCalendarEventRecordMutation: RecordUpdateHook = () => { | ||
| const [isUpdating, setIsUpdating] = useState(false); |
There was a problem hiding this comment.
P1: Using useState inside a function defined within a component body violates React's Rules of Hooks. The function useUpdateOneCalendarEventRecordMutation is recreated on every render and contains a useState call that executes when child components invoke it. Consider moving the loading state to the component level or using useCallback with state managed at the parent level.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-front/src/modules/activities/calendar/components/CalendarEventDetails.tsx, line 130:
<comment>Using `useState` inside a function defined within a component body violates React's Rules of Hooks. The function `useUpdateOneCalendarEventRecordMutation` is recreated on every render and contains a `useState` call that executes when child components invoke it. Consider moving the loading state to the component level or using `useCallback` with state managed at the parent level.</comment>
<file context>
@@ -126,14 +127,19 @@ export const CalendarEventDetails = ({
});
const useUpdateOneCalendarEventRecordMutation: RecordUpdateHook = () => {
+ const [isUpdating, setIsUpdating] = useState(false);
+
const updateEntity = ({ variables }: RecordUpdateHookParams) => {
</file context>
✅ Addressed in 1695b3c
|
@cubic-dev-ai moved the loading state to the component level and now return it via a memoized update hook, so no hooks are called inside the nested function. |
|
@salmonumbrella Thanks for opening a PR, much appreciated However, I'm not sure what we are trying to do here. Is there any issue describing the change? |
|
I'm going to close the PR for now but let's keep discussing here and you can also re-open it by commenting /twenty pr open |
|
@charlesBochet Thanks for the quick review! To clarify the use case: I created a custom field (via the API) that links to a Notion meeting page for a calendar event. This custom field lives entirely within Twenty and is separate from the synced Google Calendar data. The issue is that I'm unable to edit the URL for this custom field on the frontend. Since custom fields don't touch the actual Google Calendar variables, there's no risk of data corruption or sync conflicts—it's purely Twenty-side metadata. This PR enables editing only for custom fields while keeping the standard calendar fields (title, dates, participants, etc.) read-only as intended. Example of frontend UI: |
|
/twenty pr open |
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
| const [isUpdating, setIsUpdating] = useState(false); | ||
| const updateEntity = useCallback( | ||
| ({ variables }: RecordUpdateHookParams) => { | ||
| if (!updateOneRecord) return; |
There was a problem hiding this comment.
why this check? updateOneRecord cannot be undefined
packages/twenty-front/src/modules/activities/calendar/components/CalendarEventDetails.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/activities/calendar/components/CalendarEventDetails.tsx
Outdated
Show resolved
Hide resolved
...nt/src/modules/command-menu/pages/calendar-event/components/CommandMenuCalendarEventPage.tsx
Outdated
Show resolved
Hide resolved
| objectRecordId: viewableRecordId ?? '', | ||
| recordGqlFields: FIND_ONE_CALENDAR_EVENT_OPERATION_SIGNATURE.fields, |
There was a problem hiding this comment.
is FIND_ONE_CALENDAR_EVENT_OPERATION_SIGNATURE use elsewhere? If not we should remove it, if yes, there is likely a regression as signatures are here to make sure queries are consistent while making cache udpates
|
Thanks @charlesBochet! Addressed the notes: removed the updateOneRecord guard + standardFieldNames Set, switched to the shared read-only helper, dropped the unnecessary memoization, and removed the unused operation signature. Pushed in 346c2d5. |
| [updateOneRecord], | ||
| ); | ||
|
|
||
| const useUpdateOneCalendarEventRecordMutation: RecordUpdateHook = useCallback( |
| objectMetadataId: objectMetadataItem.id, | ||
| }); | ||
|
|
||
| const isFieldReadOnly = (fieldMetadataItem: FieldMetadataItem) => { |
There was a problem hiding this comment.
do we still need this method?
charlesBochet
left a comment
charlesBochet
left a comment
There was a problem hiding this comment.
Left a last round of comment, please re-test and then we are good to go, ty!
|
Please also fix the lint :) |
|
@charlesBochet fixed lint by making the boolean predicate explicit in the workflow story (commit 6c6b9d5). Ran successfully (only warnings about ignored generated files). |
|
@charlesBochet (follow-up) lint now clean on diff: ran nx lint:diff-with-main successfully; only warnings are for ignored generated files. |
|
@salmonumbrella I've left two comments (still not resolved) above too |
|
@charlesBochet addressed the two latest comments in CalendarEventDetails: removed the memoized wrapper for useUpdateRecord and inlined the read-only helper (commit 7153a3d). |
|
Thank you! |
|
Hey @charlesBochet! After you've done the QA of your Pull Request, you can mark it as done here. Thank you! |
LogDetails |
|
@salmonumbrella Just tried on our pre-prod environment, and I can't edit a textField on calendarEvent, could you check? |
|
I created a custom field called "Notion" on the CalendarEvent object via the API. The field appears in the Calendar Event details panel, but it's not editable - it shows as read-only like the standard fields. Expected: Custom fields (where Possible causes I investigated:
@charlesBochet Could you help investigate? |
2 participants
Summary
Testing
Notes