Show comment count on action list items #1385
Conversation
Shown/hidden based on length of action.relationships.comments array Adjustment of header & section css widths is needed
Adjustment of section css widths is needed
…ia action sidebar action.relationships.comments must be updated to trigger UI render and show correct comment count
… via action sidebar action.relationships.comments must be updated to trigger UI render and show correct comment count
WalkthroughThis pull request introduces comprehensive comment functionality across multiple components of the patient management system. The changes span various files, including Changes
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 (
|
| } | ||
|
|
||
| .patient__action-attachment { | ||
| .patient__action-attachment, | ||
| .patient__action-comment { | ||
| color: #999; |
There was a problem hiding this comment.
Can use $black-60 for these I believe...
There was a problem hiding this comment.
Updated these to use $black-60.
| @@ -140,14 +140,22 @@ | |||
| } | |||
|
|
|||
| .patient__list-meta { | |||
| width: 510px; | |||
| width: 562px; | |||
There was a problem hiding this comment.
I'm going to play around with these widths a little more on the patient flow page. When this meta section is at it's maximum potential width, things look a little funky to me.
There was a problem hiding this comment.
Min-width breaks the layout on smaller width browser windows.
I think what we have is fine right now, but I'll make a note on the card for Mark so he can take a look when this is up on QA. And see if it looks good to him and we can make adjustments if needed.
There was a problem hiding this comment.
We're really reaching our limits on the worklist when it comes to horizontal spacing for these action list items.
| } | ||
|
|
||
| .patient__action-attachment { | ||
| .patient__action-attachment, | ||
| .patient__action-comment { |
There was a problem hiding this comment.
Looking at this now, I think these two could be consolidated into one class.
There was a problem hiding this comment.
I consolidated these into one patient__action-display-icon class.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/js/entities-service/comments.js (1)
8-8: Ensure naming consistency for radio requests.
Adding'comments:collection': 'getCollection'is clear, but consider renaming it to match the pattern of'fetch:comments:collection:byAction'. For example,'fetch:comments:collection'could more clearly convey that a collection is being fetched.src/js/views/patients/patient/flow/action-item.hbs (1)
18-20: Add optional aria-label for paperclip icon
Consider addingaria-label="Attachments"(or similar accessible text) to improve screen reader support.src/js/views/patients/worklist/action-item.hbs (1)
31-33: Consider adding aria-label to comment icon
Improve accessibility by adding anaria-label="Comments"or wrapping it with<span class="sr-only">Comments</span>.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
package.json(1 hunks)src/js/apps/patients/sidebar/action-sidebar_app.js(3 hunks)src/js/entities-service/comments.js(1 hunks)src/js/entities-service/entities/actions.js(4 hunks)src/js/entities-service/entities/comments.js(1 hunks)src/js/views/patients/patient/archive/action-item.hbs(1 hunks)src/js/views/patients/patient/archive/archive_views.js(1 hunks)src/js/views/patients/patient/dashboard/action-item.hbs(1 hunks)src/js/views/patients/patient/dashboard/dashboard_views.js(1 hunks)src/js/views/patients/patient/flow/action-item.hbs(1 hunks)src/js/views/patients/patient/flow/flow_views.js(1 hunks)src/js/views/patients/patient/flow/patient-flow.scss(1 hunks)src/js/views/patients/patient/patient.scss(1 hunks)src/js/views/patients/sidebar/action/action-sidebar-activity-views.js(2 hunks)src/js/views/patients/worklist/action-item.hbs(1 hunks)src/js/views/patients/worklist/action_views.js(1 hunks)src/js/views/patients/worklist/worklist-list.scss(2 hunks)test/integration/patients/patient/archive.js(4 hunks)test/integration/patients/patient/dashboard.js(3 hunks)test/integration/patients/patient/patient-flow.js(6 hunks)test/integration/patients/sidebar/action-sidebar.js(1 hunks)test/integration/patients/worklist/worklist.js(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/js/views/patients/patient/flow/patient-flow.scss
🔇 Additional comments (47)
src/js/entities-service/entities/comments.js (1)
17-19: Guard against undefined or missing parent Action.
this.getAction()may returnundefinedif a comment has no associated action. Consider verifying thatactionis defined before callingremoveComment(this)to avoid potential runtime errors.src/js/views/patients/patient/archive/action-item.hbs (1)
14-19: Display comment icon only when count is positive.
Using{{#if commentCount}}works as long as the property is0when there are no comments. EnsurecommentCountis always numeric to avoid accidental string values like"0"that evaluate to truthy.src/js/views/patients/patient/dashboard/action-item.hbs (1)
17-22: Match behavior with archive view for consistency.
The logic and styling for displaying attachments and comments align with the archive view. Confirm consistent presentation across all other pertinent views (e.g., Patient Flow, Worklist) to maintain a uniform user experience.✅ Verification successful
Verified: Implementation matches across views ✓
The dashboard implementation perfectly aligns with archive and flow views. The worklist view uses view-specific class names while maintaining the same visual structure and behavior.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for all `{{#if commentCount}}` references to verify consistency across relevant Handlebars templates. rg -A 5 $'\\{\\{#if commentCount\\}\\}'Length of output: 2497
Script:
#!/bin/bash # Description: Search for all `{{#if hasAttachments}}` references to verify consistency across relevant Handlebars templates rg -A 3 $'\\{\\{#if hasAttachments\\}\\}'Length of output: 1612
src/js/views/patients/patient/flow/action-item.hbs (1)
21-23: Double-check zero count scenario
EnsurecommentCountcorrectly evaluates to 0 or a positive integer. If you intend to hide the icon at 0, the logic is fine. Otherwise, consider{{#if (gt commentCount 0)}}.src/js/views/patients/worklist/action-item.hbs (1)
28-30: Good margin usage
Using margin classes for the paperclip icon ensures clear visual separation.src/js/views/patients/worklist/action_views.js (1)
50-50: Exposes commentCount to the template
This addition properly surfaces the comment count for display.src/js/views/patients/patient/dashboard/dashboard_views.js (1)
91-91: Consistent addition of commentCount
Aligns well with similar contexts in other views for consistent UI updates.src/js/views/patients/patient/archive/archive_views.js (1)
87-87: Show comment count in template context
Adding thecommentCountproperty is consistent with the rest of the codebase and provides a straightforward way to display the current comment count on the UI.src/js/entities-service/entities/actions.js (4)
43-43: Ensure proper resource linkage and model updates
Defining_action: this.getResource()and immediately callingthis.addComment(commentModel);correctly associates the comment with the action. Good job linking the comment resource to the action so it can be managed consistently.Also applies to: 46-47
87-89: Consistent approach to retrieving comments
getComments()provides a single source of truth for the action’s comments. Centralizing comment retrieval here helps ensure consistent handling across the application.
158-160: Clear helper method for comment count
commentCount()cleanly wraps the usage ofgetComments().length. Straightforward implementation that makes it easy to display the count in the UI.
253-264: Robust comment add/remove logic
addCommentandremoveCommentreassign_commentsafter updating the collection, ensuring state remains in sync. Verify concurrency scenarios (like multiple simultaneous additions) to avoid race conditions.src/js/apps/patients/sidebar/action-sidebar_app.js (4)
177-180: Better lifecycle handling for ActivitiesView
Storing theactivitiesViewinstance in a variable before rendering allows for more flexible event binding. This is a good design choice for future extensibility.
183-187: Propagate remove:comment event
Listening to'remove:comment'onactivitiesViewprovides a clear, centralized way to handle the removal action at the parent level. Good approach for decoupled event handling.
209-209: Add newly created comment to action
Callingthis.action.addComment(model)right after saving the comment ensures the action's comment collection is immediately updated, keeping the UI in sync.
221-225: Clean removal of comment and websocket unsubscription
this.action.removeComment(model)plusRadio.request('ws', 'unsubscribe', model)neatly removes the comment from the collection and stops receiving updates for it. This prevents stale subscriptions.src/js/views/patients/sidebar/action/action-sidebar-activity-views.js (2)
176-176: Trigger event before model destruction
Emittingremove:commentprior tothis.model.destroy()allows external views to handle comment removal logic (e.g., updating the action). Great for decoupled design.
252-254: Proper event propagation
Forwarding'remove:comment'from the child view to the collection view ensures higher-level components can handle comment removal. This pattern keeps code modular and maintainable.src/js/views/patients/patient/flow/flow_views.js (1)
166-166: Ensure the commentCount property is handled consistently
Great addition to pass thecommentCountproperty to the template. Confirm thatthis.model.commentCount()is always defined before using it to avoid potential undefined errors. Otherwise, looks good.test/integration/patients/patient/archive.js (4)
2-2: Importing uuid
Usinguuidhere makes sense for generating unique comment IDs. No issues noted.
60-60: Verify comment relationship parameter
The code uses'files'instead of'comments'for the second parameter ingetRelationship. This might be a copy-paste mistake. Verify that'files'is the intended relationship name for comments.
227-233: Testing absence of comment icon
These lines correctly verify that the.fa-commentselector doesn’t exist for the third item, ensuring coverage for actions lacking comments.
384-391: Verifying presence of comment icon and count
Excellent test asserting the comment icon exists and checking its displayed count is 1, ensuring UI is updated correctly.test/integration/patients/patient/dashboard.js (4)
72-72: Consistent use of the comments relationship
Using'comments'is more intuitive when defining comment relationships. This is consistent with the PR’s overall intent.
412-415: Verifying empty form region
These lines confirm there’s no rendered content for the form region. Straightforward coverage with no issues.
416-427: Ensuring attachments and comments do not exist
This block validates the non-existence of.fa-paperclipand.fa-commenticons, clarifying UI behavior for items without attachments or comments.
446-453: Ensuring the first item has a comment icon and count
Good test coverage verifying the icon’s presence and the comment count is correctly displayed as “1.”test/integration/patients/sidebar/action-sidebar.js (1)
1518-1694: Comprehensive test for comment icon show/hide behavior
These lines thoroughly test the logic for displaying and removing the comment icon and its count. The sequence of adding and deleting comments, updating the icon’s visibility and count, is well covered.test/integration/patients/patient/patient-flow.js (6)
621-621: Good integration of a comment relationship for testing
This ensures that the action under test is initialized with an existing comment, aligning well with the PR objective to display comment counts.
688-689: Effective check for displayed comment icon and count
This test step confirms that the comment icon and the “1” count appear when there is exactly one associated comment, which matches the PR’s purpose of showing comment counts.
701-701: Proper validation for absence of comment icon
This line verifies that the comment icon is not displayed if there are zero comments, ensuring correct UI behavior.
2638-2638: UUID usage for test comment ID
Usinguuid()to generate a unique comment ID is a solid approach for realistic websocket tests, preventing ID collisions.
2666-2667: Initialization with empty files and comments
Defining empty relationships helps test the absence of attachments/comments. This setup accurately reflects a scenario with no comments or files.
2993-3031: Robust test for realtime comment addition/removal
These lines thoroughly test websocket-driven comment events, ensuring the UI displays the correct comment icon and count, and subsequently hides them upon removal. Great coverage for the PR’s dynamic comment-count feature.test/integration/patients/worklist/worklist.js (4)
3-3: Use of NIL_UUID and uuid
ImportingNIL as NIL_UUIDandv4 as uuidextends flexibility in test scenarios, enabling “no-owner” cases and unique ID creation for comments.
392-392: Adding a comment relationship in test actions
Supplying a UUID-based comment ensures a test scenario for a single comment, which is crucial for verifying comment count displays in the worklist.
764-770: Verifying presence of comment icon and count
The test ensures that the icon and count “1” appear in the first row. This precisely matches the PR’s functionality to show the current comment total.
771-775: Ensuring no comment icon for the second row
These lines confirm that items with zero comments do not display a comment icon, preserving accurate UI consistency.src/js/views/patients/worklist/worklist-list.scss (5)
33-39: New.worklist-list__display-icon-textclass
This style block neatly positions and formats icon-adjacent text. It aligns with the UI need to display concise numeric comment counts.
83-83: Adjusted name column width
The decrease to 17% can help accommodate additional icons or columns. Ensure no unintended text overflow at narrower screen sizes.
88-88: Broadening.worklist-list__meta
Increased to 48% widens metadata space; continue monitoring columns to ensure text remains properly truncated or wrapped.
[approve]
93-93: Expanding.worklist-list__action-meta-header
Consistent with the previous width change, this ensures meta columns line up for actions.
98-98: Expanding.worklist-list__flow-meta-header
Maintaining consistency with action meta headers by matching the 48% width helps unify the layout.src/js/views/patients/patient/patient.scss (3)
143-143: Wider.patient__list-metacontainer
Adjusting from 510px to 562px can better accommodate new icon text (e.g., comment count). Validate responsiveness on smaller displays.
146-152: Consolidated attachment/comment styling
These rules unify the icon styling for attachments and comments, simplifying maintenance while visually aligning icon usage.
154-158: Dedicated.patient__action-comment-textstyling
Adding a distinct class for comment text clarifies layout intentions and helps maintain consistent text formatting near icons.package.json (1)
115-115: Perfect alignment with the PR objective.The newly added
"comment"icon is well-suited for displaying the comment count. There are no concerns with this change.
There was a problem hiding this comment.
So in this I'm testing the following when a user manually adds or removes comments via the action sidebar:
- The comment icon is either shown or incremented when a comment is created.
- The comment icon is either hidden or decremented when a comment is deleted.
I wanted the test sequence to be this:
- Initiate test with an action that has no comments.
- Add comment and verify icon shows.
- Add second comment and verify icon count increments.
- Delete a comment and verify icon decrements.
- Delete remaining comment and verify icon becomes hidden.
But in our tests, newly added comments remain as .isNew() === true because their model isn't assigned an id after creation. And therefore, their Delete button remains hidden in the UI. I couldn't figure out how to fix this.
So I switched the test sequence around to be this:
- Initiate test with an action that has two comments.
- Delete a comment and verify icon decrements.
- Delete remaining comment and verify icon becomes hidden.
- Add comment and verify icon shows.
- Add second comment and verify icon count increments.
Doesn't seem like a big deal to me. But it may be a head scratcher for someone in the future when they look at this test.
There was a problem hiding this comment.
I don't understand the isNew issue. We should resolve this. The comment should get an id when the POST resolves. This is in our FE code... it should POST with the id and have the id after the save is complete.
There was a problem hiding this comment.
Yep, I confirmed this works as expected on QA/production. It's only an issue in the tests.
There was a problem hiding this comment.
This test is no longer in reverse sequence. In a306185, the isNew() problem is fixed and the test is refactored to be in the desired order.
| this.action.removeComment(model); | ||
|
|
||
| Radio.request('ws', 'unsubscribe', model); | ||
| }, |
There was a problem hiding this comment.
Do we need a modal.destroy() here? I don't think we do. It already happens in the view.
But the onRemoveAttachment() function in this file has it, which makes me unsure.
| @@ -140,14 +140,22 @@ | |||
| } | |||
|
|
|||
| .patient__list-meta { | |||
| width: 510px; | |||
| width: 562px; | |||
There was a problem hiding this comment.
I don't understand the isNew issue. We should resolve this. The comment should get an id when the POST resolves. This is in our FE code... it should POST with the id and have the id after the save is complete.
| .type('Test comment'); | ||
|
|
||
| cy | ||
| .intercept('POST', '/api/actions/*/relationships/comments', { |
There was a problem hiding this comment.
It's possible I suppose that the 204 is preventing the id, but I don't think it does.. we could check that though.
There was a problem hiding this comment.
The 204 code was the problem. Switched them to 201 and the response body worked 👍 thanks!
RoundingWell Care Ops Frontend
|
||||||||||||||||||||||||||||
| Project |
RoundingWell Care Ops Frontend
|
| Branch Review |
action-comment-count
|
| Run status |
|
| Run duration | 02m 41s |
| Commit |
|
| Committer | Nick Major |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
1
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
314
|
| View all changes introduced in this branch ↗︎ | |
Pull Request Test Coverage Report for Build 1723a621-1d9c-4b20-9385-200190ae412dDetails
💛 - Coveralls |
…n list items` test
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/integration/patients/sidebar/action-sidebar.js (2)
1567-1637: Consider extracting repeated selectors into constants.The test uses the same selector
.table-list__item .fa-commentmultiple times. Consider extracting it into a constant for better maintainability.+const COMMENT_ICON_SELECTOR = '.table-list__item .fa-comment'; + specify('action comments - show/hide icon in action list items', function() { // ... test setup ... cy .get('.patient__list') - .find('.table-list__item .fa-comment') + .find(COMMENT_ICON_SELECTOR) .should('exist') .next() .should('contain', '1'); // ... rest of the test ...
1639-1692: Extract repeated delete comment sequence into a helper function.The comment deletion sequence is duplicated. Consider extracting it into a helper function to improve maintainability and readability.
+const deleteComment = () => { + cy + .get('[data-activity-region]') + .find('.comment__item') + .first() + .find('.js-edit') + .click(); + + cy + .get('[data-activity-region]') + .find('.js-delete') + .click(); + + cy + .get('.modal--small') + .find('.js-submit') + .click() + .wait('@routeDeleteComment'); +}; + specify('action comments - show/hide icon in action list items', function() { // ... test setup ... - cy - .get('[data-activity-region]') - .find('.comment__item') - .first() - .find('.js-edit') - .click(); - - cy - .get('[data-activity-region]') - .find('.js-delete') - .click(); - - cy - .get('.modal--small') - .find('.js-submit') - .click() - .wait('@routeDeleteComment'); + deleteComment(); // ... verify UI updates ... - cy - .get('[data-activity-region]') - .find('.comment__item') - .first() - .find('.js-edit') - .click(); - - cy - .get('[data-activity-region]') - .find('.js-delete') - .click(); - - cy - .get('.modal--small') - .find('.js-submit') - .click() - .wait('@routeDeleteComment'); + deleteComment();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/integration/patients/sidebar/action-sidebar.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test-build
🔇 Additional comments (2)
test/integration/patients/sidebar/action-sidebar.js (2)
1518-1560: Well-structured test setup with proper isolation!The test initialization is thorough with proper route mocking and isolated test data.
1518-1693: Consider adding error case test coverage.The test thoroughly covers the happy path but doesn't verify the behavior when a comment fails to be added (e.g., server error). Consider adding a test case to verify error handling.
Example test case to add:
cy .intercept('POST', '/api/actions/*/relationships/comments', { statusCode: 500, body: { errors: [{ status: '500', title: 'Internal Server Error' }] } }) .as('routePostCommentError'); cy .get('[data-comment-region]') .find('.js-input') .type('Test comment that will fail'); cy .get('[data-comment-region]') .find('.js-post') .click() .wait('@routePostCommentError'); cy .get('.alert-box') .should('contain', 'Error posting comment'); cy .get('.patient__list') .find('.table-list__item .fa-comment') .should('not.exist');
Shortcut Story ID: [sc-58537]
This is based on the
action.relationships.commentsarray. The Icon is shown iflength > 0and hidden iflength === 0.The visibility of the icon and it's count will change when an action's comments change via manual user action in the UI or via websocket nofitications.
Comment icon is shown or incremented when a comment is added. And hidden or decremented when a comment is deleted.
Pages where icon is added:
Screenshot example of new icon: