Conversation
Rajat-Dabade
changed the title
| const deletedFromCategoryID = props.categoryBoards.id | ||
|
|
||
| // Clear localStorage entries for the deleted board | ||
| UserSettings.setLastBoardID(teamID, null) |
There was a problem hiding this comment.
More of the clean up: delete boardId and viewId from local storage when board is deleted
webapp/src/utils.ts
Outdated
| static getFrontendBaseURL(absolute?: boolean): string { | ||
| let frontendBaseURL = window.frontendBaseURL || Utils.getBaseURL() | ||
| // Always use /boards as the frontend base URL, never fall back to baseURL | ||
| let frontendBaseURL = window.frontendBaseURL || '/boards' |
There was a problem hiding this comment.
We do not use plugins/focalboard from frontend routes, so removed it.
There was a problem hiding this comment.
It seems that window.frontendBaseURL is already set to `/boards here:
so this or seems redundant
| let frontendBaseURL = window.frontendBaseURL || '/boards' | |
| let frontendBaseURL = window.frontendBaseURL |
There was a problem hiding this comment.
Added the ternary operator to make TypeScript happy.
Rajat-Dabade
requested review from
devinbinnie,
harshilsharma63 and
marianunez
Rajat-Dabade
added
2: Dev Review
There was a problem hiding this comment.
Filling in for QA Review:
I am not able to reproduce the original redirect problem. However, testing out general links and redirects, found 2 regressions:
-
When you link an invalid board (copied a valid board URL and changed a number), it no longer takes you to the error page but the Create Board page. The behavior should remain the same and be able to take you to a proper error page if it's not a valid board.
-
When clicking the link the bot sends when a new board has been linked to a channel, it goes to a blank page. It seems that before it would resolve properly the
/plugins/focalboard/team/paths.
Screen.Recording.2025-10-13.at.3.05.22.PM.mov
Fixed it ✅
This was the existing issue, nice catch. Fixed it ✅ Also added some optimisation for duplicate API calls when switching teams. |
marianunez
left a comment
marianunez
left a comment
There was a problem hiding this comment.
@Rajat-Dabade we need to make sure to validate the changes with more close attention as there was more routing changes introduced and now I'm seeing more regressions.
When you link an invalid board (copied a valid board URL and changed a number), it no longer takes you to the error page but the Create Board page. The behavior should remain the same and be able to take you to a proper error page if it's not a valid board.
Fixed it ✅
- It did indeed fix the issue of when navigating to an invalid path takes you to the error screen and even though when clicking
Back to Teamit restores you back to Boards, when you navigate back out to channels and back to Boards, it never recovers from that error state until you refresh. See video
Screen.Recording.2025-10-16.at.7.19.02.PM.mov
When clicking the link the bot sends when a new board has been linked to a channel, it goes to a blank page. It seems that before it would resolve properly the /plugins/focalboard/team/ paths.
This was the existing issue, nice catch. Fixed it ✅
I couldn't validate this one because now Creating a Board from a channel Link Boards to Channel is broken. It opens the create board screen with no template and none of the button work, even Create Empty Board does not work. See video
Screen.Recording.2025-10-16.at.7.21.23.PM.mov
|
Also, I noticed that a brand new user (no boards created or joined before) the first time they create a board they get an error until refresh. This only happens the first time, after they have boards it works properly. Screen.Recording.2025-10-16.at.7.27.24.PM.mov |
| } | ||
|
|
||
| serverRoot := baseURL + "/plugins/focalboard" | ||
| serverRoot := baseURL + "/boards" |
There was a problem hiding this comment.
Are we sure this works? Wondering how this used to work before this change. And we should also verify this change in desktop app as IIRC, there is some boards and playbook specific routing code in the desktop app that is dependent on the URL.
| const deletedFromCategoryID = props.categoryBoards.id | ||
|
|
||
| // Clear localStorage entries for the deleted board | ||
| UserSettings.setLastBoardID(teamID, null) |
There was a problem hiding this comment.
We should only do this if the boards being deleted is the current board. Currently if you are on Board A and Delete Board B, this clears the last board ID while in fact the last last board ID should continue pointing to Board A.
Additionally, I think this is causing one more issue where if you have Board A, B, C and D in that order, and you are on D and delete B, you end up navigating to C, but you should continue to stay on D
Screen.Recording.2025-10-17.at.8.52.17.AM.mov
|
|
||
| // Clear localStorage entries for the deleted board | ||
| UserSettings.setLastBoardID(teamID, null) | ||
| UserSettings.setLastViewId(deleteBoard.id, null) |
There was a problem hiding this comment.
Same applied to this - only delete if deleting the current view.
| octoClient.teamId = teamId | ||
| dispatch(setTeam(teamId)) | ||
| } else { | ||
| dispatch(setGlobalError('team-undefined')) |
There was a problem hiding this comment.
I'm not sure what this does, but for example if I open the url http://localhost:8065/boards/team/invalid-team-id on this branch, there are no errors and an empty template selector screen-
Whereas if I try to do the same thing on Hub, I get the screen with right error message-
| }, [teamId, match.params.boardId, viewId, me?.id, dispatch, loadAction]) | ||
|
|
||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
This creates a new bug. If you have two teams with following boards-
- Team A - Board A, B and C
- Team B - Board D, E, F
If you open team B in boards and open board E, navigate back to Channels, then back to boards, it should stay you on Board E, but currently it navigates to first board D.
Additionally, if you open Team B Board E, then switch to team A, then to team B, it ends up switching to board D but it should stay on the board which was previously open.
I'm not sure whats the need of resetting board ID on team switch when we store each team's last board ID in local storage.
|
@coderabbitai please review |
|
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughUpdates server frontend base path to /boards and adds runtime validations and error handling across the board page and redirect logic; introduces a view-not-found error, adjusts error-boundary redirect construction, persists/clears last-board/last-view user settings, and guards board/view loading flows. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
webapp/src/components/sidebar/sidebarCategory.tsx (1)
187-189:⚠️ Potential issue | 🟠 MajorClearing localStorage unconditionally on any board deletion is incorrect.
This clears the last board/view IDs regardless of which board is deleted. If the user is viewing Board A and deletes Board B, this incorrectly clears the last board ID. The localStorage should only be cleared if the deleted board is the currently active board.
As noted in a previous review, this also causes navigation issues where deleting a non-active board unexpectedly navigates away from the current board.
🐛 Proposed fix
- // Clear localStorage entries for the deleted board - UserSettings.setLastBoardID(teamID, null) - UserSettings.setLastViewId(deleteBoard.id, null) + // Clear localStorage entries only if deleting the currently active board + if (deleteBoard.id === props.activeBoardID) { + UserSettings.setLastBoardID(teamID, null) + UserSettings.setLastViewId(deleteBoard.id, null) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/components/sidebar/sidebarCategory.tsx` around lines 187 - 189, Only clear localStorage when the deleted board is the currently active one: replace the unconditional UserSettings.setLastBoardID(teamID, null) and UserSettings.setLastViewId(deleteBoard.id, null) with conditional checks that compare deleteBoard.id to the current values (e.g., UserSettings.getLastBoardID(teamID) and UserSettings.getLastViewId(teamID)) and only call setLastBoardID/teamID or setLastViewId(...) to null when the deleted board matches the stored active board/view ID.
🧹 Nitpick comments (1)
webapp/src/pages/boardPage/teamToBoardAndViewRedirect.tsx (1)
82-91: RedundantsetLastViewIdcall when using existing last view.When
selectedViewIDis found fromUserSettings.lastViewId[boardId], callingUserSettings.setLastViewId(boardId, selectedViewID)at line 84 is redundant since the value already exists in localStorage. This only needs to be called when setting a new view (line 89).♻️ Suggested simplification
let selectedViewID = UserSettings.lastViewId[boardId] if (selectedViewID) { - UserSettings.setLastViewId(boardId, selectedViewID) dispatch(setCurrentView(selectedViewID)) } else if (boardViews.length > 0) { // if most recent view is unavailable, pick the first view selectedViewID = boardViews[0].id UserSettings.setLastViewId(boardId, selectedViewID) dispatch(setCurrentView(selectedViewID)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/pages/boardPage/teamToBoardAndViewRedirect.tsx` around lines 82 - 91, The code redundantly calls UserSettings.setLastViewId(boardId, selectedViewID) when selectedViewID already comes from UserSettings.lastViewId; remove that call inside the if (selectedViewID) branch and only call UserSettings.setLastViewId when you assign a new selectedViewID from boardViews (the else branch). Keep dispatch(setCurrentView(selectedViewID)) in both branches and ensure setLastViewId is invoked only when you create/override selectedViewID (refer to UserSettings, setLastViewId, setCurrentView, selectedViewID, and boardViews).
🤖 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/errors.ts`:
- Around line 70-77: The "Back to board" button in the ErrorId.ViewNotFound
block currently sets button1Redirect = '/' which sends users to root; change it
to compute the board redirect like the other error handlers by extracting
boardId from URL params and setting button1Redirect to the board path (e.g.,
`/b/${boardId}`) instead of '/'; update the ErrorId.ViewNotFound branch where
errDef.button1Redirect is assigned so it uses the same helper/function used
elsewhere to build the board redirect (keep errDef.title, button1Enabled,
button1Text, and button1Fill as-is).
In `@webapp/src/pages/boardPage/boardPage.tsx`:
- Around line 265-270: Replace the semantically confusing use of
Constants.globalTeamId when checking view IDs: either add a new constant (e.g.,
Constants.globalViewId = '0') and use that in the conditional (replace
Constants.globalTeamId with Constants.globalViewId in the block that calls
dispatch(setCurrentView(viewId)) and
UserSettings.setLastViewId(match.params.boardId, viewId)), or if adding a
constant is not desired, add a one-line clarifying comment above the if (viewId
!== Constants.globalTeamId) explaining that '0' is the global/no-specific-view
sentinel and therefore should be skipped; ensure the symbols viewId,
setCurrentView, UserSettings.setLastViewId and match.params.boardId are left
intact.
In `@webapp/src/pages/boardPage/teamToBoardAndViewRedirect.tsx`:
- Line 66: The generated path is using the wrong param name: replace the
uppercase viewID with the lowercase viewId when calling generatePath so it
matches route params; update the call that builds newPath
(generatePath(Utils.getBoardPagePath(match.path), {...match.params, boardId:
boardID, viewId: undefined})) to use viewId instead of viewID so the view
parameter is properly cleared.
---
Duplicate comments:
In `@webapp/src/components/sidebar/sidebarCategory.tsx`:
- Around line 187-189: Only clear localStorage when the deleted board is the
currently active one: replace the unconditional
UserSettings.setLastBoardID(teamID, null) and
UserSettings.setLastViewId(deleteBoard.id, null) with conditional checks that
compare deleteBoard.id to the current values (e.g.,
UserSettings.getLastBoardID(teamID) and UserSettings.getLastViewId(teamID)) and
only call setLastBoardID/teamID or setLastViewId(...) to null when the deleted
board matches the stored active board/view ID.
---
Nitpick comments:
In `@webapp/src/pages/boardPage/teamToBoardAndViewRedirect.tsx`:
- Around line 82-91: The code redundantly calls
UserSettings.setLastViewId(boardId, selectedViewID) when selectedViewID already
comes from UserSettings.lastViewId; remove that call inside the if
(selectedViewID) branch and only call UserSettings.setLastViewId when you assign
a new selectedViewID from boardViews (the else branch). Keep
dispatch(setCurrentView(selectedViewID)) in both branches and ensure
setLastViewId is invoked only when you create/override selectedViewID (refer to
UserSettings, setLastViewId, setCurrentView, selectedViewID, and boardViews).
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
server/boards/boardsapp_util.gowebapp/src/components/sidebar/sidebarCategory.tsxwebapp/src/error_boundary.tsxwebapp/src/errors.tswebapp/src/pages/boardPage/boardPage.tsxwebapp/src/pages/boardPage/teamToBoardAndViewRedirect.tsxwebapp/src/utils.ts
| case ErrorId.ViewNotFound: { | ||
| errDef.title = intl.formatMessage({id: 'error.view-not-found', defaultMessage: 'View not found.'}) | ||
| errDef.button1Enabled = true | ||
| errDef.button1Text = intl.formatMessage({id: 'error.back-to-board', defaultMessage: 'Back to board'}) | ||
| errDef.button1Redirect = '/' | ||
| errDef.button1Fill = true | ||
| break | ||
| } |
There was a problem hiding this comment.
Inconsistent redirect path for "Back to board" button.
The button text says "Back to board" but button1Redirect is set to '/', which redirects to the root/home. For consistency with the user expectation, this should redirect back to the board (without the invalid viewId) rather than the root.
Consider using a function redirect similar to other error handlers to extract the boardId from URL params:
💡 Suggested approach
case ErrorId.ViewNotFound: {
errDef.title = intl.formatMessage({id: 'error.view-not-found', defaultMessage: 'View not found.'})
errDef.button1Enabled = true
errDef.button1Text = intl.formatMessage({id: 'error.back-to-board', defaultMessage: 'Back to board'})
- errDef.button1Redirect = '/'
+ errDef.button1Redirect = (params: URLSearchParams): string => {
+ const boardId = params.get('boardId')
+ const teamId = params.get('teamId')
+ if (teamId && boardId) {
+ return `/team/${teamId}/${boardId}`
+ }
+ return '/'
+ }
errDef.button1Fill = true
break
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webapp/src/errors.ts` around lines 70 - 77, The "Back to board" button in the
ErrorId.ViewNotFound block currently sets button1Redirect = '/' which sends
users to root; change it to compute the board redirect like the other error
handlers by extracting boardId from URL params and setting button1Redirect to
the board path (e.g., `/b/${boardId}`) instead of '/'; update the
ErrorId.ViewNotFound branch where errDef.button1Redirect is assigned so it uses
the same helper/function used elsewhere to build the board redirect (keep
errDef.title, button1Enabled, button1Text, and button1Fill as-is).
| if (viewId !== Constants.globalTeamId) { | ||
| dispatch(setCurrentView(viewId)) | ||
| if (viewId) { | ||
| UserSettings.setLastViewId(match.params.boardId, viewId) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find Constants.globalTeamId definition
rg "Constants\s*=" --type ts --type tsx -A 20 | head -100
echo "---"
# Find globalTeamId definition
rg "globalTeamId" --type ts --type tsx -B 2 -A 2Repository: mattermost/mattermost-plugin-boards
Length of output: 155
🏁 Script executed:
#!/bin/bash
# Read the specific file around lines 265-270 to see the context
cat -n webapp/src/pages/boardPage/boardPage.tsx | sed -n '250,280p'Repository: mattermost/mattermost-plugin-boards
Length of output: 1067
🏁 Script executed:
#!/bin/bash
# Find Constants file
fd -e ts -e tsx "Constants" --type fRepository: mattermost/mattermost-plugin-boards
Length of output: 61
🏁 Script executed:
#!/bin/bash
# Search for Constants definition more broadly
rg "globalTeamId" -B 3 -A 3 --max-count 20Repository: mattermost/mattermost-plugin-boards
Length of output: 11874
🏁 Script executed:
#!/bin/bash
# Find Constants file
find . -name "Constants*" -o -name "constants*" | grep -E "\.(ts|js)$"Repository: mattermost/mattermost-plugin-boards
Length of output: 104
🏁 Script executed:
#!/bin/bash
# Look at the imports in boardPage.tsx to find where Constants comes from
head -50 webapp/src/pages/boardPage/boardPage.tsx | grep -E "import.*Constants"Repository: mattermost/mattermost-plugin-boards
Length of output: 61
🏁 Script executed:
#!/bin/bash
# Search for Constants export or definition
rg "export.*Constants|const Constants" --max-count 20Repository: mattermost/mattermost-plugin-boards
Length of output: 133
🏁 Script executed:
#!/bin/bash
# Check if globalViewId constant exists
rg "globalViewId" --max-count 10Repository: mattermost/mattermost-plugin-boards
Length of output: 61
🏁 Script executed:
#!/bin/bash
# Check the constants file to see what constants are defined
cat -n webapp/src/constants.ts | head -60Repository: mattermost/mattermost-plugin-boards
Length of output: 2540
🏁 Script executed:
#!/bin/bash
# Continue reading constants file to find globalTeamId and related constants
cat -n webapp/src/constants.ts | grep -A 5 -B 5 "globalTeamId"Repository: mattermost/mattermost-plugin-boards
Length of output: 353
🏁 Script executed:
#!/bin/bash
# Check if there's a globalViewId or similar constant, or search for '0' usage in views
rg "setCurrentView|activeViewId|currentViewId" --max-count 15 -B 2 -A 2Repository: mattermost/mattermost-plugin-boards
Length of output: 7051
🏁 Script executed:
#!/bin/bash
# Check if there are any view-related default constants
rg "readonly.*=.*'0'" webapp/src/constants.ts -B 2 -A 2Repository: mattermost/mattermost-plugin-boards
Length of output: 207
🏁 Script executed:
#!/bin/bash
# Check what viewId types are in the store/views
rg "interface.*View|type.*View" webapp/src/store/views.ts -A 10 | head -50Repository: mattermost/mattermost-plugin-boards
Length of output: 355
🏁 Script executed:
#!/bin/bash
# Look at the full context of the effect in boardPage.tsx around line 265
cat -n webapp/src/pages/boardPage/boardPage.tsx | sed -n '259,280p'Repository: mattermost/mattermost-plugin-boards
Length of output: 816
🏁 Script executed:
#!/bin/bash
# Check if viewId being undefined is expected and how it should be handled
rg "match.params.viewId" webapp/src/pages/boardPage/ -B 3 -A 3Repository: mattermost/mattermost-plugin-boards
Length of output: 4929
🏁 Script executed:
#!/bin/bash
# Check how '0' is used in view contexts throughout the codebase
rg "viewId.*===.*'0'|'0'.*viewId" -B 3 -A 3Repository: mattermost/mattermost-plugin-boards
Length of output: 864
🏁 Script executed:
#!/antml:thinking>
Actually, I think I have enough information now. Let me analyze:
1. Constants.globalTeamId = '0'
2. viewId comes from match.params.viewId (a URL parameter)
3. The check `if (viewId !== Constants.globalTeamId)` means `if (viewId !== '0')`
4. In backward compatibility code, '0' was explicitly used as a viewId placeholder
5. There is NO Constants.globalViewId - the codebase uses '0' as the default for both teams and views
So the original review comment is PARTIALLY correct:
- The comparison IS semantically confusing (using a "Team" constant for views)
- BUT it's functionally correct - '0' IS intentionally used as a default/global view ID
- The concern about it being a "no-op guard" is incorrect - it does filter out '0' viewIds
The real issue is:
1. Code clarity: using Constants.globalTeamId for view validation is confusing
2. Should have Constants.globalViewId OR use a magic string '0' with a comment
3. The logic itself is correct
This is a MINOR issue - more about code clarity/maintainability than a functional bug.
</thinking>Repository: mattermost/mattermost-plugin-boards
Length of output: 313
🏁 Script executed:
#!/bin/bash
# Check the full view validation effect to understand the complete logic
cat -n webapp/src/pages/boardPage/boardPage.tsx | sed -n '304,320p'Repository: mattermost/mattermost-plugin-boards
Length of output: 864
Use a dedicated constant for view IDs or add a clarifying comment.
The condition viewId !== Constants.globalTeamId works correctly (comparing against '0'), but is semantically confusing: globalTeamId is a team-scoped constant being used to validate view IDs. While '0' is intentionally used as a default ID across teams, boards, and views, the code would be clearer with either a Constants.globalViewId constant or an explanatory comment (e.g., // Skip setting view if no specific view ID provided).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webapp/src/pages/boardPage/boardPage.tsx` around lines 265 - 270, Replace the
semantically confusing use of Constants.globalTeamId when checking view IDs:
either add a new constant (e.g., Constants.globalViewId = '0') and use that in
the conditional (replace Constants.globalTeamId with Constants.globalViewId in
the block that calls dispatch(setCurrentView(viewId)) and
UserSettings.setLastViewId(match.params.boardId, viewId)), or if adding a
constant is not desired, add a one-line clarifying comment above the if (viewId
!== Constants.globalTeamId) explaining that '0' is the global/no-specific-view
sentinel and therefore should be skipped; ensure the symbols viewId,
setCurrentView, UserSettings.setLastViewId and match.params.boardId are left
intact.
|
@coderabbitai do a full review and include the summary in the PR description |
|
I'll review all 7 modified files and provide detailed feedback. ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
webapp/src/errors.ts (1)
70-75:⚠️ Potential issue | 🟡 Minor
ViewNotFound“Back to board” currently routes to home.Line 74 sets
button1Redirectto'/', so users can lose board context instead of returning to the board.🔧 Suggested fix
case ErrorId.ViewNotFound: { errDef.title = intl.formatMessage({id: 'error.view-not-found', defaultMessage: 'View not found.'}) errDef.button1Enabled = true errDef.button1Text = intl.formatMessage({id: 'error.back-to-board', defaultMessage: 'Back to board'}) - errDef.button1Redirect = '/' + errDef.button1Redirect = (params: URLSearchParams): string => { + const teamId = params.get('teamId') + const boardId = params.get('boardId') + if (teamId && boardId) { + return `/team/${teamId}/${boardId}` + } + return '/' + } errDef.button1Fill = true break }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/errors.ts` around lines 70 - 75, The ViewNotFound error currently redirects users to '/' which loses board context; update the ErrorId.ViewNotFound handling so errDef.button1Redirect uses the board-specific return target instead of '/', e.g. derive the board path from available context (boardId/route params) or use the app's previous-board/router-back mechanism; modify the block that sets errDef.button1Redirect (in the ViewNotFound case) to compute and assign the correct board URL or back action so the "Back to board" button returns users to their originating board.webapp/src/pages/boardPage/teamToBoardAndViewRedirect.tsx (1)
66-67:⚠️ Potential issue | 🟡 MinorUse
viewId(lowercase) when clearing the route param.Line 66 uses
viewID, so the intendedviewIdparam is not actually cleared.🔧 Suggested fix
- const newPath = generatePath(Utils.getBoardPagePath(match.path), {...match.params, boardId: boardID, viewID: undefined}) + const newPath = generatePath(Utils.getBoardPagePath(match.path), {...match.params, boardId: boardID, viewId: undefined})#!/bin/bash # Verify incorrect key usage in this file. rg -nP --type=tsx '\bviewID\b|\bviewId\b' webapp/src/pages/boardPage/teamToBoardAndViewRedirect.tsx🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/pages/boardPage/teamToBoardAndViewRedirect.tsx` around lines 66 - 67, The route param is being cleared with the wrong key name; in the call that constructs newPath (using generatePath and Utils.getBoardPagePath) replace the incorrect match.params override of viewID with the lowercase viewId so the view route param is actually cleared—i.e., update the object spread {...match.params, boardId: boardID, viewID: undefined} to use viewId: undefined while keeping generatePath, Utils.getBoardPagePath, match.params, boardID and history.replace untouched.webapp/src/components/sidebar/sidebarCategory.tsx (1)
187-189:⚠️ Potential issue | 🟠 MajorClear
lastBoardIDconditionally to avoid wiping valid state.Line 188 always clears the team’s last board, even when deleting a different board. That can break subsequent redirects/history for the current team.
🔧 Suggested fix
- UserSettings.setLastBoardID(teamID, null) + if (UserSettings.lastBoardId[teamID] === deleteBoard.id) { + UserSettings.setLastBoardID(teamID, null) + } UserSettings.setLastViewId(deleteBoard.id, null)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/components/sidebar/sidebarCategory.tsx` around lines 187 - 189, The current code unconditionally calls UserSettings.setLastBoardID(teamID, null) which wipes the team’s stored last-board even when a different board is deleted; change this to first read the stored value with UserSettings.getLastBoardID(teamID) and only clear it (call setLastBoardID(teamID, null)) if that stored ID equals deleteBoard.id. Do the same defensive check for the view: call UserSettings.getLastViewId(deleteBoard.id) and only call UserSettings.setLastViewId(deleteBoard.id, null) if the stored view id matches the one you intend to remove. This preserves other teams’/boards’ state while still clearing state for the actually deleted board.
🤖 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/pages/boardPage/boardPage.tsx`:
- Around line 259-273: The effect in boardPage.tsx currently mixes view-related
logic with data loading so that changes to viewId re-trigger
dispatch(loadAction(match.params.boardId)); split this into two effects: keep
the existing useEffect (the one that calls dispatch(setCurrentBoard(...)),
dispatch(setCurrentView(...)), and UserSettings.setLastViewId(...)) to depend on
viewId/teamId/match.params.boardId as needed, and move
dispatch(loadAction(match.params.boardId)) into a separate effect that only
depends on match.params.boardId (and teamId/me?.id/dispatch/loadAction as
required) so switching views no longer causes loadAction to run. Ensure
loadAction is stable (memoized) or included in the new effect’s dependency
array.
- Around line 113-119: The validity check is using match.params.teamId instead
of the already-resolved teamId, causing valid fallback routes to be treated as
invalid; update the condition in the block that computes isValidTeam to compare
against the resolved teamId variable (and Constants.globalTeamId) and then
proceed to set UserSettings.lastTeamId, octoClient.teamId and
dispatch(setTeam(teamId)) when that resolved teamId is valid, otherwise dispatch
the error as before; look for references to match.params.teamId and teamId in
this component (e.g., isValidTeam, UserSettings.lastTeamId, octoClient.teamId,
dispatch(setTeam)) and replace the former with the resolved teamId in the
validation.
In `@webapp/src/pages/boardPage/teamToBoardAndViewRedirect.tsx`:
- Around line 82-90: The persisted UserSettings.lastViewId[boardId] must be
validated against the current boardViews before using it; update the logic in
teamToBoardAndViewRedirect so that when you read selectedViewID from
UserSettings.lastViewId[boardId] you check that boardViews.some(v => v.id ===
selectedViewID) (or equivalent) before calling UserSettings.setLastViewId and
dispatch(setCurrentView); if the persisted id is missing or not found, fall back
to the first available view (boardViews[0].id), set it via
UserSettings.setLastViewId(boardId, selectedViewID) and
dispatch(setCurrentView(selectedViewID)) instead.
---
Duplicate comments:
In `@webapp/src/components/sidebar/sidebarCategory.tsx`:
- Around line 187-189: The current code unconditionally calls
UserSettings.setLastBoardID(teamID, null) which wipes the team’s stored
last-board even when a different board is deleted; change this to first read the
stored value with UserSettings.getLastBoardID(teamID) and only clear it (call
setLastBoardID(teamID, null)) if that stored ID equals deleteBoard.id. Do the
same defensive check for the view: call
UserSettings.getLastViewId(deleteBoard.id) and only call
UserSettings.setLastViewId(deleteBoard.id, null) if the stored view id matches
the one you intend to remove. This preserves other teams’/boards’ state while
still clearing state for the actually deleted board.
In `@webapp/src/errors.ts`:
- Around line 70-75: The ViewNotFound error currently redirects users to '/'
which loses board context; update the ErrorId.ViewNotFound handling so
errDef.button1Redirect uses the board-specific return target instead of '/',
e.g. derive the board path from available context (boardId/route params) or use
the app's previous-board/router-back mechanism; modify the block that sets
errDef.button1Redirect (in the ViewNotFound case) to compute and assign the
correct board URL or back action so the "Back to board" button returns users to
their originating board.
In `@webapp/src/pages/boardPage/teamToBoardAndViewRedirect.tsx`:
- Around line 66-67: The route param is being cleared with the wrong key name;
in the call that constructs newPath (using generatePath and
Utils.getBoardPagePath) replace the incorrect match.params override of viewID
with the lowercase viewId so the view route param is actually cleared—i.e.,
update the object spread {...match.params, boardId: boardID, viewID: undefined}
to use viewId: undefined while keeping generatePath, Utils.getBoardPagePath,
match.params, boardID and history.replace untouched.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
server/boards/boardsapp_util.gowebapp/src/components/sidebar/sidebarCategory.tsxwebapp/src/error_boundary.tsxwebapp/src/errors.tswebapp/src/pages/boardPage/boardPage.tsxwebapp/src/pages/boardPage/teamToBoardAndViewRedirect.tsxwebapp/src/utils.ts
| const isValidTeam = allTeams.some((team) => team.id === match.params.teamId) || match.params.teamId === Constants.globalTeamId | ||
| if (isValidTeam) { | ||
| UserSettings.lastTeamId = teamId | ||
| octoClient.teamId = teamId | ||
| dispatch(setTeam(teamId)) | ||
| } else { | ||
| dispatch(setGlobalError('team-undefined')) |
There was a problem hiding this comment.
Validate against resolved teamId, not only match.params.teamId.
Line 113 can mark valid fallback routes as invalid (and set team-undefined) when teamId comes from UserSettings.lastTeamId or Constants.globalTeamId.
🔧 Suggested fix
- const isValidTeam = allTeams.some((team) => team.id === match.params.teamId) || match.params.teamId === Constants.globalTeamId
+ const isValidTeam = (
+ teamId === Constants.globalTeamId ||
+ allTeams.some((team) => team.id === teamId)
+ )📝 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.
| const isValidTeam = allTeams.some((team) => team.id === match.params.teamId) || match.params.teamId === Constants.globalTeamId | |
| if (isValidTeam) { | |
| UserSettings.lastTeamId = teamId | |
| octoClient.teamId = teamId | |
| dispatch(setTeam(teamId)) | |
| } else { | |
| dispatch(setGlobalError('team-undefined')) | |
| const isValidTeam = ( | |
| teamId === Constants.globalTeamId || | |
| allTeams.some((team) => team.id === teamId) | |
| ) | |
| if (isValidTeam) { | |
| UserSettings.lastTeamId = teamId | |
| octoClient.teamId = teamId | |
| dispatch(setTeam(teamId)) | |
| } else { | |
| dispatch(setGlobalError('team-undefined')) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webapp/src/pages/boardPage/boardPage.tsx` around lines 113 - 119, The
validity check is using match.params.teamId instead of the already-resolved
teamId, causing valid fallback routes to be treated as invalid; update the
condition in the block that computes isValidTeam to compare against the resolved
teamId variable (and Constants.globalTeamId) and then proceed to set
UserSettings.lastTeamId, octoClient.teamId and dispatch(setTeam(teamId)) when
that resolved teamId is valid, otherwise dispatch the error as before; look for
references to match.params.teamId and teamId in this component (e.g.,
isValidTeam, UserSettings.lastTeamId, octoClient.teamId, dispatch(setTeam)) and
replace the former with the resolved teamId in the validation.
| useEffect(() => { | ||
| dispatch(loadAction(match.params.boardId)) | ||
| if (!match.params.boardId) { | ||
| return | ||
| } | ||
| dispatch(setCurrentBoard(match.params.boardId)) | ||
|
|
||
| if (match.params.boardId) { | ||
| // set the active board | ||
| dispatch(setCurrentBoard(match.params.boardId)) | ||
|
|
||
| if (viewId !== Constants.globalTeamId) { | ||
| // reset current, even if empty string | ||
| dispatch(setCurrentView(viewId)) | ||
| if (viewId) { | ||
| // don't reset per board if empty string | ||
| UserSettings.setLastViewId(match.params.boardId, viewId) | ||
| } | ||
| if (viewId !== Constants.globalTeamId) { | ||
| dispatch(setCurrentView(viewId)) | ||
| if (viewId) { | ||
| UserSettings.setLastViewId(match.params.boardId, viewId) | ||
| } | ||
| } | ||
| }, [teamId, match.params.boardId, viewId, me?.id]) | ||
|
|
||
| dispatch(loadAction(match.params.boardId)) | ||
| }, [teamId, match.params.boardId, viewId, me?.id, dispatch, loadAction]) |
There was a problem hiding this comment.
loadAction is triggered on view changes due to effect coupling.
Because viewId is in the dependency list and Line 272 is inside the same effect, switching views can re-dispatch loadAction(match.params.boardId), causing unnecessary reload/API churn.
🔧 Suggested refactor
- useEffect(() => {
- if (!match.params.boardId) {
- return
- }
- dispatch(setCurrentBoard(match.params.boardId))
-
- if (viewId !== Constants.globalTeamId) {
- dispatch(setCurrentView(viewId))
- if (viewId) {
- UserSettings.setLastViewId(match.params.boardId, viewId)
- }
- }
-
- dispatch(loadAction(match.params.boardId))
- }, [teamId, match.params.boardId, viewId, me?.id, dispatch, loadAction])
+ useEffect(() => {
+ if (!match.params.boardId) {
+ return
+ }
+ dispatch(setCurrentBoard(match.params.boardId))
+ dispatch(loadAction(match.params.boardId))
+ }, [match.params.boardId, dispatch, loadAction])
+
+ useEffect(() => {
+ if (!match.params.boardId) {
+ return
+ }
+ if (viewId !== Constants.globalTeamId) {
+ dispatch(setCurrentView(viewId))
+ if (viewId) {
+ UserSettings.setLastViewId(match.params.boardId, viewId)
+ }
+ }
+ }, [match.params.boardId, viewId, dispatch])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webapp/src/pages/boardPage/boardPage.tsx` around lines 259 - 273, The effect
in boardPage.tsx currently mixes view-related logic with data loading so that
changes to viewId re-trigger dispatch(loadAction(match.params.boardId)); split
this into two effects: keep the existing useEffect (the one that calls
dispatch(setCurrentBoard(...)), dispatch(setCurrentView(...)), and
UserSettings.setLastViewId(...)) to depend on viewId/teamId/match.params.boardId
as needed, and move dispatch(loadAction(match.params.boardId)) into a separate
effect that only depends on match.params.boardId (and
teamId/me?.id/dispatch/loadAction as required) so switching views no longer
causes loadAction to run. Ensure loadAction is stable (memoized) or included in
the new effect’s dependency array.
| let selectedViewID = UserSettings.lastViewId[boardId] | ||
| if (selectedViewID) { | ||
| UserSettings.setLastViewId(boardId, selectedViewID) | ||
| dispatch(setCurrentView(selectedViewID)) | ||
| } else if (boardViews.length > 0) { | ||
| // if most recent view is unavailable, pick the first view | ||
| viewID = boardViews[0].id | ||
| UserSettings.setLastViewId(boardID, viewID) | ||
| dispatch(setCurrentView(viewID)) | ||
| selectedViewID = boardViews[0].id | ||
| UserSettings.setLastViewId(boardId, selectedViewID) | ||
| dispatch(setCurrentView(selectedViewID)) |
There was a problem hiding this comment.
Validate persisted lastViewId before using it.
If local storage contains a stale view ID, this code dispatches/redirects to a non-existent view instead of falling back to a valid one.
🔧 Suggested fix
- let selectedViewID = UserSettings.lastViewId[boardId]
- if (selectedViewID) {
- UserSettings.setLastViewId(boardId, selectedViewID)
- dispatch(setCurrentView(selectedViewID))
- } else if (boardViews.length > 0) {
+ let selectedViewID = UserSettings.lastViewId[boardId]
+ if (selectedViewID && !boardViews.some((view) => view.id === selectedViewID)) {
+ UserSettings.setLastViewId(boardId, null)
+ selectedViewID = undefined
+ }
+
+ if (selectedViewID) {
+ UserSettings.setLastViewId(boardId, selectedViewID)
+ dispatch(setCurrentView(selectedViewID))
+ } else if (boardViews.length > 0) {
// if most recent view is unavailable, pick the first view
selectedViewID = boardViews[0].id
UserSettings.setLastViewId(boardId, selectedViewID)
dispatch(setCurrentView(selectedViewID))
}📝 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.
| let selectedViewID = UserSettings.lastViewId[boardId] | |
| if (selectedViewID) { | |
| UserSettings.setLastViewId(boardId, selectedViewID) | |
| dispatch(setCurrentView(selectedViewID)) | |
| } else if (boardViews.length > 0) { | |
| // if most recent view is unavailable, pick the first view | |
| viewID = boardViews[0].id | |
| UserSettings.setLastViewId(boardID, viewID) | |
| dispatch(setCurrentView(viewID)) | |
| selectedViewID = boardViews[0].id | |
| UserSettings.setLastViewId(boardId, selectedViewID) | |
| dispatch(setCurrentView(selectedViewID)) | |
| let selectedViewID = UserSettings.lastViewId[boardId] | |
| if (selectedViewID && !boardViews.some((view) => view.id === selectedViewID)) { | |
| UserSettings.setLastViewId(boardId, null) | |
| selectedViewID = undefined | |
| } | |
| if (selectedViewID) { | |
| UserSettings.setLastViewId(boardId, selectedViewID) | |
| dispatch(setCurrentView(selectedViewID)) | |
| } else if (boardViews.length > 0) { | |
| // if most recent view is unavailable, pick the first view | |
| selectedViewID = boardViews[0].id | |
| UserSettings.setLastViewId(boardId, selectedViewID) | |
| dispatch(setCurrentView(selectedViewID)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webapp/src/pages/boardPage/teamToBoardAndViewRedirect.tsx` around lines 82 -
90, The persisted UserSettings.lastViewId[boardId] must be validated against the
current boardViews before using it; update the logic in
teamToBoardAndViewRedirect so that when you read selectedViewID from
UserSettings.lastViewId[boardId] you check that boardViews.some(v => v.id ===
selectedViewID) (or equivalent) before calling UserSettings.setLastViewId and
dispatch(setCurrentView); if the persisted id is missing or not found, fall back
to the first available view (boardViews[0].id), set it via
UserSettings.setLastViewId(boardId, selectedViewID) and
dispatch(setCurrentView(selectedViewID)) instead.
4 participants
Summary
I was not able to reproduce this issue locally, but suspect the following:
When we open the board in a new tab, and due to some reason, any error (this is still unknown, but might be related to the board not being found in the current team) occurs, this is what happens:
http://domain/plugins/focalboard/error?id=unknownSolution:
Changing to
Utils.getFrontendBaseURL(true)ensures:Ticket Link
https://mattermost.atlassian.net/browse/MM-65641
Summary
This PR fixes a critical redirect loop issue where opening a board in a new tab produces infinite redirects between
/plugins/focalboard/errorand/boards/error. The fix updates the ErrorBoundary component to usegetFrontendBaseURL(true)instead ofgetBaseURL(true), ensuring the error URL is treated as a modern route rather than triggering a legacy redirect.Key Changes:
Server Configuration – Updated serverRoot construction from
baseURL + "/plugins/focalboard"tobaseURL + "/boards"Error Handling & Routing
ViewNotFounderror case for invalid view IDsboardPage.tsxto verify team, board, and view existence before loadingBoard & View Selection – Refactored
teamToBoardAndViewRedirect.tsxlogic to:Local Storage Cleanup – Clear cached board/view IDs when deleting a board to prevent orphaned references
Board Navigation – Enhanced
showBoardutility to save last viewed board in localStorage by teamChange Impact: High
Regression Risk: HIGH – Changes modify core routing logic, error boundary behavior, and board/team validation across multiple layers. New validation paths may have untested edge cases, particularly around team switching and board resolution.
High-Risk Flows Affected:
Blast Radius: Wide – Affects all users navigating between boards and teams; touches shared navigation utilities, state management selectors, and error handling that impact fundamental application workflow.
Reasoning: This change addresses a critical user-facing bug (redirect loop) but introduces extensive validation logic across multiple core components (error boundary, board page routing, board/view selection). While these changes fix known issues, they substantially alter navigation behavior and error recovery paths, creating potential for new edge cases in untested scenarios (particularly noted: first-time board creation issues).