feat(datamapper): Allow to delete usage of a datampping by hitting De…#3211
Open
tplevko wants to merge 1 commit into
Open
feat(datamapper): Allow to delete usage of a datampping by hitting De…#3211tplevko wants to merge 1 commit into
tplevko wants to merge 1 commit into
Conversation
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR adds Delete/Backspace keyboard shortcut support to the DataMapper UI for removing selected mappings via a new ChangesDelete Hotkey Support for DataMapper Mappings
Sequence DiagramsequenceDiagram
participant User
participant Hook as useDataMapperDeleteHotkey
participant Store as useDocumentTreeStore
participant Service as MappingActionService
participant Mapper as useDataMapper
User->>Hook: Press Delete/Backspace
Hook->>Store: Get selected node path
Hook->>Mapper: Get target document tree
Hook->>Service: Check allowed actions for node
alt Delete action allowed
Hook->>Service: deleteMappingItem(target node)
Hook->>Store: clearSelection()
Hook->>User: Call onUpdate callback
else Delete action blocked
Hook->>User: No action taken
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3211 +/- ##
==========================================
+ Coverage 92.22% 92.25% +0.03%
==========================================
Files 633 637 +4
Lines 24443 24656 +213
Branches 5792 5849 +57
==========================================
+ Hits 22542 22746 +204
+ Misses 1899 1800 -99
- Partials 2 110 +108 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/ui/src/hooks/useDataMapperDeleteHotkey.hook.test.tsx (2)
75-77: ⚡ Quick winRemove unnecessary type assertions.
SonarCloud correctly identifies that these type assertions are unnecessary.
useDocumentTreeStore.setStateaccepts partial state by design, and the mock return type is already compatible withDocumentTree.♻️ Proposed simplification
mockCreateTree.mockReturnValue({ findNodeByPath: mockFindNodeByPath, - } as unknown as DocumentTree); + }); // Setup default store state useDocumentTreeStore.setState({ selectedNodePath: null, selectedNodeIsSource: false, clearSelection: mockClearSelection, - } as Partial<DocumentTreeState>); + });Apply the same pattern to lines 93-97, 139-143, and 158-162.
Also applies to: 80-84, 93-97, 139-143, 158-162
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/hooks/useDataMapperDeleteHotkey.hook.test.tsx` around lines 75 - 77, Remove the unnecessary "as unknown as DocumentTree" type assertions on mocked return values: instead of casting, pass the object directly to mockCreateTree.mockReturnValue (and to other mocks at the same pattern) because useDocumentTreeStore.setState accepts a partial state; update the occurrences that use mockCreateTree/mockFindNodeByPath (lines referenced around the other ranges) to return the plain object without the type assertion and apply the same removal to the other similar blocks (the groups noted at 80-84, 93-97, 139-143, and 158-162).
103-134: 💤 Low valueConsider verifying
preventDefaultis called.The tests invoke the hotkey callback but don't assert that
event.preventDefault()is called. While the implementation does call it (line 61 in the hook), explicitly verifying this behavior would strengthen the test coverage.📝 Optional test enhancement
it('should delete mapping when valid target node is selected and Delete is allowed', () => { renderHook(() => useDataMapperDeleteHotkey(mockOnUpdate)); const hotkeyCallback = mockHotkeys.mock.calls[0][1] as (event: KeyboardEvent) => void; const mockEvent = { preventDefault: jest.fn() } as unknown as KeyboardEvent; hotkeyCallback(mockEvent); + expect(mockEvent.preventDefault).toHaveBeenCalled(); expect(mockDeleteMappingItem).toHaveBeenCalledWith(mockTargetDocumentNodeData); });Apply the same pattern to other test cases that invoke the callback.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/hooks/useDataMapperDeleteHotkey.hook.test.tsx` around lines 103 - 134, Add assertions that event.preventDefault() is called when invoking the hotkey callback: in tests that renderHook(() => useDataMapperDeleteHotkey(mockOnUpdate)) and retrieve the hotkeyCallback from mockHotkeys.mock.calls[0][1], ensure the mockEvent used (with preventDefault: jest.fn()) has its preventDefault invoked by adding expect(mockEvent.preventDefault).toHaveBeenCalled() after hotkeyCallback(mockEvent); update the three shown tests (and any other tests that call hotkeyCallback) to include this assertion so the hook's call to preventDefault is verified.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ui/src/hooks/useDataMapperDeleteHotkey.hook.tsx`:
- Around line 59-68: The global hotkey handler in useDataMapperDeleteHotkey (the
useEffect that calls hotkeys('Delete, backspace', ...)) must ignore key events
originating from text-editable elements; change the handler to first inspect
event.target and return early if the target is an INPUT, TEXTAREA or an element
with isContentEditable (so you do not call event.preventDefault() or
handleKeyDown for those cases). Update the hotkeys callback in the useEffect
(referencing hotkeys and handleKeyDown) to perform this target check before
preventing default or invoking handleKeyDown, leaving the cleanup hotkeys.unbind
as-is.
---
Nitpick comments:
In `@packages/ui/src/hooks/useDataMapperDeleteHotkey.hook.test.tsx`:
- Around line 75-77: Remove the unnecessary "as unknown as DocumentTree" type
assertions on mocked return values: instead of casting, pass the object directly
to mockCreateTree.mockReturnValue (and to other mocks at the same pattern)
because useDocumentTreeStore.setState accepts a partial state; update the
occurrences that use mockCreateTree/mockFindNodeByPath (lines referenced around
the other ranges) to return the plain object without the type assertion and
apply the same removal to the other similar blocks (the groups noted at 80-84,
93-97, 139-143, and 158-162).
- Around line 103-134: Add assertions that event.preventDefault() is called when
invoking the hotkey callback: in tests that renderHook(() =>
useDataMapperDeleteHotkey(mockOnUpdate)) and retrieve the hotkeyCallback from
mockHotkeys.mock.calls[0][1], ensure the mockEvent used (with preventDefault:
jest.fn()) has its preventDefault invoked by adding
expect(mockEvent.preventDefault).toHaveBeenCalled() after
hotkeyCallback(mockEvent); update the three shown tests (and any other tests
that call hotkeyCallback) to include this assertion so the hook's call to
preventDefault is verified.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d4b0bb7d-1949-45b8-b15d-74f7f2412aef
📒 Files selected for processing (3)
packages/ui/src/components/View/SourceTargetView.tsxpackages/ui/src/hooks/useDataMapperDeleteHotkey.hook.test.tsxpackages/ui/src/hooks/useDataMapperDeleteHotkey.hook.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/ui/src/hooks/useDataMapperDeleteHotkey.hook.test.tsx (3)
75-77: 💤 Low valueConsider simplifying the type assertion.
SonarCloud flags this type assertion as unnecessary. Since you're creating a partial mock, consider using
Partial<DocumentTree>directly instead of the double assertion:mockCreateTree.mockReturnValue({ findNodeByPath: mockFindNodeByPath, } as Partial<DocumentTree> as DocumentTree);or better yet, if
DocumentTreeis an interface, cast directly withoutunknown.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/hooks/useDataMapperDeleteHotkey.hook.test.tsx` around lines 75 - 77, The test uses an unnecessary double type assertion when returning the mock from mockCreateTree; replace the "as unknown as DocumentTree" pattern with a single safer cast to a Partial<DocumentTree> (or direct cast to DocumentTree if DocumentTree is an interface) so the return value for mockCreateTree includes findNodeByPath: mockFindNodeByPath with a single, clearer assertion (referencing mockCreateTree, mockFindNodeByPath, and DocumentTree).
80-84: ⚡ Quick winRemove unnecessary type assertions flagged by SonarCloud.
Multiple test cases use
as Partial<DocumentTreeState>when callingsetState, but SonarCloud indicates these assertions are unnecessary. ThesetStatemethod likely already accepts partial state. Remove the type assertions for cleaner code:useDocumentTreeStore.setState({ selectedNodePath: null, selectedNodeIsSource: false, clearSelection: mockClearSelection, });Also applies to: 93-97, 139-143, 158-162
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/hooks/useDataMapperDeleteHotkey.hook.test.tsx` around lines 80 - 84, Remove the unnecessary "as Partial<DocumentTreeState>" type assertions on the useDocumentTreeStore.setState calls; update the calls to setState({ selectedNodePath: null, selectedNodeIsSource: false, clearSelection: mockClearSelection }) (and the analogous calls at the other occurrences) because useDocumentTreeStore.setState already accepts a partial state—search for useDocumentTreeStore.setState in this test file (the instances that reference DocumentTreeState and mockClearSelection) and delete the "as Partial<DocumentTreeState>" suffixes.
103-134: ⚡ Quick winConsider extracting the callback invocation pattern.
The pattern of extracting and invoking the hotkey callback is repeated across multiple tests (lines 106-109, 117-118, 128-129, 147-148, 169-170). Consider creating a helper function to reduce duplication:
const invokeHotkeyCallback = () => { const hotkeyCallback = mockHotkeys.mock.calls[0][1] as (event: KeyboardEvent) => void; const mockEvent = { preventDefault: jest.fn() } as unknown as KeyboardEvent; hotkeyCallback(mockEvent); };Then use:
invokeHotkeyCallback()in each test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/hooks/useDataMapperDeleteHotkey.hook.test.tsx` around lines 103 - 134, Extract the repeated pattern of grabbing and invoking the registered hotkey callback into a small helper used by the tests to reduce duplication: create a helper (e.g. invokeHotkeyCallback) that reads mockHotkeys.mock.calls[0][1] as the (event: KeyboardEvent) callback, constructs a mock event with preventDefault, and calls the callback; then replace the repeated lines in the tests exercising useDataMapperDeleteHotkey (the blocks that access mockHotkeys and call the callback in multiple it cases) with a single call to that helper so each test simply renders the hook and calls invokeHotkeyCallback().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ui/src/hooks/useDataMapperDeleteHotkey.hook.test.tsx`:
- Around line 179-221: Move the "tree creation and memoization" describe block
out of the "deletion blocked scenarios" describe and place it at the top level
inside the main describe('useDataMapperDeleteHotkey', ...) alongside the
existing "successful deletion" and "deletion blocked scenarios" blocks; ensure
the tests that reference useDataMapperDeleteHotkey, mockCreateTree and
mockUseDataMapper remain unchanged other than their container so they run
independently of the blocked-scenarios context and continue to call
renderHook/rerender as before.
- Around line 137-221: Add tests to cover: (1) findNodeByPath returning
null/undefined — set useDocumentTreeStore selectedNodePath and
selectedNodeIsSource=false, mockFindNodeByPath to return null/undefined,
mockGetAllowedActions to include MappingActionKind.Delete, render
useDataMapperDeleteHotkey, invoke the registered hotkey callback and assert
mockDeleteMappingItem and mockOnUpdate were not called; (2) selected source node
— set selectedNodeIsSource=true and selectedNodePath non-null,
mockFindNodeByPath to return a node and mockGetAllowedActions to include Delete,
invoke hotkey and assert deletion was blocked (mockDeleteMappingItem not
called); (3) hotkey registration — after rendering useDataMapperDeleteHotkey
assert mockHotkeys was called with the key string containing 'delete' and
'backspace'; (4) cleanup/unmount — renderHook, call unmount(), and assert
mockHotkeysUnbind (or hotkeys.unbind) was called; use the existing mocks
(mockFindNodeByPath, mockGetAllowedActions, mockHotkeys, mockHotkeysUnbind,
mockDeleteMappingItem) and the hook useDataMapperDeleteHotkey to locate where to
add these tests.
---
Nitpick comments:
In `@packages/ui/src/hooks/useDataMapperDeleteHotkey.hook.test.tsx`:
- Around line 75-77: The test uses an unnecessary double type assertion when
returning the mock from mockCreateTree; replace the "as unknown as DocumentTree"
pattern with a single safer cast to a Partial<DocumentTree> (or direct cast to
DocumentTree if DocumentTree is an interface) so the return value for
mockCreateTree includes findNodeByPath: mockFindNodeByPath with a single,
clearer assertion (referencing mockCreateTree, mockFindNodeByPath, and
DocumentTree).
- Around line 80-84: Remove the unnecessary "as Partial<DocumentTreeState>" type
assertions on the useDocumentTreeStore.setState calls; update the calls to
setState({ selectedNodePath: null, selectedNodeIsSource: false, clearSelection:
mockClearSelection }) (and the analogous calls at the other occurrences) because
useDocumentTreeStore.setState already accepts a partial state—search for
useDocumentTreeStore.setState in this test file (the instances that reference
DocumentTreeState and mockClearSelection) and delete the "as
Partial<DocumentTreeState>" suffixes.
- Around line 103-134: Extract the repeated pattern of grabbing and invoking the
registered hotkey callback into a small helper used by the tests to reduce
duplication: create a helper (e.g. invokeHotkeyCallback) that reads
mockHotkeys.mock.calls[0][1] as the (event: KeyboardEvent) callback, constructs
a mock event with preventDefault, and calls the callback; then replace the
repeated lines in the tests exercising useDataMapperDeleteHotkey (the blocks
that access mockHotkeys and call the callback in multiple it cases) with a
single call to that helper so each test simply renders the hook and calls
invokeHotkeyCallback().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e6cdbdd-9f1b-40b5-b1aa-0059ea745621
📒 Files selected for processing (3)
packages/ui/src/components/View/SourceTargetView.tsxpackages/ui/src/hooks/useDataMapperDeleteHotkey.hook.test.tsxpackages/ui/src/hooks/useDataMapperDeleteHotkey.hook.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/ui/src/hooks/useDataMapperDeleteHotkey.hook.tsx
- packages/ui/src/components/View/SourceTargetView.tsx
…lete key Co-authored-by: Copilot <copilot@github.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…lete key
fixes #1797
Summary by CodeRabbit