Draft
Conversation
Rajat-Dabade
added
2: Dev Review
📝 WalkthroughWalkthroughThe pull request introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant Router
participant InitialLoadErrorPathFix
participant GlobalErrorRedirect
Browser->>Router: Initial page load (e.g., /some-path/error)
Router->>InitialLoadErrorPathFix: Render component on first mount
InitialLoadErrorPathFix->>InitialLoadErrorPathFix: Check if first load and path has trailing /error
alt Path has trailing /error (not already at /error)
InitialLoadErrorPathFix->>Browser: Rewrite URL to remove /error segment
Browser->>Router: Navigate to rewritten path
end
Router->>GlobalErrorRedirect: Check for global error state
alt Global error exists and not on error page
GlobalErrorRedirect->>GlobalErrorRedirect: Clear error and prepare redirect
GlobalErrorRedirect->>Browser: Redirect to /error or /access-denied
else On error page or no global error
GlobalErrorRedirect->>Router: Continue normal rendering
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip You can make CodeRabbit's review stricter and more nitpicky using the `assertive` profile, if that's what you prefer.Change the |
Contributor
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@webapp/src/router.tsx`:
- Around line 191-195: The current loop only strips trailing "/error" segments
(using validPath.endsWith('/error')), which fails for leaked basename suffixes
like "/team/.../error/plugins/focalboard/error"; change the normalization in the
router (variables: pathname, validPath, history.replace) to repeatedly remove
any "/error" segment and everything after it instead of only trimming a trailing
"/error"—e.g., while validPath.includes('/error') and validPath !== '/error',
set validPath = validPath.slice(0, validPath.indexOf('/error')) || '/'—then call
history.replace(validPath + location.search) so all leaked "/error" basename
suffixes are normalized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d8ab5086-3aba-47a9-80cd-3207a31e89a7
📒 Files selected for processing (1)
webapp/src/router.tsx
Comment on lines
+191
to
+195
| let validPath = pathname | ||
| while (validPath.endsWith('/error') && validPath !== '/error') { | ||
| validPath = validPath.replace(/\/error$/, '') || '/' | ||
| } | ||
| history.replace(validPath + location.search) |
There was a problem hiding this comment.
Normalize leaked basename suffixes, not just trailing /error.
At Line [193], the loop strips only /error. For corrupted URLs like /team/.../error/plugins/focalboard/error, this can stop at /team/.../error/plugins/focalboard, which is still invalid.
Proposed fix
useEffect(() => {
if (hasRun.current) return
hasRun.current = true
const pathname = location.pathname
+ const leakedBase = Utils.getFrontendBaseURL().replace(/^\/+|\/+$/g, '')
+ const escapedBase = leakedBase.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')
+ const basePart = leakedBase ? `${escapedBase}/` : ''
+ const trailingErrorPattern = new RegExp(`/(?:${basePart})?error$`)
+
if (pathname === '/error' || pathname === '/access-denied') return
- if (!pathname.endsWith('/error')) return
+ if (!trailingErrorPattern.test(pathname)) return
let validPath = pathname
- while (validPath.endsWith('/error') && validPath !== '/error') {
- validPath = validPath.replace(/\/error$/, '') || '/'
+ while (trailingErrorPattern.test(validPath) && validPath !== '/error') {
+ validPath = validPath.replace(trailingErrorPattern, '') || '/'
}
history.replace(validPath + location.search)
}, [location.pathname, location.search, history])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webapp/src/router.tsx` around lines 191 - 195, The current loop only strips
trailing "/error" segments (using validPath.endsWith('/error')), which fails for
leaked basename suffixes like "/team/.../error/plugins/focalboard/error"; change
the normalization in the router (variables: pathname, validPath,
history.replace) to repeatedly remove any "/error" segment and everything after
it instead of only trimming a trailing "/error"—e.g., while
validPath.includes('/error') and validPath !== '/error', set validPath =
validPath.slice(0, validPath.indexOf('/error')) || '/'—then call
history.replace(validPath + location.search) so all leaked "/error" basename
suffixes are normalized.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Findings on the Infinite Redirect Issue for Boards
While trying to reproduce the infinite redirect issue in Boards, I noticed a pattern related to URLs that end with
/error.When the page is reloaded while the URL contains
/error, the following sequence happens:Example URL:
The application error boundary catches an error and runs this redirect:
Since the path doesn’t start with /, the browser appends it to the current URL instead of treating it as a root path.
So the new URL becomes:
Route mismatch happens
This new URL does not match the
/errorroute, so the app loads BoardPage (or a similar route) with incorrect parameters.Another error is triggered
BoardPage fails again, which causes:
another error
the error boundary to run again
another redirect
URL keeps growing
Each redirect appends more segments to the path, such as:
This creates the infinite redirect loop.
Why the error boundary kept triggering
Routes like:
are matched by the BoardPage route:
In this case: cardId = "error"
BoardPage then tries to load a card with ID “error”, which does not exist.
This throws an error, which the error boundary catches again, restarting the same redirect process.
Fix
The component can be added so it runs on initial load and, when the path ends with
/error(or corrupted variants), redirects to the clean path instead of letting the app hit the error boundary with a bad URL. That breaks the loop before it can start.Before:
After:
Ticket Link
https://mattermost.atlassian.net/browse/MM-65641
Change Impact: 🟡 Medium
Reasoning: The changes introduce a new error path fixing component that runs on initial load to prevent redirect loops in board navigation. While the modification is localized to the router layer and addresses a specific infinite loop issue, it affects core routing logic and error boundary behavior that touches user-facing critical paths (board loading). The changes handle a delicate flow involving URL rewriting and redirect interception.
Regression Risk: The InitialLoadErrorPathFix component runs on every initial load and rewrites URLs ending with
/error, which could potentially interfere with legitimate error page access or other routes that coincidentally end with/error. The enhancement to GlobalErrorRedirect adds location-dependent logic for redirect decisions, which introduces additional state-based behavior that could have edge cases. There is moderate risk of affecting error handling flows in unexpected ways if edge cases in the error path clearing are not fully covered.QA Recommendation: Comprehensive manual QA is recommended, specifically testing: (1) board loading flows from various entry points (direct URLs, navigation from boards list, shared links); (2) error boundary triggering and error page display under various failure conditions; (3) URLs that legitimately end with
/erroror contain/errorin their paths; (4) navigation with query parameters to ensure they are preserved; (5) the clean redirect path behavior on cold loads versus navigations. Given this touches error handling and routing—critical paths that affect user experience—skipping manual QA poses notable risk.