fix: router failing to match channel route when 'First Channel After Login' starts with a hash #37656
Conversation
…Login' starts with a hash Signed-off-by: Abhinav Kumar <[email protected]>
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: e29a177 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughNormalize the First_Channel_After_Login setting by stripping a leading Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20-30 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 0
🧹 Nitpick comments (1)
apps/meteor/client/views/root/MainLayout/LayoutWithSidebarV2.tsx (1)
23-24: Consider extracting the normalization logic to reduce duplication.The normalization logic is identical in both
LayoutWithSidebar.tsx(lines 18-19) andLayoutWithSidebarV2.tsx(lines 23-24). While the current implementation is correct, extracting this into a shared utility function could improve maintainability.Example:
// In a shared utility file export const normalizeChannelName = (channel: string): string => channel.startsWith('#') ? channel.slice(1) : channel;Then use in both files:
-const roomName = firstChannelAfterLogin.startsWith('#') ? firstChannelAfterLogin.slice(1) : firstChannelAfterLogin; +const roomName = normalizeChannelName(firstChannelAfterLogin);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.spec.tsx(1 hunks)apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.tsx(2 hunks)apps/meteor/client/views/root/MainLayout/LayoutWithSidebarV2.spec.tsx(1 hunks)apps/meteor/client/views/root/MainLayout/LayoutWithSidebarV2.tsx(3 hunks)
🧰 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/views/root/MainLayout/LayoutWithSidebar.spec.tsxapps/meteor/client/views/root/MainLayout/LayoutWithSidebarV2.tsxapps/meteor/client/views/root/MainLayout/LayoutWithSidebarV2.spec.tsxapps/meteor/client/views/root/MainLayout/LayoutWithSidebar.tsx
🧠 Learnings (11)
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.spec.tsxapps/meteor/client/views/root/MainLayout/LayoutWithSidebarV2.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.spec.tsxapps/meteor/client/views/root/MainLayout/LayoutWithSidebarV2.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.spec.tsxapps/meteor/client/views/root/MainLayout/LayoutWithSidebarV2.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure clean state for each test execution in Playwright tests
Applied to files:
apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Prefer web-first assertions (`toBeVisible`, `toHaveText`, etc.) in Playwright tests
Applied to files:
apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.spec.tsxapps/meteor/client/views/root/MainLayout/LayoutWithSidebarV2.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.spec.tsxapps/meteor/client/views/root/MainLayout/LayoutWithSidebarV2.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.spec.tsxapps/meteor/client/views/root/MainLayout/LayoutWithSidebarV2.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.spec.tsxapps/meteor/client/views/root/MainLayout/LayoutWithSidebarV2.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Store commonly used locators in variables/constants for reuse
Applied to files:
apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/client/views/root/MainLayout/LayoutWithSidebarV2.spec.tsx
🧬 Code graph analysis (4)
apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.spec.tsx (3)
packages/ui-contexts/src/index.ts (2)
useCurrentRoutePath(31-31)useRoute(63-63)apps/meteor/client/lib/appLayout.tsx (1)
render(26-28)packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)
apps/meteor/client/views/root/MainLayout/LayoutWithSidebarV2.tsx (2)
packages/ui-contexts/src/index.ts (1)
useSetting(71-71)packages/ui-contexts/src/RouterContext.ts (1)
IRouterPaths(5-14)
apps/meteor/client/views/root/MainLayout/LayoutWithSidebarV2.spec.tsx (3)
packages/ui-contexts/src/index.ts (2)
useCurrentRoutePath(31-31)useRouter(62-62)apps/meteor/client/lib/appLayout.tsx (1)
render(26-28)packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)
apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.tsx (1)
packages/ui-contexts/src/index.ts (1)
useSetting(71-71)
⏰ 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). (3)
- GitHub Check: ⚙️ Variables Setup
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (7)
apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.tsx (2)
18-19: LGTM! Clean normalization logic.The explicit type parameter and default value ensure type safety, and the normalization correctly handles the leading '#' character. The edge case where the setting value is just '#' results in an empty
roomName, which is properly handled by the guard at line 30.
23-41: LGTM! Redirect logic is correctly implemented.The guard conditions properly prevent redirects on non-home routes and when the setting is empty. The
redirected.currentref ensures idempotency across re-renders, and the dependency array correctly includesroomNameinstead of the previousfirstChannelAfterLogin.apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.spec.tsx (2)
1-32: LGTM! Well-structured test setup.The mock configuration is appropriate and the
setupChannelRouteMockhelper effectively reduces duplication across test cases.
34-101: LGTM! Comprehensive test coverage.The test suite effectively covers the critical scenarios including '#' normalization (lines 45-54), empty setting handling, route-specific behavior, and idempotency across re-renders. The assertions clearly validate the expected behavior.
apps/meteor/client/views/root/MainLayout/LayoutWithSidebarV2.tsx (1)
28-45: LGTM! Redirect logic correctly implemented.The implementation mirrors
LayoutWithSidebar.tsxappropriately, with the correct use ofrouter.navigatefor V2. The type assertion at line 44 is necessary due to the dynamic nature of channel routes, though this does bypass TypeScript's type checking for the route path.apps/meteor/client/views/root/MainLayout/LayoutWithSidebarV2.spec.tsx (2)
1-41: LGTM! Comprehensive mock setup for V2 components.The test setup appropriately mocks all V2-specific components. Note that the
FeaturePreviewmocks (lines 23-27) render both On and Off children simultaneously, which differs from runtime behavior but is acceptable for testing the navigation logic in isolation.
43-110: LGTM! Complete test coverage for V2 layout.The test suite provides comprehensive coverage of the navigation behavior, including the critical '#' normalization (lines 54-63), and correctly validates the V2-specific routing API using
router.navigate.
Signed-off-by: Abhinav Kumar <[email protected]>
📦 Docker Image Size Report📈 Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/15 22:28", "11/16 01:28", "11/17 23:50", "11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 01:16", "12/11 10:47 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12]
line "omnichannel-transcript-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "queue-worker-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "rocketchat" [0.36, 0.36, 0.35, 0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.35]
line "stream-hub-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
Statistics (last 18 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Thu, 11 Dec 2025 10:47:56 GMT |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #37656 +/- ##
===========================================
+ Coverage 67.70% 68.73% +1.03%
===========================================
Files 3452 3367 -85
Lines 113975 114342 +367
Branches 20941 20709 -232
===========================================
+ Hits 77166 78595 +1429
+ Misses 34680 33637 -1043
+ Partials 2129 2110 -19
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.tsx (1)
34-37: Consider removing the inline comment per coding guidelines.The guard logic correctly prevents routing for channel names starting with
@or?, which would create malformed URLs. However, the inline comment on lines 35-36 can be removed as per the coding guideline to "avoid code comments in the implementation."As per coding guidelines, consider applying this diff:
if (roomName.startsWith('@') || roomName.startsWith('?')) { - // Because this will break url routing. Eg: /channel/#roomName and /channel/?roomName which will route to path /channel return; }apps/meteor/client/views/root/MainLayout/LayoutWithSidebarV2.tsx (2)
23-25: Channel name normalization fixes the#fragment issue; consider trimming first for odd whitespace casesUsing
useSetting<string>('First_Channel_After_Login', '')and derivingroomNameby stripping a leading#and trimming solves the browser-fragment problem and keeps the rest of the logic simple.If you care about inputs like
' #general ', you could normalize whitespace first and then strip the hash so those cases also work:- const firstChannelAfterLogin = useSetting<string>('First_Channel_After_Login', ''); - const roomName = (firstChannelAfterLogin.startsWith('#') ? firstChannelAfterLogin.slice(1) : firstChannelAfterLogin).trim(); + const firstChannelAfterLogin = useSetting<string>('First_Channel_After_Login', '').trim(); + const roomName = firstChannelAfterLogin.startsWith('#') ? firstChannelAfterLogin.slice(1) : firstChannelAfterLogin;This keeps the behavior identical for typical values but is more robust against leading spaces around the setting.
Please double-check that the
useSettingtyping forFirst_Channel_After_Loginguarantees a string so the.trim()call is always safe.
35-41: Guard clauses correctly avoid bad redirects; comment is slightly out of dateThe new guards:
if (!roomName) return;if (roomName.startsWith('@') || roomName.startsWith('?')) return;nicely prevent redirects when the setting is empty or clearly not a valid channel name, which aligns with SUP-916’s objective of not breaking login/sidebars on invalid configuration.
The inline comment still mentions
/channel/#roomNameeven though the#case is now handled during normalization and@is now also blocked; if you touch this area again, it may be worth updating or dropping the comment to avoid confusion, but it’s not functionally problematic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.tsx(2 hunks)apps/meteor/client/views/root/MainLayout/LayoutWithSidebarV2.tsx(3 hunks)
🧰 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/views/root/MainLayout/LayoutWithSidebarV2.tsxapps/meteor/client/views/root/MainLayout/LayoutWithSidebar.tsx
🧠 Learnings (1)
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.tsx
🧬 Code graph analysis (1)
apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.tsx (1)
packages/ui-contexts/src/index.ts (1)
useSetting(71-71)
⏰ 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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (5)
apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.tsx (4)
18-19: LGTM! Core fix correctly normalizes the channel name.The normalization logic properly strips the leading
#and trims whitespace, resolving the routing bug when "First Channel After Login" starts with a hash. The typeduseSetting<string>with a default value prevents potential runtime errors.
30-32: LGTM! Guard correctly prevents redirect on empty channel name.The empty check on the normalized
roomNameappropriately blocks routing when the setting is empty, whitespace-only, or just a hash character.
45-45: LGTM! Route push correctly uses the normalized channel name.The redirect now uses
roomName(without the leading#) instead of the raw setting value, ensuring the router can correctly match and navigate to the channel.
46-46: LGTM! Effect dependencies correctly updated.The dependency array now includes
roomNameinstead offirstChannelAfterLogin, which is correct since the effect logic relies on the normalizedroomNamevalue.apps/meteor/client/views/root/MainLayout/LayoutWithSidebarV2.tsx (1)
49-50: Redirect to/channel/${roomName}using the normalized value and updated deps looks consistentUsing the normalized
roomNamein:router.navigate({ name: `/channel/${roomName}` as keyof IRouterPaths });and adding
roomNameto the effect dependency array ensures:
- The redirect uses the cleaned channel name (without leading
#/ whitespace).- The effect reacts correctly if either the current route path or the setting-derived
roomNamechanges, whileredirected.currentstill prevents multiple redirects on re-render.This matches the PR objective of fixing routing when
First_Channel_After_Loginstarts with#without changing other behavior.Please rely on the updated
LayoutWithSidebarV2tests (and any router-type checks) to confirm that/channel/${roomName}remains a valid key ofIRouterPathsfor all expected configurations.
Hey @MartinSchoeler, Based on your suggestion, I explored a few possible approaches and here’s what I found:
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.spec.tsx(1 hunks)apps/meteor/client/views/root/MainLayout/LayoutWithSidebarV2.spec.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/client/views/root/MainLayout/LayoutWithSidebarV2.spec.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/views/root/MainLayout/LayoutWithSidebar.spec.tsx
🧠 Learnings (10)
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Prefer web-first assertions (`toBeVisible`, `toHaveText`, etc.) in Playwright tests
Applied to files:
apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure clean state for each test execution in Playwright tests
Applied to files:
apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.spec.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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (5)
apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.spec.tsx (5)
1-5: LGTM!The imports are appropriate for unit testing the LayoutWithSidebar component with mocked dependencies.
7-16: LGTM!The mocking strategy correctly isolates the component's routing behavior while preserving other context functionality.
18-32: LGTM!The test setup ensures proper isolation with
beforeEach, and the helper function effectively reduces duplication across test cases.
34-65: LGTM!These test cases correctly validate the core fix (stripping "#") and the edge case handling for "?" prefix as documented in the PR objectives.
78-123: LGTM!These test cases comprehensively cover the redirect behavior on different routes, empty settings, and ensure the redirect fires only once to prevent loops.
apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.spec.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: Abhinav Kumar <[email protected]>
abhinavkrin
force-pushed
the
fix/missing-sidebar-invalid-first-channel
branch
from
c2601f2 to
a8c7ae9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.tsx (2)
11-21: Room-name normalization and invalid-prefix list look sound; consider centralizing and re-checking '@' coverageThe
useSetting<string>+roomNamenormalization (strip single leading#, then trim) looks correct and keeps the effect dependencies aligned with what’s actually used. TheINVALID_ROOM_NAME_PREFIXESguard is a good safety net for values like'?foo'.Two small follow-ups:
- If
LayoutWithSidebarV2is using the same prefix list, consider extracting this normalization/validation into a shared helper to avoid future drift.- Given the earlier analysis that
@-prefixed values could cause routing issues, double‑check whether they are still problematic here and, if so, whetherINVALID_ROOM_NAME_PREFIXESshould also include'@'or be handled via a shared sanitizer for this setting.
36-39: Prefix guard is good, but inline comment conflicts with “no implementation comments” guidelineBlocking
roomNamevalues that start with#or?is a sensible defense against malformed/channel/#…and/channel/?…URLs.However, the inline explanatory comment on Line 37 goes against the “avoid code comments in the implementation” guideline. The behavior is already encoded in the condition; consider removing this comment or capturing the rationale in tests/PR description instead.
apps/meteor/client/views/root/MainLayout/LayoutWithSidebarV2.tsx (1)
41-44: LGTM! Validation correctly blocks problematic prefixes post-sanitization.The check correctly validates the sanitized
roomName(after the leading#has been stripped). This catches edge cases like:
##test→ becomes#test→ blocked#?test→ becomes?test→ blocked?test→ stays?test→ blockedConsider enhancing the comment to clarify this is post-sanitization validation:
- // Because this will break url routing. Eg: /channel/#roomName and /channel/?roomName which will route to path /channel + // After sanitization, block names that still start with problematic characters. + // These break URL routing (e.g., /channel/?roomName routes to path /channel instead of the room).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.spec.tsx(1 hunks)apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.tsx(2 hunks)apps/meteor/client/views/root/MainLayout/LayoutWithSidebarV2.spec.tsx(1 hunks)apps/meteor/client/views/root/MainLayout/LayoutWithSidebarV2.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.spec.tsx
- apps/meteor/client/views/root/MainLayout/LayoutWithSidebarV2.spec.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/views/root/MainLayout/LayoutWithSidebar.tsxapps/meteor/client/views/root/MainLayout/LayoutWithSidebarV2.tsx
🧠 Learnings (2)
📚 Learning: 2025-11-27T17:56:26.050Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37557
File: apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx:115-116
Timestamp: 2025-11-27T17:56:26.050Z
Learning: In Rocket.Chat, the GET /v1/abac/rooms endpoint (implemented in ee/packages/abac/src/index.ts) only returns rooms where abacAttributes exists and is not an empty array (query: { abacAttributes: { $exists: true, $ne: [] } }). Therefore, in components consuming this endpoint (like AdminABACRooms.tsx), room.abacAttributes is guaranteed to be defined for all returned rooms, and optional chaining before calling array methods like .join() is sufficient without additional null coalescing.
Applied to files:
apps/meteor/client/views/root/MainLayout/LayoutWithSidebarV2.tsx
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/client/views/root/MainLayout/LayoutWithSidebarV2.tsx
🧬 Code graph analysis (1)
apps/meteor/client/views/root/MainLayout/LayoutWithSidebarV2.tsx (1)
packages/ui-contexts/src/RouterContext.ts (1)
IRouterPaths(5-14)
⏰ 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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (7)
apps/meteor/client/views/root/MainLayout/LayoutWithSidebar.tsx (2)
32-35: Early-return on emptyroomNameis correct and avoids bogus redirectsThe explicit
!roomNameguard is a good addition: it prevents pushing/channel/''when the setting is blank or whitespace-only, and keeps the effect logic simple.
47-48: Redirect using sanitizedroomNamewith updated deps looks correctUsing
channelRoute.push({ name: roomName })together with the[channelRoute, currentRoutePath, roomName]dependency array and theredirectedref gives you a one-time redirect from/or/homewithout risking loops when the route path changes.apps/meteor/client/views/root/MainLayout/LayoutWithSidebarV2.tsx (5)
25-26: LGTM! Clear sanitization of user input.The logic correctly strips the leading
#(common channel notation) and trims whitespace before validation. This allows users to configure#generalin the setting while the actual channel name isgeneral.
37-39: LGTM! Appropriate guard for empty values.The check correctly handles cases where the setting is empty or becomes empty after sanitization (e.g., if the setting value was just
#or whitespace).
51-51: LGTM! Correctly uses sanitized roomName.The redirect now uses the sanitized
roomName(with leading#stripped), ensuring proper routing. The type assertionas keyof IRouterPathsis necessary for dynamic channel routes that aren't in the static router type definition.
52-52: LGTM! Dependencies correctly updated.The effect dependency array now correctly includes
roomNameinstead offirstChannelAfterLogin, matching the actual value used within the effect body.
16-16: No changes needed. The implementation correctly excludes@fromINVALID_ROOM_NAME_PREFIXES—@is not a valid room name prefix in Rocket.Chat (it's reserved for user mentions). The test suite confirms this behavior is intentional, validating only#and?as problematic prefixes.
abhinavkrin
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
dionisio-bot
bot
removed
the
stat: ready to merge
abhinavkrin
added
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
This PR fixes an issue where the router failed to match the channel route when the “First Channel After Login” setting started with a hash (
#).Because URLs interpret
#as the beginning of a fragment, the browser stops parsing the path at that point, causing the router to treat the channel name as invalid. As a result, the client loaded an not-found error page, and the sidebar did not appear.This fix removes the leading hash before routing, ensuring the channel is resolved correctly, and the client loads normally after login.
Issue(s)
Steps to test or reproduce
Further comments
SUP-916
Summary by CodeRabbit
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.