-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: record group add new #8925
Conversation
Closing for now, let's re-open once ready to keep the history clean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
This PR adds an "Add new" button feature for record groups and significantly improves context management across the codebase. Here's a concise summary of the key changes:
- Introduced
/utils/create-required-context.ts
utility to prevent silent failures when contexts are used outside providers, replacing unsafe empty object context initialization pattern - Added
RecordTableRecordGroupSectionAddNew
component with "Add new" button functionality for record groups - Added
recordTablePendingRecordIdByGroupComponentFamilyState
to track pending records per group - Added
RecordTablePendingRecordGroupRow
to handle pending record UI within groups - Refactored record creation logic from
RecordIndexPage
to be closer to where it's used, with proper group-specific handling
63 file(s) reviewed, 22 comment(s)
Edit PR Review Bot Settings | Greptile
packages/twenty-front/src/modules/object-record/record-group/hooks/useCurrentRecordGroupId.ts
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/object-record/record-group/hooks/useCurrentRecordGroupId.ts
Outdated
Show resolved
Hide resolved
...ges/twenty-front/src/modules/object-record/record-index/components/RecordIndexPageHeader.tsx
Show resolved
Hide resolved
...ules/object-record/record-table/components/__stories__/perf/RecordTableCell.perf.stories.tsx
Show resolved
Hide resolved
...-record/record-table/record-table-section/components/RecordTableRecordGroupSectionAddNew.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/testing/decorators/RecordTableDecorator.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/object-record/record-group/hooks/useCurrentRecordGroupId.ts
Outdated
Show resolved
Hide resolved
...wenty-front/src/modules/object-record/record-table/components/RecordTableContextProvider.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the approach seems fine to me, I have doubt on:
- wether we should try to make the new record logic work with both no record group and record groups or if we should duplicate it so simplify a bit the logic. even, If we choose to handle both in the same place, I think we should split very early in the logic to choose if we are in RecordGroup case and not call any recordGroup logic if we know we are not in a recordGroup case (not a fan of the overload with allow undefined)
- I don't fully understand the need for "create-required-context". I think the devXP was confusing and you want to throw a clear error which is a good idea but wouldn't a useContextOrThrow work with less complexity? we create a regular context and when we read it only we throw?
packages/twenty-front/src/modules/object-record/record-group/hooks/useCurrentRecordGroupId.ts
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/object-record/record-table/hooks/useCreateNewTableRecords.ts
Show resolved
Hide resolved
...odules/object-record/record-table/record-table-cell/components/RecordTableCellFieldInput.tsx
Outdated
Show resolved
Hide resolved
import { PageAddButton } from '@/ui/layout/page/components/PageAddButton'; | ||
import { PageHotkeysEffect } from '@/ui/layout/page/components/PageHotkeysEffect'; | ||
|
||
export const RecordIndexPageTableAddButtonNoGroup = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be in record table or record index ? does it work for Board too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it doesn't work for RecordBoard for now, I put a TODO
because to make it work we need to merge some states.
I put the file in this directory because the Kanban one is already there, I think we should move them during the refactor.
import { PageAddButton } from '@/ui/layout/page/components/PageAddButton'; | ||
import { PageHotkeysEffect } from '@/ui/layout/page/components/PageHotkeysEffect'; | ||
|
||
export const RecordIndexPageTableAddButtonNoGroup = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
love it, much clearer to me
children?: ReactNode; | ||
}; | ||
|
||
export const RecordTableNoRecordGroupBodyContextProvider = ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bravo @magrinj, very clear and easy to read and understand
Thanks for all the side improvements too (removed unused props, useless wrapping)
nice we have new context api too
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Fix #8757
This PR is adding the Add new button on view groups.
Also this PR fix an issue where the pending record can be draggable, and is causing error.
It also start to issues with the way we're using Context.
We're initializing pretty much all Context like this:
This is causing issues when by mistake we use the context like this outside the Provider hierarchy:
This is going to fail silently, and all the context variables become undefined...
To fix this I've introduced an util called
createRequiredContext
, this one is returning an array containing the provider and the hook to retrieve the context.The context is initialized to undefined inside this utility, this way we can check if the value has been initialized with the provider to check if we're inside it. It'll throw an error if this one is used outside the provider.
The return values are properly typed, so
undefined
is not added to the value of the Context.I'll create a followup ticket to use this new utility function, if that's ok and replace it everywhere in the codebase.
We can also consider adding a eslint rule to warn about the use of
createContext
directly.