-
Notifications
You must be signed in to change notification settings - Fork 8
History notifications not being removed #2151
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: John Bair <[email protected]>
…nsaction nodes Signed-off-by: John Bair <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2151 +/- ##
=======================================
Coverage 99.85% 99.85%
=======================================
Files 175 175
Lines 6832 6832
Branches 1315 1315
=======================================
Hits 6822 6822
Misses 10 10 🚀 New features to boost your workflow:
|
Signed-off-by: John Bair <[email protected]>
Signed-off-by: John Bair <[email protected]>
Signed-off-by: John Bair <[email protected]>
Signed-off-by: John Bair <[email protected]>
ericleponner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
My comments are cosmetic.
front-end/src/renderer/pages/Transactions/components/TransactionNodeRow.vue
Outdated
Show resolved
Hide resolved
front-end/src/renderer/pages/Transactions/components/TransactionNodeTable.vue
Outdated
Show resolved
Hide resolved
svienot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…iption composable Signed-off-by: John Bair <[email protected]>
…hub.com/hashgraph/hedera-transaction-tool into history-notifications-not-being-removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request refactors WebSocket subscription management and enhances notification filtering to support multiple notification types. The changes introduce a reusable useWebsocketSubscription composable that consolidates WebSocket event handling across the application, and updates the notification system to properly track and display notifications for various transaction states in the History collection.
Key changes include:
- Created a new
useWebsocketSubscriptioncomposable to standardize WebSocket event subscriptions - Updated
useFilterNotificationsto accept arrays of notification types instead of single types - Enhanced transaction node rows to show notifications for multiple relevant states in the History collection
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| useWebsocketSubscription.ts | New composable that encapsulates WebSocket subscription logic with automatic connection/reconnection handling |
| useFilterNotifications.ts | Updated to accept and filter by multiple notification types simultaneously |
| TransactionNodeRow.vue | Modified to use notification type arrays and compute hasOldNotifications for proper notification display |
| TransactionNodeTable.vue | Integrated WebSocket subscription and useMarkNotifications with specified notification types |
| History.vue | Refactored to use the new useWebsocketSubscription composable, removing manual subscription code |
| TransactionGroupDetails.vue | Refactored to use the new useWebsocketSubscription composable, removing manual subscription code |
| TransactionDetails.vue | Refactored to use the new useWebsocketSubscription composable, removing manual subscription code |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
front-end/src/renderer/pages/Transactions/components/TransactionNodeTable.vue
Show resolved
Hide resolved
front-end/src/renderer/pages/Transactions/components/TransactionNodeRow.vue
Show resolved
Hide resolved
ericleponner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description:
This pull request enhances the notification filtering logic and improves the handling of transaction node updates in the transaction table. The main improvements include supporting multiple notification types in the notification filtering composable, updating the transaction node row to filter by multiple notification types for certain collections, and setting up real-time updates for transaction nodes via WebSocket events.
Notification Filtering Enhancements:
useFilterNotificationsto accept and handle an array of notification types, allowing filtering by multiple types at once. [1] [2]TransactionNodeRow.vueto use this new capability, especially for theHISTORYcollection, which now filters notifications by several relevant types. [1] [2]Transaction Node Table Improvements:
TransactionNodeTable.vueto subscribe to WebSocketTRANSACTION_ACTIONevents, triggering a refresh of transaction nodes when relevant backend events occur.Related issue(s):
Fixes #2148
Checklist