chore: Housekeeping of List Item and Entity Item #38524
Conversation
WalkthroughThe pull request introduces several enhancements to the design system's list and entity explorer components. The changes primarily focus on improving event handling, adding test capabilities, and refining component interactions. Key modifications include updating event callbacks, adding unique identifiers, and implementing more robust event management across multiple files in the design system package. Changes
Sequence DiagramsequenceDiagram
participant User
participant EntityItem
participant ListItem
participant EventHandler
User->>EntityItem: Interact with item
EntityItem->>ListItem: Trigger event
ListItem->>EventHandler: Process event
EventHandler-->>ListItem: Return handled event
ListItem-->>EntityItem: Update component state
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
the
skip-changelog
![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 (2)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.tsx (1)
7-7: Consider using consistent import aliasThe import alias
clxis used for classnames. Consider usingclassNamesto match common conventions.app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.stories.tsx (1)
39-39: Consider demonstrating id prop variationsWhile the hardcoded "storyItem" id works, consider showing different id values in various stories to better demonstrate the prop's usage.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/client/packages/design-system/ads/src/List/List.tsx(4 hunks)app/client/packages/design-system/ads/src/List/List.types.tsx(2 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/Editable/useEditableText.test.tsx(4 hunks)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(2 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.types.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-build / client-build
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
- GitHub Check: chromatic-deployment
🔇 Additional comments (9)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.types.ts (1)
8-9: Well-documented addition of the id propertyGood addition of the id property with clear JSDoc documentation explaining its purpose in the markup.
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.tsx (1)
81-84: Good addition of test and identification attributesThe changes improve component testability and identification:
- Proper class composition using classnames
- Descriptive data-testid
- Consistent id prefix pattern
app/client/packages/design-system/ads/src/Templates/EntityExplorer/Editable/useEditableText.ts (1)
77-78: Verify the necessity of stopping event propagationAdding
stopPropagation()prevents parent components from receiving these keyboard events. While this might be intentional to prevent unwanted side effects, it could also break existing functionality that relies on these events.✅ Verification successful
Stopping event propagation is appropriate here
The addition of
stopPropagation()in thehandleKeyUpfunction follows the established patterns in the codebase for handling keyboard events. This prevents unwanted interactions with global keyboard shortcuts or parent components while editing text in the EntityExplorer component.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for parent components that might be listening for keyboard events rg -B 2 -A 2 "onKeyUp|onKeyDown|addEventListener.*keyup|addEventListener.*keydown" --type ts --type tsxLength of output: 133
Script:
#!/bin/bash # Search for keyboard event handlers with correct file types rg -t ts "onKeyUp|onKeyDown|addEventListener.*keyup|addEventListener.*keydown" -B 2 -A 2 # Search for the component hierarchy to understand event bubbling ast-grep --pattern 'const useEditableText = $_' -A 10Length of output: 67517
app/client/packages/design-system/ads/src/Templates/EntityExplorer/Editable/useEditableText.test.tsx (1)
85-85: LGTM! Test coverage is comprehensiveThe test modifications correctly mock the event's
stopPropagationmethod and cover all relevant scenarios including valid saves, invalid saves, and cancellation cases.Also applies to: 117-117, 139-142, 163-166
app/client/packages/design-system/ads/src/List/List.types.tsx (1)
2-2: LGTM! Improved type safety for click handlersThe addition of MouseEvent type for onClick callback provides better type safety and access to event properties.
Also applies to: 14-14
app/client/packages/design-system/ads/src/List/List.tsx (4)
40-57: LGTM! Performance optimization in tooltip handlingReplaced effect-based checking with event-based approach using useEventCallback. This is more efficient as it computes tooltip visibility only when needed.
95-101: LGTM! Consistent event handling patternThe handleDoubleClick implementation follows the same pattern as handleOnClick, maintaining consistency in event handling.
103-105: Verify empty handler intentionThe handleRightControlClick only stops propagation without any handler logic. Is this intentional or should it handle the rightControl click events?
✅ Verification successful
Empty handler is intentional and correct
The empty handler with stopPropagation() is the correct implementation. It prevents click events on rightControl elements from triggering the parent ListItem's onClick handler, allowing rightControl components to handle their own click events independently.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for rightControl usage patterns rg "rightControl.*onClick" app/client/packages/Length of output: 49
Script:
#!/bin/bash # Search for rightControl prop definition and usage in List component ast-grep --pattern 'interface $_ { $$$ rightControl?: $_; $$$ }' # Look for the List component implementation cat app/client/packages/design-system/ads/src/List/List.tsx # Search for similar event handlers in List component rg "handle.*Click.*stopPropagation" app/client/packages/design-system/ads/src/List/Length of output: 8257
87-93: Verify stopPropagation impact on EntityListTreeThe addition of stopPropagation is good for preventing event bubbling, but please verify this doesn't interfere with any required event bubbling in EntityListTree usage.
✅ Verification successful
stopPropagation usage is safe
The codebase analysis shows no direct usage of the List component in tree-related components. The stopPropagation addition won't interfere with any tree component functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for EntityListTree usage of List components rg -l "EntityListTree.*List" app/client/packages/Length of output: 51
Script:
#!/bin/bash # Search for EntityListTree files echo "=== Searching for EntityListTree files ===" fd EntityListTree # Search for tree-like components importing List echo "=== Searching for List imports in tree components ===" rg "import.*List.*from.*ads" -A 5 --glob "*Tree*.{ts,tsx}" # Search for List component usage in tree components echo "=== Searching for List usage in tree components ===" rg "<List" --glob "*Tree*.{ts,tsx}"Length of output: 443
Script:
#!/bin/bash # Search for any tree-related components echo "=== Searching for tree-related files ===" fd -e tsx -e ts --glob "*[Tt]ree*" # Search for List component usage in any file echo "=== Searching for List component usage ===" rg -l "import.*List.*from.*ads" # Search for recent changes to tree components echo "=== Searching for tree-related changes ===" git diff --name-only HEAD~5 HEAD | grep -i "tree"Length of output: 5355
Description
General housekeeping items to ADS ListItem, Editable and Entity Item. In prep for usage of EntityListTree we found these items missing. PR #38493
Automation
/ok-to-test tags="@tag.IDE, @tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12665028794
Commit: 3f68ddf
Cypress dashboard.
Tags:
@tag.IDE, @tag.SanitySpec:
Wed, 08 Jan 2025 06:51:47 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
Improvements
Technical Updates
useEventCallbackhook for more efficient event handling