Add worklist subscription and filters #1380
Conversation
WalkthroughThe changes enhance real-time update capabilities and message handling across several modules. In the worklist application and WebSocket service, new subscription methods are introduced to process messages with added filters, invoking dedicated data fetching methods. The Backbone model now tracks fetching state and queues messages appropriately, while the entity action fetch method is updated to request specific attributes. Integration tests are added to verify proper UI updates after socket notifications. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Worklist App
participant WS as WebSocket Service
participant Server as Socket Server
App->>WS: subscribe(resources, {shouldPersist, filters})
WS->>Server: Send subscribe request with filters and clientKey
Server-->>WS: Acknowledge subscription
WS-->>App: Receive message notification
alt Message category is "flow"
App->>App: _fetchFlow(model)
else Message category is "action"
App->>App: _fetchAction(model)
end
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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 (
|
RoundingWell Care Ops Frontend
|
||||||||||||||||||||||||||||
| Project |
RoundingWell Care Ops Frontend
|
| Branch Review |
socket-filters
|
| Run status |
|
| Run duration | 02m 49s |
| Commit |
|
| Committer | Nick Major |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
316
|
| View all changes introduced in this branch ↗︎ | |
Pull Request Test Coverage Report for Build 7e251c34-b1ca-4403-aab9-e03eb8c5d664Details
💛 - Coveralls |
There’s probably a better way to do this, but might need more use cases first
Apparently Bref requires something be sent at point when there’s nothing to send.. so we’ll just exit out here. But I can’t test it because it doesn’t seem possible to send nothing in the node websocket setup.
flow_states aren’t able to be filtered on the BE so we’re doing an extra filter here
paulfalgout
force-pushed
the
socket-filters
branch
from
11b8447 to
9218a77
Compare
Set to `true` when model is being fetched In worklist app, we can use that to not do duplicate `model.fetch()` requests after ws notifications are received
| this.listenTo(channel, 'message', ({ category, payload }) => { | ||
| if (category !== 'ResourceCreated') return; | ||
|
|
||
| const model = Radio.request('entities', 'get:store', payload.resource); |
There was a problem hiding this comment.
I wanted to do this ☝️ in the onMessage() of the /js/services/ws.js file.
Specifically I wanted to do something like:
if (data.category === 'ResourceCreated') {
model = Radio.request('entities', 'get:store', data.payload.resource);
channel.trigger('message', data, model);
return;
}But that would break our handling of the ResourceCreated ws notification for actions on the patient flow page.
Both of these ResourceCreated's have both a data.resource and data.payload.resource. So I'm not sure how to differentiate between the two in that onMessage() function.
nmajor25
force-pushed
the
socket-filters
branch
from
de639b3 to
913f992
Compare
…ns on worklists Instead of `data.resource`, which contains the patient data
nmajor25
force-pushed
the
socket-filters
branch
from
913f992 to
160eb5d
Compare
| @@ -161,14 +161,21 @@ export default App.extend({ | |||
|
|
|||
| channel.request('subscribe', this.collection.models, { filters: { [filterKey]: this.filters } }); | |||
|
|
|||
| this.listenTo(channel, 'message', (message, model) => { | |||
| this.listenTo(channel, 'message', ({ category, resource, payload }) => { | |||
| if (!category) return; | |||
There was a problem hiding this comment.
In my local dev environment I'm getting a ws notification that contains a lambda error. When that happens this listenTo is called with category, resource, and payload as undefined.
I don't know if this is something that could happen in production or not.
There was a problem hiding this comment.
we shouldn't need the if here. should be able to handle those errors prior to triggering the message
There was a problem hiding this comment.
I'm looking at that currently. Would be in the next commit. I have a event.data.category check added to the onMessage() here.
So at the top of that function it would be:
if (!event.data || !event.data.category) return;But still need to test it locally and think about it a little more.
src/js/base/model.js
Outdated
| if (!this.notifications) { | ||
| this.notifications = [data]; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Is there a way to set model.notifications: [] initially for each model? Do we want to do that?
If we did that we wouldn't have to check if this.notifications exists here. And just do the push().
There was a problem hiding this comment.
And wasn't sure what to use for naming. But went with model.notifications for now.
There was a problem hiding this comment.
there is not a good place to instantiate this, but it should probably be _notifications and it should be {}. where this.notifications['StateChanged'] would always be the latest StateChanged
There was a problem hiding this comment.
_notifications naming makes sense.
But why an object {} instead of an array [{}]?
There was a problem hiding this comment.
because we only need to run the most recent of a particular category
There was a problem hiding this comment.
Ah okay, right. Using an object we can just replace the previous notification in the queue if one already exists with the same notification category (StateChanged, OwnerChanged, etc.).
| @@ -99,6 +103,9 @@ export default App.extend({ | |||
| }, | |||
|
|
|||
| onMessage(event) { | |||
| /* istanbul ignore next: Can't test this bref functionality in node websockets */ | |||
| if (!event.data) return; | |||
|
|
|||
There was a problem hiding this comment.
I think we should handle event.data.error near the pong return.. such that we trigger error instead of message and not have a if (!category) check in the message handler.
src/js/base/model.js
Outdated
| this.notifications.push(data); | ||
| }, | ||
| handleMessage(data) { | ||
| if (this.isFetching()) { |
There was a problem hiding this comment.
I think extending the attributes with the updated_at should happen here so that it's stored with the queue.. that timestamp is when the action occurred, not when it was processed.
src/js/base/model.js
Outdated
| if (!this.notifications) { | ||
| this.notifications = [data]; | ||
| return; | ||
| } |
There was a problem hiding this comment.
there is not a good place to instantiate this, but it should probably be _notifications and it should be {}. where this.notifications['StateChanged'] would always be the latest StateChanged
| this.collection.add(model); | ||
| channel.request('add', model); | ||
|
|
||
| if (model.notifications) { |
There was a problem hiding this comment.
we should either entirely handle this inside the base model, or entirely handle it here.. perhaps the base model is better.. I can't think of why we wouldn't always want this to happen
There was a problem hiding this comment.
I think it would work to do this in the base model.
But does this.collection.add(model) need to happen before we handle the queued notifications? Or does that order not matter?
For example if the order of things happened like this, would the correct flow.state show in the UI?
- A
StateChangednotification is queued while a new flow is fetched. - Flow fetch finishes.
- Queued
StateChangedws notification is handled and state is updated on the flow model. - Flow model is added to the worklist collection via
this.collection.add(model), which makes it show in the UI.
Does the order of 3 & 4 matter? My assumptions is that it doesn't matter. Either way the flow's state should show correctly in the UI.
There was a problem hiding this comment.
better to do data before render
nmajor25
force-pushed
the
socket-filters
branch
from
aba9957 to
ab272e3
Compare
src/js/base/model.js
Outdated
| // Resolve with entity if successful | ||
| return fetcher.then(response => { | ||
| this._isFetching = false; | ||
|
|
||
| if (this._notifications) { |
There was a problem hiding this comment.
no need for the if.. I think each(this._notifications, this._handleMessage); is fine here.
| @@ -161,14 +161,21 @@ export default App.extend({ | |||
|
|
|||
| channel.request('subscribe', this.collection.models, { filters: { [filterKey]: this.filters } }); | |||
|
|
|||
| this.listenTo(channel, 'message', (message, model) => { | |||
| this.listenTo(channel, 'message', ({ category, resource, payload }) => { | |||
| if (!category) return; | |||
There was a problem hiding this comment.
we shouldn't need the if here. should be able to handle those errors prior to triggering the message
nmajor25
force-pushed
the
socket-filters
branch
from
ab272e3 to
19830a8
Compare
| @@ -98,12 +109,30 @@ export default Backbone.Model.extend(extend({ | |||
|
|
|||
| return isFunction(messages[category]) ? messages[category] : this[messages[category]]; | |||
| }, | |||
| handleMessage({ category, resource, author, payload }) { | |||
| payload.attributes = extend({}, payload.attributes, { updated_at: dayjs.utc().format() }); | |||
| queueMessage(data) { | |||
There was a problem hiding this comment.
So this function looks complex to me, but I get why.. but we could start using some of the newer operators to make this a bit simpler:
queueMessage(data) {
return (this._notifications ||= {})[data.category] = data;
}There was a problem hiding this comment.
That's pretty cool, didn't know ||= existed until now 👍
But vite-plugin-eslint doesn't support it. It's using eslint v8.21.0 (link). Eslint added support logical assignment operators in v8.24.0 (source).
That package's repo seems to be abandoned, so we probably want to switch to something different.
There was a problem hiding this comment.
That's just the dev dependency. We're using 9 and it's a peer dependency https://github.com/gxmari007/vite-plugin-eslint/blob/main/package.json#L60
There was a problem hiding this comment.
FWIW it looks like the vite community doesn't run eslint as part of the build process. They're using it in pre-commit pre-build hooks.. I find that mildly annoying
There was a problem hiding this comment.
FWIW backend runs static analysis (including lint) on commit and on test (CI).
|
|
||
| if (this.collection.get(model) || model.isFetching()) return; | ||
|
|
||
| model.fetch().then(() => { |
There was a problem hiding this comment.
I think this'll be:
const include = [
'program-action.program',
'flow.program-flow.program',
].join()
const data = {
fields: { flows: ['name', 'state'] },
include,
};
model.fetch({ data }).then(...
Honestly that's probably enough logic to move it to a Radio.request('entities', 'fetch:action..
Or maybe just add the fields to 'fetch:actions:model' and use that over model.fetch here.
There was a problem hiding this comment.
Great thanks, I just got this working in my local dev environment.
For now I'm just doing the following:
// js/apps/patients/worklist/worklist_app.js
_getWsFetchOptions(filterKey) {
if (filterKey === 'flows') return {};
return {
data: {
fields: { flows: ['name', 'state'] },
include: ['program-action.program', 'flow.program-flow.program'].join(),
},
};
},
subscribe() {
. . .
const options = this._getWsFetchOptions(filterKey);
model.fetch(options).then(() => {
. . .
})
},There was a problem hiding this comment.
Having this working now allows me to finish creating the initial tests for the action version of the worklist. After that I'll come back and do some refactoring on this and other parts too.
| @@ -151,6 +154,29 @@ export default App.extend({ | |||
|
|
|||
| this.showList(); | |||
| }, | |||
| subscribe() { | |||
There was a problem hiding this comment.
Alright I think this whole function needs refactoring..
we should probably just split based on filterKey and hardcode types for this one..
also somewhere we can call break this up into another function
subscribe() {
this.listenTo(... () => {
fetch().then(() => {. // <- one or both of these could be a separate handler..
But I think we still want to check the store for the resource.. but if we need to fetch it, as mentioned below.. switch to either the action or flow entity request rather than adding the fetch specifications here for include/fields.
There was a problem hiding this comment.
I took a crack at refactoring this in 836c7c0, let me know what you think.
…on for that model. And then handle those ws notifications once the fetch finishes
nmajor25
force-pushed
the
socket-filters
branch
from
a7e5d6a to
03f5c69
Compare
So the app doesnt handle ws notifications that are errors, only actual entity events
There was a problem hiding this comment.
Summary of what I'm testing here.
Flow list notifications:
- Load owned-by worklist with one flow
- Test each ws notification category that applies on preloaded flow
- NameChanged
- Verify updated_at date
- OwnerChanged
- StateChanged
- NameChanged
- Add flow to list via
ResourceCreatedws notification (w/ trailingStateChangedmsg)- Verify included is not in request URL string
- Verify flow count
- Verify flow was added to list
- Verify trailing
StateChangedwas queued and processed after model fetch finishes - Verify new flow is subscribed to future ws notifications
- Add new flow to list via
StateChangedws notification - Add new flow to list via
OwnerChangedws notification
- Test each ws notification category that applies on preloaded flow
Action list notifications:
- Load owned-by worklist with one action
- Test each ws notification category that applies on preloaded action
- NameChanged
- Verify updated_at date
- OwnerChanged
- ActionDueChanged
- DetailsChanged
- StateChanged
- CommentAdded
- AttachmentAdded
- SharingUpdated
- NameChanged
- Add action to list via
ResourceCreatedws notification (w/ trailingStateChangedmsg)- Verify included data is in request URL string
- Verify action count
- Verify action was added to list
- Verify trailing
StateChangedwas queued and processed after model fetch finishes - Verify new action is subscribed to future ws notifications
- Add new action to list via
StateChangedws notification - Add new action to list via
OwnerChangedws notification - Handle ws notification for new action with a flow state not in the current filters
- In this scenario, the action should not be added to list
- Test each ws notification category that applies on preloaded action
| id: 'owned-by', | ||
| listType: 'flows', | ||
| flowsSortId: 'sortCreatedDesc', | ||
| clinicianId: '11111', |
There was a problem hiding this comment.
Don't love using new string ids.. is this getCurrentClinician().id?
There was a problem hiding this comment.
Yep, that's the current clinician (i.e. getCurrentClinician()). I updated the tests to reference that directly instead of using 11111.
| id: testPatient1.id, | ||
| }, | ||
| payload: { | ||
| resource: { |
There was a problem hiding this comment.
I do badly want destructing within an object like
resource: { id, type } = testPatient1, but alas none of these types of proposals go anywhere right now in TC39 😿
we could make some sort of generic helper. function foo({ id, type ]) => { id, type };
resource: foo(testNewSocketFlow)
thought for another day
There was a problem hiding this comment.
Ya that would be pretty nice 👍
| relationships: { | ||
| state: getRelationship(stateTodo), | ||
| flow: getRelationship(testFlow), | ||
| files: getRelationship([]), |
There was a problem hiding this comment.
it's ok to do so, but I suspect we don't need to set a lot of this for the tests? details? due_time? duration. files relationship? If they're just random values from getAction does that potentially cause flake? Ideally were setting whatever minimum test data we need and letting getAction do the rest
There was a problem hiding this comment.
Agreed, I cleaned up a number of these: https://github.com/RoundingWell/care-ops-frontend/compare/003a41f45cc3b2abdda8b3aa33189122edb965a0..54708cca95f3c34880bfa2975ca936e7e887260c
| .routeFormByAction() | ||
| .routeFormDefinition() | ||
| .routeLatestFormResponse() | ||
| .visitOnClock('/worklist/owned-by', { now: testTs() }) |
There was a problem hiding this comment.
the visitOnClock is to set the time to now or why is this needed?
There was a problem hiding this comment.
It's to avoid flakiness when checking that the updated_at timestamp is updated correctly in the UI.
First introduced in this commit: 8eb7d81.
nmajor25
force-pushed
the
socket-filters
branch
from
003a41f to
54708cc
Compare
nmajor25
force-pushed
the
socket-filters
branch
from
54708cc to
6620319
Compare
|
|
||
| this.collection.add(model); | ||
|
|
||
| Radio.channel('ws').request('add', model); |
There was a problem hiding this comment.
Radio.request('ws', 'add',... here and above
| return; | ||
| } | ||
|
|
||
| if (filterKey === 'actions') { |
There was a problem hiding this comment.
I think you'll need to remove the if here and have a default, otherwise we'll have an uncovered else 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
🧹 Nitpick comments (7)
test/integration/patients/worklist/worklist.js (2)
306-587: Consider splitting this large test into smaller, focused specs.This end-to-end scenario is comprehensive; however, multiple smaller test cases can improve readability and maintainability, making it easier to diagnose failures.
1134-1605: Promote reuse of repeated WebSocket steps or test data.The extensive real-time action notification test could benefit from extracting repeated patterns (e.g., flow creation, queued notifications, etc.) into shared helper functions or custom commands.
src/js/services/ws.js (2)
40-47: Exercise caution when sending large resource payloads.Building the
dataobject with fullthis.resourcesand optionalfiltersis flexible. However, if resources or filters grow significantly, consider strategies to prevent large payload overhead (e.g., pagination or chunked updates).
132-135: Offer help implementing a more robust filter management approach.A single
this.filtersmight be insufficient for multiple resource subscriptions. Happy to propose or implement a design that stores each subscription’s filters more granularly.src/js/apps/patients/worklist/worklist_app.js (2)
181-188: Add error handling for_fetchFlow.
At present, the.then()callback does not handle rejections. Consider adding a.catch()block or similar mechanism to surface errors if the fetch fails (e.g., network error or invalid data). This ensures that the app can gracefully handle fetch failures and provide feedback._fetchFlow(model) { const fetchFlow = Radio.request('entities', 'fetch:flows:model', model.id); + fetchFlow + .then(() => { this.collection.add(model); Radio.channel('ws').request('add', model); + }) + .catch((error) => { + // handle or log error, e.g. show an alert or retry + }); }
189-201: CheckgetFlow()safely and handle fetch errors.
Ifmodel.getFlow()isundefinedor missing a valid state,model.getFlow().getState()will throw an error. Also, add a.catch()block to address potential fetch failures._fetchAction(model) { const flowStates = this.getState('flowStates'); const fetchAction = Radio.request('entities', 'fetch:actions:model', model.id); - fetchAction.then(() => { - if (!flowStates.includes(model.getFlow().getState().id)) return; + fetchAction + .then(() => { + if (!model.getFlow() || !model.getFlow().getState()) return; + if (!flowStates.includes(model.getFlow().getState().id)) return; this.collection.add(model); Radio.channel('ws').request('add', model); + }) + .catch((error) => { + // handle or log error appropriately + }); }src/js/base/model.js (1)
110-119: Consider clearing processed notifications or trimming outdated ones.
New messages overwrite notifications of the same category withqueueMessage({ category, ... }). After processing them in_handleMessage, consider cleaning up_notificationsto prevent potential buildup of stale notifications, especially if many categories accumulate.Also applies to: 126-135
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/js/apps/patients/worklist/worklist_app.js(4 hunks)src/js/base/model.js(3 hunks)src/js/entities-service/actions.js(1 hunks)src/js/services/ws.js(3 hunks)test/integration/patients/worklist/worklist.js(3 hunks)
🔇 Additional comments (7)
src/js/entities-service/actions.js (1)
20-25: Looks good. Consider verifying backend support for partial field retrieval.The addition of
fields.flowsaligns with JSON:API conventions for sparse fieldsets. Ensure that the backend expectsfields[flows]=name,stateto avoid unexpected behavior.test/integration/patients/worklist/worklist.js (1)
3-3: Good use of NIL UUID constants.Importing
NIL as NIL_UUIDhelps avoid magic strings for “no-owner” or “no-clinician” use cases.src/js/services/ws.js (2)
106-108: Early return for empty messages looks fine.Skipping processing for empty
event.datais a safe fallback. Ensure the server never sends essential data in an empty payload.
115-115: Verify that skipping messages without a category is intentional.This condition silently ignores all non-pong messages lacking a
category. Confirm no other message types are impacted.src/js/apps/patients/worklist/worklist_app.js (3)
129-132: Good approach for managing filters.
Storing the filter in a local property (this.filters) ensures that the filter is consistently referenced throughout the app, preventing repetitive calls tothis.getState().getEntityFilter().
157-179: Validatemodelavailability before fetching.
There's a possibility thatRadio.request('entities', 'get:store', modelResource)might returnundefinedif the resource is not in the store. Consider verifying thatmodelexists before proceeding, to avoid potential errors when callingmodel.isFetching().
261-261: No significant change detected.
src/js/base/model.js
Outdated
| this._isFetching = true; | ||
|
|
||
| // Resolve with entity if successful | ||
| return fetcher.then(response => { | ||
| this._isFetching = false; | ||
|
|
There was a problem hiding this comment.
Ensure _isFetching is reset on fetch errors.
Currently, _isFetching is only set to false on success. If the request fails or is aborted, _isFetching might remain true indefinitely, preventing message handling. Consider adding a .catch() to handle failures and reset _isFetching.
fetch(options) {
const fetcher = Backbone.Model.prototype.fetch.call(this, extend({ abort: true }, options));
this._isFetching = true;
- return fetcher.then(response => {
+ return fetcher
+ .then(response => {
this._isFetching = false;
each(this._notifications, this._handleMessage, this);
if (!response || response.ok) return this;
return response;
+ })
+ .catch(error => {
+ this._isFetching = false;
+ // Optionally, handle or log the error
+ throw error;
+ });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this._isFetching = true; | |
| // Resolve with entity if successful | |
| return fetcher.then(response => { | |
| this._isFetching = false; | |
| fetch(options) { | |
| const fetcher = Backbone.Model.prototype.fetch.call(this, extend({ abort: true }, options)); | |
| this._isFetching = true; | |
| return fetcher | |
| .then(response => { | |
| this._isFetching = false; | |
| each(this._notifications, this._handleMessage, this); | |
| if (!response || response.ok) return this; | |
| return response; | |
| }) | |
| .catch(error => { | |
| this._isFetching = false; | |
| // Optionally, handle or log the error | |
| throw error; | |
| }); | |
| } |
There was a problem hiding this comment.
Correct me if I'm wrong here @paulfalgout, but I think we'd want this._isFetching to remain true in the scenario of a fetch error. Since we wouldn't want the model's queued ws notifications to be processed. So I think the code we current have is good.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
hmm no I think we should add this catch with the rethrow of the error. It is more correct for the state of the base model, regardless of how it affects us downstream. if a fetch fails though, we're not necessarily doing anyhing in order to process the queue correctly anyhow.
There was a problem hiding this comment.
That's been added in commit 0222306.
Nice catch @coderabbitai 👍
There was a problem hiding this comment.
@nmajor25 Thank you for implementing the fix! Adding the .catch() block ensures that _isFetching will be properly reset even when fetch operations fail, which makes the code more robust and prevents potential issues with message handling. It's great to see that the team agreed on the approach and incorporated the suggestion so quickly.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/js/apps/patients/worklist/worklist_app.js (2)
169-178: Unnecessary return statements can be removed.There are explicit
returnstatements after calling_fetchFlowand_fetchActionthat aren't necessary since they're the last statements in their respective code blocks. Consider removing them for cleaner code.- this._fetchFlow(model); - - return; + this._fetchFlow(model); + return;
190-190: Consider caching flowStates for performance.The call to
this.getState('flowStates')might be more efficiently handled by caching this value at the class level, similar to how you're handlingthis.filters, especially if it's accessed frequently.subscribe() { const channel = Radio.channel('ws'); const filterKey = this.getState().getType(); + this.flowStates = this.getState('flowStates'); channel.request('subscribe', this.collection.models, { filters: { [filterKey]: this.filters } }); // ... rest of the method }, _fetchAction(model) { - const flowStates = this.getState('flowStates'); + const flowStates = this.flowStates; // ... rest of the method },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/js/apps/patients/worklist/worklist_app.js(4 hunks)src/js/entities-service/actions.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/js/entities-service/actions.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test-build
🔇 Additional comments (8)
src/js/apps/patients/worklist/worklist_app.js (8)
128-133: Good centralization of filter state.Storing
this.filtersas a class property improves code organization and avoids redundant getter calls, making the code more maintainable.
147-148: Subscription initialization looks good.Adding the WebSocket subscription during the app start process ensures real-time updates are properly configured.
161-161: Approve filter-based WebSocket subscription.Using filters when subscribing to WebSocket updates is a good approach to ensure you only receive relevant notifications, reducing unnecessary processing.
163-168: Good optimization to avoid duplicate processing.The check for
this.collection.get(model) || model.isFetching()prevents redundant processing of models that are already in the collection or currently being fetched, which is an important optimization.
180-188: Good implementation of flow fetching.The
_fetchFlowmethod effectively fetches the flow model and adds it to the collection upon successful retrieval. The call toRadio.request('ws', 'add', model)ensures that the WebSocket service is aware of the new model.
189-201: Flow state validation in action fetching is handled correctly.The
_fetchActionmethod properly validates that the action's flow state is included in the allowed flow states before adding it to the collection. This ensures that only relevant actions are displayed in the worklist.
157-201:❓ Verification inconclusive
Consider adding tests for WebSocket subscription.
The PR objectives mention that tests will be added using Cypress. Make sure to include tests that verify:
- WebSocket subscription with filters works correctly
- New models are properly fetched and added to the collection
- Flow state validation prevents irrelevant actions from being added
This is important since real-time updates are a critical feature that needs reliable behavior.
🏁 Script executed:
#!/bin/bash # Check for existing WebSocket tests echo "Looking for existing WebSocket tests..." fd -e js -e jsx test.*ws js/testLength of output: 220
Action: Add Cypress tests for WebSocket subscriptions
The current PR does not appear to include any tests verifying the WebSocket subscription behavior. Please add Cypress tests that ensure:
- The WebSocket subscription applies filters correctly.
- New models are fetched and added to the collection as expected.
- Flow state validation prevents irrelevant models/actions from being processed.
Note: The initial search under “js/test” using
fddid not yield any results, so please verify if the tests have been placed in a different directory (e.g., “cypress/integration”).
186-186:✅ Verification successful
Verify WebSocket 'add' method implementation.
The code calls
Radio.request('ws', 'add', model)after adding models to the collection, but there was a previous review comment suggesting to use'add'instead of another term. Let's verify this is the correct method name in the WebSocket service.Also applies to: 199-199
🏁 Script executed:
#!/bin/bash # Check for the 'add' method in the WebSocket service echo "Checking for 'add' method in WebSocket service..." rg -A 3 -B 3 "add.*: function\(" src/js/services/ws.jsLength of output: 171
I'll run an additional script to search for an ES6-style method definition of the
addmethod in the WebSocket service. The previous search looked specifically for the patternadd: function(, which might not catch shorthand method definitions. Let's try searching for any occurrence ofadd(in the file:
🏁 Script executed:
#!/bin/bash echo "Searching for ES6-style definition of 'add' in WebSocket service (src/js/services/ws.js)..." rg -n "add\s*\(" src/js/services/ws.jsLength of output: 378
Confirmed: The WebSocket service correctly implements the
addmethod.
- The ES6-style
add(resources, { shouldPersist } = {})method is present insrc/js/services/ws.js(around line 151).- The usage of
Radio.request('ws', 'add', model)insrc/js/apps/patients/worklist/worklist_app.js(lines 186 and 199) correctly calls this method.No further changes are necessary.
| subscribe() { | ||
| const channel = Radio.channel('ws'); | ||
| const filterKey = this.getState().getType(); | ||
|
|
||
| channel.request('subscribe', this.collection.models, { filters: { [filterKey]: this.filters } }); | ||
|
|
||
| this.listenTo(channel, 'message', ({ category, resource, payload }) => { | ||
| const modelResource = category === 'ResourceCreated' ? payload.resource : resource; | ||
| const model = Radio.request('entities', 'get:store', modelResource); | ||
|
|
||
| if (this.collection.get(model) || model.isFetching()) return; | ||
|
|
||
| if (filterKey === 'flows') { | ||
| this._fetchFlow(model); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| this._fetchAction(model); | ||
|
|
||
| return; | ||
| }); | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider adding error handling for undefined message properties.
The message handler doesn't check if category, resource, or payload are undefined, which could cause runtime errors. Based on previous review comments, there have been cases where lambda errors resulted in undefined properties in WebSocket notifications.
🏁 Script executed:
#!/bin/bash
# Check for any existing error handling in the WebSocket service
# Look for error handling in the WebSocket service
echo "Checking for error handling in WebSocket service..."
rg -A 5 -B 5 "onMessage" src/js/services/ws.jsLength of output: 917
Action Required: Add Defensive Checks for Message Properties in the Event Handler
-
In src/js/apps/patients/worklist/worklist_app.js (lines 157‑179), the event listener for the
'message'event destructures{ category, resource, payload }directly without verifying their existence. -
Although the WebSocket service (in src/js/services/ws.js) checks for the presence of
event.datain itsonMessagehandler, this alone does not guarantee that the message object passed to the worklist handler will include validcategory,resource, orpayloadproperties. -
Consider adding guard clauses at the start of the listener callback to ensure that these properties are defined before proceeding. For example:
this.listenTo(channel, 'message', ({ category, resource, payload }) => { if ( typeof category === 'undefined' || typeof payload === 'undefined' || (category === 'ResourceCreated' && typeof payload.resource === 'undefined') || (category !== 'ResourceCreated' && typeof resource === 'undefined') ) { console.warn('Received message with missing properties', { category, resource, payload }); return; } const modelResource = category === 'ResourceCreated' ? payload.resource : resource; const model = Radio.request('entities', 'get:store', modelResource); if (this.collection.get(model) || model.isFetching()) return; if (filterKey === 'flows') { this._fetchFlow(model); return; } this._fetchAction(model); });
-
This update will help avoid runtime errors when WebSocket messages fail to include the expected properties.
There was a problem hiding this comment.
In src/js/apps/patients/worklist/worklist_app.js we now check that the data contains a category. And we don't handle that web socket notification if it doesn't have that.
If there's no category, that's a good indication that the data isn't going to be a valid web socket notification.
There was a problem hiding this comment.
🧩 Analysis chain
Looking at the WebSocket service implementation to see the changes related to category checking.
@nmajor25 Thank you for explaining the check that was implemented. You're right - checking for the presence of category in the WebSocket notification is an effective way to validate that the data is a proper notification worth processing.
I see that this check was implemented in the WebSocket service's onMessage method, which provides a centralized validation before any message is dispatched to subscribers. This approach is more efficient than adding defensive checks in each individual subscriber since it prevents invalid messages from propagating in the first place.
By filtering out notifications without a category at the service level, you've effectively addressed the concern I raised about potential runtime errors from undefined properties. This is a clean solution that maintains the simplicity of your subscriber code while still ensuring robustness against malformed messages.
🏁 Script executed:
#!/bin/bash
# Check the updated onMessage method in the WebSocket service
rg -A 10 "onMessage" src/js/services/ws.js
Length of output: 702
@nmajor25 You're absolutely right. I see from the WebSocket service code that there's already a centralized validation check in place:
onMessage(event) {
/* istanbul ignore next: Can't test this bref functionality in node websockets */
if (!event.data) return;
let model;
const channel = this.getChannel();
const data = JSON.parse(event.data);
if (!data.category || data.name === 'pong') return;This is an excellent approach - validating the message data at the service level with the check for !data.category || data.name === 'pong' prevents invalid messages from ever reaching subscribers. Since this validation happens centrally, there's no need for redundant checks in each subscriber like the worklist app.
This is a clean architectural choice that:
- Prevents propagation of invalid messages
- Keeps subscriber code simpler and more focused
- Centralizes validation logic in one place
Thanks for clarifying how the validation is implemented in your system!
|
@paulfalgout coveralls coverage looks good 👍 |
When that happens, set `_isFetching = false` and throw the error
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/js/base/model.js (1)
116-125: Consider simplifying the notification queue initialization.The code correctly implements notification queueing with the discussed design of using object properties keyed by category. However, the initialization logic could be simplified.
queueMessage(data) { const category = data.category; - if (!this._notifications) { - this._notifications = { [category]: data }; - return; - } - - this._notifications[category] = data; + this._notifications = this._notifications || {}; + this._notifications[category] = data; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/base/model.js(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test-build
🔇 Additional comments (6)
src/js/base/model.js (6)
1-1: Clean import addition for required utility.The addition of
eachfrom underscore is correctly added to support the new notification processing functionality.
39-40: LGTM: Well-designed state tracking introduction.The addition of
_isFetchingprivate property creates a clear tracking mechanism for the model's fetch state.
42-56: Robust fetch handling with proper state management.This implementation correctly handles both success and error scenarios, ensuring
_isFetchingis reset properly in both cases. The error handling is particularly important to avoid leaving models in an incorrect state if a fetch fails.The notification processing after a fetch completes is a clean solution to the message queuing problem.
58-60: Simple and effective public accessor method.The
isFetching()method provides a clean public API to check the model's fetching state without directly accessing the private_isFetchingproperty.
126-131: Well-designed message handling logic.The private
_handleMessagemethod is well-structured and properly delegates to specific handlers while also triggering a general message event.
132-141:✅ Verification successful
Ensure payload mutation is intentional.
The
handleMessagemethod now extends the payload with a timestamp and implements the fetch-aware message queueing logic correctly. However, note that this directly mutates the incoming payload object.
🏁 Script executed:
#!/bin/bash # Search for other places in the codebase that might be affected by the payload mutation # Check for direct references to payload.attributes that might expect the original structure echo "Searching for direct references to payload.attributes..." rg -A 3 -B 3 "payload\.attributes" --type jsLength of output: 641
Payload Mutation Verified: No Unintended Side Effects
The change in
src/js/base/model.jswherepayload.attributesis extended with a timestamp (usingdayjs.utc().format()) has been verified. The search results confirm that the mutation is localized to this file and no other parts of the codebase rely on an unmodifiedpayload.attributes. Please ensure that this inline mutation approach is consistent with our design decisions regarding object immutability and side effects.
|
The error is coveralls 504ing. |
Shortcut Story ID: [sc-59124]
So to implement the filters messages we have to grab a model from the store.. rather than copy this pattern to a bunch of places I moved it to the entities channel.
Then I looked at other places where this pattern existed and changed it to
Model.getRelationshipwhich will coincide eventually with simplify the various relationships once #1320 is complete.For not the filters implementation on the ws service is overly simplistic, but I don't know what kind of filter maintenance we're going to run into yet or if it's always a simple replace.
But I'm opening this as a draft as I think I need to real-world test the socket filters before adding tests in cypress etc.
Summary by CodeRabbit
New Features
Bug Fixes
Tests