Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
WalkthroughThe changes introduce the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant API
User->>UI: Request to view persons
UI->>API: Fetch persons data
API-->>UI: Return persons data
UI->>User: Display persons in a table
User->>UI: Adjust column order (drag-and-drop)
UI->>User: Update table display
User->>UI: Request to delete a person
UI->>API: Send delete request
API-->>UI: Confirm deletion
UI->>User: Update UI to reflect changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
apps/web/app/(app)/environments/[environmentId]/(people)/people/components/PersonTableColumn.tsx (1)
81-84: Consider adding a cell rendering logic for the email column.The email column is currently defined without a specific cell rendering logic. To display the email value in the table, you can add a
cellproperty to theemailColumndefinition, similar to the other columns.Here's an example of how you can modify the
emailColumndefinition:const emailColumn: ColumnDef<TPersonTableData> = { accessorKey: "email", header: "Email", + cell: ({ row }) => { + const email = row.original.email; + return <p className="truncate text-slate-900">{email}</p>; + }, };
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (34)
- apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/AddQuestionButton.tsx (1 hunks)
- apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/EditorCardMenu.tsx (1 hunks)
- apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/QuestionCard.tsx (1 hunks)
- apps/web/app/(app)/environments/[environmentId]/(people)/attributes/loading.tsx (2 hunks)
- apps/web/app/(app)/environments/[environmentId]/(people)/attributes/page.tsx (2 hunks)
- apps/web/app/(app)/environments/[environmentId]/(people)/people/[personId]/components/DeletePersonButton.tsx (1 hunks)
- apps/web/app/(app)/environments/[environmentId]/(people)/people/actions.ts (1 hunks)
- apps/web/app/(app)/environments/[environmentId]/(people)/people/components/PersonCard.tsx (0 hunks)
- apps/web/app/(app)/environments/[environmentId]/(people)/people/components/PersonDataView.tsx (1 hunks)
- apps/web/app/(app)/environments/[environmentId]/(people)/people/components/PersonSecondaryNavigation.tsx (1 hunks)
- apps/web/app/(app)/environments/[environmentId]/(people)/people/components/PersonTable.tsx (1 hunks)
- apps/web/app/(app)/environments/[environmentId]/(people)/people/components/PersonTableColumn.tsx (1 hunks)
- apps/web/app/(app)/environments/[environmentId]/(people)/people/components/pagination.tsx (0 hunks)
- apps/web/app/(app)/environments/[environmentId]/(people)/people/loading.tsx (2 hunks)
- apps/web/app/(app)/environments/[environmentId]/(people)/people/page.tsx (2 hunks)
- apps/web/app/(app)/environments/[environmentId]/(people)/segments/loading.tsx (2 hunks)
- apps/web/app/(app)/environments/[environmentId]/(people)/segments/page.tsx (2 hunks)
- apps/web/app/(app)/environments/[environmentId]/integrations/notion/components/AddIntegrationModal.tsx (1 hunks)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseTable.tsx (6 hunks)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseTableCell.tsx (1 hunks)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseTableColumns.tsx (2 hunks)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/QuestionSummaryHeader.tsx (1 hunks)
- packages/lib/person/service.ts (2 hunks)
- packages/lib/utils/questions.tsx (1 hunks)
- packages/lib/utils/templates.ts (1 hunks)
- packages/types/people.ts (2 hunks)
- packages/ui/DataTable/components/ColumnSettingsDropdown.tsx (1 hunks)
- packages/ui/DataTable/components/DataTableHeader.tsx (1 hunks)
- packages/ui/DataTable/components/DataTableSettingsModal.tsx (2 hunks)
- packages/ui/DataTable/components/DataTableSettingsModalItem.tsx (2 hunks)
- packages/ui/DataTable/components/DataTableToolbar.tsx (1 hunks)
- packages/ui/DataTable/components/SelectedRowSettings.tsx (4 hunks)
- packages/ui/DataTable/index.tsx (1 hunks)
- packages/ui/DataTable/lib/utils.ts (1 hunks)
Files not reviewed due to no reviewable changes (2)
- apps/web/app/(app)/environments/[environmentId]/(people)/people/components/PersonCard.tsx
- apps/web/app/(app)/environments/[environmentId]/(people)/people/components/pagination.tsx
Files skipped from review due to trivial changes (13)
- apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/EditorCardMenu.tsx
- apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/QuestionCard.tsx
- apps/web/app/(app)/environments/[environmentId]/(people)/attributes/loading.tsx
- apps/web/app/(app)/environments/[environmentId]/(people)/attributes/page.tsx
- apps/web/app/(app)/environments/[environmentId]/(people)/people/components/PersonSecondaryNavigation.tsx
- apps/web/app/(app)/environments/[environmentId]/(people)/people/loading.tsx
- apps/web/app/(app)/environments/[environmentId]/(people)/segments/loading.tsx
- apps/web/app/(app)/environments/[environmentId]/(people)/segments/page.tsx
- apps/web/app/(app)/environments/[environmentId]/integrations/notion/components/AddIntegrationModal.tsx
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseTableColumns.tsx
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/QuestionSummaryHeader.tsx
- packages/lib/person/service.ts
- packages/lib/utils/templates.ts
Additional context used
Biome
packages/lib/utils/questions.tsx
[error] 235-235: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
Additional comments not posted (55)
packages/ui/DataTable/index.tsx (1)
1-5: LGTM! This is a good pattern for organizing and exporting related components.The introduction of this
index.tsxfile as a central export hub for theDataTableHeader,DataTableSettingsModal, andDataTableToolbarcomponents brings several benefits:
It enhances the modularity and reusability of these components by making them easily accessible from a single entry point.
It allows other parts of the application to import these components directly from the
DataTablemodule, streamlining the import process.It promotes better organization of the codebase by consolidating exports related to the DataTable component.
This pattern is commonly used in React projects and aligns with best practices for structuring and exporting components.
packages/ui/DataTable/lib/utils.ts (1)
4-11: LGTM!The
getCommonPinningStylesutility function is well-implemented and provides a reusable solution for generating common CSS styles for pinned columns in a data table. The function correctly calculates the necessary styles, such as the left position, sticky positioning, width, and z-index, to ensure proper alignment and behavior of pinned columns.The use of TypeScript generics and the
Column<T>type enhances type safety and flexibility, allowing the function to work with different types of data tables.Overall, this utility function promotes code modularity, maintainability, and improves the visual presentation and usability of data tables by enabling fixed columns during scrolling.
packages/types/people.ts (2)
12-17: LGTM!The new
ZPersonTableDataschema is well-structured and includes all necessary fields for validating person table data. The use of Zod for validation is a good practice to ensure data integrity. Referencing the existingZAttributesschema promotes code reuse and consistency.
19-19: LGTM!Inferring the
TPersonTableDatatype from theZPersonTableDataschema is a good practice for maintaining type safety. The inferred type can be used throughout the codebase for type-safe usage of person table data, reducing the chances of type-related errors.packages/ui/DataTable/components/ColumnSettingsDropdown.tsx (3)
3-3: LGTM!The simplified import statement for
DropdownMenucomponents looks good and improves readability.
5-7: Great work on making the component more flexible!The introduction of the generic type parameter
<T>in theColumnSettingsDropdownPropsinterface and the corresponding update to thecolumnprop type enhances the reusability of the component. It allows the component to work with different types of column data, making it more versatile.
10-13: Excellent update to the component declaration!The
ColumnSettingsDropdowncomponent now correctly uses the generic type parameter<T>and the updatedColumnSettingsDropdownProps<T>type. This ensures that the component is properly typed and aligned with the changes made to theColumnSettingsDropdownPropsinterface. Well done!apps/web/app/(app)/environments/[environmentId]/(people)/people/actions.ts (2)
16-26: LGTM!The
getPersonsActionis well-structured and follows best practices:
- It uses
authenticatedActionClientto ensure only authorized users can access the data.- It validates the input using Zod schema
ZGetPersonsAction.- It checks the user's authorization, confirming that the user has read access to the environment.
- It uses the
getPeopleservice to fetch the paginated list of persons.The implementation looks good and is ready to be merged.
33-43: LGTM!The
getPersonAttributesActionis well-structured and follows best practices:
- It uses
authenticatedActionClientto ensure only authorized users can access the data.- It validates the input using Zod schema
ZGetPersonAttributesAction.- It checks the user's authorization, confirming that the user has read access to the environment.
- It uses the
getAttributesservice to fetch the attributes for the specified person.The implementation looks good and is ready to be merged.
apps/web/app/(app)/environments/[environmentId]/(people)/people/page.tsx (5)
1-2: LGTM!The imports are consistent with the new naming convention and use the correct relative paths.
6-6: LGTM!The import is consistent with the changes mentioned in the AI summary and uses the correct path.
11-13: LGTM!The changes to the
Pagecomponent are consistent with the AI summary:
- The
searchParamsprop has been removed, indicating that pagination is no longer handled in this component.- The
getPersonCountfunction is used instead ofgetPeopleandgetPeopleCount, simplifying the data fetching logic.
33-33: LGTM!The
PersonSecondaryNavigationcomponent usage is consistent with the new naming convention and the props are passed correctly.
35-35: LGTM!The usage of the
PersonDataViewcomponent is consistent with the changes mentioned in the AI summary:
- The
environment,personCount, anditemsPerPageprops are passed correctly.- The
ITEMS_PER_PAGEconstant is used for theitemsPerPageprop, ensuring consistency with the rest of the application.apps/web/app/(app)/environments/[environmentId]/(people)/people/[personId]/components/DeletePersonButton.tsx (1)
8-8: Approve the import change for improved modularity and reusability.The change in the import source of
deletePersonActionfrom a local path to@formbricks/ui/DataTable/actionsis a good refactor. It promotes a more modular and reusable approach for the deletion action, potentially allowing for better maintainability and consistency across different components that may utilize this action.The functionality of the
DeletePersonButtoncomponent remains intact, as the action itself hasn't changed, only its import source.packages/ui/DataTable/components/DataTableToolbar.tsx (5)
4-4: LGTM!The import path has been updated to a relative path, which is a good practice for maintainability.
5-5: LGTM!The
SelectedRowSettingscomponent is being imported correctly.
7-13: LGTM!The
DataTableToolbarPropsinterface has been updated to be more generic and flexible:
- The use of a generic type parameter
<T>allows the component to work with various data structures.- The renaming of
deleteResponsestodeleteRowsaligns with the generic nature of the component.- The addition of the
typeprop allows for conditional rendering or behavior based on the type of data.These changes enhance the component's reusability and maintainability.
16-23: LGTM!The component has been updated to align with the changes made to the
DataTableToolbarPropsinterface:
- The component name has been changed to
DataTableToolbar, reflecting its broader application.- The component now uses the generic type parameter
<T>, enhancing its flexibility.- The
deleteRowsandtypeprops have been added to the component's destructured props, aligning with the updated interface.These changes improve the component's reusability and maintainability.
27-27: LGTM!The
SelectedRowSettingscomponent is being used correctly:
- The component is being passed the necessary props (
table,deleteRows, andtype).- This change suggests a unified approach to handling selected rows across different data types.
The usage of the
SelectedRowSettingscomponent aligns with the overall refactoring of theDataTableToolbarcomponent.apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseTableCell.tsx (1)
5-5: LGTM!The updated import path for
getCommonPinningStylesaligns with the objective of improving code organization and maintainability by sourcing utility functions from a centralized location. This change promotes code reusability without altering the functionality of theResponseTableCellcomponent.apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/AddQuestionButton.tsx (1)
8-12: LGTM! Verify the new module path.The import path change for utility functions related to questions looks good. It indicates a potential restructuring of the codebase for better modularity and clarity.
Please ensure that the new module path
@formbricks/lib/utils/questionsis correct and the imported functions (getQuestionDefaults,questionTypes, anduniversalQuestionPresets) are available at the new location.Run the following script to verify the module path and function availability:
Verification successful
Verification successful: Import path change is correct and consistent
The new import path
@formbricks/lib/utils/questionsforgetQuestionDefaults,questionTypes, anduniversalQuestionPresetshas been verified:
- The functions are exported from
packages/lib/utils/questions.tsx.- Multiple files in the project use the same import path.
This change aligns with the codebase structure and improves consistency across the project.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the new module path and function availability. # Test 1: Check if the new module path exists. Expect: The module path should exist. fd --type d --hidden --exclude node_modules '@formbricks/lib/utils/questions' # Test 2: Check if the imported functions are available in the new module. Expect: The functions should be exported from the module. rg --type typescript -g '@formbricks/lib/utils/questions' -e $'export (const|function) (getQuestionDefaults|questionTypes|universalQuestionPresets)'Length of output: 260
Script:
#!/bin/bash # Description: Verify the new module path and function availability with adjusted approach. # Test 1: Search for the 'questions.ts' file in the entire repository echo "Searching for questions.ts file:" fd questions.ts # Test 2: If found, check the content of the file for the exported functions echo "Checking content of questions.ts file(s):" fd questions.ts -x grep -n 'export \(const\|function\) \(getQuestionDefaults\|questionTypes\|universalQuestionPresets\)' # Test 3: Search for imports of these functions in the entire repository echo "Searching for imports of the functions:" grep -r --include="*.ts" --include="*.tsx" "import.*{.*\(getQuestionDefaults\|questionTypes\|universalQuestionPresets\).*} from" .Length of output: 1318
packages/ui/DataTable/components/DataTableSettingsModal.tsx (5)
13-13: LGTM!The import path update for the
Modalcomponent is correct and does not affect the functionality.
14-14: LGTM!The import statement for the
DataTableSettingsModalItemcomponent is correct and relevant to the component.
16-22: LGTM!The updates to the
DataTableSettingsModalPropsinterface are beneficial:
- The generic type parameter
Tallows the component to work with various table data types, enhancing its flexibility.- Making the
surveyprop optional provides flexibility in using the component in different contexts.These changes improve the reusability and modularity of the component.
25-32: LGTM!The
DataTableSettingsModalcomponent has been correctly updated to:
- Use the generic type parameter
T, consistent with the updated props interface.- Destructure the optional
surveyprop from the props.These changes align with the updates made to the
DataTableSettingsModalPropsinterface.
65-68: LGTM!The updates to the rendering logic within the component are appropriate:
- The exclusion of "select" and "createdAt" columns from rendering is a deliberate choice based on the component's requirements.
- The
DataTableSettingsModalItemcomponent is now used for rendering the modal items, reflecting the renaming of the component.- The
surveyprop is correctly passed to theDataTableSettingsModalItemcomponent.These changes align with the overall updates made to the component.
packages/ui/DataTable/components/DataTableSettingsModalItem.tsx (4)
7-7: LGTM!The import path has been correctly updated to use the new path from
@formbricks/lib/utils/questions. This change aligns with the refactoring of utility functions.
11-13: Great work on enhancing the component's flexibility and reusability!The introduction of the generic type parameter
<T>allows for greater flexibility in the type of data the column can handle. Additionally, making thesurveyprop optional enables the component to function without a survey being provided, further increasing its versatility.These changes align with the goal of improving modularity and reusability within the codebase.
16-16: LGTM!The component has been correctly updated to accept the generic type parameter
<T>, which is consistent with the updated interface. This change ensures that the component's props are correctly typed based on the provided generic type.
42-42: Nice work on improving the component's robustness!The updated logic for finding a question from the survey now uses optional chaining (
survey?.questions) to account for the optional nature of thesurveyprop. This change introduces a safeguard against potential runtime errors whensurveyis not provided, preventing access to properties ofundefined.This enhancement ensures that the component can handle scenarios where the
surveyprop is not available, improving its overall reliability.packages/ui/DataTable/components/DataTableHeader.tsx (1)
3-3: LGTM!The
DataTableHeadercomponent changes are well-structured and consistent, enhancing its reusability and type safety. The updates align with the PR objectives of implementing a generic data table for persons.Specifically:
- The component renaming from
ResponseTableHeadertoDataTableHeadermakes it more generic and reusable.- The prop interface update to use a generic type parameter allows for greater flexibility in the types of data that can be passed to the header.
- Moving the
getCommonPinningStylesfunction to be defined locally and updating its type parameter is consistent with the component's generic type update.- The import adjustments are necessary to support the component changes.
Overall, the changes improve the component's maintainability and are approved.
Also applies to: 7-9, 11-12, 16-16
packages/ui/DataTable/components/SelectedRowSettings.tsx (7)
5-8: LGTM!The new imports enhance the component's functionality and user experience:
capitalizeFirstLetterfor better message formattingDeleteDialogfor deletion confirmationdeleteResponseActionanddeletePersonActionfor handling both response and person row deletions
10-13: Great refactor!The interface changes make the component more reusable and maintainable:
- The rename to
SelectedRowSettingsPropsand the addition of the generic type parameterTallow the component to handle different row types.- The
typeproperty provides a clear way to distinguish between response and person rows.
16-16: Excellent update!The component changes improve its reusability and alignment with the interface:
- The rename to
SelectedRowSettingsmakes the component more generic.- The generic type parameter
Tallows handling different row types.- Destructuring the
typeproperty provides access to the row type within the component.
30-49: Nice consolidation!The
handleDeletefunction updates improve maintainability and user experience:
- Conditionally deleting rows based on the
typeproperty reduces code duplication.- Using the appropriate deletion action (
deleteResponseActionordeletePersonAction) ensures correct behavior for each row type.- The updated success and error messages provide clearer feedback to the user by including the capitalized row type.
68-70: Good update!The selected row count message now includes the row type based on the
typeproperty:
- Provides clearer information to the user about the type of rows selected.
- Dynamically inserts the correct row type, improving the user experience.
84-84: Nice touch!Setting the
deleteWhatprop of theDeleteDialogcomponent to thetypeproperty:
- Ensures the correct row type is displayed in the delete dialog.
- Improves the user experience by providing a clear and accurate message.
85-85: Good integration!Setting the
onDeleteprop of theDeleteDialogcomponent to thehandleDeletefunction:
- Ensures the correct deletion logic is triggered when confirming the delete action in the dialog.
- Maintains the consolidated deletion logic for both response and person rows.
apps/web/app/(app)/environments/[environmentId]/(people)/people/components/PersonTableColumn.tsx (6)
1-7: LGTM!The import statements are relevant and necessary for the functionality of the file.
8-108: LGTM!The
generatePersonTableColumnsfunction is well-structured and generates an array of column definitions for a person table. The column definitions cover various aspects of person data, including selection, date, user information, email, and attributes. The function handles different scenarios, such as missing or empty attributes, and adjusts the layout based on theisExpandedstate.
9-32: LGTM!The
selectionColumnobject is well-defined and provides the functionality for selecting rows in the table. The column header includes a checkbox for selecting all rows, while each row in the column has a checkbox for individual row selection. The checkboxes are properly controlled using thecheckedandonCheckedChangeproperties, and thearia-labelattribute is used for accessibility.
34-61: LGTM!The
dateColumnobject is well-defined and handles the formatting and display of the creation date and time for each person. ThecreatedAttimestamp is extracted from therow.originalobject and converted to aDateobject. The date and time are then formatted usingtoLocaleStringwith specific options to provide a user-friendly representation. The formatted date and time are displayed in separate<p>elements within the cell.
63-79: LGTM!The
userColumnanduserIdColumnobjects are well-defined and handle the display of the person ID and user ID, respectively. TheaccessorKeyis set appropriately for each column, and theheaderproperty defines the header text. Thecellproperty extracts the relevant data from therow.originalobject and displays it in a<p>element within the cell.
86-105: LGTM!The
attributesColumnobject is well-defined and handles the dynamic display of key-value pairs of attributes for each person. The column properly handles cases where attributes are missing or empty by returningnull. TheObject.entriesmethod is used to iterate over the key-value pairs of theattributesobject, and each pair is displayed in a<div>element. The layout of the attributes is adjusted based on theisExpandedstate using thecnfunction for conditional class names, providing flexibility in the presentation of the attributes.apps/web/app/(app)/environments/[environmentId]/(people)/people/components/PersonDataView.tsx (1)
1-107: Excellent work on thePersonDataViewcomponent!The component is well-structured, modular, and follows best practices. Here are some positive aspects:
- Effective use of React hooks for managing state and side effects.
- Proper implementation of pagination logic for fetching and displaying persons in chunks.
- Optimization of initial load time by separately fetching additional attributes for each person.
- Correct handling of loading states and error scenarios.
- Clear separation of concerns and reusability of the component.
The component enhances the user interface by providing a paginated table view of persons, improving the overall user experience. The delete functionality allows for seamless removal of persons from the list.
Great job on this implementation!
packages/lib/utils/questions.tsx (1)
232-237: Type annotation and explicit typing enhance type safety and readability.The addition of the type annotation
Record<TSurveyQuestionTypeEnum, JSX.Element>to theQUESTIONS_ICON_MAPconstant and the explicit typing of the initial value of the accumulator in thereducefunction improve type safety and code readability by making the expected structure of the object clear.Tools
Biome
[error] 235-235: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
apps/web/app/(app)/environments/[environmentId]/(people)/people/components/PersonTable.tsx (4)
1-31: LGTM!The imports and component props are well-organized, follow consistent naming conventions, and provide a clear interface for the component. There are no unused imports or props.
41-75: LGTM!The state management and memoization are implemented correctly:
- State variables are initialized with appropriate default values.
- The
useMemohook is used effectively to avoid unnecessary re-computations of table data and columns.- The memoized table columns include a loading skeleton when data is not loaded, enhancing the user experience.
77-118: LGTM!The table configuration and column generation are implemented correctly:
- The
useReactTablehook is used effectively to configure the table with various options like column visibility, row selection, column order, and pagination.- The initial column order is set using the
useEffecthook, ensuring a consistent starting state.- The
handleDragEndfunction correctly updates the column order based on the drag and drop events.
120-207: LGTM!The rendering and event handling are implemented correctly:
- The component structure is clear and easy to follow, with a logical flow of rendering.
- The
DndContextprovider is used effectively to enable drag and drop functionality for column reordering.- The table is rendered using reusable UI components, promoting consistency and maintainability.
- The table rows are rendered dynamically based on the table data and column configuration, ensuring flexibility and adaptability.
- Event handling, such as row selection and column reordering, is implemented correctly.
apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseTable.tsx (4)
3-3: LGTM!The updated import statement aligns with the renaming of the
generateColumnsfunction togenerateResponseTableColumns, which improves code readability.
24-24: LGTM!The updated import statement for data table components from the
@formbricks/ui/DataTablemodule enhances consistency and maintainability across the application.
65-65: LGTM!The renaming of the
generateColumnsfunction togenerateResponseTableColumnsimproves code readability by providing a more descriptive name.
145-151: LGTM!The component replacements (
DataTableToolbar,DataTableHeader, andDataTableSettingsModal) align with the shift towards a more unified data table interface, enhancing consistency and maintainability.The renaming of the
deleteResponsesprop todeleteRowsin theDataTableToolbarcomponent reflects a broader change in terminology or functionality within the application.The addition of the
typeprop with value "response" provides context to theDataTableToolbarcomponent about the type of data being handled.Also applies to: 208-214
| export const QUESTIONS_ICON_MAP: Record<TSurveyQuestionTypeEnum, JSX.Element> = questionTypes.reduce( | ||
| (prev, curr) => ({ | ||
| ...prev, | ||
| [curr.id]: <curr.icon className="h-4 w-4" />, |
There was a problem hiding this comment.
Avoid using spread syntax on accumulators due to performance issues.
The use of spread syntax on the accumulator in the reduce function can cause performance issues with a time complexity of O(n^2), especially for large datasets.
Consider using alternative methods like Object.assign or Map to avoid the performance overhead. Here's an example using Object.assign:
-export const QUESTIONS_ICON_MAP: Record<TSurveyQuestionTypeEnum, JSX.Element> = questionTypes.reduce(
- (prev, curr) => ({
- ...prev,
- [curr.id]: <curr.icon className="h-4 w-4" />,
- }),
- {} as Record<TSurveyQuestionTypeEnum, JSX.Element>
-);
+export const QUESTIONS_ICON_MAP: Record<TSurveyQuestionTypeEnum, JSX.Element> = questionTypes.reduce(
+ (prev, curr) => Object.assign(prev, {
+ [curr.id]: <curr.icon className="h-4 w-4" />,
+ }),
+ {} as Record<TSurveyQuestionTypeEnum, JSX.Element>
+);Committable suggestion was skipped due to low confidence.
Tools
Biome
[error] 235-235: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
|
hey @Dhruwang UI looks good except for if I remove columns the last one isn't rounded anymore (pls also check on Response t able): And please retain survey settings (incl. collapse / dont collapse) in local state for Person Table and Response table. Thanks! |
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (1)
apps/web/app/(app)/environments/[environmentId]/(people)/people/components/PersonTable.tsx (1)
195-201: Enhance the "No results" message.Consider providing a more informative message when there are no results to display in the table. You could include suggestions or actions the user can take when encountering an empty table.
Apply this diff to enhance the message:
- No results. + No results found. Please try adjusting your filters or search criteria.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- apps/web/app/(app)/environments/[environmentId]/(people)/people/components/PersonTable.tsx (1 hunks)
- packages/ui/Table/index.tsx (1 hunks)
Additional comments not posted (3)
packages/ui/Table/index.tsx (1)
6-6: Verify the impact of changing the table width.Changing the width of the table container from
w-fulltow-fitmay affect the layout and responsiveness of the table in different contexts. It's important to ensure that this change does not introduce any unintended visual or functional issues.To verify the impact, run the following script:
Please test the table in various scenarios, such as different screen sizes, content lengths, and parent container dimensions, to ensure that the new width behavior (
w-fit) works as intended and does not break the layout or cause any visual inconsistencies.apps/web/app/(app)/environments/[environmentId]/(people)/people/components/PersonTable.tsx (2)
1-223: Comprehensive and well-structured component!The
PersonTablecomponent is well-organized and follows best practices. It effectively utilizes libraries like@dnd-kitfor drag-and-drop functionality and@tanstack/react-tablefor managing the table state. The component handles asynchronous data loading, incorporates skeleton loaders, and provides a user-friendly interface for interacting with the table.The code is modular, with separate functions for generating table columns and handling various user interactions. It also efficiently manages state using hooks like
useStateanduseEffect.Great job on implementing features like saving user preferences to local storage and handling column reordering through drag-and-drop.
Overall, the component is well-structured, maintainable, and provides a smooth user experience.
206-212: Verify the "Load More" button's visibility and functionality.Please ensure that the "Load More" button is displayed only when there are more records to load and that clicking the button triggers the
fetchNextPagefunction to load the next page of data.Run the following script to verify the button's visibility and functionality:
Verification successful
"Load More" button implementation verified successfully
The "Load More" button's visibility and functionality have been verified:
- The button is correctly displayed only when there's more data to load (
hasMoreis true) and when there's existing data (data.length > 0).- Clicking the button triggers the
fetchNextPagefunction as required.- This implementation is consistent across different components in the codebase.
No issues were found with the current implementation.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the "Load More" button's visibility and functionality. # Test 1: Search for the button when `hasMore` is true and `data` is not empty. # Expect: The button should be present. rg --type tsx -A 5 $'hasMore && data.length > 0' # Test 2: Search for the `fetchNextPage` function. # Expect: It should be called when the button is clicked. rg --type tsx -A 5 $'onClick={fetchNextPage}'Length of output: 239
Script:
#!/bin/bash # Description: Verify the "Load More" button's visibility and functionality. # Search for the "Load More" button implementation echo "Searching for 'Load More' button implementation:" rg --type ts -A 10 -B 5 'Load More' # Search for the fetchNextPage function call echo "Searching for fetchNextPage function call:" rg --type ts -A 5 -B 5 'onClick=\{fetchNextPage\}' # Search for the fetchNextPage function definition or import echo "Searching for fetchNextPage function definition or import:" rg --type ts 'function fetchNextPage|import.*fetchNextPage'Length of output: 10763
| data: TPersonTableData[]; | ||
| fetchNextPage: () => void; | ||
| hasMore: boolean; | ||
| deletePerons: (personIds: string[]) => void; |
There was a problem hiding this comment.
Typo in prop name.
The prop name deletePerons has a typo. It should be deletePersons.
Apply this diff to fix the typo:
- deletePerons: (personIds: string[]) => void;
+ deletePersons: (personIds: string[]) => void;Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| deletePerons: (personIds: string[]) => void; | |
| deletePersons: (personIds: string[]) => void; |
| setIsTableSettingsModalOpen={setIsTableSettingsModalOpen} | ||
| isExpanded={isExpanded} | ||
| table={table} | ||
| deleteRows={deletePerons} |
There was a problem hiding this comment.
Typo in prop usage.
The prop usage deletePerons has a typo. It should be deletePersons to match the corrected prop name.
Apply this diff to fix the typo:
- deleteRows={deletePerons}
+ deleteRows={deletePersons}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| deleteRows={deletePerons} | |
| deleteRows={deletePersons} |
| data, | ||
| fetchNextPage, | ||
| hasMore, | ||
| deletePerons, |
There was a problem hiding this comment.
Typo in prop usage.
The prop usage deletePerons has a typo. It should be deletePersons to match the corrected prop name.
Apply this diff to fix the typo:
- deletePerons,
+ deletePersons,Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| deletePerons, | |
| deletePersons, |
| type="person" | ||
| /> | ||
| <div> | ||
| <Table style={{ width: table.getCenterTotalSize(), tableLayout: "fixed", borderRadius: "2.5rem" }}> |
There was a problem hiding this comment.
Inconsistent CSS class for the outer <div> of the Table component.
The CSS class for the outer <div> of the Table component is inconsistent with the rest of the codebase. In other places, the Table component is wrapped with a <div className="overflow-x-auto"> to handle horizontal overflow.
Apply this diff to maintain consistency:
- <div>
+ <div className="overflow-x-auto">
<Table style={{ width: table.getCenterTotalSize(), tableLayout: "fixed", borderRadius: "2.5rem" }}>Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseTable.tsx (3)
3-3: Rename the function togenerateColumnsfor consistency.The function name
generateResponseTableColumnsis inconsistent with the naming convention used for other similar functions in the codebase. Consider renaming it togenerateColumnsto maintain consistency and improve readability.Apply this diff to rename the function:
-import { generateResponseTableColumns } from "@/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseTableColumns"; +import { generateColumns } from "@/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseTableColumns";
66-66: Update the function name to match the import.If you decide to rename the
generateResponseTableColumnsfunction togenerateColumnsas suggested in the previous comment, make sure to update the function name here as well to maintain consistency.Apply this diff to update the function name:
- const columns = generateResponseTableColumns(survey, isExpanded, isViewer); + const columns = generateColumns(survey, isExpanded, isViewer);
163-169: Rename thedeleteRowsprop todeleteResponsesfor clarity.The
deleteRowsprop name is a bit generic and doesn't clearly convey that it specifically deletes responses. Consider renaming it todeleteResponsesto improve clarity and maintain consistency with the naming convention used in other parts of the code.Apply this diff to rename the prop:
- deleteRows={deleteResponses} + deleteResponses={deleteResponses}Make sure to update the prop name in the
DataTableToolbarcomponent as well to avoid any type errors.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/SurveyMenuBar.tsx (3 hunks)
- apps/web/app/(app)/environments/[environmentId]/(people)/people/components/PersonDataView.tsx (1 hunks)
- apps/web/app/(app)/environments/[environmentId]/(people)/people/components/PersonTable.tsx (1 hunks)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseTable.tsx (6 hunks)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseTableCell.tsx (1 hunks)
- packages/lib/person/service.ts (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- apps/web/app/(app)/environments/[environmentId]/(people)/people/components/PersonDataView.tsx
- apps/web/app/(app)/environments/[environmentId]/(people)/people/components/PersonTable.tsx
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseTableCell.tsx
- packages/lib/person/service.ts
Additional comments not posted (8)
apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseTable.tsx (5)
24-24: LGTM!The import statement for the data table components is correct and follows the proper naming convention.
178-178: LGTM!The usage of the
DataTableHeadercomponent is correct and follows the proper prop passing convention.
226-226: LGTM!The usage of the
DataTableSettingsModalcomponent is correct and follows the proper prop passing convention.
Line range hint
1-256: Ensure responsiveness and test thoroughly.The changes introduced in this file are quite significant, involving refactoring of the table components and the addition of new features like column reordering and visibility persistence. While the code looks good overall, it's crucial to thoroughly test the responsiveness of the table and ensure that all the new features work as expected without breaking any existing functionality.
Here are a few key areas to focus on during testing:
Responsiveness:
- Test the table on various screen sizes and devices to ensure that it remains responsive and usable.
- Pay special attention to the column resizing and reordering behavior on different devices.
Column reordering:
- Test the drag-and-drop functionality for reordering columns.
- Ensure that the column order persists correctly when the page is refreshed or navigated away from and back.
- Verify that the reordering works smoothly and doesn't cause any visual glitches or unexpected behavior.
Column visibility:
- Test the column visibility toggle in the table settings modal.
- Ensure that the visibility state persists correctly when the page is refreshed or navigated away from and back.
- Verify that the table renders correctly with different combinations of visible and hidden columns.
Pagination and loading:
- Test the pagination functionality, including the "Load More" button.
- Ensure that the loading state is displayed correctly when fetching data.
- Verify that the table behaves correctly when there are no results or when the data is empty.
Interaction with other components:
- Test the interaction between the table and other components like the
ResponseCardModal.- Ensure that opening and closing the modal doesn't affect the table state or cause any unexpected behavior.
By thoroughly testing these areas and conducting comprehensive quality assurance, you can ensure that the refactored
ResponseTablecomponent provides a smooth and reliable user experience.
69-91: Verify the impact of loading and saving settings on other parts of the application.Loading and saving table settings (column order and visibility) to local storage is a good way to persist user preferences. However, it's important to ensure that this change doesn't introduce any unintended side effects or break existing functionality in other parts of the application.
To verify the impact, you can run the following script:
If the tests don't reveal any concerning usage or potential breakages, then the changes should be safe to merge. However, if the tests highlight any issues, please investigate and address them accordingly.
Verification successful
Changes to survey settings are isolated and don't impact other parts of the application.
Based on the verification performed, we can confirm that the loading and saving of table settings (column order and visibility) to local storage in the ResponseTable component does not introduce any unintended side effects or break existing functionality in other parts of the application. The reasons for this conclusion are:
- No usage of
survey.id-columnOrderorsurvey.id-columnVisibilitywas found outside of the ResponseTable.tsx file.- No error handling or null checks related to these survey settings were found in other parts of the codebase.
These findings suggest that the changes are isolated to the ResponseTable component and don't impact other areas of the application. The implementation appears to be safe and can be merged without concerns about breaking existing functionality.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: # - Check if the survey settings are being used correctly in other parts of the application. # - Ensure that loading and saving settings doesn't break any existing functionality. # Test 1: Search for usage of survey settings in other files. # Expect: No direct usage of survey settings outside of the ResponseTable component. rg --type typescript -g '!ResponseTable.tsx' $'survey.id-columnOrder|survey.id-columnVisibility' # Test 2: Search for potential side effects or breakages related to survey settings. # Expect: No error handling or null checks related to survey settings. rg --type typescript -A 5 $'survey.id-columnOrder|survey.id-columnVisibility'Length of output: 246
Script:
#!/bin/bash # Search for usage of survey.id-columnOrder in TypeScript files echo "Searching for survey.id-columnOrder usage:" ast-grep --lang typescript --pattern 'localStorage.getItem($"${survey.id}-columnOrder")' | grep -v "ResponseTable.tsx" echo "\nSearching for survey.id-columnVisibility usage:" ast-grep --lang typescript --pattern 'localStorage.getItem($"${survey.id}-columnVisibility")' | grep -v "ResponseTable.tsx" # Search for potential error handling or null checks related to survey settings echo "\nSearching for error handling or null checks:" ast-grep --lang typescript --pattern 'try { $$$ localStorage.getItem($"${survey.id}-columnOrder") $$$ } catch ($_) { $$$ }' ast-grep --lang typescript --pattern 'try { $$$ localStorage.getItem($"${survey.id}-columnVisibility") $$$ } catch ($_) { $$$ }'Length of output: 822
apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/SurveyMenuBar.tsx (3)
243-243: TheclearSurveyLocalStoragefunction is called at the appropriate time during the survey saving process.The function is invoked after the survey segment is updated and before the survey is saved via the
updateSurveyAction. This ensures that any outdated column order and visibility settings are cleared from local storage prior to saving the updated survey data.
289-289: TheclearSurveyLocalStoragefunction is called at the appropriate time when publishing the survey.Similar to the save process, the function is invoked after updating the survey segment and before calling
updateSurveyActionto publish the survey. This clears any stale local storage data before the survey is published.
89-94: Ensure theclearSurveyLocalStoragefunction is called at appropriate times.The
clearSurveyLocalStoragefunction is responsible for removing survey-specific column order and visibility settings from local storage. Make sure this function is invoked whenever the survey is saved or updated to prevent stale data from persisting.To verify the usage of
clearSurveyLocalStorage, run the following script:Verification successful
The
clearSurveyLocalStoragefunction is correctly implemented and called at appropriate times.The function is called before updating the survey in both the
handleSurveySaveandhandleSurveyPublishfunctions. This ensures that any stale data related to column order and visibility is removed from localStorage when the survey is saved or published. The implementation aligns with the review comment's suggestion and no further action is needed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify `clearSurveyLocalStorage` is called when saving or updating the survey. # Test 1: Search for invocations of `clearSurveyLocalStorage`. Expect: At least 2 occurrences. rg --type typescript -A 5 $'clearSurveyLocalStorage\(\)' # Test 2: Search for invocations within `handleSurveySave`. Expect: 1 occurrence before `updateSurveyAction`. ast-grep --lang typescript --pattern $'const handleSurveySave = async () => { $$$ clearSurveyLocalStorage() $$$ updateSurveyAction($$$) $$$ }' # Test 3: Search for invocations within `handleSurveyPublish`. Expect: 1 occurrence before `updateSurveyAction`. ast-grep --lang typescript --pattern $'const handleSurveyPublish = async () => { $$$ clearSurveyLocalStorage() $$$ updateSurveyAction($$$) $$$ }'Length of output: 405
Script:
#!/bin/bash # Description: Verify `clearSurveyLocalStorage` is called when saving or updating the survey. # Test 1: Search for invocations of `clearSurveyLocalStorage`. Expect: At least 2 occurrences. echo "Test 1: Searching for clearSurveyLocalStorage invocations" rg -t ts -t tsx -A 5 'clearSurveyLocalStorage\(\)' || echo "No matches found or error occurred" # Test 2: Search for invocations within `handleSurveySave`. echo "Test 2: Searching for clearSurveyLocalStorage within handleSurveySave" ast-grep --lang typescript --pattern 'const handleSurveySave = async () => { $$$ clearSurveyLocalStorage() $$$ }' || echo "No matches found or error occurred" # Test 3: Search for invocations within `handleSurveyPublish`. echo "Test 3: Searching for clearSurveyLocalStorage within handleSurveyPublish" ast-grep --lang typescript --pattern 'const handleSurveyPublish = async () => { $$$ clearSurveyLocalStorage() $$$ }' || echo "No matches found or error occurred" # Test 4: List all TypeScript files in the repository echo "Test 4: Listing all TypeScript files" fd -e ts -e tsx || echo "No TypeScript files found or error occurred"Length of output: 71698
Script:
#!/bin/bash # Description: Verify `clearSurveyLocalStorage` is called when saving or updating the survey. echo "Test 1: Searching for clearSurveyLocalStorage invocations" rg 'clearSurveyLocalStorage\(\)' -C 5 || echo "No matches found or error occurred" echo "Test 2: Searching for clearSurveyLocalStorage with ast-grep" ast-grep --lang typescript --pattern 'clearSurveyLocalStorage()' || echo "No matches found or error occurred" echo "Test 3: Displaying content of SurveyMenuBar.tsx" cat apps/web/app/\(app\)/\(survey-editor\)/environments/\[environmentId\]/surveys/\[surveyId\]/edit/components/SurveyMenuBar.tsx || echo "File not found or error occurred"Length of output: 17696
Screen.Recording.2024-09-19.at.10.53.39.AM.mp4Fixed above mentioned issues 😊 |
jobenjada
left a comment
jobenjada
left a comment
There was a problem hiding this comment.
pls fix the corner of the scroll bar and then LGTM 🪨
2 participants
What does this PR do?
Fixes https://github.com/formbricks/internal/issues/355
How should this be tested?
Test people page
Checklist
Required
pnpm buildconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
Release Notes
New Features
PersonTablecomponent for displaying and managing person data with customizable column order through drag-and-drop functionality.ResponseTablewith improved column management and user preferences saved in local storage.Improvements
ResponseTableCellfor improved visual consistency.Bug Fixes