-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/upgrade react router dom #1050
Conversation
…e has been made w/ burnettk
…ntial errors w/ burnettk
WalkthroughWalkthroughThe update focuses on modernizing the frontend by upgrading Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 3
Configuration used: .coderabbit.yaml
Files selected for processing (10)
- spiffworkflow-frontend/package-lock.json (5 hunks)
- spiffworkflow-frontend/package.json (1 hunks)
- spiffworkflow-frontend/src/App.tsx (2 hunks)
- spiffworkflow-frontend/src/ContainerForExtensions.tsx (2 hunks)
- spiffworkflow-frontend/src/components/MarkdownRenderer.tsx (2 hunks)
- spiffworkflow-frontend/src/components/ReactDiagramEditor.tsx (3 hunks)
- spiffworkflow-frontend/src/interfaces.ts (2 hunks)
- spiffworkflow-frontend/src/routes/EditorRoutes.tsx (1 hunks)
- spiffworkflow-frontend/src/routes/Extension.tsx (6 hunks)
- spiffworkflow-frontend/src/routes/ProcessModelEditDiagram.tsx (9 hunks)
Files skipped from review due to trivial changes (1)
- spiffworkflow-frontend/src/routes/EditorRoutes.tsx
Additional comments: 16
spiffworkflow-frontend/src/App.tsx (1)
- 12-16: Reordering context providers can impact the availability of context values in nested components. Verify that moving
APIErrorProvider
outside ofAbilityContext.Provider
does not affect any components relying on context values provided byAbilityContext
.spiffworkflow-frontend/src/components/MarkdownRenderer.tsx (3)
- 2-2: Importing
FormattingService
introduces a new dependency. Ensure thatFormattingService
is properly tested and its methods do not introduce side effects that could affect the rendering of markdown content.- 6-6: The introduction of
newMarkdown
variable usingFormattingService.checkForSpiffFormats(source)
modifies the original markdown source. Confirm that this modification aligns with the application's requirements for markdown rendering and does not alter the intended content or formatting in an undesirable way.- 16-16: Updating props passed to
MDEditor.Markdown
to include the modifiedsource
using spread syntax is a significant change. Ensure that all properties are correctly merged and that no properties are unintentionally overridden, leading to unexpected behavior.spiffworkflow-frontend/package.json (1)
- 54-54: Updating
react-router-dom
to"^6.22.0"
is a major version change. Confirm that this version is fully compatible with the rest of the project's dependencies and that there are no breaking changes affecting the application's routing functionality.spiffworkflow-frontend/src/ContainerForExtensions.tsx (3)
- 2-2: Switching to
createBrowserRouter
,RouterProvider
, andOutlet
fromreact-router-dom
represents a significant change in routing strategy. Ensure that this new approach is compatible with the application's architecture and that all routes are correctly configured and accessible.- 103-111: The transition to an array-based format for defining routes requires careful validation to ensure that all paths are correctly mapped to their respective components. Double-check that the paths and elements are correctly associated and that no routes are missing or incorrectly configured.
- 142-148: Creating the router with
createBrowserRouter
and configuring it with the new route structure is a critical part of the routing update. Verify that theComponent
property in the route configuration is correctly utilized and that thechildren
routes are properly nested and rendered.spiffworkflow-frontend/src/interfaces.ts (1)
- 1-1: Adding
FunctionComponent
from 'react' as a type for values inObjectWithStringKeysAndFunctionValues
is a type refinement. Ensure that all uses ofObjectWithStringKeysAndFunctionValues
throughout the application are compatible with this change and that it does not introduce type errors or require additional prop definitions for the components.spiffworkflow-frontend/src/components/ReactDiagramEditor.tsx (3)
- 95-95: The
disableSaveButton
prop is correctly added to theOwnProps
type, ensuring type safety for the new functionality.- 126-126: Ensure the
disableSaveButton
prop is destructured from the component's props, aligning with the existing pattern of prop management.- 714-716: The
disableSaveButton
prop is effectively used to conditionally disable the Save button. This implementation follows best practices for conditional rendering based on prop values.spiffworkflow-frontend/src/routes/ProcessModelEditDiagram.tsx (4)
- 1051-1057: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1054-1063]
The handling of
process_model_id
beingnull
ingeneratePath
calls is correct, ensuring the application can gracefully handle cases where theprocess_model_id
is not available.
- 1085-1085: The addition of
disableSaveButton={!diagramHasChanges}
in two instances correctly implements conditional disabling of the Save button based on whether there are unsaved changes. This enhances user interaction by preventing unnecessary save operations when there are no changes.Also applies to: 1122-1122
- 1143-1152: The
unsavedChangesMessage
function correctly displays a notification for unsaved changes, improving user feedback and preventing data loss. This is a valuable addition to the user interface.- 1154-1164: Consolidating modal components into the
pageModals
function improves code organization and maintainability by centralizing modal rendering logic. This is a good practice for managing multiple modal components in a single file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 2
Configuration used: .coderabbit.yaml
Files selected for processing (8)
- spiffworkflow-frontend/cypress/e2e/process_instances.cy.js (2 hunks)
- spiffworkflow-frontend/cypress/e2e/process_models.cy.js (2 hunks)
- spiffworkflow-frontend/src/components/Notification.tsx (4 hunks)
- spiffworkflow-frontend/src/components/ReactDiagramEditor.tsx (3 hunks)
- spiffworkflow-frontend/src/extension_ui_schema_interfaces.ts (2 hunks)
- spiffworkflow-frontend/src/interfaces.ts (1 hunks)
- spiffworkflow-frontend/src/routes/Extension.tsx (8 hunks)
- spiffworkflow-frontend/src/routes/ProcessModelEditDiagram.tsx (8 hunks)
Files skipped from review as they are similar to previous changes (4)
- spiffworkflow-frontend/src/components/ReactDiagramEditor.tsx
- spiffworkflow-frontend/src/interfaces.ts
- spiffworkflow-frontend/src/routes/Extension.tsx
- spiffworkflow-frontend/src/routes/ProcessModelEditDiagram.tsx
Additional comments: 6
spiffworkflow-frontend/src/components/Notification.tsx (1)
- 21-21: The addition of the
'data-qa'
prop is a good practice for facilitating testing. It's correctly defined as an optional string prop and conditionally applied to the component's root div, enhancing testability without affecting production behavior.spiffworkflow-frontend/src/extension_ui_schema_interfaces.ts (1)
- 7-7: Importing
FunctionComponent
from 'react' is necessary for the new type definition introduced in this file. Good practice to keep imports organized.spiffworkflow-frontend/cypress/e2e/process_models.cy.js (2)
- 85-86: Replacing direct
cy.contains('Save').click()
calls withcy.getBySel('process-model-file-save-button').click()
is a good improvement for test readability and maintainability. Using custom Cypress commands likegetBySel
helps abstract specific selectors, making the tests more robust and easier to update.- 105-105: The use of custom Cypress commands for saving actions, as seen here, enhances test readability and maintainability. This practice should be consistently applied across all tests for consistency and ease of updates.
spiffworkflow-frontend/cypress/e2e/process_instances.cy.js (2)
- 39-39: The update to use
cy.getBySel('process-model-file-save-button').click()
for saving actions is a good practice, enhancing test readability and maintainability. Consistently using custom Cypress commands across tests is beneficial.- 50-50: Using custom Cypress commands for saving actions, as done here, improves test readability and maintainability. This approach should be consistently applied to ensure uniformity across the test suite.
This updates the react-router-dom to match the react-router package in the frontend. It also updates the frontend to support this.
Changes:
Summary by CodeRabbit
react-router-dom
and related dependencies to improve navigation and routing within the app.process_model_id
values.