-
Notifications
You must be signed in to change notification settings - Fork 12.4k
feat: Resizable contextualbars out of feature preview #37022
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
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: 56bd04e The changes in this PR will be included in the next version bump. This PR includes changesets to release 39 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 |
WalkthroughPromotes Contextualbar resizable from preview to stable by removing feature-flag branches, deleting related i18n keys, and updating feature preview lists. Adjusts Storybook decorators to use Box instead of Contextualbar. Updates e2e tests to align with UI changes. Bumps @rocket.chat/ui-client, @rocket.chat/i18n, and @rocket.chat/meteor major versions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as ContextualbarDialog
participant Resizer as ContextualbarResizable
participant Bar as Contextualbar
Note over UI: New stabilized flow (feature preview removed)
User->>UI: Open contextual dialog
UI->>Resizer: Render with defaultWidth from contextualBar
Resizer->>Bar: Render width='100%'
User-->>Resizer: Drag to resize (optional)
Resizer-->>Bar: Apply new width
Note over UI,Bar: No FeaturePreview gating path
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-8.0.0 #37022 +/- ##
=================================================
- Coverage 66.43% 66.39% -0.05%
=================================================
Files 3276 3312 +36
Lines 109596 111646 +2050
Branches 20860 20935 +75
=================================================
+ Hits 72814 74123 +1309
- Misses 34116 34846 +730
- Partials 2666 2677 +11
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (5)
apps/meteor/tests/e2e/page-objects/omnichannel-units.ts (2)
54-54: Avoid coordinate-based click; close dropdown semanticallyCoordinate clicks are flaky across DPI/layouts. Prefer closing the listbox via keyboard and wait for it to detach.
- await this.contextualBar.click({ position: { x: 3, y: 3 } }); + await this.page.keyboard.press('Escape'); + await this.page.locator('#position-container').waitFor({ state: 'detached' });
60-60: Repeat: replace coordinate click with deterministic closeSame rationale as above: use Escape + explicit wait.
- await this.contextualBar.click({ position: { x: 3, y: 3 } }); + await this.page.keyboard.press('Escape'); + await this.page.locator('#position-container').waitFor({ state: 'detached' });.changeset/happy-carpets-draw.md (1)
7-7: Copy tweak for clarity and consistencyMinor grammar/terminology improvement.
-Promotes Resizable Contextualbars from preview state to stable. +Promote contextual bar resizing from feature preview to stable.packages/ui-client/src/hooks/useFeaturePreviewList.ts (1)
3-3: Guard against legacy entries in stored settingsAfter narrowing FeaturesAvailable, persisted entries like contextualbarResizable may still exist. Filter unknown features at runtime to keep unseenFeatures and merging stable.
Add a guard before computing unseen/merged:
const validFeatures = featuresList?.filter((f) => enabledDefaultFeatures.some((df) => df.name === f.name)) ?? []; // then use validFeatures instead of featuresList belowapps/meteor/tests/e2e/omnichannel/omnichannel-units.spec.ts (1)
87-87: Prefer role-based, web-first assertionUse an accessible role locator to reduce flakiness and align with guidelines.
- await expect(page.locator('h3 >> text="No results found"')).toBeVisible(); + await expect(page.getByRole('heading', { name: 'No results found' })).toBeVisible();
📜 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 ignored due to path filters (4)
apps/meteor/client/omnichannel/cannedResponses/contextualBar/CannedResponse/__snapshots__/CannedResponseList.spec.tsx.snapis excluded by!**/*.snapapps/meteor/client/views/marketplace/AppDetailsPage/tabs/AppLogs/Filters/__snapshots__/AppLogsFilterContextualBar.spec.tsx.snapis excluded by!**/*.snapapps/meteor/client/views/room/contextualBar/RoomMembers/InviteUsers/__snapshots__/InviteUsers.spec.tsx.snapis excluded by!**/*.snapapps/meteor/public/images/featurePreview/resizable-contextual-bar.pngis excluded by!**/*.png
📒 Files selected for processing (11)
.changeset/happy-carpets-draw.md(1 hunks)apps/meteor/client/components/Contextualbar/ContextualbarDialog.tsx(1 hunks)apps/meteor/tests/e2e/feature-preview.spec.ts(0 hunks)apps/meteor/tests/e2e/omnichannel/omnichannel-units.spec.ts(1 hunks)apps/meteor/tests/e2e/page-objects/omnichannel-units.ts(1 hunks)packages/i18n/src/locales/en.i18n.json(0 hunks)packages/i18n/src/locales/nb.i18n.json(0 hunks)packages/i18n/src/locales/nn.i18n.json(0 hunks)packages/i18n/src/locales/pt-BR.i18n.json(0 hunks)packages/i18n/src/locales/sv.i18n.json(0 hunks)packages/ui-client/src/hooks/useFeaturePreviewList.ts(1 hunks)
💤 Files with no reviewable changes (6)
- packages/i18n/src/locales/en.i18n.json
- apps/meteor/tests/e2e/feature-preview.spec.ts
- packages/i18n/src/locales/nb.i18n.json
- packages/i18n/src/locales/sv.i18n.json
- packages/i18n/src/locales/nn.i18n.json
- packages/i18n/src/locales/pt-BR.i18n.json
🧰 Additional context used
📓 Path-based instructions (4)
apps/meteor/tests/e2e/page-objects/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Utilize and place Page Object implementations under apps/meteor/tests/e2e/page-objects/
Files:
apps/meteor/tests/e2e/page-objects/omnichannel-units.ts
apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx}: Write concise, technical TypeScript/JavaScript with accurate typing
Follow DRY by extracting reusable logic into helper functions or page objects
Avoid code comments in the implementation
Files:
apps/meteor/tests/e2e/page-objects/omnichannel-units.tsapps/meteor/tests/e2e/omnichannel/omnichannel-units.spec.ts
apps/meteor/tests/e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,tsx}: Avoid using page.locator(); prefer semantic locators like page.getByRole, page.getByLabel, page.getByText, and page.getByTitle
Store commonly used locators in variables/constants for reuse
Use page.waitFor() with specific conditions and avoid hardcoded timeouts
Implement proper wait strategies for dynamic content
Follow the Page Object Model pattern consistently
Files:
apps/meteor/tests/e2e/page-objects/omnichannel-units.tsapps/meteor/tests/e2e/omnichannel/omnichannel-units.spec.ts
apps/meteor/tests/e2e/**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.spec.ts: All Playwright test files must be located under apps/meteor/tests/e2e/ and use the .spec.ts extension (e.g., login.spec.ts)
Use descriptive test names that clearly communicate expected behavior
Use test.beforeAll() and test.afterAll() for setup and teardown
Use test.step() to organize complex test scenarios
Group related tests in the same file
Utilize Playwright fixtures (test, page, expect) consistently
Prefer web-first assertions (e.g., toBeVisible, toHaveText)
Use expect matchers (toEqual, toContain, toBeTruthy, toHaveLength, etc.) instead of assert statements
Maintain test isolation between test cases
Ensure a clean state for each test execution
Ensure tests run reliably in parallel without shared state conflicts
Files:
apps/meteor/tests/e2e/omnichannel/omnichannel-units.spec.ts
🧠 Learnings (11)
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/page-objects/**/*.ts : Utilize and place Page Object implementations under apps/meteor/tests/e2e/page-objects/
Applied to files:
apps/meteor/tests/e2e/page-objects/omnichannel-units.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx} : Follow the Page Object Model pattern consistently
Applied to files:
apps/meteor/tests/e2e/page-objects/omnichannel-units.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx} : Avoid using page.locator(); prefer semantic locators like page.getByRole, page.getByLabel, page.getByText, and page.getByTitle
Applied to files:
apps/meteor/tests/e2e/page-objects/omnichannel-units.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use descriptive test names that clearly communicate expected behavior
Applied to files:
apps/meteor/tests/e2e/omnichannel/omnichannel-units.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure a clean state for each test execution
Applied to files:
apps/meteor/tests/e2e/omnichannel/omnichannel-units.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Prefer web-first assertions (e.g., toBeVisible, toHaveText)
Applied to files:
apps/meteor/tests/e2e/omnichannel/omnichannel-units.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use expect matchers (toEqual, toContain, toBeTruthy, toHaveLength, etc.) instead of assert statements
Applied to files:
apps/meteor/tests/e2e/omnichannel/omnichannel-units.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/tests/e2e/omnichannel/omnichannel-units.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (test, page, expect) consistently
Applied to files:
apps/meteor/tests/e2e/omnichannel/omnichannel-units.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases
Applied to files:
apps/meteor/tests/e2e/omnichannel/omnichannel-units.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx} : Avoid code comments in the implementation
Applied to files:
apps/meteor/tests/e2e/omnichannel/omnichannel-units.spec.ts
🧬 Code graph analysis (1)
apps/meteor/client/components/Contextualbar/ContextualbarDialog.tsx (1)
apps/meteor/tests/e2e/page-objects/omnichannel-units.ts (1)
contextualBar(72-74)
🔇 Additional comments (1)
apps/meteor/client/components/Contextualbar/ContextualbarDialog.tsx (1)
42-44: Confirm resizer handle orientation; pass position if supportedIf ContextualbarResizable uses bar position (left/right) to place the handle, consider passing it through. Otherwise, ignore.
Example (only if the component supports it):
<ContextualbarResizable defaultWidth={contextualBar} position={position}> <Contextualbar ref={callbackRef} width='100%' position={position} {...dialogProps} {...props} /> </ContextualbarResizable>
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/meteor/client/views/marketplace/AppDetailsPage/tabs/AppLogs/Filters/AppLogsFilterContextualBar.stories.tsx (1)
37-37: Story container fidelity: optionally constrain width to mirror a contextual barTo better simulate the contextual bar environment in Storybook, consider constraining width and hiding overflow.
Apply this diff:
- <Box height='100vh'> {fn()}</Box> + <Box height='100vh' width='380px' minWidth='320px' maxWidth='600px' overflow='hidden'> + {fn()} + </Box>apps/meteor/client/views/room/contextualBar/RoomMembers/InviteUsers/InviteUsers.stories.tsx (1)
18-28: Decorator order: place mockAppRoot first; also consider width constraintsKeeping mockAppRoot as the outermost decorator aligns with other stories and ensures providers wrap the container. While here, optionally constrain Box width for realism.
Apply this diff:
export default { @@ - decorators: [ - (fn) => <Box height='100vh'>{fn()}</Box>, - mockAppRoot() - .withTranslations('en', 'core', { + decorators: [ + mockAppRoot() + .withTranslations('en', 'core', { 'Edit_Invite': 'Edit invite', 'Invite_Users': 'Invite users', 'Invite_Link': 'Invite link', 'Expiration_(Days)': 'Expiration (Days)', 'Max_number_of_uses': 'Max number of uses', }) .buildStoryDecorator(), + (fn) => <Box height='100vh' width='380px' minWidth='320px' maxWidth='600px' overflow='hidden'>{fn()}</Box>, ],
📜 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 ignored due to path filters (3)
apps/meteor/client/omnichannel/cannedResponses/contextualBar/CannedResponse/__snapshots__/CannedResponseList.spec.tsx.snapis excluded by!**/*.snapapps/meteor/client/views/marketplace/AppDetailsPage/tabs/AppLogs/Filters/__snapshots__/AppLogsFilterContextualBar.spec.tsx.snapis excluded by!**/*.snapapps/meteor/client/views/room/contextualBar/RoomMembers/InviteUsers/__snapshots__/InviteUsers.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
apps/meteor/client/omnichannel/cannedResponses/contextualBar/CannedResponse/CannedResponseList.stories.tsx(1 hunks)apps/meteor/client/views/marketplace/AppDetailsPage/tabs/AppLogs/Filters/AppLogsFilterContextualBar.stories.tsx(2 hunks)apps/meteor/client/views/room/contextualBar/RoomMembers/InviteUsers/InviteUsers.stories.tsx(2 hunks)
⏰ 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). (7)
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 📦 Meteor Build - coverage
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (3)
apps/meteor/client/views/marketplace/AppDetailsPage/tabs/AppLogs/Filters/AppLogsFilterContextualBar.stories.tsx (1)
1-1: Box import swap looks goodConsistent with the PR’s direction of decoupling stories from Contextualbar.
apps/meteor/client/views/room/contextualBar/RoomMembers/InviteUsers/InviteUsers.stories.tsx (1)
1-1: Box import addition — OKMatches the move away from Contextualbar wrappers in stories.
apps/meteor/client/omnichannel/cannedResponses/contextualBar/CannedResponse/CannedResponseList.stories.tsx (1)
78-78: Replace all Contextualbar decorators in Storybook stories
All stories importingContextualbarand wrapping content in<Contextualbar height='100vh'>…</Contextualbar>must use the Box-based decorator with width constraints instead:-decorators: [(fn) => <Contextualbar height='100vh'>{fn()}</Contextualbar>]; +decorators: [(fn) => ( + <Box height='100vh' width='380px' minWidth='320px' maxWidth='600px' overflow='hidden'> + {fn()} + </Box> +)];Apply this change across all affected
.stories.tsxfiles.Likely an incorrect or invalid review comment.
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.
LGTM!
Co-authored-by: Douglas Fabris <[email protected]>
Co-authored-by: Douglas Fabris <[email protected]>
Co-authored-by: Douglas Fabris <[email protected]>
Co-authored-by: Douglas Fabris <[email protected]>
Co-authored-by: Douglas Fabris <[email protected]>
Co-authored-by: Douglas Fabris <[email protected]>
Co-authored-by: Douglas Fabris <[email protected]>
Proposed changes (including videos or screenshots)
Move resizable contextualbar outside feature preview
Issue(s)
Steps to test or reproduce
Further comments
CORE-1350
Summary by CodeRabbit