feat(web): Level 2 navigation for parent subpages should be visible despite theme #17100
Conversation
WalkthroughThe changes in this pull request focus on enhancing the error handling logic within the Changes
Possibly related PRs
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
apps/web/pages/s/[...slugs]/index.tsx (1)
323-333: Consider refactoring duplicated error handling logicThe error handling logic for
StandaloneParentSubpage.getPropsis duplicated between the 2-segment and 3-segment URL cases. Consider extracting this into a helper function to improve maintainability.+ async function tryStandaloneParentSubpage(context) { + try { + return { + page: { + type: PageType.STANDALONE_PARENT_SUBPAGE, + props: await StandaloneParentSubpage.getProps(context), + }, + } + } catch (error) { + if (!(error instanceof CustomNextError)) { + throw error + } + return null + } + }Then use it in both places:
const standaloneResult = await tryStandaloneParentSubpage(modifiedContext) if (standaloneResult) return standaloneResult
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
apps/web/pages/s/[...slugs]/index.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/web/pages/s/[...slugs]/index.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (1)
apps/web/pages/s/[...slugs]/index.tsx (1)
323-333: Verify error handling behavior
The current implementation silently swallows CustomNextError exceptions and falls back to other page types. Please verify that this behavior is intentional and doesn't mask legitimate 404 errors or other important error states.
✅ Verification successful
Let me generate additional scripts to understand the full context of error handling in this file.
Let me generate one more script to check the error handling in the getServerSidePropsWrapper utility since it's responsible for handling errors at the top level.
Based on the comprehensive analysis of the codebase, I can now provide a final response about the error handling behavior:
The error handling behavior is intentional and properly implemented
The current implementation of swallowing CustomNextError exceptions is part of a deliberate fallback strategy where:
-
If a
CustomNextErrorwith 404 status is caught, it's handled by the top-levelgetServerSidePropsWrapperwhich:- Logs the error
- Attempts to find a redirect URL
- Falls back to Next.js's native
notFoundhandling if no redirect is found
-
The try-catch block in the page routing logic allows for graceful degradation through different page types, only proceeding if a specific page type is not found, which is a valid routing pattern.
-
All other error types (non-CustomNextError) are properly re-thrown, ensuring that actual system errors are not masked.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other instances of CustomNextError handling to understand the standard error handling pattern
# Search for CustomNextError usage patterns
echo "Searching for CustomNextError handling patterns..."
rg -A 2 "CustomNextError" --type ts
# Search for similar error handling patterns
echo "Searching for similar try-catch patterns..."
ast-grep --pattern 'try {
$$$
} catch (error) {
if (!(error instanceof $_)) {
$$$
}
}'
Length of output: 38478
Script:
#!/bin/bash
# Get the full content of the file to understand the error handling flow
echo "Checking the complete file content..."
cat "apps/web/pages/s/[...slugs]/index.tsx"
# Check for any error handling utilities or middleware
echo -e "\nChecking for error handling utilities..."
rg -g "*.ts" -g "*.tsx" "getServerSidePropsWrapper" -A 5
Length of output: 84273
Script:
#!/bin/bash
# Check the error handling implementation in getServerSidePropsWrapper
cat "apps/web/utils/getServerSidePropsWrapper.ts"
# Check for any 404 handling utilities
echo -e "\nChecking for 404 handling utilities..."
rg "404" "apps/web/utils/fetch404RedirectUrl.ts"
Length of output: 2567
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17100 +/- ##
==========================================
- Coverage 35.68% 35.68% -0.01%
==========================================
Files 6946 6946
Lines 147391 147393 +2
Branches 41897 41897
==========================================
Hits 52595 52595
- Misses 94796 94798 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
Datadog ReportBranch report: ✅ 0 Failed, 84 Passed, 0 Skipped, 24.8s Total Time |
…-level2-visible-despite-theme
kodiakhq
bot
deleted the
feature/web-organization-page-parent-subpage-level2-visible-despite-theme
branch
Level 2 navigation for parent subpages should be visible despite theme
What
Follow up PR after #17096
The previous PR didn't handle "level 2" navigation, here we are adding the same logic for those pages.
It no longer matters if the theme is set to standalone or not
Summary by CodeRabbit
These changes aim to provide a smoother user experience by addressing potential issues that may arise during page property retrieval.