Handle attachments web socket notifications on patient flow page #1371
Conversation
RoundingWell Care Ops Frontend
|
||||||||||||||||||||||||||||
| Project |
RoundingWell Care Ops Frontend
|
| Branch Review |
attachments-ws-msgs
|
| Run status |
|
| Run duration | 02m 52s |
| Commit |
|
| Committer | Nick Major |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
1
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
312
|
| View all changes introduced in this branch ↗︎ | |
Pull Request Test Coverage Report for Build b078a67b-a2e7-4662-867d-744bfcca0ea6Details
💛 - Coveralls |
| }); | ||
|
|
||
| this.set({ _files: newFilesRelationship }); | ||
| }, |
There was a problem hiding this comment.
The paperclip attachments icon on action list items is shown or hidden based on the length of action.get('_files').
We're currently not updating that when a file is removed manually by a user. And therefore it doesn't work out of the box for the FileRemoved ws notification.
So i added this method to the action entity service that filters out the file from _files by id. And then sets a new _files relationship array for the action.
This allows for the paperclip icon to be hidden after attachment deletion.
There was a problem hiding this comment.
If this is how we want to implement this, we would just need to call action.removeAttachment(model) in the action sidebar app when a file is manually deleted by a user.
There was a problem hiding this comment.
Currently in the app, the .paperclip icon isn't shown when an attachment is added and isn't hidden when an attachment is removed manually by a user in the sidebar.
I created a separate story here, that I can tackle in a separate PR.
WalkthroughThis pull request introduces comprehensive enhancements to the attachment management functionality across multiple files. The changes focus on improving how attachments are handled in the patient sidebar application, including adding new methods for attachment addition and removal, updating WebSocket communication, and extending the model's message handling capabilities. The modifications span across JavaScript files in the patient actions, entities service, and test integration, providing a more robust and interactive attachment management system. Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🔇 Additional comments (7)
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 (
|
![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
🧹 Nitpick comments (7)
test/integration/patients/patient/patient-flow.js (3)
111-112: Use descriptive test IDs
Using separate UUIDs is appropriate for file-related tests, but consider clarifying their usage with a more descriptive variable name or comment to improve test readability.
124-129: Ensure multi-file extension testing
Defining allowed_uploads as only ['pdf'] is fine for this test, but consider adding more file types or creating separate tests for other file extensions if needed.
2630-2631: Maintain consistent naming
Another UUID for socket attachments is consistent. Consider naming it more descriptively (e.g., testSocketFileIdForFlow) if it helps clarity.src/js/entities-service/entities/files.js (1)
21-35: Handle file replacement and removal carefully
The messages object effectively updates file paths on FileReplaced and cleans up references in FileRemoved. Ensure you handle any concurrency issues if multiple replacements arrive in quick succession.src/js/apps/patients/sidebar/action-sidebar_app.js (2)
247-248: Unsubscribe before destroy
Destroying the model after unsubscribing is logical but consider reversing the order if unsubscribing depends on the model’s existence.
251-251: Bulk subscription approach
Combining comments and attachments is convenient. Potentially skip duplicates if a model appears in both arrays.src/js/entities-service/entities/actions.js (1)
56-71: Append-based approach for AttachmentAdded
Appended file references correctly expand _files. Consider checking for duplicates if the same file ID arrives twice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/js/apps/patients/sidebar/action-sidebar_app.js(3 hunks)src/js/base/model.js(2 hunks)src/js/entities-service/entities/actions.js(2 hunks)src/js/entities-service/entities/files.js(2 hunks)test/integration/patients/patient/patient-flow.js(6 hunks)
🔇 Additional comments (21)
test/integration/patients/patient/patient-flow.js (15)
137-137: Keep consistent approach for allowed_uploads
Line 137 matches the approach in lines 124-129. This is consistent. Ensure both places reflect any future expansions of allowed uploads.
140-146: Relationship definitions look correct
All relationships—flow, state, owner, form, patient, program-action, files—are properly set for the test scenario.
169-170: Including program action in fixture
Pushing the testProgramAction into fx.included is correct for ensuring backend returns the new program action.
178-194: Fixture for existing file clarity
Providing an existing file id and path ensures coverage for scenarios where an action already has attachments. Nicely done.
453-466: Check for new attachments in DOM
Verifying the UI updates for the newly added file is good. Keep an eye on potential slow DOM updates in real scenarios.
467-472: Download link test
Ensuring the download link is correct improves coverage for file retrieval paths.
473-490: FileReplaced scenario coverage
Replacing the file and verifying updated path/urls is valuable for ensuring re-upload logic works as expected.
491-507: UI checks after FileReplaced
Ensuring UI re-renders and shows the updated file is crucial for user experience.
508-526: Ensure FileRemoved test coverage
This confirms file removal updates the UI. Nicely done.
2935-2960: Real-time AttachmentAdded coverage
Good test verifying real-time file addition via WebSocket. No issues spotted.
2961-2965: Paperclip icon verification
Ensuring the paperclip icon appears gives confidence that attachment awareness is correct.
2966-2974: Real-time FileRemoved coverage
This scenario confirms correct removal logic on dynamic WebSocket updates. Excellent coverage.
2975-2979: Final icon cleanup check
Ensuring the paperclip icon is gone prevents stale UI references. Good job.
152-156: Confirm forced setting usage in tests
Explicitly pushing { id: 'upload_attachments', attributes: { value: true } } ensures upload tests pass. Make sure this doesn't mask unexpected side effects if real settings differ.
✅ Verification successful
Let me gather more information about the usage of upload_attachments setting to verify if there are any potential side effects.
Let me search for more specific patterns to understand how this setting is used in the application.
Setting is properly implemented with safeguards
The upload_attachments setting is used correctly in the codebase. The implementation in src/js/apps/patients/sidebar/action-sidebar_app.js shows proper validation where uploads are only shown when both the setting is enabled (upload_attachments=true) AND the action has allowed uploads (hasAllowedUploads()). The test setting this to true is necessary to test the upload functionality and doesn't mask any side effects since the code has proper guards in place.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for other occurrences of upload_attachments setting
rg -l "upload_attachments" --type js
# Look for the actual implementation where this setting is used
ast-grep --pattern 'upload_attachments'
# Search for related test files and settings
rg "routeSettings.*upload_attachments" --type js -C 2
Length of output: 388
Script:
#!/bin/bash
# Look for the implementation details in action sidebar files
rg "upload_attachments" test/integration/programs/sidebar/action-sidebar.js test/integration/patients/sidebar/action-sidebar.js src/js/apps/programs/sidebar/action-sidebar_app.js src/js/apps/patients/sidebar/action-sidebar_app.js -C 3
# Search for any conditional logic related to upload_attachments
ast-grep --pattern 'if ($$$upload_attachments$$$) { $$$ }'
# Look for setting definitions or configurations
rg "upload_attachments.*value" -C 2
Length of output: 8792
427-452: WebSocket test: verify AttachmentAdded
This test thoroughly checks the AttachmentAdded category. Ensure that large file edge cases are covered in separate tests if necessary.
✅ Verification successful
Let me search for related test files and WebSocket handling to verify the coverage of large file edge cases.
These scripts will help us understand:
- Where AttachmentAdded tests are implemented
- If there are any existing large file tests
- File upload configurations and S3 related tests
- Any file size limitations in the codebase
Let me search for more specific information about file attachment tests and any size-related handling.
These scripts will help us understand:
- The complete context of the AttachmentAdded test
- Any file validation or size handling logic in tests
- Test suites dealing with file attachments
Based on the search results, I can now generate a final response:
WebSocket test: AttachmentAdded coverage is sufficient
The test adequately covers the AttachmentAdded WebSocket event handling. The test suite includes comprehensive attachment testing with:
- File upload permission validation
- Attachment display verification
- Multiple attachment scenarios
- Integration with patient actions and clinician data
No specific file size edge cases are needed as these should be handled at the API/backend level, not in WebSocket event handling tests which verify the UI updates after successful uploads.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for WebSocket-related test files and AttachmentAdded tests
rg -l "AttachmentAdded" test/
Length of output: 79
Script:
#!/bin/bash
# Search for file size or large file related tests
rg -i "file.*size|large.*file" test/
Length of output: 38
Script:
#!/bin/bash
# Look for any file upload or attachment related test configurations
rg "bucket.*name|\.s3\." test/
Length of output: 4041
Script:
#!/bin/bash
# Search for any file size limits or constraints in the codebase
rg -i "maxFileSize|file.*limit|size.*limit" .
Length of output: 65701
Script:
#!/bin/bash
# Search for the full context around AttachmentAdded test
rg -B 5 -A 10 "AttachmentAdded" test/integration/patients/patient/patient-flow.js
Length of output: 802
Script:
#!/bin/bash
# Look for any file validation or attachment handling code
rg -i "attachment|file.*validation|validate.*file" test/
Length of output: 5254
Script:
#!/bin/bash
# Search for any test setup or configuration related to file attachments
ast-grep --pattern 'describe($_, function() { $$$ file $$$ })'
Length of output: 64
src/js/base/model.js (1)
8-8: Clarify destroy behavior
The comment explaining isDeleted helps maintainers understand the difference between front-end and back-end deletion states. No issues here.
src/js/apps/patients/sidebar/action-sidebar_app.js (4)
51-51: Function naming conflict
“onAttachmentAdded” is very similar to the existing “onAddAttachment.” Consider renaming to onWsAttachmentAdded to reduce confusion.
255-255: Guard for incomplete requests
Exiting early if comments or attachments are not yet available avoids errors. This is a good defensive check.
257-257: Bulk unsubscribe approach
Unsubscribing from both collections is consistent. No immediate issues.
239-240: Check WebSocket subscription object
Using Radio.request('ws', 'add', file) might not subscribe to the final model. Evaluate whether to pass the newly created model for consistency with other events.
✅ Verification successful
Let me gather more information about the WebSocket subscription and model handling in the codebase.
This will help us understand:
- How WebSocket subscriptions are handled throughout the codebase
- The pattern of model subscriptions
- Context around the specific usage in question
Based on the search results, I can now generate the final response:
WebSocket subscription pattern is consistent
Looking at the codebase, passing file directly to Radio.request('ws', 'add', file) is consistent with the existing pattern. The same file shows multiple examples where both models and raw objects are passed to the WebSocket subscription system, including collections of models, individual models, and file objects. The implementation handles both cases appropriately.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for WebSocket related Radio requests
rg "Radio\.(request|trigger)\('ws'" -A 3 -B 3
# Search for file model creation patterns
ast-grep --pattern 'Radio.request("ws", "add", $_)'
# Look for other WebSocket subscriptions
ast-grep --pattern 'Radio.request("ws", $_)'
Length of output: 4082
src/js/entities-service/entities/actions.js (1)
253-260: Efficient removeAttachment
Filtering out the matching resource from _files is straightforward. If partial matches or type mismatches occur, ensure you handle them gracefully.
| handleMessage({ category, resource, author, payload }) { | ||
| payload.attributes = extend({}, payload.attributes, { updated_at: dayjs.utc().format() }); | ||
| payload.resource = resource; |
There was a problem hiding this comment.
💡 Codebase verification
Potential mismatch in handleMessage parameters
The implementation in src/js/base/model.js expects an object with { category, resource, author, payload }, but in src/js/services/ws.js, the method is called with the entire data object directly. This could lead to runtime errors if the data structure doesn't match the expected parameters.
src/js/services/ws.js:resource.handleMessage(data)src/js/base/model.js:handleMessage({ category, resource, author, payload })
🔗 Analysis chain
Expanded handleMessage signature
Including resource and updating updated_at is helpful for robust message handling. Confirm that external references to handleMessage are updated accordingly.
Let me search for references to handleMessage in the codebase to verify if external calls are aligned with the updated signature.
Let me check the implementation in ws.js to understand how the method is being called.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for handleMessage method calls
rg "handleMessage\(" -A 2
# Search for handleMessage method definitions
ast-grep --pattern 'handleMessage($_) { $$$ }'
Length of output: 439
Script:
#!/bin/bash
# Get more context around the handleMessage call in ws.js
rg -B 5 -A 5 "handleMessage\(" src/js/services/ws.js
Length of output: 322
| onAttachmentAdded(model) { | ||
| this.attachments.add(model); | ||
|
|
||
| Radio.request('ws', 'add', model); | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Separate from onAddAttachment
Having both onAttachmentAdded and onAddAttachment is potentially confusing. Following the suggestion to rename it clearly differentiates WS vs. manual additions.
…or to `beforeStart()` finishing To protect the `removeSubscriptions()` function from running before comments or attachments api data is retrieved for the action sidebar
Shortcut Story ID: [sc-56327] [sc-57309]
In this PR, these are being handled only in the action sidebar on the patient flow page.
Coinciding with the backend PR: https://github.com/RoundingWell/care-ops-backend/pull/8787.
AttachmentAddednotificationsFileReplacednotificationsFileRemovednotificationsSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements
Testing