[Security Solution] fix: react to deleting notes in timelines table#249777
[Security Solution] fix: react to deleting notes in timelines table#249777kelvtanv merged 10 commits intoelastic:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where deleting notes from the timelines table doesn't automatically update the UI. The solution refactors the delete functionality to use Redux state management (as already implemented in the notes management page) instead of maintaining separate local state in the timeline components.
Changes:
- Removed timeline-specific note deletion state management (
confirmingNoteId) in favor of the centralized notes Redux state - Integrated the shared
DeleteConfirmModalcomponent into the timelines table - Updated the delete button to dispatch Redux actions instead of managing local modal state
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
x-pack/solutions/security/plugins/security_solution/public/timelines/store/reducer.ts |
Removed setConfirmingNoteId action handler from timeline reducer |
x-pack/solutions/security/plugins/security_solution/public/timelines/store/model.ts |
Removed confirmingNoteId property from TimelineModel interface |
x-pack/solutions/security/plugins/security_solution/public/timelines/store/actions.ts |
Removed setConfirmingNoteId action creator |
x-pack/solutions/security/plugins/security_solution/public/timelines/components/open_timeline/translations.ts |
Added TIMELINE_TABLE_CAPTION translation for accessibility |
x-pack/solutions/security/plugins/security_solution/public/timelines/components/open_timeline/timelines_table/index.tsx |
Integrated DeleteConfirmModal and added table caption for accessibility |
x-pack/solutions/security/plugins/security_solution/public/timelines/components/open_timeline/timelines_table/index.test.tsx |
Added test coverage for delete confirmation modal rendering |
x-pack/solutions/security/plugins/security_solution/public/timelines/components/open_timeline/note_previews/index.tsx |
Refactored delete button to use Redux actions and removed local modal state management |
x-pack/solutions/security/plugins/security_solution/public/timelines/components/open_timeline/note_previews/index.test.tsx |
Updated test to verify Redux action dispatch instead of mutation calls |
x-pack/solutions/security/plugins/security_solution/public/timelines/components/open_timeline/index.tsx |
Added effect to clean up expanded notes when notes are deleted from Redux state |
x-pack/solutions/security/plugins/security_solution/public/notes/components/delete_confirm_modal.tsx |
Added optional callbacks and test selector for reusability across different contexts |
kqualters-elastic
left a comment
There was a problem hiding this comment.
desk tested this, works well. LGTM 👍
PhilippeOberti
left a comment
There was a problem hiding this comment.
Desk tested and code LGTM. I left a small comment though: I think you can simplify all the test files by removing the ThemeProvider.
jbudz
left a comment
There was a problem hiding this comment.
packages/kbn-babel-preset/styled_components_files.js LGTM
⏳ Build in-progress, with failures
Failed CI Steps
Test Failures
History
|
|
@kelvtanv you have a failing Cypress test (this guy). Looking at the screenshot, I think it could be a wrong You can find the info to run Cypress locally here (let me know if you need any help on this). |
|
@elasticmachine merge upstream |
|
Pinging @elastic/security-threat-hunting (Team:Threat Hunting) |
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
Summary
See #245363
Notes being deleted via the timelines table does not automatically update on the UI because the timeline states that include the notes are not being updated properly. Refactor the notes preview delete button in the timelines table to use the redux state as we do in the notes management page/popup.
Checklist
Check the PR satisfies following conditions.
Reviewers should verify this PR satisfies this list as well.
release_note:breakinglabel should be applied in these situations.release_note:*label is applied per the guidelinesbackport:*labels.