Websocket general implementation #1314
Conversation
WalkthroughThe changes introduce a new function, Changes
Possibly related PRs
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (4)
Files skipped from review as they are similar to previous changes (4)
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 (
|
| @@ -47,7 +47,8 @@ const _Model = BaseModel.extend({ | |||
| }, | |||
| getOwner() { | |||
| const owner = this.get('_owner'); | |||
| return Radio.request('entities', `${ owner.type }:model`, owner.id); | |||
There was a problem hiding this comment.
@nmajor25 this is specifically the pattern needed for author to support clinician or patient for the form history. Store.get(author.type) ensures the correct model and request where Radio.request('entities', ${ owner.type }:model`` does not.
RoundingWell Care Ops Frontend
|
||||||||||||||||||||||||||||
| Project |
RoundingWell Care Ops Frontend
|
| Branch Review |
websocket
|
| Run status |
|
| Run duration | 02m 54s |
| Commit |
|
| Committer | Paul Falgout |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
311
|
| View all changes introduced in this branch ↗︎ | |
paulfalgout
force-pushed
the
websocket
branch
from
29a0251
to
de05519
Compare
Pull Request Test Coverage Report for Build f9dd93e0-6e79-44a6-8068-f6c4a0d2d6a5Details
💛 - Coveralls |
paulfalgout
force-pushed
the
websocket
branch
from
de05519
to
f2bae91
Compare
paulfalgout
force-pushed
the
websocket
branch
from
f2bae91
to
43ecb16
Compare
|
This still needs tests, but I think it's close |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (2)
src/js/config.js (2)
10-17: LGTM! The newfetchWebsocketfunction aligns with the PR objectives.The function retrieves the WebSocket endpoint from the server and updates the
appConfigobject accordingly, contributing to the foundational implementation of websocket messages across various entities within the application.Consider the following suggestions for further improvement:
- Add error handling for the fetch request to gracefully handle any potential failures in retrieving the WebSocket configuration.
- Assess the impact of introducing the new
appConfig.wsproperty on existing code that relies on theappConfigobject. Ensure that the addition of this property does not introduce any unintended side effects or break existing functionality.
Line range hint
19-36: LGTM! The modifications tofetchConfigintroduce flexibility in the configuration fetching process.The addition of the
isFormparameter allows for different behaviors based on the context in whichfetchConfigis invoked. IfisFormis true, thesuccesscallback is executed immediately after fetching the main configuration, while ifisFormis false, the function proceeds to callfetchWebsocketto retrieve the WebSocket configuration.Consider the following suggestions for further improvement:
- Add code comments to clarify the purpose and behavior of the
isFormparameter. Explain the different configuration fetching flows based on the value ofisFormto enhance code readability and maintainability.- Assess the impact of the modified control flow on existing code that relies on
fetchConfig. Ensure that the introduction of theisFormparameter and the conditional execution offetchWebsocketdo not introduce any unintended side effects or break existing functionality.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- src/js/apps/patients/patient/flow/flow_app.js (4 hunks)
- src/js/base/model.js (2 hunks)
- src/js/config.js (2 hunks)
- src/js/entities-service/entities/actions.js (2 hunks)
- src/js/entities-service/entities/flows.js (2 hunks)
- src/js/index.js (0 hunks)
- src/js/services/ws.js (2 hunks)
- test/support/commands.js (1 hunks)
- test/support/websockets.js (1 hunks)
Files not reviewed due to no reviewable changes (1)
- src/js/index.js
Additional comments not posted (17)
test/support/websockets.js (1)
82-92: Great addition for testing WebSocket interactions!The new
routeWebsocketsCypress command is a valuable addition to the testing capabilities. It allows tests to intercept and control the responses from the WebSocket API, enabling more comprehensive testing scenarios.By mocking the WebSocket API responses, tests can simulate different states and behaviors, improving the overall test coverage and reliability. This command will be particularly useful for testing WebSocket-related features and ensuring their proper functionality.
The implementation looks clean and straightforward, making it easy to understand and use in tests.
src/js/services/ws.js (2)
4-4: LGTM!The import statement for
Storefrombackbone.storeis syntactically correct and aligns with the purpose mentioned in the AI-generated summary.
68-72: Excellent enhancement for handling WebSocket messages with resources!The added logic in the
_onMessagemethod aligns perfectly with the AI-generated summary. It enables the application to dynamically manage resources based on incoming WebSocket messages by:
- Checking for the presence of a
resourceproperty in the message.- Retrieving the appropriate resource type from the
Storeusingdata.resource.type.- Creating a new instance of the retrieved resource type using
data.resource.id.- Invoking the
handleMessagemethod on the resource instance with the parsed message data.This enhancement allows for resource-specific processing of WebSocket messages, improving the application's ability to handle and manage resources dynamically.
test/support/commands.js (1)
84-84: LGTM! The changes enhance WebSocket support in Cypress tests.The addition of the
.routeWebsockets()and.mockWs()methods within the overwrittenvisitcommand enables the routing and mocking of WebSocket connections during the visit process. This enhancement allows for testing real-time data or interactions that may occur on the page being visited.By establishing the WebSocket routing before executing the original visit logic, the changes potentially improve the reliability and responsiveness of tests that depend on WebSocket communication.
The original
visitfunction is still wrapped with a timeout of 60000 milliseconds, which is the defaultpageLoadTimeoutfor Cypress visits, ensuring that the visit process does not exceed the specified timeout.Overall, these changes provide a solid foundation for handling WebSocket connections in Cypress tests and align with the PR objectives of implementing WebSocket messages across various entities within the application.
src/js/entities-service/entities/flows.js (2)
18-25: LGTM!The new message handlers
OwnerChangedandStateChangedare implemented correctly. They update the model attributes based on the received data and merge additional attributes using the spread operator. The use of object destructuring and default values forattributesis a good practice.
37-38: LGTM!The modification to the
getOwnermethod simplifies the retrieval of owner entities by directly instantiating a new owner object using theStore.getmethod. This change streamlines the retrieval logic and potentially improves performance.src/js/entities-service/entities/actions.js (2)
23-30: LGTM!The new message handlers
OwnerChangedandStateChangedprovide a clean and structured way for the_Modelclass to update its internal state based on changes to the owner and state. The handlers use object destructuring to extract the relevant data from the received message payload and update the model attributes using thesetmethod, which triggers change events. The handlers also merge additional attributes provided in the message payload, ensuring that the model stays in sync with the latest owner and state information.The addition of these message handlers enhances the functionality of the
_Modelclass by enabling it to respond to specific events related to ownership and state changes.
58-59: Excellent improvement!The modification to the
getOwnermethod enhances the encapsulation of the owner retrieval process by directly creating an instance of the owner model based on its type and ID. It eliminates the need for a radio request to retrieve the owner model, reducing the coupling between the_Modelclass and the radio system.This change aligns with the provided past review comment, which suggests using
Store.get(owner.type)to ensure the correct model and request. The comment has been addressed effectively.src/js/base/model.js (2)
73-81: Implementation of OverriddensetMethod is CorrectThe overridden
setmethod properly calls the original Backbonesetmethod, checks for validation, updates the__cached_tstimestamp, and returns the model instance. This ensures that the model's data freshness is accurately tracked.
85-89: Message Handler Retrieval Logic is Well-ImplementedThe
_getMessageHandlermethod effectively retrieves the appropriate message handler based on thecategory. It accounts for handlers defined as functions or as method names on the model instance, enhancing flexibility in message processing.src/js/apps/patients/patient/flow/flow_app.js (7)
19-21: DefiningINCREMENTandDECREMENTconstants improves code clarityDefining
INCREMENTandDECREMENTconstants enhances readability and maintainability by avoiding magic numbers in the code.
77-80: Added'message'event listener to flowAdding the
'message'event listener allows the application to handle incoming messages related to the flow, enabling dynamic updates.
92-99: Correctly adding new action and updating flow progressThe
_addActionmethod properly adds the action to the collections and updates the flow's progress.
109-109: Updating flow progress upon action state changeThe call to
this._setFlowProgressaccurately updates the flow's progress based on the action's completion state.
114-114: Adjusting flow progress when an action is destroyedThe flow's progress counters are correctly decremented when an action is destroyed.
258-258: Adding action and updating flow progress after savingCalling
this._addAction(action);after saving the action ensures the new action is added and the flow progress is updated.
84-91: Undefined variablescompleteandtotalin_setFlowProgressThe variables
completeandtotalare used without prior definition. Ensure they are retrieved fromthis.flow.get('_progress')before using them.Apply this diff to fix the issue:
+ const { complete, total } = this.flow.get('_progress'); this.flow.set({ _progress: { complete: complete + completeChange, total: total + totalChange, } });Likely invalid or redundant comment.
| handleMessage({ category, timestamp, payload }) { | ||
| // Ignores messages that may be from recent local events | ||
| if (dayjs(this.get('__cached_ts')).add(10, 'seconds').isAfter(timestamp)) return; | ||
|
|
||
| const handler = this._getMessageHandler(category); | ||
| if (handler) handler.call(this, payload); | ||
|
|
||
| this.trigger('message', { category, payload }); | ||
| }, |
There was a problem hiding this comment.
Review Timestamp Comparison Logic in handleMessage
The timestamp comparison in handleMessage may be affected by differences between client and server clocks. Since __cached_ts is set using the client's current time, and timestamp likely originates from the server or other clients, discrepancies due to clock drift or time zone differences could lead to messages being incorrectly ignored.
Consider the following suggestions:
- Ensure Consistent Timezones: Confirm that both
__cached_tsandtimestampare in UTC and use the same format to minimize discrepancies. - Use Server Timestamps: Rely on server-generated timestamps for both caching and message handling to maintain consistency across clients.
- Implement Tolerance Window: Introduce a reasonable time window or threshold to account for minor clock differences between systems.
There was a problem hiding this comment.
The dates should be fine, and the tolerance window is add(10, 'seconds')
There was a problem hiding this comment.
@paulfalgout, understood. Thank you for the clarification regarding the dates and the 10-second tolerance window.
(\_/)
(•_•)
/>📝
Learnings added
Learnt from: paulfalgout
PR: RoundingWell/care-ops-frontend#1314
File: src/js/base/model.js:90-98
Timestamp: 2024-09-19T18:33:59.456Z
Learning: In the `handleMessage` method, there is already a 10-second tolerance window applied using `add(10, 'seconds')` to account for clock discrepancies.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| onFlowMessage({ category, payload }) { | ||
| if (category !== 'ActionCreated') return; | ||
| const { action } = payload; | ||
|
|
||
| if (this.actions.get(action.id)) return; | ||
|
|
||
| const fetchAction = Radio.request('entities', 'fetch:actions:model', action.id); | ||
| fetchAction.then(this._addAction); |
There was a problem hiding this comment.
Add error handling for fetchAction in onFlowMessage
The promise returned by fetchAction may fail if the action cannot be fetched. Consider adding error handling to manage rejections gracefully.
Apply this diff to handle errors:
const fetchAction = Radio.request('entities', 'fetch:actions:model', action.id);
- fetchAction.then(this._addAction);
+ fetchAction.then(this._addAction).catch(error => {
+ // Handle the error appropriately
+ console.error('Failed to fetch action:', error);
+ });Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
not sure an error here would be very useful
There was a problem hiding this comment.
@paulfalgout, understood. Thanks for clarifying.
(._.)
Learnings added
Learnt from: paulfalgout
PR: RoundingWell/care-ops-frontend#1314
File: src/js/apps/patients/patient/flow/flow_app.js:116-123
Timestamp: 2024-09-19T18:34:21.887Z
Learning: In the `onFlowMessage` function in `flow_app.js`, adding error handling for `fetchAction` is not necessary even if the promise may fail.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
paulfalgout
changed the title
paulfalgout
force-pushed
the
websocket
branch
from
43ecb16
to
d60827d
Compare
paulfalgout
changed the title
This is a simple example of how we can implement websocket messages across entities.. subscriptions would be managed per app context, and then can optionally be handled per-entity, but then the messages are also globally published for app-specific handling. But the main purpose is to updated invalidated FE cache contextually.
We could at some point extend updating the cache to any cached entity such that, if the user has loaded a model, it will be updated behind the scenes such that it would not need to be refetched on navigation.. we could potentially purge neglected cached models after a period of time as well... or keep a stack of possible subscriptions where as things that fall off are cleared.
But for now this seems reasonable.
This is a WIP as we negotiate message structure.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests