fix(application-system): Possible to go back to hidden screens #17162
Conversation
WalkthroughThe changes introduce a new boolean property Changes
Suggested labels
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
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17162 +/- ##
==========================================
- Coverage 35.75% 35.73% -0.02%
==========================================
Files 6935 6933 -2
Lines 148068 148273 +205
Branches 42212 42293 +81
==========================================
+ Hits 52942 52989 +47
- Misses 95126 95284 +158
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 34 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 4 Total Test Services: 0 Failed, 4 Passed Test Services
|
…and.is into fix/back-to-hidden-section
![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 (2)
libs/application/ui-shell/src/reducer/reducerUtils.ts (1)
184-201: LGTM! Consider optimizing the implementation.The
canGoBackfunction is well-implemented with clear logic and proper TypeScript typing. However, it could be optimized for better readability and performance.Consider using
Array.prototype.findIndexfor a more concise implementation:export const canGoBack = ( screens: FormScreen[], screenIndex: number, ): boolean => { // Check if we're already at the start if (screenIndex <= 0) { return false } - // Look for any navigable screen behind where we are - for (let i = screenIndex - 1; i >= 0; i--) { - if (screens[i].isNavigable) { - return true - } - } - - return false + // Find the last navigable screen before current index + return screens + .slice(0, screenIndex) + .findIndex((screen) => screen.isNavigable) !== -1 }libs/application/ui-shell/src/components/Screen.tsx (1)
79-79: Add JSDoc documentation for the canGoBack propTo improve reusability and maintainability across different NextJS apps, please add JSDoc documentation explaining the purpose and behavior of this prop.
+ /** Determines if the back navigation should be enabled for this screen */ canGoBack: boolean
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
libs/application/ui-shell/src/components/Screen.tsx(3 hunks)libs/application/ui-shell/src/components/ScreenFooter.tsx(2 hunks)libs/application/ui-shell/src/lib/FormShell.tsx(2 hunks)libs/application/ui-shell/src/reducer/reducerUtils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
libs/application/ui-shell/src/components/ScreenFooter.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/ui-shell/src/lib/FormShell.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/ui-shell/src/components/Screen.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/ui-shell/src/reducer/reducerUtils.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (6)
libs/application/ui-shell/src/components/ScreenFooter.tsx (2)
30-30: LGTM! Clean interface extension.
The addition of canGoBack to FooterProps interface is well-typed and properly integrated with existing navigation-related properties.
72-72: LGTM! Improved navigation control logic.
The canGoBack property is effectively utilized to enhance the navigation control, ensuring that back navigation is properly restricted based on the application state.
Also applies to: 78-78
libs/application/ui-shell/src/lib/FormShell.tsx (2)
28-28: LGTM! Clean import addition.
The canGoBack utility import is properly placed with other reducer-related imports.
146-146: LGTM! Well-integrated prop passing.
The canGoBack prop is properly passed to the Screen component, utilizing the utility function with the correct parameters.
libs/application/ui-shell/src/components/Screen.tsx (2)
Line range hint 421-425: LGTM: Proper integration with ScreenFooter
The canGoBack prop is correctly passed to ScreenFooter alongside other navigation-related props, effectively implementing the navigation control requirement.
✅ Verification successful
Verified: ScreenFooter properly implements canGoBack prop
The canGoBack prop is correctly implemented in the ScreenFooter component:
- It's properly typed as a boolean in the component's props
- It's used to control the visibility of the back button through
showGoBack = canGoBack && (!isLastScreen || renderLastScreenBackButton) - The prop flows correctly from FormShell → Screen → ScreenFooter, with the value determined by the
canGoBackutility function
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify ScreenFooter's handling of canGoBack prop
ast-grep --pattern 'const ScreenFooter: FC<$_> = ({ canGoBack, $_ }) => {
$$$
}'
Length of output: 84
Script:
#!/bin/bash
# Let's try a broader search for ScreenFooter component and its props
rg -A 10 "ScreenFooter.*Props.*=.*{" --type typescript
# Also search for any usage of canGoBack prop in TypeScript files
rg "canGoBack.*=" --type typescript -A 3
Length of output: 248
Script:
#!/bin/bash
# Search for ScreenFooter component definition with fixed regex
rg -A 10 "ScreenFooter.*Props"
# Search for files containing ScreenFooter
fd ScreenFooter
# Search for canGoBack usage
rg "canGoBack"
Length of output: 1866
111-111: Verify consistent usage of canGoBack across the application system
The addition of this prop affects navigation behavior. Let's verify its implementation across different screens.
✅ Verification successful
The canGoBack prop is properly integrated across the application system
Based on the code analysis:
- The
canGoBackprop is properly defined in the Screen component's interface - It's correctly implemented in the
reducerUtils.tswith a dedicated function to determine navigation state - It's properly passed through the component hierarchy: FormShell -> Screen -> ScreenFooter
- The prop controls navigation visibility through
showGoBacklogic in ScreenFooter - The implementation follows a consistent pattern across the application system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Screen component usage to verify canGoBack prop implementation
rg -A 3 "<Screen\s" --type tsx
Length of output: 60
Script:
#!/bin/bash
# Search for Screen component usage with common TypeScript/JavaScript extensions
rg -t ts -t jsx -t tsx "<Screen\s" -A 3
# Also search for Screen imports to understand usage patterns
rg -t ts -t jsx -t tsx "import.*Screen.*from"
Length of output: 145
Script:
#!/bin/bash
# Search for Screen component usage and imports using ripgrep without file type filters
rg "<Screen\s" -A 3
# Search for Screen imports
rg "import.*Screen.*from"
# Find all potential Screen-related files
fd -e ts -e tsx -e js -e jsx Screen
Length of output: 44751
Script:
#!/bin/bash
# Let's examine the Screen component implementation and its usage of canGoBack
ast-grep --pattern 'interface $_Props {
$$$
canGoBack$_
$$$
}'
# Also check the actual Screen component implementation
ast-grep --pattern 'const Screen = ({ $$$, canGoBack, $$$ }: $_Props) => {
$$$
}'
Length of output: 156
Script:
#!/bin/bash
# Let's examine the Screen component file content
cat libs/application/ui-shell/src/components/Screen.tsx
# Also check for any direct references to canGoBack in the codebase
rg "canGoBack" -A 2 -B 2
Length of output: 16444
Fixing: Possible to go back to a section that should not be navigable
Task link
What
Shouldnt be able to press the "Til baka" button and end up on a section that has its condition be false
Why
Because currently its possible for users to go back to screens they arent meant to see.
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
New Features
canGoBackproperty to enhance navigation capabilities, allowing users to determine if they can return to the previous screen.canGoBacklogic.Bug Fixes
Chores
canGoBackto streamline navigation checks across screens.