Flow routes with sidebar context #1337
Conversation
WalkthroughThe changes enhance the routing and event handling within the application, specifically by adding new routes and modifying existing methods to better manage sidebar interactions during routing events. The Changes
Possibly related PRs
Suggested reviewers
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
|
| Run status |
|
| Run duration | 02m 57s |
| Commit |
|
| Committer | Paul Falgout |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
1
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
312
|
Tests for review
test/integration/patients/worklist/worklist.js • 1 failed test • care-ops-e2e
| Test | Artifacts | |
|---|---|---|
| worklist page > action list |
Test Replay
Screenshots
|
|
![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
🧹 Outside diff range and nitpick comments (3)
src/js/services/sidebar.js (2)
54-57: LGTM! Consider adding JSDoc for clarity.The changes to the
closeSidebarmethod look good. The newisRoutingparameter allows for more flexible handling of routing-related scenarios.Consider adding JSDoc to clarify the purpose of the
isRoutingparameter:/** * Closes the sidebar. * @param {Object} options - The options object. * @param {boolean} [options.isRouting=false] - Indicates if the sidebar is being closed due to routing. */ closeSidebar({ isRouting } = {}) { // ... existing implementation ... }
64-67: LGTM! Consider adding JSDoc and a safety check.The changes to the
stopSidebarAppmethod look good. The newisRoutingparameter allows for more flexible handling of routing-related scenarios.Consider adding JSDoc to clarify the purpose of the
isRoutingparameter:/** * Stops the current sidebar app. * @param {Object} options - The options object. * @param {boolean} [options.isRouting=false] - Indicates if the app is being stopped due to routing. */ stopSidebarApp({ isRouting } = {}) { // ... existing implementation ... }Additionally, consider adding a safety check to ensure
currentApp.stopcan handle an object parameter:stopSidebarApp({ isRouting } = {}) { if (!this.currentApp) return; if (typeof this.currentApp.stop === 'function') { this.currentApp.stop({ isRouting }); } else { this.currentApp.stop(); } delete this.currentApp; }This change ensures backwards compatibility if
currentApp.stopdoesn't expect an object parameter in some cases.src/js/apps/patients/patients-main_app.js (1)
46-49: LGTM! Consider adding a comment for clarity.The new 'flow:details' route is well-integrated and consistent with the existing routing structure. It logically extends the flow functionality to include a details view.
Consider adding a brief comment above this new route to explain its purpose, e.g.:
// Route for displaying detailed view of a specific flow 'flow:details': { action: 'showFlow', route: 'flow/:id/details', },This would enhance code readability and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- src/js/apps/patients/patient/flow/flow_app.js (3 hunks)
- src/js/apps/patients/patients-main_app.js (1 hunks)
- src/js/base/routerapp.js (1 hunks)
- src/js/services/sidebar.js (1 hunks)
🧰 Additional context used
🔇 Additional comments (8)
src/js/apps/patients/patients-main_app.js (1)
46-49: Verify handling of 'flow:details' in FlowAppThe new 'flow:details' route uses the existing
showFlowaction. To ensure complete functionality:
- Verify that the
FlowApp(imported at the top of this file) is prepared to handle this new detailed view.- Confirm that the
showFlowmethod in this file or its parent class can differentiate between the main flow view and the details view based on the route.To assist in this verification, you can run the following script:
This script will help identify if and how the new route is handled in the
FlowAppand theshowFlowmethod implementation.src/js/base/routerapp.js (1)
84-84: LGTM! Consider verifying sidebar handling and updating docs.The addition of
{ isRouting: true }to the sidebar close request is a good improvement. It provides context to the sidebar component about the reason for closing, which aligns with the PR objectives of "Flow routes with sidebar context".To ensure this change is properly handled, please verify:
- The sidebar component correctly interprets the
isRoutingparameter.- Any relevant documentation is updated to reflect this new behavior.
Run the following script to check for corresponding changes in the sidebar component:
If no results are found, consider updating the sidebar component to handle this new parameter.
✅ Verification successful
Let's locate the actual path of the sidebar component to verify the handling of the
isRoutingparameter.[/run_scripts]
Let's verify if the
isRoutingparameter is handled in any of the sidebar components.[/run_scripts]
Verified Sidebar Handling of
isRoutingParameterThe
isRoutingparameter is successfully handled insrc/js/services/sidebar.js, ensuring that sidebar closing operations are context-aware during routing actions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for handling of isRouting parameter in sidebar component # Test: Search for isRouting parameter usage in sidebar-related files rg --type js 'isRouting' src/js/views/globals/sidebarLength of output: 122
Script:
#!/bin/bash # Description: Locate all sidebar-related JavaScript files in the codebase fd sidebar src/js --type f --extension jsLength of output: 1473
Script:
#!/bin/bash # Description: Search for usage of 'isRouting' parameter in all identified sidebar-related JavaScript files rg --type js 'isRouting' src/js/apps/clinicians/sidebar/clinician-sidebar_app.js \ src/js/apps/patients/patient/sidebar/sidebar_app.js \ src/js/apps/patients/sidebar/action-sidebar_app.js \ src/js/apps/patients/sidebar/filters-sidebar_app.js \ src/js/apps/patients/sidebar/flow-sidebar_app.js \ src/js/apps/patients/sidebar/patient-sidebar_app.js \ src/js/apps/programs/sidebar/action-sidebar_app.js \ src/js/apps/programs/sidebar/flow-sidebar_app.js \ src/js/apps/programs/sidebar/program-sidebar_app.js \ src/js/services/sidebar.js \ src/js/views/clinicians/sidebar/clinician-sidebar_views.js \ src/js/views/patients/patient/sidebar/sidebar_views.js \ src/js/views/patients/sidebar/action/action-sidebar-action_views.js \ src/js/views/patients/sidebar/action/action-sidebar-activity-views.js \ src/js/views/patients/sidebar/action/action-sidebar-attachments-views.js \ src/js/views/patients/sidebar/action/action-sidebar-forms_views.js \ src/js/views/patients/sidebar/action/action-sidebar_views.js \ src/js/views/patients/sidebar/filters/filters-sidebar_views.js \ src/js/views/patients/sidebar/flow/flow-sidebar-activity-views.js \ src/js/views/patients/sidebar/flow/flow-sidebar_views.js \ src/js/views/patients/sidebar/patient/patient-sidebar_views.js \ src/js/views/programs/program/sidebar/sidebar-views.js \ src/js/views/programs/sidebar/action/action-sidebar_views.js \ src/js/views/programs/sidebar/flow/flow-sidebar_views.js \ src/js/views/programs/sidebar/program/programs-sidebar_views.jsLength of output: 1719
src/js/apps/patients/patient/flow/flow_app.js (6)
1-1: Importingnoopfrom 'underscore' is appropriateThe addition of
noopis necessary as it is used in theeventRoutes.
28-30: Enhancement of routing with new event routesAdding
'flow': noopand'flow:details': 'editFlow'toeventRouteseffectively handles new routing scenarios.
291-296: Proper handling ofisRoutinginstopeventThe inclusion of the
isRoutingparameter in the'stop'event handler allows conditional navigation, preventing unnecessary route triggers when routing is in progress.
300-300: Correct invocation ofstartCurrentwithactionIdEnsures that the action app starts with the correct action ID, maintaining expected functionality.
304-305: UpdatedonEditFlowto triggerflow:detailseventChanging
onEditFlowto trigger'flow:details'enhances the routing flow when editing a flow.
307-314: Implementation ofeditFlowmethod is accurateThe new
editFlowmethod correctly starts the sidebar for editing the flow and handles the'stop'event with proper consideration of theisRoutingparameter.
paulfalgout
force-pushed
the
flow-routes
branch
from
c8c457d to
f18051d
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -1,6 +1,6 @@ | |||
| import _ from 'underscore'; | |||
|
|
|||
| import { testTs, testTsSubtract } from 'helpers/test-timestamp'; | |||
| import { testTs, testTsSubtract, testTsAdd } from 'helpers/test-timestamp'; | |||
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/js/apps/patients/patient/flow/flow_app.js (3 hunks)
- src/js/apps/patients/patients-main_app.js (1 hunks)
- test/integration/patients/patient/patient-flow.js (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/js/apps/patients/patient/flow/flow_app.js
- src/js/apps/patients/patients-main_app.js
🧰 Additional context used
🪛 GitHub Check: CodeQL
test/integration/patients/patient/patient-flow.js
[notice] 3-3: Unused variable, import, function or class
Unused import testTsAdd.
| @@ -1,6 +1,6 @@ | |||
| import _ from 'underscore'; | |||
|
|
|||
| import { testTs, testTsSubtract } from 'helpers/test-timestamp'; | |||
| import { testTs, testTsSubtract, testTsAdd } from 'helpers/test-timestamp'; | |||
There was a problem hiding this comment.
Remove unused import testTsAdd
The import testTsAdd is unused in this file. Removing it will clean up the code.
Apply this diff to remove the unused import:
-import { testTs, testTsSubtract, testTsAdd } from 'helpers/test-timestamp';
+import { testTs, testTsSubtract } from 'helpers/test-timestamp';📝 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.
| import { testTs, testTsSubtract, testTsAdd } from 'helpers/test-timestamp'; | |
| import { testTs, testTsSubtract } from 'helpers/test-timestamp'; |
🧰 Tools
🪛 GitHub Check: CodeQL
[notice] 3-3: Unused variable, import, function or class
Unused import testTsAdd.
| .find('[data-action-region] .action-sidebar__name') | ||
| .should('contain', 'First Action'); |
There was a problem hiding this comment.
Correct the sidebar assertion to match the updated action
After updating fx.data to testFlowActions[1] (which is "Second Action"), the assertion should check for "Second Action" instead of "First Action".
Apply this diff to fix the assertion:
.find('[data-action-region] .action-sidebar__name')
- .should('contain', 'First Action');
+ .should('contain', 'Second Action');📝 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.
| .find('[data-action-region] .action-sidebar__name') | |
| .should('contain', 'First Action'); | |
| .find('[data-action-region] .action-sidebar__name') | |
| .should('contain', 'Second Action'); |
Shortcut Story ID: [sc-###]
Alternate to #1334
Summary by CodeRabbit
New Features
Bug Fixes
Documentation