-
Notifications
You must be signed in to change notification settings - Fork 46
Fix: Board redirect on itself issue #140
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
base: main
Are you sure you want to change the base?
Changes from all commits
ca034e7
a9d4dde
a1b8d16
4d16e4e
6ad75ce
c164249
efaf170
208a487
d931c04
7c7ca26
ed386d2
e37580a
97083d2
8f48757
47d8958
e46cad3
3e0d08c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,6 +39,7 @@ import {TOUR_SIDEBAR, SidebarTourSteps, TOUR_BOARD, FINISHED} from '../../compon | |
| import telemetryClient, {TelemetryActions, TelemetryCategory} from '../../telemetry/telemetryClient' | ||
|
|
||
| import {getCurrentTeam} from '../../store/teams' | ||
| import {UserSettings} from '../../userSettings' | ||
|
|
||
| import ConfirmationDialogBox, {ConfirmationDialogBoxProps} from '../confirmationDialogBox' | ||
|
|
||
|
|
@@ -183,6 +184,10 @@ const SidebarCategory = (props: Props) => { | |
| // Capture the category ID before deletion | ||
| const deletedFromCategoryID = props.categoryBoards.id | ||
|
|
||
| // Clear localStorage entries for the deleted board | ||
| UserSettings.setLastBoardID(teamID, null) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. More of the clean up: delete boardId and viewId from local storage when board is deleted
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| UserSettings.setLastViewId(deleteBoard.id, null) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same applied to this - only delete if deleting the current view. |
||
|
|
||
| mutator.deleteBoard( | ||
| deleteBoard, | ||
| intl.formatMessage({id: 'Sidebar.delete-board', defaultMessage: 'Delete board'}), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ enum ErrorId { | |
| NotLoggedIn = 'not-logged-in', | ||
| InvalidReadOnlyBoard = 'invalid-read-only-board', | ||
| BoardNotFound = 'board-not-found', | ||
| ViewNotFound = 'view-not-found', | ||
| } | ||
|
|
||
| type ErrorDef = { | ||
|
|
@@ -66,6 +67,14 @@ function errorDefFromId(id: ErrorId | null): ErrorDef { | |
| errDef.button1Fill = true | ||
| break | ||
| } | ||
| 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 | ||
| } | ||
|
Comment on lines
+70
to
+77
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inconsistent redirect path for "Back to board" button. The button text says "Back to board" but 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 |
||
| case ErrorId.NotLoggedIn: { | ||
| errDef.title = intl.formatMessage({id: 'error.not-logged-in', defaultMessage: 'Your session may have expired or you\'re not logged in. Log in again to access Boards.'}) | ||
| errDef.button1Enabled = true | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -27,12 +27,13 @@ import { | |||||||||||||||||||||||||||||||||||
| setCurrent as setCurrentBoard, | ||||||||||||||||||||||||||||||||||||
| fetchBoardMembers, | ||||||||||||||||||||||||||||||||||||
| addMyBoardMemberships, | ||||||||||||||||||||||||||||||||||||
| getBoards, | ||||||||||||||||||||||||||||||||||||
| } from '../../store/boards' | ||||||||||||||||||||||||||||||||||||
| import {getCurrentViewId, setCurrent as setCurrentView, updateViews} from '../../store/views' | ||||||||||||||||||||||||||||||||||||
| import {getCurrentViewId, getCurrentBoardViews, setCurrent as setCurrentView, updateViews} from '../../store/views' | ||||||||||||||||||||||||||||||||||||
| import ConfirmationDialog from '../../components/confirmationDialogBox' | ||||||||||||||||||||||||||||||||||||
| import {initialLoad, initialReadOnlyLoad, loadBoardData} from '../../store/initialLoad' | ||||||||||||||||||||||||||||||||||||
| import {useAppSelector, useAppDispatch} from '../../store/hooks' | ||||||||||||||||||||||||||||||||||||
| import {setTeam} from '../../store/teams' | ||||||||||||||||||||||||||||||||||||
| import {getAllTeams, setTeam} from '../../store/teams' | ||||||||||||||||||||||||||||||||||||
| import {updateCards} from '../../store/cards' | ||||||||||||||||||||||||||||||||||||
| import {updateComments} from '../../store/comments' | ||||||||||||||||||||||||||||||||||||
| import {updateAttachments} from '../../store/attachments' | ||||||||||||||||||||||||||||||||||||
|
|
@@ -82,6 +83,10 @@ const BoardPage = (props: Props): JSX.Element => { | |||||||||||||||||||||||||||||||||||
| const category = useAppSelector(getCategoryOfBoard(activeBoardId)) | ||||||||||||||||||||||||||||||||||||
| const [showJoinBoardDialog, setShowJoinBoardDialog] = useState<boolean>(false) | ||||||||||||||||||||||||||||||||||||
| const history = useHistory() | ||||||||||||||||||||||||||||||||||||
| const allTeams = useAppSelector(getAllTeams) | ||||||||||||||||||||||||||||||||||||
| const allBoards = useAppSelector(getBoards) | ||||||||||||||||||||||||||||||||||||
| const boardViews = useAppSelector(getCurrentBoardViews) | ||||||||||||||||||||||||||||||||||||
| const [loadedBoardId, setLoadedBoardId] = useState<string>('') | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| // if we're in a legacy route and not showing a shared board, | ||||||||||||||||||||||||||||||||||||
| // redirect to the new URL schema equivalent | ||||||||||||||||||||||||||||||||||||
|
|
@@ -102,10 +107,18 @@ const BoardPage = (props: Props): JSX.Element => { | |||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| // TODO: Make this less brittle. This only works because this is the root render function | ||||||||||||||||||||||||||||||||||||
| useEffect(() => { | ||||||||||||||||||||||||||||||||||||
| UserSettings.lastTeamId = teamId | ||||||||||||||||||||||||||||||||||||
| octoClient.teamId = teamId | ||||||||||||||||||||||||||||||||||||
| dispatch(setTeam(teamId)) | ||||||||||||||||||||||||||||||||||||
| }, [teamId]) | ||||||||||||||||||||||||||||||||||||
| if (allTeams.length === 0) { | ||||||||||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| 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')) | ||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Comment on lines
+113
to
+119
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Validate against resolved Line 113 can mark valid fallback routes as invalid (and set 🔧 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| }, [teamId, allTeams]) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| const loadAction: (boardId: string) => any = useMemo(() => { | ||||||||||||||||||||||||||||||||||||
| if (props.readonly) { | ||||||||||||||||||||||||||||||||||||
|
|
@@ -194,6 +207,21 @@ const BoardPage = (props: Props): JSX.Element => { | |||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| const joinBoard = async (myUser: IUser, boardTeamId: string, boardId: string, allowAdmin: boolean) => { | ||||||||||||||||||||||||||||||||||||
| const boardExists = await octoClient.getBoard(boardId) | ||||||||||||||||||||||||||||||||||||
| if (!boardExists) { | ||||||||||||||||||||||||||||||||||||
| UserSettings.setLastBoardID(boardTeamId, null) | ||||||||||||||||||||||||||||||||||||
| UserSettings.setLastViewId(boardId, null) | ||||||||||||||||||||||||||||||||||||
| dispatch(setGlobalError('board-not-found')) | ||||||||||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| if (boardExists.teamId !== boardTeamId && boardExists.teamId !== Constants.globalTeamId) { | ||||||||||||||||||||||||||||||||||||
| UserSettings.setLastBoardID(boardTeamId, null) | ||||||||||||||||||||||||||||||||||||
| UserSettings.setLastViewId(boardId, null) | ||||||||||||||||||||||||||||||||||||
| dispatch(setGlobalError('board-not-found')) | ||||||||||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| const member = await octoClient.joinBoard(boardId, allowAdmin) | ||||||||||||||||||||||||||||||||||||
| if (!member) { | ||||||||||||||||||||||||||||||||||||
| if (myUser.permissions?.find((s) => s === 'manage_system' || s === 'manage_team')) { | ||||||||||||||||||||||||||||||||||||
|
|
@@ -229,28 +257,62 @@ const BoardPage = (props: Props): JSX.Element => { | |||||||||||||||||||||||||||||||||||
| }, []) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| 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) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
Comment on lines
+265
to
270
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 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 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||
| }, [teamId, match.params.boardId, viewId, me?.id]) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| dispatch(loadAction(match.params.boardId)) | ||||||||||||||||||||||||||||||||||||
| }, [teamId, match.params.boardId, viewId, me?.id, dispatch, loadAction]) | ||||||||||||||||||||||||||||||||||||
|
Comment on lines
259
to
+273
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Because 🔧 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 |
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| useEffect(() => { | ||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This creates a new bug. If you have two teams with following boards-
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. |
||||||||||||||||||||||||||||||||||||
| setLoadedBoardId('') | ||||||||||||||||||||||||||||||||||||
| }, [teamId]) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| useEffect(() => { | ||||||||||||||||||||||||||||||||||||
| if (!match.params.boardId || !me || props.readonly) { | ||||||||||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| if (loadedBoardId === match.params.boardId) { | ||||||||||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| const boardsLoaded = Object.keys(allBoards).length > 0 | ||||||||||||||||||||||||||||||||||||
| if (!boardsLoaded) { | ||||||||||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| const board = allBoards[match.params.boardId] | ||||||||||||||||||||||||||||||||||||
| if (board && board.teamId !== teamId && board.teamId !== Constants.globalTeamId) { | ||||||||||||||||||||||||||||||||||||
| dispatch(setGlobalError('board-not-found')) | ||||||||||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| setLoadedBoardId(match.params.boardId) | ||||||||||||||||||||||||||||||||||||
| loadOrJoinBoard(me, teamId, match.params.boardId) | ||||||||||||||||||||||||||||||||||||
| }, [teamId, match.params.boardId, me, props.readonly, allBoards, loadOrJoinBoard, dispatch, loadedBoardId]) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| // Validate that the viewId exists in the board's views | ||||||||||||||||||||||||||||||||||||
| useEffect(() => { | ||||||||||||||||||||||||||||||||||||
| if (match.params.boardId && !props.readonly && me) { | ||||||||||||||||||||||||||||||||||||
| loadOrJoinBoard(me, teamId, match.params.boardId) | ||||||||||||||||||||||||||||||||||||
| if (!match.params.viewId || props.readonly || props.new || !activeBoardId || boardViews.length === 0) { | ||||||||||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| const viewExists = boardViews.some((view) => view.id === match.params.viewId) | ||||||||||||||||||||||||||||||||||||
| if (!viewExists) { | ||||||||||||||||||||||||||||||||||||
| UserSettings.setLastViewId(activeBoardId, null) | ||||||||||||||||||||||||||||||||||||
| dispatch(setGlobalError('view-not-found')) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| }, [teamId, match.params.boardId, me?.id]) | ||||||||||||||||||||||||||||||||||||
| }, [match.params.viewId, activeBoardId, boardViews, props.readonly, props.new]) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| const handleUnhideBoard = async (boardID: string) => { | ||||||||||||||||||||||||||||||||||||
| if (!me || !category) { | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,7 +1,7 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Copyright (c) 2020-present Mattermost, Inc. All Rights Reserved. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // See LICENSE.txt for license information. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import {useEffect} from 'react' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import {useEffect, useMemo} from 'react' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import {generatePath, useHistory, useRouteMatch} from 'react-router-dom' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import {getBoards, getCurrentBoardId} from '../../store/boards' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -22,66 +22,80 @@ const TeamToBoardAndViewRedirect = (): null => { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const boards = useAppSelector(getBoards) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const teamId = match.params.teamId || UserSettings.lastTeamId || Constants.globalTeamId | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const boardCount = useMemo(() => Object.keys(boards).length, [boards]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const categoryCount = useMemo(() => categories.length, [categories]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| useEffect(() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let boardID = match.params.boardId | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!match.params.boardId) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // first preference is for last visited board | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| boardID = UserSettings.lastBoardId[teamId] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // if last visited board is unavailable, use the first board in categories list | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!boardID && categories.length > 0) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let goToBoardID: string | null = null | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for (const category of categories) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for (const boardMetadata of category.boardMetadata) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // pick the first category board that exists and is not hidden | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!boardMetadata.hidden && boards[boardMetadata.boardID]) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| goToBoardID = boardMetadata.boardID | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| break | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (match.params.boardId) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (boardCount === 0 && categoryCount === 0) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let boardID: string | undefined = undefined | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const lastBoardId = UserSettings.lastBoardId[teamId] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (lastBoardId) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const board = boards[lastBoardId] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (board && (board.teamId === teamId || board.teamId === Constants.globalTeamId)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| boardID = lastBoardId | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| UserSettings.setLastBoardID(teamId, null) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!boardID && categoryCount > 0) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for (const category of categories) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for (const boardMetadata of category.boardMetadata) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const board = boards[boardMetadata.boardID] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Pick the first category board that exists, is not hidden, and belongs to this team | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!boardMetadata.hidden && board && (board.teamId === teamId || board.teamId === Constants.globalTeamId)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| boardID = boardMetadata.boardID | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| break | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // there may even be no boards at all | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (goToBoardID) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| boardID = goToBoardID | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (boardID) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| break | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (boardID) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const newPath = generatePath(Utils.getBoardPagePath(match.path), {...match.params, boardId: boardID, viewID: undefined}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| history.replace(newPath) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // return from here because the loadBoardData() call | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // will fetch the data to be used below. We'll | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // use it in the next render cycle. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (boardID) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const newPath = generatePath(Utils.getBoardPagePath(match.path), {...match.params, boardId: boardID, viewID: undefined}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| history.replace(newPath) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let viewID = match.params.viewId | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, [teamId, match.params.boardId, boardCount, categoryCount, boards, categories, history, match.path, match.params]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const viewCount = useMemo(() => boardViews.length, [boardViews]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| useEffect(() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const viewID = match.params.viewId | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // when a view isn't open, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // but the data is available, try opening a view | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if ((!viewID || viewID === '0') && boardId && boardId === match.params.boardId && boardViews && boardViews.length > 0) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if ((!viewID || viewID === '0') && boardId && boardId === match.params.boardId && viewCount > 0) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // most recent view gets the first preference | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| viewID = UserSettings.lastViewId[boardID] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (viewID) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| UserSettings.setLastViewId(boardID, viewID) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| dispatch(setCurrentView(viewID)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+82
to
+90
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Validate persisted 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (viewID) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const newPath = generatePath(Utils.getBoardPagePath(match.path), {...match.params, viewId: viewID}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (selectedViewID) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const newPath = generatePath(Utils.getBoardPagePath(match.path), {...match.params, viewId: selectedViewID}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| history.replace(newPath) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, [teamId, match.params.boardId, match.params.viewId, categories.length, boardViews.length, boardId]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, [match.params.boardId, match.params.viewId, viewCount, boardId, boardViews, dispatch, history, match.path, match.params]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return null | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||


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.
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.