feat: add Go back action to NotFound page with safe fallback#38583
feat: add Go back action to NotFound page with safe fallback#38583gauravsingh001-cyber wants to merge 4 commits intoRocketChat:developfrom
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
|
WalkthroughNotFoundState adds a new "Go back" action that navigates back via Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. No actionable comments were generated in the recent review. 🎉 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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 `@apps/meteor/client/components/NotFoundState.tsx`:
- Around line 27-34: The guard using window.history.length in handleGoBackClick
is unreliable and can send users outside the app; replace it by tracking in-app
navigation state and using that instead: add a ref/flag (e.g., in a
component-level ref or a shared router hook) that is set on route changes and
check that flag inside handleGoBackClick to decide between calling
window.history.back() and router.navigate('/home'); update any route-change
listener or use the router's navigation/canGoBack API if available to maintain
the flag so handleGoBackClick only calls history.back() for internal
navigations.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/client/components/NotFoundState.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/components/NotFoundState.tsx
🧠 Learnings (1)
📚 Learning: 2025-11-17T15:07:13.273Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37398
File: packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx:357-363
Timestamp: 2025-11-17T15:07:13.273Z
Learning: In packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx, IconElement is a presentational, non-actionable element that does not require wrapping in AppIdProvider, similar to plain_text and mrkdwn renderers. Only actionable elements (those with actions, actionId, or interactive behavior) should be wrapped in AppIdProvider.
Applied to files:
apps/meteor/client/components/NotFoundState.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (3)
apps/meteor/client/components/NotFoundState.tsx (3)
1-9: Imports look good.Reformatted to multi-line; no functional changes. The added
StatesActionandStatesActionsimports align with their usage below.
45-47: Existing Homepage action preserved — LGTM.
49-51:⚠️ Potential issue | 🔴 CriticalAdd
Go_backtranslation key to i18n files.The
t('Go_back')translation key is missing from the i18n resource files. The keyHomepage(used on line 46) exists inen.i18n.json, butGo_back(line 50) does not, which will cause it to render the raw key string instead of a translated button label. Add theGo_backkey to all language files.⛔ Skipped due to learnings
Learnt from: d-gubert Repo: RocketChat/Rocket.Chat PR: 37547 File: packages/i18n/src/locales/en.i18n.json:634-634 Timestamp: 2025-11-19T12:32:29.696Z Learning: Repo: RocketChat/Rocket.Chat Context: i18n workflow Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/meteor/client/components/NotFoundState.tsx">
<violation number="1" location="apps/meteor/client/components/NotFoundState.tsx:29">
P2: `window.history.length > 1` is not a reliable guard for in-app back navigation. The `history.length` property counts all session history entries including those from external origins, meaning users arriving from external sites would be navigated out of the app entirely when clicking "Go back". Additionally, some browsers initialize `history.length` at values > 1 for new tabs.
Consider using `document.referrer` to check if the user came from within the app, or track in-app navigation state separately.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
This is a UI-only change on the NotFound page and does not require a version bump. |
Thanks for the review! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/meteor/client/components/NotFoundState.tsx`:
- Around line 27-37: The function handleGoBackClick is mis-indented outside the
component body; move and indent it to match handleGoHomeClick inside the
NotFoundState component so both handlers are defined at the same indentation
level (use the same tab/space style as handleGoHomeClick). Ensure the references
to document.referrer, window.location.origin, window.history.back(), and
router.navigate('/home') remain unchanged and that handleGoBackClick is placed
alongside handleGoHomeClick within the component scope.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/client/components/NotFoundState.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/components/NotFoundState.tsx
🧠 Learnings (1)
📚 Learning: 2025-11-17T15:07:13.273Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37398
File: packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx:357-363
Timestamp: 2025-11-17T15:07:13.273Z
Learning: In packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx, IconElement is a presentational, non-actionable element that does not require wrapping in AppIdProvider, similar to plain_text and mrkdwn renderers. Only actionable elements (those with actions, actionId, or interactive behavior) should be wrapped in AppIdProvider.
Applied to files:
apps/meteor/client/components/NotFoundState.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (2)
apps/meteor/client/components/NotFoundState.tsx (2)
49-55: New "Go back" action alongside "Homepage" looks good.Both actions are clearly presented and the existing Homepage behavior is preserved.
28-36:document.referrercan be empty or suppressed even for in-app navigations.
document.referreris cleared on manual address-bar entry, bookmarks, and when a strictReferrer-Policy(e.g.no-referrer) is active. The router from@rocket.chat/ui-contextsdoes not expose acanGoBackflag or navigation history stack. The current implementation with its fallback to/homeis the safest approach for handling this case.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| const handleGoBackClick = () => { | ||
| const isInternalReferrer = | ||
| document.referrer && | ||
| document.referrer.startsWith(window.location.origin); | ||
|
|
||
| if (isInternalReferrer) { | ||
| window.history.back(); | ||
| } else { | ||
| router.navigate('/home'); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Inconsistent indentation — handleGoBackClick is not indented inside the component body.
handleGoHomeClick (lines 23–25) is indented with a tab inside the component, but handleGoBackClick starts at column 0. This looks like a copy-paste artifact.
Proposed fix
-const handleGoBackClick = () => {
- const isInternalReferrer =
- document.referrer &&
- document.referrer.startsWith(window.location.origin);
-
- if (isInternalReferrer) {
- window.history.back();
- } else {
- router.navigate('/home');
- }
-};
+ const handleGoBackClick = () => {
+ const isInternalReferrer =
+ document.referrer &&
+ document.referrer.startsWith(window.location.origin);
+
+ if (isInternalReferrer) {
+ window.history.back();
+ } else {
+ router.navigate('/home');
+ }
+ };📝 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 handleGoBackClick = () => { | |
| const isInternalReferrer = | |
| document.referrer && | |
| document.referrer.startsWith(window.location.origin); | |
| if (isInternalReferrer) { | |
| window.history.back(); | |
| } else { | |
| router.navigate('/home'); | |
| } | |
| }; | |
| const handleGoBackClick = () => { | |
| const isInternalReferrer = | |
| document.referrer && | |
| document.referrer.startsWith(window.location.origin); | |
| if (isInternalReferrer) { | |
| window.history.back(); | |
| } else { | |
| router.navigate('/home'); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In `@apps/meteor/client/components/NotFoundState.tsx` around lines 27 - 37, The
function handleGoBackClick is mis-indented outside the component body; move and
indent it to match handleGoHomeClick inside the NotFoundState component so both
handlers are defined at the same indentation level (use the same tab/space style
as handleGoHomeClick). Ensure the references to document.referrer,
window.location.origin, window.history.back(), and router.navigate('/home')
remain unchanged and that handleGoBackClick is placed alongside
handleGoHomeClick within the component scope.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/meteor/client/components/NotFoundState.tsx">
<violation number="1" location="apps/meteor/client/components/NotFoundState.tsx:28">
P2: Using document.referrer to gate history.back() breaks SPA navigation: referrer isn’t updated by client-side routing, so the button often falls back to /home even when there is a valid history stack. Also, without checking history length, opening an internal link in a new tab (referrer present, history length 1) calls history.back() and does nothing, leaving a dead button.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const isInternalReferrer = | ||
| document.referrer && | ||
| document.referrer.startsWith(window.location.origin); | ||
|
|
||
| if (isInternalReferrer) { | ||
| window.history.back(); |
There was a problem hiding this comment.
I'm quite sure you can get this from the router
There was a problem hiding this comment.
Thanks for the suggestion!
Good point — I’ve updated the implementation to rely on router-based navigation instead of using document.referrer / browser history, to better align with SPA navigation behavior.
Please let me know if this approach looks good now.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/meteor/client/components/NotFoundState.tsx`:
- Around line 28-32: The conditional using router.canGoBack?.() and
router.back() is invalid because RouterContextValue doesn't expose those
methods; replace this logic by calling router.navigate(-1) for back navigation
(i.e., use router.navigate with a negative delta) and remove the optional
chaining, or alternatively add a proper canGoBack boolean and back() method to
the RouterContextValue implementation and types so you can conditionally disable
the button; update the NotFoundState component to use router.navigate(-1) (or
the new canGoBack/back API) and adjust any type definitions for
RouterContextValue accordingly.
- Around line 50-52: The button in NotFoundState.tsx uses the i18n key "Go_back"
(rendered inside StatesAction with onClick handler handleGoBackClick) which is
missing from the locale files; add "Go_back": "Go back" to each translation JSON
(e.g., en.i18n.json and other locale files) so the token resolves to a localized
label instead of displaying the raw key.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/client/components/NotFoundState.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/components/NotFoundState.tsx
🧠 Learnings (1)
📚 Learning: 2025-11-17T15:07:13.273Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37398
File: packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx:357-363
Timestamp: 2025-11-17T15:07:13.273Z
Learning: In packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx, IconElement is a presentational, non-actionable element that does not require wrapping in AppIdProvider, similar to plain_text and mrkdwn renderers. Only actionable elements (those with actions, actionId, or interactive behavior) should be wrapped in AppIdProvider.
Applied to files:
apps/meteor/client/components/NotFoundState.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (2)
apps/meteor/client/components/NotFoundState.tsx (2)
1-9: LGTM — import reformatting is clean.
46-52: Good UX addition — dual navigation actions.The "Go back" + "Homepage" pairing follows standard NotFound page patterns and addresses the PR objective well. The
router.canGoBack?.()approach (assuming it exists on the router) is a proper fix over the earlierwindow.history.lengthapproach, as it avoids navigating users outside the app.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| <StatesAction onClick={handleGoBackClick}> | ||
| {t('Go_back')} | ||
| </StatesAction> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if Go_back translation key exists
echo "=== Searching for Go_back in i18n/translation files ==="
rg -n 'Go_back' -g '*.json' -g '*.i18n' -g '*.yml' -g '*.yaml' | head -30
echo ""
echo "=== Also check .ts/.tsx for any dynamic key registration ==="
rg -n 'Go_back' -g '*.ts' -g '*.tsx' | head -30
echo ""
echo "=== Check for common i18n directory structures ==="
fd -t f -e json | grep -i 'i18n\|translation\|locale\|lang' | head -20Repository: RocketChat/Rocket.Chat
Length of output: 1014
🏁 Script executed:
#!/bin/bash
# Find main app i18n/translation directories
echo "=== Search for i18n files in apps/meteor ==="
fd -t f \( -name '*.json' -o -name '*.yml' -o -name '*.yaml' \) | grep -E 'apps/meteor.*i18n|apps/meteor.*translation|apps/meteor.*locale|apps/meteor.*lang' | head -20
echo ""
echo "=== Check root-level i18n structures ==="
find . -maxdepth 3 -type d \( -name 'i18n' -o -name 'translations' -o -name 'locales' \) 2>/dev/null | head -20
echo ""
echo "=== Search for Go_back across entire repo without file type restriction ==="
rg -n 'Go_back' --type-add 'translation:*.json' --type translation 2>/dev/null || rg -n 'Go_back' 2>/dev/null | head -30
echo ""
echo "=== Check how t() function is defined/imported ==="
rg -n "import.*from.*i18n\|from.*translate" apps/meteor/client/components/NotFoundState.tsx -A 2 -B 2Repository: RocketChat/Rocket.Chat
Length of output: 618
🏁 Script executed:
#!/bin/bash
# Explore packages/i18n structure
echo "=== Files in packages/i18n ==="
ls -la packages/i18n/ | head -20
echo ""
echo "=== Search for Go_back in packages/i18n ==="
rg 'Go_back' packages/i18n/ 2>/dev/null || echo "No matches found for Go_back"
echo ""
echo "=== Count translation files ==="
find packages/i18n -type f -name '*.json' | wc -l
echo ""
echo "=== Sample translation file to understand structure ==="
find packages/i18n -type f -name '*.json' | head -1 | xargs head -50
echo ""
echo "=== Check import in NotFoundState.tsx ==="
head -30 apps/meteor/client/components/NotFoundState.tsx | rg "import|useTranslation"Repository: RocketChat/Rocket.Chat
Length of output: 1195
🏁 Script executed:
#!/bin/bash
# Explore packages/i18n/src structure
echo "=== Files in packages/i18n/src ==="
ls -la packages/i18n/src/ | head -20
echo ""
echo "=== Look for translation JSON files ==="
find packages/i18n/src -name '*.json' | head -10
echo ""
echo "=== Search for Go_back in packages/i18n/src ==="
rg 'Go_back' packages/i18n/src/ 2>/dev/null || echo "No matches found for Go_back in packages/i18n/src"
echo ""
echo "=== Sample of first translation file ==="
find packages/i18n/src -name '*.json' -type f | head -1 | xargs head -100Repository: RocketChat/Rocket.Chat
Length of output: 9303
🏁 Script executed:
#!/bin/bash
# Search for Go_back in all locale files
echo "=== Search for Go_back in all .i18n.json files ==="
rg '"Go_back"' packages/i18n/src/locales/ || echo "No 'Go_back' key found"
echo ""
echo "=== Search for go_back (case insensitive) ==="
rg -i 'go.back' packages/i18n/src/locales/ | head -20
echo ""
echo "=== Check en.i18n.json (English) for the key ==="
if [ -f "packages/i18n/src/locales/en.i18n.json" ]; then
rg 'Go_back' packages/i18n/src/locales/en.i18n.json || echo "Not found in English"
echo "=== Nearby keys in en.i18n.json ==="
rg -B2 -A2 '"Go' packages/i18n/src/locales/en.i18n.json
else
echo "en.i18n.json not found, listing available locale files:"
ls packages/i18n/src/locales/ | head -20
fiRepository: RocketChat/Rocket.Chat
Length of output: 5647
The Go_back i18n key is missing from translation files.
The key used at line 51 does not exist in any of the translation files in packages/i18n/src/locales/. The button will render the raw key string Go_back instead of a localized label. Add the key to the translation files (e.g., en.i18n.json): "Go_back": "Go back".
🤖 Prompt for AI Agents
In `@apps/meteor/client/components/NotFoundState.tsx` around lines 50 - 52, The
button in NotFoundState.tsx uses the i18n key "Go_back" (rendered inside
StatesAction with onClick handler handleGoBackClick) which is missing from the
locale files; add "Go_back": "Go back" to each translation JSON (e.g.,
en.i18n.json and other locale files) so the token resolves to a localized label
instead of displaying the raw key.
| const handleGoBackClick = () => { | ||
| router.navigate(-1); |
There was a problem hiding this comment.
This way fails when there is no history; it returns to the previous website instead of going home.
4 participants
Proposed changes
This PR adds a Go back action to the NotFound page.
/homewhen the page is accessed directlyIssue(s)
Closes #38581
Steps to test or reproduce
/homeFurther comments
UI-only change. No breaking behavior.
AfterChange
1stPR.mp4
Summary by CodeRabbit