feat: ADS Entity Item #38442
feat: ADS Entity Item #38442
Conversation
WalkthroughThis pull request introduces a comprehensive enhancement to the entity list item functionality, focusing on creating an editable name feature for list items. The changes span multiple files across the design system and IDE components, introducing a new Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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
Documentation and Community
|
github-actions
bot
added
IDE Pod
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/utils.ts (1)
1-8: Consider edge cases and potential collisions in name normalization.
For instance, empty or non-alphanumeric values might return empty strings or too many underscores. You could handle edge cases and ensure the function doesn’t inadvertently produce leading/trailing underscores or empty strings.app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.tsx (1)
26-38: Styling within the component.
Defining style props inline is acceptable for quick adjustments but consistent usage of styled components or theme-based styling might keep codebase uniform.app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.stories.tsx (1)
65-74: Rename story for clarity.Using
InEditingModeis descriptive, but you could also name it something likeDefaultEditingStateto better align with typical Storybook naming patterns.app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/useNameEditor.ts (2)
32-36: Use caution with the exit logic.Ensuring
exitWithoutSaving()properly reverts changes is critical. Consider adding tests around quick rename toggles to avoid partial states.
93-103: Focus out listenerAutomatically saving on blur is a convenient approach. Consider whether more robust user feedback is needed before auto-save.
app/client/packages/design-system/ads/src/List/List.styles.tsx (1)
144-146: Disabled itemsDisabling pointer events is excellent. Just ensure focus states aren’t incorrectly applied to disabled items.
app/client/packages/design-system/ads/src/List/List.tsx (1)
111-116: Double-Click Handler
AddinghandleDoubleClickenhances interactivity. Consider accessibility implications since double-click interactions can sometimes be missed by keyboard users.app/client/packages/design-system/ads/src/List/List.types.tsx (1)
15-16: NewonDoubleClick
Allowing for a dedicated double-click callback is useful. Be mindful of edge cases where double-click might conflict with single-click usage.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
app/client/packages/design-system/ads/src/List/List.styles.tsx(5 hunks)app/client/packages/design-system/ads/src/List/List.tsx(2 hunks)app/client/packages/design-system/ads/src/List/List.types.tsx(2 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.stories.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.styled.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.types.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/index.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/useNameEditor.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/utils.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/index.ts(1 hunks)app/client/src/pages/Editor/IDE/EditorTabs/EditableTab.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/index.ts
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.styled.ts
🔇 Additional comments (28)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/index.ts (1)
8-8: Check for unintentional exports.
Exporting everything from "./EntityItem" is convenient but can inadvertently expose internal utilities or types you don’t intend to publish. Consider selectively exporting only the necessary entities if that’s a concern.
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.types.ts (1)
1-24: Interface looks comprehensive.
The Omit usage helps refine the final props interface. Just verify that the omitted properties won’t be needed by other downstream consumers of this component.
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.tsx (5)
1-3: Imports are clear and minimal.
This is aligned with your organized naming convention. Good job!
4-7: Separation of types, styled components, and hooks
It’s good that EntityItem cleanly imports types from EntityItem.types, style definitions from EntityItem.styled, and the custom hook from useNameEditor.
8-24: Destructuring config from props maintains clarity.
This approach makes the rest of the component simpler to read.
40-58: Tooltip for validation errors is a user-friendly approach.
It clearly communicates constraints to the user. Looks great!
60-61: Returning a ListItem
Using customTitleComponent to inject your editable name is consistent with your list design pattern. Code looks solid!
app/client/src/pages/Editor/IDE/EditorTabs/EditableTab.tsx (1)
60-60: Add 'dispatch' to the dependencies intentionally.
It's essential to include dispatch to ensure this callback picks up any changes. Make sure you have verified there are no accidental re-renders.
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.stories.tsx (4)
1-4: Story file structure is clear.
The story definitions and minimal console logs are straightforward and can help with debugging.
17-35: Consider more extensive event tests.
Currently, logs appear in the console. If you want to ensure thorough coverage, consider adding interactive tests or using controls to better test various states.
75-83: Inline error states
Demonstrating how errors appear helps ensure the UI remains consistent. Consider referencing a real validation function in subsequent tests.
85-105: Ensure thorough coverage of all states.
Your stories effectively cover selected and disabled states. Validate that these use cases match the final acceptance criteria.
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/useNameEditor.ts (3)
53-67: Re-check the edit logic.
Any naming change triggers auto-save, which is good. Ensure there's no race condition if rapidly toggling the edit mode.
76-82: Keyboard shortcuts are well-structured.
Supporting enter/escape is user-friendly. Continue to ensure these shortcuts work as expected in all browsers.
114-126: Temporary focus fix
This workaround is reasonable. Just remember to clean it up or refine it when adjusting the focus retention.
app/client/packages/design-system/ads/src/List/List.styles.tsx (4)
52-53: Enhanced layout for the right control.
Flex alignment improves the visual consistency of the right control region.
65-65: Prevent unexpected shrinking.
flex-shrink: 0; helps preserve text visibility in more complex layouts.
133-133: Hover-based control visibility
Conveys a desirable UI pattern for contextual actions. Confirm that keyboard-accessibility guidelines are met.
160-160: Accessible focus outline
This properly aids keyboard-only users. Continue to confirm the entire list item is navigable.
app/client/packages/design-system/ads/src/List/List.tsx (4)
94-94: Rename Function Confirmation
Renaming listItemhandleKeyDown to listItemHandleKeyDown is a good move to maintain consistent casing and readability.
124-125: Explicit Event Handling
The separate handlers for onClick and onKeyDown keep the component modular and clarify input-device intentions. This is a clean approach.
127-127: Commented Out tabIndex
Keeping tabIndex commented out may break keyboard navigability for non-clickable wrappers. If needed for accessibility, consider restoring it conditionally.
129-129: Bind Double-Click Here
Binding onDoubleClick at ContentTextWrapper is consistent with the newly introduced handler. Just ensure double-click usage is tested across devices.
app/client/packages/design-system/ads/src/List/List.types.tsx (5)
6-11: BaseListItemProps Enhancement
Defining a clearer, more self-contained BaseListItemProps with rightControlVisibility and required onClick storms up the potential for composable list items.
35-36: Interface End
The separation of concerns, with BaseListItemProps concluding here, is logically sound for layered interfaces.
37-41: ListItemWithTitle
Offering a path for simple textual titles is user-friendly. Using customTitleComponent?: never ensures mutual exclusivity with the custom component interface.
43-46: ListItemWithCustomTitleComponent
Defining a dedicated shape with title?: never and customTitleComponent enforces a clear contract. This clarifies when the component is replaced entirely.
49-51: Union-Based ListItemProps
Great approach to unify item types. This strongly typed union pattern prevents accidental passing of both title and customTitleComponent.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12572359570. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.types.ts (1)
8-22: Consider enhancing type safety for name validationThe interface looks good, but consider these improvements:
- Add a more specific return type for validateName (e.g., ValidationError | null)
- Consider adding maxLength constraint for names
Here's a suggested enhancement:
nameEditorConfig: { canEdit: boolean; isEditing: boolean; isLoading: boolean; onEditComplete: () => void; onNameSave: (newName: string) => void; - validateName: (newName: string) => string | null; + validateName: (newName: string) => ValidationError | null; + maxLength?: number; }; } + type ValidationError = string;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/client/packages/design-system/ads/src/List/List.tsx(3 hunks)app/client/packages/design-system/ads/src/List/List.types.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.stories.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- app/client/packages/design-system/ads/src/List/List.types.tsx
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.tsx
- app/client/packages/design-system/ads/src/List/List.tsx
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.stories.tsx
🔇 Additional comments (2)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.types.ts (2)
1-7: LGTM! Clean interface extension
Good use of TypeScript's Omit utility type to create a focused interface by removing conflicting props.
19-21: Verify consistent validation patterns
Let's ensure the validation pattern aligns with other similar components in the codebase.
✅ Verification successful
Validation pattern aligns with existing implementation
The validation pattern follows the established pattern in the codebase, particularly matching the EditableName component's implementation which returns string | null for validation errors. The pattern is consistent with:
useNameEditor.tsvalidation implementation that returns error messages or null- Reusable validation utilities like
isValidVariableNameindefCreatorUtils.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar validation patterns in the codebase
echo "Searching for similar name validation patterns..."
ast-grep --pattern 'validateName: ($_) => $_'
echo "Searching for existing validation implementations..."
rg -A 3 'validateName.*=.*=>'
Length of output: 303
Script:
#!/bin/bash
# Let's try different search patterns to find validation implementations
# Search for validation-related type definitions
echo "Searching for validation type definitions..."
rg -g '*.ts' -g '*.tsx' 'type.*Valid.*Name'
# Search for name validation functions
echo "Searching for name validation functions..."
rg -g '*.ts' -g '*.tsx' -g '*.js' -g '*.jsx' '(validate.*Name|isValid.*Name).*\('
# Search for validation utility functions
echo "Searching for validation utilities..."
fd "validation" --type f --exec rg -l "export"
# Look for similar entity-related validation
echo "Searching for entity validation patterns..."
rg -g '*.ts' -g '*.tsx' 'validate.*\(.*:.*string\).*=>.*string'
Length of output: 68841
Script:
#!/bin/bash
# Let's search for validation implementations in the EntityItem component and related files
# Search for validation implementations in EntityItem
echo "Searching for validation in EntityItem..."
rg -g '*.ts' -g '*.tsx' -A 5 'validateName.*=.*\(' app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/
# Search for validation patterns in EditableName component
echo "Searching for validation in EditableName..."
rg -g '*.ts' -g '*.tsx' -A 5 'validateName.*=.*\(' app/client/src/IDE/Components/EditableName/
# Search for name validation utilities
echo "Searching for name validation utilities..."
rg -g '*.ts' -g '*.tsx' 'isValid.*Name' app/client/src/utils/
Length of output: 2222
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/src/IDE/Components/EditableName/EditableName.tsx (1)
5-6: Keep track of the temporary import
Remember to remove the temporary import once you migrate the component to the design system, to avoid confusion or unused dependencies in the future.app/client/packages/design-system/ads/src/Templates/EntityExplorer/Editable/utils.ts (1)
1-8: Consider edge cases withnormaliseName
This function replaces any sequence of non-word characters with underscores and truncates to 30 by default. If the input can contain leading/trailing underscores or empty strings, consider adding logic to handle those cases more gracefully.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/Editable/index.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/Editable/useEditableText.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/Editable/utils.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.tsx(1 hunks)app/client/src/IDE/Components/EditableName/EditableName.tsx(2 hunks)app/client/src/IDE/Components/EditableName/useValidateEntityName.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/Editable/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.tsx
🔇 Additional comments (15)
app/client/src/IDE/Components/EditableName/EditableName.tsx (3)
1-3: Great import organization
The new import statements and usage of useValidateEntityName tidy up the dependencies nicely.
40-40: Entity name validation
Using useValidateEntityName here provides a clean separation of concerns for validation logic.
44-50: Destructuring from the custom hook
Destructuring all relevant references and handlers in one place helps maintain clarity and reduce local state usage.
app/client/src/IDE/Components/EditableName/useValidateEntityName.ts (4)
1-1: Imports look good
No concerns with the dependencies and usage of redux selectors and constants.
10-12: Updated interface naming
Renaming UseNameEditorProps to UseValidateEntityNameProps clarifies the hook’s intent, improving maintainability.
18-20: Hook naming consistency
Renaming the hook to useValidateEntityName prevents confusion between editing and validating. This is a clear improvement.
28-28: Efficient validation via callback
Returning a memoized callback for validation ensures performance is preserved when the parent component re-renders.
app/client/packages/design-system/ads/src/Templates/EntityExplorer/Editable/useEditableText.ts (8)
1-13: Comprehensive import set
Good to see typed React events and third-party hooks for better safety and consistency. The normaliseName import indicates a structured approach for input sanitization.
14-36: Well-defined hook signature
Breaking out exitEditing, validation, and name saving into parameters fosters reusability and clarity. The exitWithoutSaving callback is straightforward and neatly resets local state.
38-51: Centralized validation
The validate callback systematically updates the local error state, improving readability and preventing redundant checks elsewhere.
53-75: Graceful save logic
The attemptSave function handles unchanged names, validation failures, and successful saves distinctly. This separation makes debugging easier.
76-91: Keyboard handlers
Using useEventCallback for these handlers is a great approach, avoiding re-renders tied to parent changes and thus boosting performance.
93-103: Focus-out triggers
The event listener ensures that editing is not left incomplete, providing a better user experience by finalizing or reverting changes on focus loss.
105-128: Robust synchronization
Syncing the editableName state with prop changes and recapturing focus addresses corner cases, minimizing unexpected user experiences.
130-137: Clean return structure
Returning the refs, name, error, and handlers in an array is consistent with typical React hooks convention. Straightforward to consume.
hetunandu
added
ok-to-test
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12579045650. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/client/src/IDE/Components/EditableName/EditableName.test.tsx (2)
8-11: MockinguseEditableTextwithin the same module scope.If you decide to override internal hook behavior in future tests, confirm that it doesn't break parallel test runs due to shared mock state.
18-26: Default mocking return value.The array structure here strongly couples your test to the internal implementation of
useEditableText. If the hook changes its return shape, these tests might silently break. Consider verifying only the relevant indices or mocking destructured properties individually for greater stability.app/client/packages/design-system/ads/src/Templates/EntityExplorer/Editable/useEditableText.test.tsx (1)
4-5: Redundant usage ofrenderwithrenderHook.Since you rely primarily on
renderHook, consider removing any unused imports to keep test code concise if not needed for later test expansions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/Editable/useEditableText.test.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/index.ts(1 hunks)app/client/src/IDE/Components/EditableName/EditableName.test.tsx(1 hunks)app/client/src/IDE/Components/EditableName/EditableName.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/index.ts
🔇 Additional comments (16)
app/client/src/IDE/Components/EditableName/EditableName.test.tsx (2)
6-7: Use ofuseEditableTextimport.This addition looks good. Make sure any helper methods from
@appsmith/adsare consistently used throughout the testing suite for clarity.
16-17:mockValidatorusage.Defining a single mock validator in this suite is straightforward; just verify that test coverage includes both valid and invalid flows to keep it robust.
app/client/packages/design-system/ads/src/Templates/EntityExplorer/Editable/useEditableText.test.tsx (11)
1-3: Imports organization.All import references look valid. Ensure that references to test utilities in
@testing-library/react-hooksremain up-to-date if that package evolves.
7-8: Test suite structure.Good naming convention. The
describe("useEditableText")block is helpful for quickly locating these tests.
8-10: Re-initializing mocks in each test.Mock callbacks are fine. Just ensure we avoid hidden global side effects if new tests are added in the future.
16-17:initial statetest coverage.Purposefully verifying default states & ensuring no validation error is a solid baseline. Looks clear.
18-33: Destructured hook return values.Because this hook returns multiple items, verifying the correct order is crucial. Minor caution: if the hook’s return signature changes, maintainers must remember to update these tests accordingly.
35-59:handle name changescenario.Checks logic for dynamic name changes. This test ensures that the validation error remains null. Great coverage for typical usage.
61-90: Valid name save on Enter key.Neatly checks
onNameSavecall. Looks standard. Confirm that no concurrency issues occur if you add asynchronous validations later.
92-121: Invalid name save on Enter key.Ensures no unintended save occurs. Good guard. Makes sense to still exit editing for user-friendliness.
123-142: Exit without saving on Escape key.This test succinctly checks user cancellation flow. Straightforward approach.
144-163: Exit without saving on no name change.Great for ensuring we don’t spam the save function with identical data. Minimizes wasted calls.
165-205:focusOutevent triggers save if changed.Clear scenario illustrating real user interactions. The coverage is thorough for typical blur actions.
app/client/src/IDE/Components/EditableName/EditableName.tsx (3)
1-3: Updated imports for streamlined logic.Importing
useValidateEntityNameanduseEditableTextis consistent with the new approach. Good synergy with the rest of the codebase.
5-5:useEditableTextimport from@appsmith/ads.Ensure that version control references for that package remain consistent with the rest of the design system.
39-49:validateNameanduseEditableTextusage.Neat approach to separate validation from input logic. This keeps the component lean.
|
Deploy-Preview-URL: https://ce-38442.dp.appsmith.com |
…o feat/ads/entity-item
…/appsmith into feat/ads/entity-item
…/appsmith into feat/ads/entity-item
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/packages/design-system/ads/src/List/List.tsx (1)
120-120: Evaluate accessibility impact of removing tabIndex.
Consider re-enabling tabIndex for keyboard navigation or remove entirely if not needed.app/client/packages/design-system/ads/src/List/List.styles.tsx (1)
135-135: Consider keyboard accessibility for hover-based visibility.
In addition to hover, consider displaying controls when the item is tab-focused for an inclusive UX.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/client/packages/design-system/ads/src/List/List.stories.tsx(2 hunks)app/client/packages/design-system/ads/src/List/List.styles.tsx(5 hunks)app/client/packages/design-system/ads/src/List/List.tsx(3 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.styles.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.tsx(1 hunks)app/client/src/pages/Editor/IDE/LeftPane/DataSidePane.tsx(0 hunks)
💤 Files with no reviewable changes (1)
- app/client/src/pages/Editor/IDE/LeftPane/DataSidePane.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.tsx
🔇 Additional comments (15)
app/client/packages/design-system/ads/src/List/List.stories.tsx (2)
213-213: Looks perfectly fine.
Adding a consistentstartIconfor the block description story is aligned with existing usage.
223-224: Good approach for testing overflow behavior.
This extended title ensures that the overflow handling is well tested.app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.styles.ts (2)
1-2: Imports are properly declared
No concerns with version or usage. Proceed as is.
4-16: Cross-browser verification for 'text-overflow: unset'
Older browsers may require a fallback as they may not supporttext-overflow: unset.app/client/packages/design-system/ads/src/List/List.tsx (4)
6-13: Imports look good.
Straightforward addition of new styled components for top/bottom content wrappers. No issues noticed.
93-94: Clear block and inline description checks.
Logic is well-structured; ensure there's a fallback or graceful handling whendescriptionis undefined.
102-106: Verify potential overlap with single-click events.
Ensure the double-click handler does not unintentionally interfere with single-click in certain browsers.
122-162: Layout enhancements are well-defined.
Shifting to top/bottom content containers and inline descriptions looks more readable and maintainable.app/client/packages/design-system/ads/src/List/List.styles.tsx (7)
34-43: RightControlWrapper is neatly constructed.
Using flex for layout and containing the button styling is clean and concise.
45-51: TopContentWrapper structure is intuitive.
Provides a clear horizontal layout for icons and titles.
53-55: BottomContentWrapper for vertical arrangement is good.
Ensures descriptions and other items remain distinct and readable.
68-68: Verify container height constraints.
Double-check that always settingheight: 100%won't cause layout issues if the parent height is unset.
71-71: Spacing and layout adjustments look solid.
The gap, flex-shrink, and column direction usage properly define the layout.Also applies to: 73-74
146-146: Disabled state styling is consistent.
The not-allowed cursor and opacity changes clearly convey the item is inactive.
162-162: Focus indicator applied.
Providing a visible outline on focus greatly improves keyboard navigation accessibility.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/client/packages/design-system/ads/src/List/List.tsx (1)
92-93: Consider combining description type checksThe implementation is clean, but you could simplify these checks into a single constant.
- const isBlockDescription = descriptionType === "block" && description; - const isInlineDescription = descriptionType === "inline" && description; + const descriptionStyle = description ? descriptionType : null;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/packages/design-system/ads/src/List/List.styles.tsx(4 hunks)app/client/packages/design-system/ads/src/List/List.tsx(4 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.styles.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.styles.ts
- app/client/packages/design-system/ads/src/List/List.styles.tsx
🔇 Additional comments (2)
app/client/packages/design-system/ads/src/List/List.tsx (2)
101-105: LGTM: Double-click handler implementationClean implementation that properly handles the disabled state and follows the existing pattern.
Line range hint
122-161: LGTM: Improved layout structureThe new wrapper components provide a cleaner and more maintainable structure for the list item content.
| size={size} | ||
| tabIndex={props.isDisabled ? -1 : 0} | ||
| // tabIndex={props.isDisabled ? -1 : 0} |
There was a problem hiding this comment.
Restore the tabIndex for keyboard accessibility
Commenting out the tabIndex prevents keyboard navigation to list items, which is a critical accessibility requirement.
- // tabIndex={props.isDisabled ? -1 : 0}
+ tabIndex={props.isDisabled ? -1 : 0}📝 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.
| // tabIndex={props.isDisabled ? -1 : 0} | |
| tabIndex={props.isDisabled ? -1 : 0} |
app/client/packages/design-system/ads/src/Templates/EntityExplorer/Editable/useEditableText.ts
Outdated
Show resolved
Hide resolved
| style: { | ||
| paddingTop: 0, | ||
| paddingBottom: 0, | ||
| height: "32px", |
There was a problem hiding this comment.
I'm not completely ok with adding height separately for the inputs. Maybe we can have a conversation with Momcilo regarding this later. Not a blocker for this PR.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12595632026. |
|
Deploy-Preview-URL: https://ce-38442.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.tsx (2)
20-32: Consider refactoring useEditableText hook return typeThe array destructuring pattern makes the code brittle to changes in the hook's return values order. Consider refactoring the hook to return an object instead.
- const [ - inputRef, - editableName, - validationError, - handleKeyUp, - handleTitleChange, - ] = useEditableText( + const { + inputRef, + editableName, + validationError, + handleKeyUp, + handleTitleChange, + } = useEditableText(
77-83: Consider being explicit about passed propsUsing props spread operator could pass unwanted props to ListItem. Consider destructuring only the needed props.
- <ListItem - {...props} - customTitleComponent={customTitle} - startIcon={startIcon} - /> + <ListItem + title={props.title} + onClick={props.onClick} + isDisabled={props.isDisabled} + isSelected={props.isSelected} + customTitleComponent={customTitle} + startIcon={startIcon} + />app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.stories.tsx (3)
24-24: Remove console.log statementsReplace console.log with proper logging mechanism for production code.
- const onNameSave = (name: string) => console.log("Name saved" + name); + const onNameSave = (name: string) => {/* Add proper logging here */}; - const onClick = () => console.log("Add clicked"); + const onClick = () => {/* Add proper logging here */};Also applies to: 26-26
71-71: Enhance validation examples in storiesThe validation examples could be more realistic to better demonstrate actual use cases.
- validateName: () => null, + validateName: (name: string) => { + if (name.length < 3) return "Name must be at least 3 characters"; + if (!/^[a-zA-Z0-9_]+$/.test(name)) return "Only alphanumeric and underscore allowed"; + return null; + }, - validateName: () => "This is a sample error", + validateName: (name: string) => { + if (name === "EntityName") return "This name is already taken"; + return null; + },Also applies to: 91-91
117-126: Consider adding more edge case storiesThe current stories cover the main use cases well, but consider adding stories for:
- Very long entity names
- Special characters in names
- RTL text support
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/Editable/useEditableText.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.stories.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/Editable/useEditableText.ts
🔇 Additional comments (1)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.tsx (1)
38-43: Consider moving inline styles to styled componentsInline styles reduce maintainability and make it harder to enforce consistent styling across components.
Consider moving these styles to the EntityEditableName styled component.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
app/client/src/pages/Editor/IDE/LeftPane/DataSidePane.test.tsx (3)
50-50: Ensure test indexing remains stableRelying on specific item indices (e.g.
[0],[1],[2]) can introduce fragility if the order of list items changes in the future. Consider using more descriptive selectors or dynamically searching by text to increase test resilience.
50-67: Validate message strings in separate testsCurrently, each message string (e.g., "No queries in this app", "2 queries in this app") is tested in the same scenario. Extract each message check into its own smaller test or use table-driven tests to simplify maintenance and reading.
89-104: Maintain optional description clarityThe tests validate presence/absence of descriptions accurately, but they rely on string containment checks. If these are subject to frequent copy changes, consider either using more flexible partial matches or providing a data-testid attribute for stable reference.
app/client/packages/design-system/ads/src/List/List.tsx (2)
92-93: Combine description checksThe variables
isBlockDescriptionandisInlineDescriptionoverlap in purpose and slightly increase complexity. A concise approach might unify them or storedescriptionTypein a single variable to keep the logic simpler.
145-149: Prevent rightControl from absorbing pointer events unnecessarilyStopping propagation here makes sense to prevent click confusion, but if the user expects nested controls inside
rightControl, ensure those events bubble up correctly if needed for any future combined interactions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/packages/design-system/ads/src/List/List.tsx(4 hunks)app/client/src/pages/Editor/IDE/LeftPane/DataSidePane.test.tsx(2 hunks)
🔇 Additional comments (2)
app/client/packages/design-system/ads/src/List/List.tsx (2)
101-105: Consider event precedence for double-clickWhile adding an
onDoubleClickcallback, ensure the intended user interactions (context menu, single click, etc.) aren’t interrupted. In certain cases, immediateonClicktriggers might interfere with double-click actions if not carefully handled.
128-134: Ensure error states are visually prominentUsing
var(--ads-v2-color-fg-error)is a good start for denoting error states. Confirm that the text color is conspicuous enough relative to the background to pass accessibility color contrast standards.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12627652332. |
|
Deploy-Preview-URL: https://ce-38442.dp.appsmith.com |
Description
Creates an Entity Item component in ADS
Fixes #37896
Automation
/ok-to-test tags="@tag.Datasource, @tag.IDE"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12627573635
Commit: 0978b1c
Cypress dashboard.
Tags:
@tag.Datasource, @tag.IDESpec:
Mon, 06 Jan 2025 07:28:27 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
useEditableText) for managing inline text editing.EntityItemcomponent for enhanced list item interactions.EntityEditableNamefor managing text overflow during editing.EntityItemcomponent to showcase various states.Improvements
DataSidePanetests for better validation of rendered items.