Handle DetailsChanged websocket notifications on flow page
#1346
Conversation
WalkthroughThe pull request introduces a new message handler, Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (3)test/integration/patients/patient/patient-flow.js (3)
This issue was previously identified. The test waits for routes that haven't been intercepted, which could cause test flakiness.
The test effectively verifies that:
The test effectively verifies that flow details are updated in the UI when WebSocket notifications are received. 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
CodeRabbit Configuration File (
|
| 'change:_state': 'onChangeActionState', | ||
| 'change:_owner': 'onChangeOwner', | ||
| 'change:due_date': 'onChangeDue', | ||
| 'change:due_time': 'onChangeDue', | ||
| 'change:duration': 'onChangeDuration', | ||
| }, | ||
| onChangeDetails() { | ||
| this.showEditForm(); | ||
| }, |
There was a problem hiding this comment.
A user could be making edits to the action details in the action sidebar and have those edits wiped out and replaced if a DetailsChanged web socket message comes in.
Is that okay from a UI/user experience perspective?
There was a problem hiding this comment.
So this might be tough.. but if the user is currently modifying the details we do not need to rerender the form.. we can just let them override them, but if they cancel the form, it should show the socket details.
There was a problem hiding this comment.
Okay ya that seems like the way to go. See if I can get that working.
There was a problem hiding this comment.
Just pushed a commit with a potential implementation.
| @@ -130,12 +130,16 @@ const ActionView = View.extend({ | |||
| duration: '[data-duration-region]', | |||
| }, | |||
| modelEvents: { | |||
| 'change:details': 'onChangeDetails', | |||
There was a problem hiding this comment.
We'll need to add something similar for the action name here in the sidebar.
Currently the action name won't be updated in the sidebar if a NameChanged ws message comes in.
There was a problem hiding this comment.
I actually think not doing this is a bug... but I don't think action names change much. We have to handle events in the entire context of a page.
There was a problem hiding this comment.
Okay, I can tackle this in a separate story/PR.
RoundingWell Care Ops Frontend
|
||||||||||||||||||||||||||||
| Project |
RoundingWell Care Ops Frontend
|
| Branch Review |
ws-details-changed
|
| Run status |
|
| Run duration | 03m 01s |
| Commit |
|
| Committer | Nick Major |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
3
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
312
|
| View all changes introduced in this branch ↗︎ | |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/js/entities-service/entities/flows.js (1)
28-30: Consider enhancing message handlers with validation and documentation.While the implementation is correct, consider these architectural improvements for all message handlers:
- Add type validation for parameters
- Document the expected shape of data
- Add JSDoc comments for better IDE support
Example enhancement:
/** * Updates the flow details and additional attributes * @param {Object} params - The update parameters * @param {Object} params.details - The new details object * @param {Object} [params.attributes={}] - Additional attributes to update */ DetailsChanged({ details, attributes = {} }) { if (!details || typeof details !== 'object') { throw new Error('Details must be an object'); } this.set({ details, ...attributes }); }Would you like me to create an issue to track these architectural improvements?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/js/entities-service/entities/actions.js(1 hunks)src/js/entities-service/entities/flows.js(1 hunks)src/js/views/patients/sidebar/action/action-sidebar-action_views.js(1 hunks)test/integration/patients/patient/patient-flow.js(2 hunks)
🔇 Additional comments (7)
src/js/entities-service/entities/flows.js (2)
28-30: LGTM! Implementation follows established patterns.
The DetailsChanged message handler maintains consistency with existing handlers in terms of structure and behavior, properly handling both the main attribute and optional attributes spread.
28-30: Verify websocket notification handling integration.
Since this handler is for websocket notifications, let's ensure consistent implementation across the system:
✅ Verification successful
Implementation appears consistent with entity pattern
Based on the verification results:
- Found identical
DetailsChangedhandler implementations in bothflows.jsandactions.js - Both handlers follow the same pattern of accepting
detailsand optionalattributes - The implementation matches the standard entity attribute update pattern with
this.set() - No conflicting or inconsistent websocket handling patterns were found
The change maintains consistency with the existing codebase patterns for entity detail updates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other DetailsChanged handlers and websocket related code
echo "Searching for other DetailsChanged handlers..."
rg "DetailsChanged.*details.*attributes" -g "*.js"
echo "\nChecking websocket notification handling..."
rg "websocket.*details.*changed" -i -g "*.js"
echo "\nChecking for related test coverage..."
rg "describe.*DetailsChanged" -g "*test*.js"
Length of output: 628
Script:
#!/bin/bash
# Let's check the implementation of both DetailsChanged handlers and their context
echo "Checking DetailsChanged handler in flows.js:"
rg -B 5 -A 5 "DetailsChanged.*details.*attributes" src/js/entities-service/entities/flows.js
echo "\nChecking DetailsChanged handler in actions.js:"
rg -B 5 -A 5 "DetailsChanged.*details.*attributes" src/js/entities-service/entities/actions.js
# Let's also check for websocket event registration/handling
echo "\nChecking websocket event handling:"
rg "details.changed" -i -g "*.js" -B 5 -A 5
# Check for any tests related to details changes
echo "\nChecking for details change tests:"
rg "test.*details.*changed" -i -g "*test*.js"
Length of output: 1311
Script:
#!/bin/bash
# Let's check for websocket event registration patterns
echo "Checking for websocket event registration patterns:"
rg -B 5 -A 5 "socket\.on.*details" -g "*.js"
# Let's also check the base model or parent class implementation
echo "\nChecking for base model or parent class:"
ast-grep --pattern 'class $_ extends $_ {
$$$
}' src/js/entities-service/entities/flows.js
# Check for event emission patterns
echo "\nChecking for event emission patterns:"
rg "emit.*details.changed" -i -g "*.js"
Length of output: 480
src/js/entities-service/entities/actions.js (1)
39-41: LGTM! Implementation follows established patterns.
The new DetailsChanged message handler follows the same pattern as other handlers and properly handles attribute updates.
Let's verify the handler usage and related view listeners:
✅ Verification successful
The verification results show that there are multiple views listening to 'change:details' events, which is consistent with the implementation of the new DetailsChanged handler. The views are properly set up to respond to detail changes, and no other conflicting implementations of DetailsChanged were found.
Let's verify one more aspect to ensure the handler's implementation aligns with the existing pattern of other message handlers in the same file:
Implementation verified and consistent with codebase patterns
The verification confirms:
- Multiple views correctly listen to 'change:details' events
- The
setmethod usage inDetailsChangedfollows the same pattern as other handlers (spreading additional attributes) - No conflicting implementations exist in other models
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for views listening to 'change:details' events and verify consistent implementation across models
# Test 1: Find views listening to 'change:details'
echo "=== Views listening to 'change:details' ==="
rg "change:details" -g "!*.test.js"
# Test 2: Check for similar DetailsChanged handlers in other models
echo -e "\n=== Other DetailsChanged implementations ==="
ast-grep --pattern 'DetailsChanged({ details, attributes = {} })'
Length of output: 969
Script:
#!/bin/bash
# Check other message handlers in the same file for consistency
echo "=== Other message handlers in actions.js ==="
ast-grep --pattern 'messages: {
$$$
}'
# Also check the set method usage pattern
echo -e "\n=== Usage of set method ==="
rg "\.set\(" src/js/entities-service/entities/actions.js -A 1
Length of output: 621
src/js/views/patients/sidebar/action/action-sidebar-action_views.js (2)
133-139: LGTM! Event handler follows established patterns.
The addition of 'change:details': 'onChangeDetails' follows the existing pattern of model change event handlers and maintains consistency with the codebase.
140-142: Verify the implementation approach for real-time updates.
The current implementation shows the edit form whenever details change, which differs from other change handlers that directly update their respective components. Consider:
- Should we check if the view is rendered first? (see
onChangeOwner) - Would directly updating the details view be less disruptive than showing the full edit form during real-time updates?
- Are there edge cases to handle (e.g., concurrent edits)?
Let's check how other views handle real-time updates:
✅ Verification successful
Let me generate additional scripts to check the implementation of other change handlers and the showEditForm method:
This will help us understand:
- How
onChangeOwnerimplements the render check - What exactly
showEditFormdoes - If other handlers follow a pattern of checking render state
Fixed the regex patterns to properly escape special characters and handle different function declaration styles. This will help us understand the implementation patterns.
Let me try one more search to find the showEditForm implementation and other change handlers:
This will help us understand the full context of the edit form display and how other handlers behave.
The implementation approach is correct and consistent with the codebase patterns
The current implementation is appropriate because:
-
The
showEditForm()method follows the established pattern in the codebase for handling editable content:- It clones the action model for cancelable editing
- Sets up change listeners for the save button
- Shows the details view
- Manages the save region appropriately
-
Unlike
onChangeOwnerand other simple property changes,onChangeDetailstriggers the edit form because it's part of a larger editing workflow that requires:- A cloned model for cancelable changes
- Save button visibility
- Details view refresh
-
The render check isn't necessary here because
showEditFormis called in other critical paths (likeonRenderandonCancel) where the view is guaranteed to be rendered.
The implementation correctly handles real-time updates by providing proper isolation through model cloning and maintaining consistency with the codebase's editing patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar websocket notification handlers
rg -A 5 "DetailsChanged|change:details" --type js
# Look for patterns in change handlers
ast-grep --pattern 'onChange$_($$$) {
$$$
}'
Length of output: 4739
Script:
#!/bin/bash
# Look for onChangeOwner implementation
rg -A 5 "onChangeOwner.*{" --type js
# Look for showEditForm implementation
rg -A 10 "showEditForm.*{" --type js
# Look for other similar change handlers
ast-grep --pattern 'onChange$_($$$) {
if (!this.isRendered()) return;
$$$
}'
Length of output: 350
Script:
#!/bin/bash
# Look for onChangeOwner implementation
rg -A 5 "onChangeOwner\s*\{|onChangeOwner:\s*function\s*\{" --type js
# Look for showEditForm implementation
rg -A 10 "showEditForm\s*\{|showEditForm:\s*function\s*\{" --type js
# Look for isRendered checks in change handlers
rg -B 2 -A 3 "if.*!this\.isRendered.*return" --type js
Length of output: 1832
Script:
#!/bin/bash
# Find showEditForm method with more context
rg -B 5 -A 10 "showEditForm" --type js
# Find other change handlers in the same file
rg -B 2 -A 5 "onChangeDue|onChangeDuration|onChangeActionState" src/js/views/patients/sidebar/action/action-sidebar-action_views.js
Length of output: 8680
test/integration/patients/patient/patient-flow.js (2)
156-171: LGTM: WebSocket test for action details update is properly implemented.
The test correctly simulates a DetailsChanged WebSocket event for a patient action and asserts that the action details are updated in the UI as expected.
2294-2309: LGTM: WebSocket test for flow details update is properly implemented.
The test accurately verifies that upon receiving a DetailsChanged WebSocket event for a flow, the flow details are updated in the UI as intended.
Pull Request Test Coverage Report for Build 58efb554-a4ca-4f6e-9e70-e4ba6d3bb7f0Details
💛 - Coveralls |
| 'change:_state': 'onChangeActionState', | ||
| 'change:_owner': 'onChangeOwner', | ||
| 'change:due_date': 'onChangeDue', | ||
| 'change:due_time': 'onChangeDue', | ||
| 'change:duration': 'onChangeDuration', | ||
| }, | ||
| onChangeDetails() { | ||
| this.showEditForm(); | ||
| }, |
There was a problem hiding this comment.
So this might be tough.. but if the user is currently modifying the details we do not need to rerender the form.. we can just let them override them, but if they cancel the form, it should show the socket details.
| @@ -181,14 +183,22 @@ const ActionView = View.extend({ | |||
| this.showChildView('save', new SaveView({ model: this.clonedAction })); | |||
| }, | |||
| onSave() { | |||
| this.isEditingDetails = false; | |||
|
|
|||
| this.getRegion('save').empty(); | |||
| }, | |||
There was a problem hiding this comment.
Could this function just be:
onSave() {
this.showEditForm();
}showEditForm() calls this.getRegion('save').empty().
There was a problem hiding this comment.
so showEditForm immediately clones the action.. I think that would occur prior to the onSave in the app which copies the details from the clone to the model.. so saving would revert the data I think?
There was a problem hiding this comment.
but it probably does make sense to put these two lines in some for of "stopEditing" that is called both here and within showEditForm
There was a problem hiding this comment.
Agreed, I added a stopEditingDetails()
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
test/integration/patients/patient/patient-flow.js (1)
2332-2347: Consider adding error case tests for websocket notifications.While the happy path is well tested, consider adding test cases for:
- Malformed websocket messages
- Missing or null details
- Race conditions between manual updates and websocket notifications
Example test case:
cy.sendWs({ category: 'DetailsChanged', resource: { type: 'flows', id: testSocketFlow.id, }, payload: { details: null // Test null details handling }, }); cy .get('[data-header-region]') .find('.patient-flow__details') .should('be.empty');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/js/views/patients/sidebar/action/action-sidebar-action_views.js(2 hunks)test/integration/patients/patient/patient-flow.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/js/views/patients/sidebar/action/action-sidebar-action_views.js
🔇 Additional comments (2)
test/integration/patients/patient/patient-flow.js (2)
146-149: Past review comment is still valid.
The previous review already identified the missing intercepts issue.
156-209: LGTM! Comprehensive test coverage for details changes.
The test thoroughly covers both manual updates and websocket notifications for action details, including proper UI state verification.
There was a problem hiding this comment.
So to test this I did:
- User makes manual edit to details by typing in text area.
- Web socket notification is received. Verify user's manual edits are NOT overridden.
- Save the user's manual edited details. Now the user is no longer editing the details.
- Web socket notification is received. Verify text area content is replaced with the socket details.
There was a problem hiding this comment.
I'm not sure I understand 4. The user will not get notifications for the modifications they make themselves.
There was a problem hiding this comment.
1-3 is for when the current user is actively making edits in the details text area. To make sure that when a web socket notification comes in, their edits are not overridden.
For 3, I submitted the user's edits so the form won't be in the isEditingDetails = true state anywhere. To make testing 4 possible.
4 represents a web socket notification from another user. Where the details are not being edited by the current user and therefore the details text area contents are replaced.
If that makes sense...
There was a problem hiding this comment.
I changed the test so the user cancel's their manually added edits. Instead of submitting them. I think that made the test confusing and overly complicated.
Now is this sequence:
- User makes manual edit to details by typing in text area.
- Web socket notification is received. Verify user's manual edits are NOT overridden.
- User clicks cancel button to clear their changes. Now the user is no longer editing the details.
- Web socket notification is received. Verify text area content is replaced with the socket details.
| @@ -130,12 +130,16 @@ const ActionView = View.extend({ | |||
| duration: '[data-duration-region]', | |||
| }, | |||
| modelEvents: { | |||
| 'change:details': 'onChangeDetails', | |||
There was a problem hiding this comment.
I actually think not doing this is a bug... but I don't think action names change much. We have to handle events in the entire context of a page.
There was a problem hiding this comment.
I'm not sure I understand 4. The user will not get notifications for the modifications they make themselves.
| @@ -181,14 +183,22 @@ const ActionView = View.extend({ | |||
| this.showChildView('save', new SaveView({ model: this.clonedAction })); | |||
| }, | |||
| onSave() { | |||
| this.isEditingDetails = false; | |||
|
|
|||
| this.getRegion('save').empty(); | |||
| }, | |||
There was a problem hiding this comment.
so showEditForm immediately clones the action.. I think that would occur prior to the onSave in the app which copies the details from the clone to the model.. so saving would revert the data I think?
| @@ -181,14 +183,22 @@ const ActionView = View.extend({ | |||
| this.showChildView('save', new SaveView({ model: this.clonedAction })); | |||
| }, | |||
| onSave() { | |||
| this.isEditingDetails = false; | |||
|
|
|||
| this.getRegion('save').empty(); | |||
| }, | |||
There was a problem hiding this comment.
but it probably does make sense to put these two lines in some for of "stopEditing" that is called both here and within showEditForm
nmajor25
force-pushed
the
ws-details-changed
branch
2 times, most recently
from
6d599dc to
55d1057
Compare
|
|
||
| this.getRegion('save').empty(); | ||
| this.listenTo(this.clonedAction, 'change:details', () => { |
There was a problem hiding this comment.
can we make this line this.listenTo(this.clonedAction, 'change:details', this.startEditingDetails); that's the flag and the this.showChildView line from showSave?
There was a problem hiding this comment.
Nice, I like it. Just pushed that change 👇
nmajor25
force-pushed
the
ws-details-changed
branch
from
55d1057 to
d92e9c4
Compare
Shortcut Story ID: [sc-56329]
In the action sidebar, the details text area content will NOT be updated if the user is currently making edits.