New page access denied page instead of something went wrong #166
New page access denied page instead of something went wrong #166Rajat-Dabade merged 10 commits intomainfrom
Conversation
Rajat-Dabade
requested review from
harshilsharma63,
isacikgoz,
matthewbirtch and
yasserfaraazkhan
Rajat-Dabade
added
1: UX Review
|
@coderabbitai review this PR |
|
✅ Actions performedReview triggered.
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces an "access denied" error handling feature. It adds a new AccessDeniedPage component displayed when users lack board access permissions. Changes include new i18n keys, an ErrorId enum member, API error handling for 403 responses, error detection in load operations, router redirection logic, and related UI components. Changes
Sequence DiagramsequenceDiagram
actor User
participant BoardPage
participant octoClient
participant API
participant Store as Store/<br/>Global Error
participant Router
participant AccessDeniedPage
User->>BoardPage: Attempt to access board
BoardPage->>octoClient: joinBoard(boardId)
octoClient->>API: Request board access
API-->>octoClient: 403 Forbidden
octoClient->>octoClient: Throw error<br/>(message: 'access-denied'<br/>status: 403)
octoClient-->>BoardPage: Error thrown
BoardPage->>Store: Dispatch setGlobalError('access-denied')
Store->>Router: Global error state updated
Router->>Router: Detect AccessDenied error
Router->>AccessDeniedPage: Redirect to /access-denied
AccessDeniedPage->>User: Render access denied page<br/>with illustration & button
User->>AccessDeniedPage: Click "Back to your boards"
AccessDeniedPage->>Store: Clear global error
AccessDeniedPage->>Router: Navigate to /
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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: 1
🤖 Fix all issues with AI agents
In `@webapp/src/pages/boardPage/boardPage.tsx`:
- Around line 218-229: In the access-denied branch of the catch block (where
err?.status === 403 or err?.message === 'access-denied'), clear the stored
lastBoardID and lastViewId the same way the 'board-not-found' branch does to
avoid TeamToBoardAndViewRedirect sending users back to an unauthorized board; if
the user is not an admin (i.e., after checking myUser.permissions and before
calling dispatch(setGlobalError(ErrorId.AccessDenied))), dispatch the same
actions or invoke the same logic used to clear lastBoardID and lastViewId so the
redirect loop is prevented.
🧹 Nitpick comments (3)
webapp/src/store/initialLoad.ts (1)
68-79: Consider centralizing access-denied detection to avoid message heuristics.Relying on message substrings is brittle. If feasible, normalize access-denied errors in the client (status/code) and key off that here.
webapp/src/svg/access-denied-illustation.tsx (1)
6-46: Mark the decorative SVG as aria-hidden.This avoids screen readers announcing a non-informative illustration.
♿ Suggested accessibility tweak
- <svg width="400" height="290" viewBox="0 0 347 213" fill="none" xmlns="http://www.w3.org/2000/svg"> + <svg width="400" height="290" viewBox="0 0 347 213" fill="none" xmlns="http://www.w3.org/2000/svg" aria-hidden="true" focusable="false">webapp/src/pages/accessDeniedPage.tsx (1)
29-32: Consider hiding the decorative illustration from assistive tech.
If the SVG is purely decorative, wrapping it witharia-hidden='true'avoids screen‑reader noise.♿ Suggested tweak
- <AccessDeniedIllustration/> + <span aria-hidden='true'> + <AccessDeniedIllustration/> + </span>
matthewbirtch
left a comment
matthewbirtch
left a comment
There was a problem hiding this comment.
Looks good. Just need to make some tweaks to support themes better @Rajat-Dabade
harshilsharma63
left a comment
harshilsharma63
left a comment
There was a problem hiding this comment.
The image and text color looks incorrect on a dark theme.
|
@harshilsharma63 Fix the dark theme issue 
|
yasserfaraazkhan
added
the
Setup Cloud Test Server
|
Plugin Spinwick PR #166 🎉 Test server created! Access here: https://boards-pr-166-mcgzq.test.mattermost.cloud
The test server was created successfully, but there was an issue installing or enabling the plugin automatically:
You can manually install the plugin:
Future commits will still attempt to automatically update the plugin. Installation ID: Credentials: Posted securely in this Mattermost channel - Look for PR #166 |
|
@harshilsharma63 Just a friendly reminder for reviewing. Thank you. |
|
Plugin test server update attempted, but encountered an issue: The test server is still available. You can manually download and install the updated plugin using the artifact link below. Updated with git commit
|
|
Plugin test server update attempted, but encountered an issue: The test server is still available. You can manually download and install the updated plugin using the artifact link below. Updated with git commit
|
harshilsharma63
left a comment
harshilsharma63
left a comment
There was a problem hiding this comment.
Approved with one suggestion.
|
|
||
| const handleBackToHome = useCallback(() => { | ||
| // Clear any error state before navigating | ||
| dispatch(setGlobalError('')) |
There was a problem hiding this comment.
Should we, as precaution, clear out the last board and view IDs here? What happens if local storage contains last board/view ID of a board user doesn't have access to. I know we are clearing those before sending the user to this page, but maybe just to be extra careful, we could clear them here as well.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@webapp/src/pages/boardPage/boardPage.tsx`:
- Around line 238-244: In loadOrJoinBoard, don't unconditionally return on a
loadBoardData rejection; instead detect an access-denied rejection and rethrow
it so upstream can handle clearing last-viewed IDs (mirror the earlier pattern):
after awaiting dispatch(loadBoardData(boardId)) check if result.type ===
loadBoardData.rejected.type and if result.payload?.type === 'access-denied' then
throw result.payload (or return Promise.reject(result.payload)); otherwise keep
the existing early return for other errors. Target symbols: loadOrJoinBoard and
loadBoardData.
|
Plugin test server update attempted, but encountered an issue: The test server is still available. You can manually download and install the updated plugin using the artifact link below. Updated with git commit
|
|
Plugin test server update attempted, but encountered an issue: The test server is still available. You can manually download and install the updated plugin using the artifact link below. Updated with git commit
|
yasserfaraazkhan
removed
the
Setup Cloud Test Server
|
Test server destroyed |
|
/update-branch |
yasserfaraazkhan
added
the
Setup Cloud Test Server
|
Plugin Spinwick PR #166 🎉 Test server created! Access here: https://boards-pr-166-m5agz.test.mattermost.cloud
The test server was created successfully, but there was an issue installing or enabling the plugin automatically:
You can manually install the plugin:
Future commits will still attempt to automatically update the plugin. Installation ID: Credentials: Posted securely in this Mattermost channel - Look for PR #166 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webapp/src/pages/boardPage/boardPage.tsx (1)
238-269:⚠️ Potential issue | 🟡 MinorMissing dependencies in
useCallbackcould cause stale closure issues.The
loadOrJoinBoardcallback has an empty dependency array[]but referencesjoinBoard,dispatch, andsetShowJoinBoardDialogwhich could become stale. Whiledispatchis stable from Redux,joinBoardis defined inline and capturesteamIdand other state.🛠️ Suggested fix
- }, []) + }, [dispatch, joinBoard])Note: Since
joinBoardis not memoized withuseCallback, this may cause frequent re-creation ofloadOrJoinBoard. Consider also wrappingjoinBoardinuseCallbackif performance becomes a concern.
🤖 Fix all issues with AI agents
In `@webapp/src/pages/boardPage/boardPage.tsx`:
- Around line 295-301: The effect watching match.params.boardId and currentBoard
can call loadOrJoinBoard twice on mount; add a guard to only treat the case as
"board was removed" (not "not yet loaded") by tracking previous board presence
and only calling loadOrJoinBoard when a board that previously existed is now
missing: use a ref like prevBoardIdRef or prevHasBoardRef to capture prior
currentBoard (or its id) and in the useEffect check that prevBoardIdRef.current
!== undefined && currentBoard === undefined && match.params.boardId &&
!props.readonly && me before invoking loadOrJoinBoard; update the ref after
render so future changes behave normally (references: useEffect,
loadOrJoinBoard, match.params.boardId, props.readonly, me, currentBoard).
🧹 Nitpick comments (3)
webapp/src/pages/accessDeniedPage.tsx (1)
75-75: Prefer CSS for spacing instead of<br/>.Using
<br/>for visual spacing is less maintainable than CSS margin/padding. Consider adding margin to the.subtitleor button container in the SCSS file instead.♻️ Suggested fix
- <br/> <ButtonThen add spacing in
accessDeniedPage.scss:.AccessDeniedPage .subtitle { margin-bottom: 1.5rem; /* or appropriate value */ }webapp/src/pages/boardPage/boardPage.tsx (2)
254-259: Empty catch block silently swallows errors.While the comment states errors are handled in
joinBoard, any unexpected errors (e.g., network failures not caught by the inner try/catch, or errors thrown after the catch block) would be silently swallowed. Consider logging unexpected errors for debugging.♻️ Optional improvement
try { await joinBoard(myUser, boardTeamId, boardId, false) } catch (error: unknown) { - // Error already handled in joinBoard + // Most errors are handled in joinBoard, but log unexpected ones + Utils.log('Unexpected error in loadOrJoinBoard:', error) }
200-236: UseErrorId.BoardNotFoundenum instead of string literal for consistency.The function uses the string literal
'board-not-found'at lines 210 and 233, while line 231 correctly uses theErrorId.AccessDeniedenum. SinceErrorId.BoardNotFoundis already defined inerrors.ts, update both occurrences to use the enum for consistency and type safety.♻️ Suggested fix
UserSettings.setLastBoardID(boardTeamId, null) UserSettings.setLastViewId(boardId, null) - dispatch(setGlobalError('board-not-found')) + dispatch(setGlobalError(ErrorId.BoardNotFound)) return }And similarly at line 233:
} else { - dispatch(setGlobalError('board-not-found')) + dispatch(setGlobalError(ErrorId.BoardNotFound)) }
|
@yasserfaraazkhan, a friendly reminder for the review. |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
webapp/src/pages/boardPage/boardPage.tsx (2)
242-254: Non-access-denied rejections are silently ignored.When
loadBoardDatais rejected with an error other thanAccessDenied, the code returns early without any error handling (line 253). This could leave users in an unclear state if the rejection is due to network issues or other transient errors.Consider handling other rejection types explicitly:
♻️ Proposed improvement for error handling
if (result.type === loadBoardData.rejected.type) { if (result.error?.message === ErrorId.AccessDenied) { const isAdmin = myUser.permissions?.find((s) => s === 'manage_system' || s === 'manage_team') if (isAdmin) { setShowJoinBoardDialog(true) return } UserSettings.setLastBoardID(boardTeamId, null) UserSettings.setLastViewId(boardId, null) dispatch(setGlobalError(ErrorId.AccessDenied)) + } else { + // Handle other errors (network issues, etc.) + dispatch(setGlobalError('board-not-found')) } return }🤖 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 242 - 254, The current branch for loadBoardData.rejected only handles ErrorId.AccessDenied and silently returns for all other rejections; update the rejection handling in the block that checks result.type === loadBoardData.rejected.type to add an else path that handles non-access-denied errors (e.g., dispatch a generic/global error via dispatch(setGlobalError(...)), log the error, and/or show a user-facing message) while keeping the existing AccessDenied flow (isAdmin check, setShowJoinBoardDialog, UserSettings.setLastBoardID/setLastViewId). Use result.error?.message or a fallback ErrorId/Unknown value when calling setGlobalError so transient/network failures aren’t ignored.
255-261: Empty catch block swallows potential errors.The catch block at lines 258-260 is empty with a comment stating errors are handled in
joinBoard. However, ifjoinBoardthrows an unexpected error not handled internally, it will be silently swallowed.Consider logging unexpected errors for debugging:
♻️ Optional improvement
try { await joinBoard(myUser, boardTeamId, boardId, false) } catch (error: unknown) { - // Error already handled in joinBoard + // Most errors handled in joinBoard, log unexpected ones + Utils.logError('Unexpected error in joinBoard:', error) }🤖 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 255 - 261, The empty catch in the block that calls joinBoard(myUser, boardTeamId, boardId, false) swallows unexpected errors; update the catch to at minimum log the error (including context like myUser.id, boardId, boardTeamId, and result.payload?.blocks length) using the existing logger or console.error, and consider rethrowing or forwarding the error if it should bubble up; ensure you reference the joinBoard call and preserve the comment that joinBoard handles known errors while still surfacing any unexpected exceptions.webapp/src/router.tsx (1)
180-197: Duplicatedispatch(setGlobalError(''))calls in both branches.The global error is cleared in both the early-return guard (line 184) and before the redirect (line 188). While this works correctly, the second dispatch at line 188 could be moved after the conditional to avoid redundancy.
♻️ Optional refactor to reduce duplication
useEffect(() => { if (globalError) { // Don't redirect if we're already on an error page if (location.pathname === '/error' || location.pathname === '/access-denied') { dispatch(setGlobalError('')) return } - dispatch(setGlobalError('')) // Redirect to access denied page for access denied errors if (globalError === ErrorId.AccessDenied || globalError === ErrorId.InvalidReadOnlyBoard) { const currentPath = location.pathname + location.search + dispatch(setGlobalError('')) history.replace(`/access-denied?r=${encodeURIComponent(currentPath)}`) } else { + dispatch(setGlobalError('')) history.replace(`/error?id=${globalError}`) } } }, [globalError, history, location, dispatch])Alternatively, keep as-is since the current structure is clear about always clearing before navigation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/router.tsx` around lines 180 - 197, The useEffect handling globalError calls dispatch(setGlobalError('')) twice (once in the early-return guard when location.pathname is '/error' or '/access-denied', and again before redirecting); refactor by removing the duplicate call and performing a single dispatch(setGlobalError('')) after the conditional guard so the error is cleared exactly once before any navigation; update the useEffect that references globalError, history, location, and setGlobalError to call dispatch(setGlobalError('')) in one place (outside/below the if/else redirect logic) and keep the existing redirects for ErrorId.AccessDenied / ErrorId.InvalidReadOnlyBoard and the generic error case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@webapp/src/pages/boardPage/boardPage.tsx`:
- Around line 242-254: The current branch for loadBoardData.rejected only
handles ErrorId.AccessDenied and silently returns for all other rejections;
update the rejection handling in the block that checks result.type ===
loadBoardData.rejected.type to add an else path that handles non-access-denied
errors (e.g., dispatch a generic/global error via dispatch(setGlobalError(...)),
log the error, and/or show a user-facing message) while keeping the existing
AccessDenied flow (isAdmin check, setShowJoinBoardDialog,
UserSettings.setLastBoardID/setLastViewId). Use result.error?.message or a
fallback ErrorId/Unknown value when calling setGlobalError so transient/network
failures aren’t ignored.
- Around line 255-261: The empty catch in the block that calls joinBoard(myUser,
boardTeamId, boardId, false) swallows unexpected errors; update the catch to at
minimum log the error (including context like myUser.id, boardId, boardTeamId,
and result.payload?.blocks length) using the existing logger or console.error,
and consider rethrowing or forwarding the error if it should bubble up; ensure
you reference the joinBoard call and preserve the comment that joinBoard handles
known errors while still surfacing any unexpected exceptions.
In `@webapp/src/router.tsx`:
- Around line 180-197: The useEffect handling globalError calls
dispatch(setGlobalError('')) twice (once in the early-return guard when
location.pathname is '/error' or '/access-denied', and again before
redirecting); refactor by removing the duplicate call and performing a single
dispatch(setGlobalError('')) after the conditional guard so the error is cleared
exactly once before any navigation; update the useEffect that references
globalError, history, location, and setGlobalError to call
dispatch(setGlobalError('')) in one place (outside/below the if/else redirect
logic) and keep the existing redirects for ErrorId.AccessDenied /
ErrorId.InvalidReadOnlyBoard and the generic error case.
|
Plugin test server update attempted, but encountered an issue: The test server is still available. You can manually download and install the updated plugin using the artifact link below. Updated with git commit
|
marianunez
removed
1: UX Review
|
Test server destroyed |
8 participants
Summary
If do not have board access
Before
Visiting the board that you don't have access to:
After:
Ticket Link
https://mattermost.atlassian.net/browse/MM-67078
Summary by CodeRabbit