fix: slashcommand query incorrectly removing commands from UI #37654
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 |
🦋 Changeset detectedLatest commit: 72ede3a 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 |
WalkthroughBroadens stream-key filtering for app slash-commands, moves React Query enablement and structuralSharing into the hook with inline data processing, makes telemetry POST fire-and-forget, adds QueryClient injection to the mock test builder, and updates tests to inject the client, assert invalidations, and test pagination. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 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)
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 |
d-gubert
force-pushed
the
fix/faulty-slashcommand-query
branch
from
04c6543 to
bb4e679
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🧹 Nitpick comments (3)
packages/mock-providers/src/MockedAppRootBuilder.tsx (1)
263-268: Clean QueryClient initialization with test-friendly defaults.Disabling retries is appropriate for test environments to ensure fast failure detection. The builder pattern with
withQueryClientenables test injection for spying on query operations.Minor: Missing semicolon at line 268 (after the closing brace).
private queryClient = new QueryClient({ defaultOptions: { queries: { retry: false }, mutations: { retry: false }, }, - }) + });apps/meteor/client/hooks/useAppSlashCommands.spec.tsx (2)
3-4: Unused import:waitForElementToBeRemoved.The
waitForElementToBeRemovedimport is not used in the test file.-import { renderHook, waitFor, waitForElementToBeRemoved } from '@testing-library/react'; +import { renderHook, waitFor } from '@testing-library/react';
130-168: Consider addingwithQueryClient(queryClient)for consistency.This test doesn't inject the
queryClientunlike the other event tests (removed, disabled, updated). While the test works because it asserts on the resulting state rather thaninvalidateQueries, adding the injection would improve consistency across all event tests.renderHook(() => useAppSlashCommands(), { wrapper: mockAppRoot() .withJohnDoe() + .withQueryClient(queryClient) .withStream('apps', streamRef) .withEndpoint('GET', '/v1/commands.list', mockGetSlashCommands) .build(), });
📜 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/hooks/useAppSlashCommands.spec.tsx(6 hunks)apps/meteor/client/hooks/useAppSlashCommands.ts(3 hunks)apps/meteor/client/lib/chats/flows/processSlashCommand.ts(1 hunks)packages/mock-providers/src/MockedAppRootBuilder.tsx(2 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/hooks/useAppSlashCommands.tsapps/meteor/client/hooks/useAppSlashCommands.spec.tsxapps/meteor/client/lib/chats/flows/processSlashCommand.tspackages/mock-providers/src/MockedAppRootBuilder.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/hooks/useAppSlashCommands.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/hooks/useAppSlashCommands.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/hooks/useAppSlashCommands.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/hooks/useAppSlashCommands.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/hooks/useAppSlashCommands.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/hooks/useAppSlashCommands.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.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests
Applied to files:
apps/meteor/client/hooks/useAppSlashCommands.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/hooks/useAppSlashCommands.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/hooks/useAppSlashCommands.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/hooks/useAppSlashCommands.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 : Implement proper wait strategies for dynamic content in Playwright tests
Applied to files:
apps/meteor/client/hooks/useAppSlashCommands.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/lib/chats/flows/processSlashCommand.ts (1)
76-78: Fire-and-forget telemetry is appropriate here.Using
voidto make the telemetry POST non-blocking is a reasonable approach since telemetry shouldn't delay command execution. The tradeoff is that telemetry failures will be silent, but this is acceptable for non-critical analytics data.apps/meteor/client/hooks/useAppSlashCommands.ts (1)
32-38: Broadened event filtering correctly addresses the root cause.The change from a fixed set of events to
key.startsWith('command/')ensures all command-related events trigger invalidation. The delete logic forcommand/removedandcommand/disabledwith thetypeof command === 'string'guard is correct.packages/mock-providers/src/MockedAppRootBuilder.tsx (1)
644-650: LGTM!The
withQueryClientmethod follows the existing builder pattern correctly, and thebuild()method properly destructures and uses the class-scoped client.apps/meteor/client/hooks/useAppSlashCommands.spec.tsx (2)
37-51: Good test setup with QueryClient spy.Creating a fresh
QueryClientper test with retries disabled and spying oninvalidateQueriesenables proper isolation and assertion of query invalidation behavior.
76-101: Good coverage for command removal and disable events.Both tests properly verify that the command is deleted from
slashCommands.commandsand thatinvalidateQueriesis called with the correct query key. The newcommand/disabledtest aligns with the implementation change that now handles bothcommand/removedandcommand/disabledevents.Also applies to: 103-128
📦 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 14:05", "12/04 15:16 (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]
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]
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]
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]
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]
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]
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.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]
Statistics (last 13 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Thu, 04 Dec 2025 15:16:33 GMT |
d-gubert
force-pushed
the
fix/faulty-slashcommand-query
branch
2 times, most recently
from
dca34f4 to
27cb4b9
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/client/hooks/useAppSlashCommands.spec.tsx (1)
130-168: Test doesn't verify query invalidation despite title claiming so.The test title states "by invalidating queries" but the test doesn't inject the
queryClientor assert thatinvalidateQuerieswas called, unlike the other event tests.Either update the title to reflect actual behavior, or add the missing verification:
it('should handle command/added event by invalidating queries', async () => { const streamRef: StreamControllerRef<'apps'> = {}; renderHook(() => useAppSlashCommands(), { wrapper: mockAppRoot() .withJohnDoe() + .withQueryClient(queryClient) .withStream('apps', streamRef) .withEndpoint('GET', '/v1/commands.list', mockGetSlashCommands) .build(), });Then add the assertion after verifying commands exist:
expect(slashCommands.commands['/test']).toBeDefined(); expect(slashCommands.commands['/weather']).toBeDefined(); + + expect(queryClient.invalidateQueries).toHaveBeenCalledWith( + expect.objectContaining({ queryKey: ['apps', 'slashCommands'] }) + ); });
🧹 Nitpick comments (1)
packages/mock-providers/src/MockedAppRootBuilder.tsx (1)
265-275: Consider lazy initialization for the default QueryClient.The getter creates a new
QueryClienton every access when_providedQueryClientis undefined. While current usage (single destructure inbuild()) is safe, this could cause subtle issues if the getter is accessed multiple times beforebuild().private _providedQueryClient: QueryClient | undefined; +private _defaultQueryClient: QueryClient | undefined; private get queryClient(): QueryClient { return ( this._providedQueryClient || - new QueryClient({ + (this._defaultQueryClient ??= new QueryClient({ defaultOptions: { queries: { retry: false }, mutations: { retry: false }, }, - }) + })) ); }
📜 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/hooks/useAppSlashCommands.spec.tsx(6 hunks)apps/meteor/client/hooks/useAppSlashCommands.ts(3 hunks)apps/meteor/client/lib/chats/flows/processSlashCommand.ts(1 hunks)packages/mock-providers/src/MockedAppRootBuilder.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/client/hooks/useAppSlashCommands.ts
🧰 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/lib/chats/flows/processSlashCommand.tspackages/mock-providers/src/MockedAppRootBuilder.tsxapps/meteor/client/hooks/useAppSlashCommands.spec.tsx
🧠 Learnings (10)
📓 Common learnings
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
📚 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/hooks/useAppSlashCommands.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/hooks/useAppSlashCommands.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/hooks/useAppSlashCommands.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/hooks/useAppSlashCommands.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/hooks/useAppSlashCommands.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.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests
Applied to files:
apps/meteor/client/hooks/useAppSlashCommands.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/hooks/useAppSlashCommands.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/hooks/useAppSlashCommands.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/hooks/useAppSlashCommands.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). (16)
- GitHub Check: 🔨 Test API (EE) / MongoDB 5.0 (1/1)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (5/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.2 [legacy watchers] coverage (3/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (4/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (1/5)
- GitHub Check: 🔨 Test API (EE) / MongoDB 8.2 coverage (1/1)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (3/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (2/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.2 [legacy watchers] coverage (5/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.2 [legacy watchers] coverage (1/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.2 [legacy watchers] coverage (2/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.2 [legacy watchers] coverage (4/5)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.2 (1/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.2 (3/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.2 (4/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.2 (2/4)
🔇 Additional comments (7)
apps/meteor/client/lib/chats/flows/processSlashCommand.ts (1)
76-78: LGTM! Fire-and-forget telemetry improves performance.The
voidprefix correctly implements non-blocking telemetry. Command execution no longer waits for statistics tracking to complete, improving responsiveness without impacting functionality.packages/mock-providers/src/MockedAppRootBuilder.tsx (2)
651-654: LGTM!The
withQueryClientmethod follows the established builder pattern consistently with otherwith*methods.
656-670: LGTM!The
build()method correctly captures the QueryClient via destructuring and integrates it into the provider hierarchy.apps/meteor/client/hooks/useAppSlashCommands.spec.tsx (4)
37-51: LGTM!Test setup correctly initializes a fresh
QueryClientper test with retries disabled and spies oninvalidateQueriesfor assertions.
76-101: LGTM!The test correctly validates both the synchronous command removal from the registry and the subsequent query invalidation.
103-128: LGTM!New test for
command/disabledevent follows the established pattern and correctly verifies both command removal and query invalidation.
170-196: LGTM!The test correctly verifies query invalidation on
command/updatedevent and confirms commands remain registered.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #37654 +/- ##
===========================================
+ Coverage 68.78% 68.79% +0.01%
===========================================
Files 3363 3362 -1
Lines 114202 114181 -21
Branches 20617 20611 -6
===========================================
- Hits 78556 78553 -3
+ Misses 33551 33532 -19
- Partials 2095 2096 +1
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 (4)
.changeset/fuzzy-teachers-juggle.md (1)
1-6: Changeset scope and description look appropriatePatch bump for both
@rocket.chat/mock-providersand@rocket.chat/meteormatches the nature of the fix, and the description accurately reflects the issue being addressed. If you want to nitpick, you could change “slashcommands” to “slash commands” for readability, but it’s not required.apps/meteor/client/hooks/useAppSlashCommands.spec.tsx (3)
3-3: Consider cleaning up the sharedQueryClientbetween testsCreating a single
QueryClientper test viabeforeEachand spying oninvalidateQueriesis good for deterministic behavior, and disabling retries is appropriate for unit tests. One improvement is to explicitly clear/destroy the client inafterEachto avoid any chance of cached state leaking between tests that usewithQueryClient(queryClient).For example:
afterEach(() => { - jest.clearAllMocks(); + jest.clearAllMocks(); + queryClient.clear(); // or the recommended cleanup API for your React Query version });This keeps each test fully isolated at the cache level as well.
Please double‑check the recommended cleanup API for
QueryClientin@tanstack/react-query@5.65.1in your docs before applying this change.Also applies to: 37-38, 41-48
130-168: Align “command/added” test name with its assertions (or assert invalidation as well)The test name says “should handle command/added event by invalidating queries”, but the assertions only verify that
/newcommand(and the existing commands) end up inslashCommands.commands. That implicitly tests the refetch path but does not assert thatinvalidateQuerieswas actually called, and this test doesn’t inject the sharedqueryClientor spy.To reduce ambiguity:
Either:
- Inject
withQueryClient(queryClient)and add an assertion similar to the other tests:renderHook(() => useAppSlashCommands(), { wrapper: mockAppRoot() .withJohnDoe()
.withQueryClient(queryClient) .withStream('apps', streamRef) .withEndpoint('GET', '/v1/commands.list', mockGetSlashCommands) .build(),
…
streamRef.controller?.emit('apps', [['command/added', ['/newcommand']]]);- await waitFor(() => {
- expect(queryClient.invalidateQueries).toHaveBeenCalledWith(
expect.objectContaining({ queryKey: ['apps', 'slashCommands'] }),- );
- });
- Or, if you prefer to keep it state‑based, rename the test to something like “should add newly fetched command after command/added event” to better match what’s asserted. Please ensure the suggested `invalidateQueries` expectation matches the actual options signature for your `@tanstack/react-query` version. --- `209-234`: **Batch‑fetch test correctly exercises paginated loading; optionally assert call pattern** The new “fetch all commands in batches” test closely mirrors the recursive `fetchBatch` logic in `useAppSlashCommands` and correctly verifies that all 120 commands are ultimately loaded into `slashCommands.commands` when `count` is 50. If you want to document the batching behavior more explicitly, you could additionally assert the call pattern on `mockGetSlashCommands` (e.g., that it’s called three times with offsets `0`, `50`, and `100`), but that’s optional and would couple the test more tightly to the implementation details. Current assertions are sufficient. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **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. <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 27cb4b95d274048222d9f8a76787e09ad033798c and 5e6c356c1771cdc8b3ad820ecb57b79723e3433b. </details> <details> <summary>📒 Files selected for processing (2)</summary> * `.changeset/fuzzy-teachers-juggle.md` (1 hunks) * `apps/meteor/client/hooks/useAppSlashCommands.spec.tsx` (7 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Path-based instructions (1)</summary> <details> <summary>**/*.{ts,tsx,js}</summary> **📄 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/hooks/useAppSlashCommands.spec.tsx` </details> </details><details> <summary>🧠 Learnings (10)</summary> <details> <summary>📚 Learning: 2025-11-24T17:08:17.065Z</summary>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/hooks/useAppSlashCommands.spec.tsx` </details> <details> <summary>📚 Learning: 2025-11-24T17:08:17.065Z</summary>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 : Useexpectmatchers for assertions (toEqual,toContain,toBeTruthy,toHaveLength, etc.) instead ofassertstatements in Playwright tests**Applied to files:** - `apps/meteor/client/hooks/useAppSlashCommands.spec.tsx` </details> <details> <summary>📚 Learning: 2025-11-24T17:08:17.065Z</summary>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/hooks/useAppSlashCommands.spec.tsx` </details> <details> <summary>📚 Learning: 2025-11-24T17:08:17.065Z</summary>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/hooks/useAppSlashCommands.spec.tsx` </details> <details> <summary>📚 Learning: 2025-11-24T17:08:17.065Z</summary>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/hooks/useAppSlashCommands.spec.tsx` </details> <details> <summary>📚 Learning: 2025-11-24T17:08:17.065Z</summary>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 : Usetest.beforeAll()andtest.afterAll()for setup/teardown in Playwright tests**Applied to files:** - `apps/meteor/client/hooks/useAppSlashCommands.spec.tsx` </details> <details> <summary>📚 Learning: 2025-11-24T17:08:17.065Z</summary>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 : Usetest.step()for complex test scenarios to improve organization in Playwright tests**Applied to files:** - `apps/meteor/client/hooks/useAppSlashCommands.spec.tsx` </details> <details> <summary>📚 Learning: 2025-11-24T17:08:17.065Z</summary>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/hooks/useAppSlashCommands.spec.tsx` </details> <details> <summary>📚 Learning: 2025-11-24T17:08:17.065Z</summary>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/hooks/useAppSlashCommands.spec.tsx` </details> <details> <summary>📚 Learning: 2025-11-24T17:08:17.065Z</summary>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 inapps/meteor/tests/e2e/directory**Applied to files:** - `apps/meteor/client/hooks/useAppSlashCommands.spec.tsx` </details> </details><details> <summary>🧬 Code graph analysis (1)</summary> <details> <summary>apps/meteor/client/hooks/useAppSlashCommands.spec.tsx (2)</summary><blockquote> <details> <summary>packages/mock-providers/src/MockedAppRootBuilder.tsx (2)</summary> * `queryClient` (265-275) * `StreamControllerRef` (88-93) </details> <details> <summary>apps/meteor/client/hooks/useAppSlashCommands.ts (1)</summary> * `useAppSlashCommands` (11-66) </details> </blockquote></details> </details> </details> <details> <summary>🔇 Additional comments (1)</summary><blockquote> <details> <summary>apps/meteor/client/hooks/useAppSlashCommands.spec.tsx (1)</summary><blockquote> `82-83`: **Invalidate‑on‑event coverage and QueryClient injection look solid** Using `withQueryClient(queryClient)` together with a spy on `queryClient.invalidateQueries` gives good, direct coverage that `useAppSlashCommands` invalidates the `['apps', 'slashCommands']` query on `command/removed`, `command/disabled`, and `command/updated` events (as implemented in `useAppSlashCommands.ts`). The expectations via `expect.objectContaining({ queryKey: ['apps', 'slashCommands'] })` are appropriately loose while still verifying the key behavior. No changes needed here. If you upgrade `@tanstack/react-query` in the future, please re‑confirm that `invalidateQueries` is still called with an options object containing `queryKey` as used here. Also applies to: 98-101, 103-128, 176-177, 191-191 </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
d-gubert
force-pushed
the
fix/faulty-slashcommand-query
branch
2 times, most recently
from
d858546 to
946dc0a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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 (5)
.changeset/fuzzy-teachers-juggle.md(1 hunks)apps/meteor/client/hooks/useAppSlashCommands.spec.tsx(7 hunks)apps/meteor/client/hooks/useAppSlashCommands.ts(3 hunks)apps/meteor/client/lib/chats/flows/processSlashCommand.ts(1 hunks)packages/mock-providers/src/MockedAppRootBuilder.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/meteor/client/hooks/useAppSlashCommands.spec.tsx
- apps/meteor/client/lib/chats/flows/processSlashCommand.ts
- packages/mock-providers/src/MockedAppRootBuilder.tsx
- .changeset/fuzzy-teachers-juggle.md
🧰 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/hooks/useAppSlashCommands.ts
🧠 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/hooks/useAppSlashCommands.ts
⏰ 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 (1)
apps/meteor/client/hooks/useAppSlashCommands.ts (1)
46-47: Verify necessity ofstructuralSharing: false.Disabling structural sharing prevents React Query from preserving object references between query results, which can cause unnecessary re-renders and increased memory usage. This option is typically only needed for specific edge cases involving mutable data or reference-dependent logic.
Please clarify why
structuralSharing: falseis required here. If it's not essential, consider removing it for better performance:queryKey: ['apps', 'slashCommands'] as const, enabled: !!uid, - structuralSharing: false, queryFn: async () => {
d-gubert
force-pushed
the
fix/faulty-slashcommand-query
branch
from
946dc0a to
361f0ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/meteor/client/lib/chats/flows/processSlashCommand.ts (1)
76-78: Fire‑and‑forget telemetry is fine; consider swallowing rejectionsMaking the telemetry POST non‑blocking with
voidis appropriate here, but ifsdk.rest.postcan reject, this may surface as an unhandled promise rejection. If you truly want to ignore failures, you can locally swallow them:- void sdk.rest.post('/v1/statistics.telemetry', { - params: [{ eventName: 'slashCommandsStats', timestamp: Date.now(), command: commandName }], - }); + void sdk.rest + .post('/v1/statistics.telemetry', { + params: [{ eventName: 'slashCommandsStats', timestamp: Date.now(), command: commandName }], + }) + .catch(() => undefined);packages/mock-providers/src/MockedAppRootBuilder.tsx (1)
263-275: QueryClient injection pattern looks solid; optional caching improvementThe
_providedQueryClient+withQueryClient+build()wiring is correct and gives tests control over theQueryClientinstance. One small optional improvement would be to cache the default client so repeatedbuild()calls on the same builder reuse a single instance:- private get queryClient(): QueryClient { - return ( - this._providedQueryClient || - new QueryClient({ - defaultOptions: { - queries: { retry: false }, - mutations: { retry: false }, - }, - }) - ); - } + private get queryClient(): QueryClient { + if (!this._providedQueryClient) { + this._providedQueryClient = new QueryClient({ + defaultOptions: { + queries: { retry: false }, + mutations: { retry: false }, + }, + }); + } + + return this._providedQueryClient; + }This keeps behavior identical while avoiding creating multiple default clients per builder instance.
Also applies to: 651-671
📜 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 (5)
.changeset/fuzzy-teachers-juggle.md(1 hunks)apps/meteor/client/hooks/useAppSlashCommands.spec.tsx(7 hunks)apps/meteor/client/hooks/useAppSlashCommands.ts(3 hunks)apps/meteor/client/lib/chats/flows/processSlashCommand.ts(1 hunks)packages/mock-providers/src/MockedAppRootBuilder.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/client/hooks/useAppSlashCommands.ts
🧰 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:
packages/mock-providers/src/MockedAppRootBuilder.tsxapps/meteor/client/hooks/useAppSlashCommands.spec.tsxapps/meteor/client/lib/chats/flows/processSlashCommand.ts
🧠 Learnings (12)
📓 Common learnings
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37654
File: apps/meteor/client/hooks/useAppSlashCommands.ts:32-38
Timestamp: 2025-12-02T22:23:49.593Z
Learning: In apps/meteor/client/hooks/useAppSlashCommands.ts, the `data?.forEach((command) => slashCommands.add(command))` call during render is intentional. The query is configured with `structuralSharing: false` to prevent React Query from keeping stable data references, and `slashCommands.add` is idempotent, so executing on every render is acceptable and ensures the command registry stays current.
📚 Learning: 2025-11-19T12:32:29.696Z
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.
Applied to files:
.changeset/fuzzy-teachers-juggle.md
📚 Learning: 2025-11-19T18:20:07.720Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.720Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
Applied to files:
.changeset/fuzzy-teachers-juggle.md
📚 Learning: 2025-12-02T22:23:49.593Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37654
File: apps/meteor/client/hooks/useAppSlashCommands.ts:32-38
Timestamp: 2025-12-02T22:23:49.593Z
Learning: In apps/meteor/client/hooks/useAppSlashCommands.ts, the `data?.forEach((command) => slashCommands.add(command))` call during render is intentional. The query is configured with `structuralSharing: false` to prevent React Query from keeping stable data references, and `slashCommands.add` is idempotent, so executing on every render is acceptable and ensures the command registry stays current.
Applied to files:
.changeset/fuzzy-teachers-juggle.mdapps/meteor/client/hooks/useAppSlashCommands.spec.tsxapps/meteor/client/lib/chats/flows/processSlashCommand.ts
📚 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/hooks/useAppSlashCommands.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/hooks/useAppSlashCommands.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/hooks/useAppSlashCommands.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/hooks/useAppSlashCommands.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/hooks/useAppSlashCommands.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.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests
Applied to files:
apps/meteor/client/hooks/useAppSlashCommands.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/hooks/useAppSlashCommands.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/hooks/useAppSlashCommands.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 (3)
.changeset/fuzzy-teachers-juggle.md (1)
1-6: Changeset text and scope look correctPatch bumps for
@rocket.chat/mock-providersand@rocket.chat/meteorand the description accurately reflect the slash command visibility fix; no issues here.apps/meteor/client/hooks/useAppSlashCommands.spec.tsx (2)
37-49: QueryClient wiring and invalidation assertions look correctUsing a per‑test
QueryClientwith retries disabled, injecting it via.withQueryClient(queryClient), and spying oninvalidateQueriesprovides good coverage of the invalidation behavior forcommand/removed,command/disabled, andcommand/updated. ThewaitForblocks around both the initial population and the invalidation checks are appropriate for the async stream + React Query flow, and state isolation viabeforeEach/afterEachonslashCommands.commandsand mocks looks sound.Also applies to: 76-101, 103-128, 170-192
209-234: Batch fetch test accurately exercises pagination behaviorThe
should fetch all commands in batches if total exceeds counttest correctly simulates a paginated/v1/commands.listendpoint viamockImplementationwith{ offset, count }and asserts thatuseAppSlashCommandsiterates until all 120 commands are registered inslashCommands.commands. This directly guards the regression around incomplete lists on larger totals; no issues found.
d-gubert
force-pushed
the
fix/faulty-slashcommand-query
branch
from
cf68868 to
a6fcfad
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/client/hooks/useAppSlashCommands.ts (1)
8-8: Unify query keys by reusingappsQueryKeys.slashCommands()ininvalidateQueries.Behavior is correct because
['apps', 'slashCommands']matches the same query, but now that you’ve introducedappsQueryKeys.slashCommands()foruseQuery, it would be cleaner and less error‑prone to reuse it in the invalidation as well.You could do:
- const invalidate = useDebouncedCallback( - () => { - queryClient.invalidateQueries({ - queryKey: ['apps', 'slashCommands'], - }); - }, + const invalidate = useDebouncedCallback( + () => { + queryClient.invalidateQueries({ + queryKey: appsQueryKeys.slashCommands(), + }); + },This also fully addresses Martin’s earlier suggestion to move off hardcoded keys.
Also applies to: 18-26, 48-50
📜 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/hooks/useAppSlashCommands.ts(3 hunks)apps/meteor/client/lib/queryKeys.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/client/lib/queryKeys.ts
🧰 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/hooks/useAppSlashCommands.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37654
File: apps/meteor/client/hooks/useAppSlashCommands.ts:32-38
Timestamp: 2025-12-02T22:23:49.593Z
Learning: In apps/meteor/client/hooks/useAppSlashCommands.ts, the `data?.forEach((command) => slashCommands.add(command))` call during render is intentional. The query is configured with `structuralSharing: false` to prevent React Query from keeping stable data references, and `slashCommands.add` is idempotent, so executing on every render is acceptable and ensures the command registry stays current.
📚 Learning: 2025-12-02T22:23:49.593Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37654
File: apps/meteor/client/hooks/useAppSlashCommands.ts:32-38
Timestamp: 2025-12-02T22:23:49.593Z
Learning: In apps/meteor/client/hooks/useAppSlashCommands.ts, the `data?.forEach((command) => slashCommands.add(command))` call during render is intentional. The query is configured with `structuralSharing: false` to prevent React Query from keeping stable data references, and `slashCommands.add` is idempotent, so executing on every render is acceptable and ensures the command registry stays current.
Applied to files:
apps/meteor/client/hooks/useAppSlashCommands.ts
📚 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/hooks/useAppSlashCommands.ts
📚 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/hooks/useAppSlashCommands.ts
📚 Learning: 2025-10-24T17:32:05.348Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
Applied to files:
apps/meteor/client/hooks/useAppSlashCommands.ts
🧬 Code graph analysis (1)
apps/meteor/client/hooks/useAppSlashCommands.ts (1)
apps/meteor/client/lib/queryKeys.ts (1)
appsQueryKeys(122-125)
⏰ 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 (2)
apps/meteor/client/hooks/useAppSlashCommands.ts (2)
33-38: Event filtering and removal logic forcommand/*notifications looks solid.Broadening to
key.startsWith('command/')plus explicitly deleting oncommand/removedandcommand/disabled(while still invalidating the query for allcommand/*) aligns with the described bug and keeps behavior robust even if new command events are added server‑side.Also applies to: 41-41
49-51: Render‑phase registration withstructuralSharing: falseis consistent with the intended design.Using
enabled: !!uidwithstructuralSharing: falseand then callingdata?.forEach((command) => slashCommands.add(command))in the render phase matches the documented approach for this hook: each refetch produces a new data reference,slashCommands.addis idempotent, and this guarantees the singleton stays in sync without depending on React Query’s structural sharing behavior.Based on learnings, this pattern is intentional and acceptable for this hook.
Also applies to: 69-69
fix: query client on MockAppRootBuilder
This reverts commit 7afc79e.
d-gubert
force-pushed
the
fix/faulty-slashcommand-query
branch
from
9b9d415 to
ce80521
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/client/hooks/useAppSlashCommands.ts (1)
48-50:useQueryconfig + render-phase registration align with the singleton design; consider trimming the long rationale comment
enabled: !!uidandstructuralSharing: falseon theuseQuerycall, combined withdata?.forEach((command) => slashCommands.add(command)), implement the intended strategy of always re-registering commands from the latest query result while avoiding React Query structural sharing pitfalls; the render-phase registration is acceptable here given the idempotent, non-rendering nature ofslashCommands.add. The large JSDoc-style block is useful to justify this non-obvious pattern, but if you want to keep closer to the “avoid code comments in implementation” guideline, you could move most of the rationale into higher-level docs or shorten the inline comment to a brief note about the deliberate render-phase mutation. Based on learnings, this render-phase pattern itself does not require changes. As per coding guidelines, consider tightening or relocating the explanatory comment.If you haven’t recently, please confirm in the current @tanstack/react-query v5 docs that
structuralSharing: falseremains the recommended way to disable structural sharing for this query.Also applies to: 69-80
📜 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 (1)
apps/meteor/client/hooks/useAppSlashCommands.ts(4 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/hooks/useAppSlashCommands.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37654
File: apps/meteor/client/hooks/useAppSlashCommands.ts:32-38
Timestamp: 2025-12-02T22:23:49.593Z
Learning: In apps/meteor/client/hooks/useAppSlashCommands.ts, the `data?.forEach((command) => slashCommands.add(command))` call during render is intentional. The query is configured with `structuralSharing: false` to prevent React Query from keeping stable data references, and `slashCommands.add` is idempotent, so executing on every render is acceptable and ensures the command registry stays current.
📚 Learning: 2025-12-02T22:23:49.593Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37654
File: apps/meteor/client/hooks/useAppSlashCommands.ts:32-38
Timestamp: 2025-12-02T22:23:49.593Z
Learning: In apps/meteor/client/hooks/useAppSlashCommands.ts, the `data?.forEach((command) => slashCommands.add(command))` call during render is intentional. The query is configured with `structuralSharing: false` to prevent React Query from keeping stable data references, and `slashCommands.add` is idempotent, so executing on every render is acceptable and ensures the command registry stays current.
Applied to files:
apps/meteor/client/hooks/useAppSlashCommands.ts
📚 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/hooks/useAppSlashCommands.ts
📚 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/hooks/useAppSlashCommands.ts
📚 Learning: 2025-10-24T17:32:05.348Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
Applied to files:
apps/meteor/client/hooks/useAppSlashCommands.ts
🧬 Code graph analysis (1)
apps/meteor/client/hooks/useAppSlashCommands.ts (1)
apps/meteor/client/lib/queryKeys.ts (1)
appsQueryKeys(122-125)
⏰ 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 (2)
apps/meteor/client/hooks/useAppSlashCommands.ts (2)
8-8: Consistent query key usage viaappsQueryKeys.slashCommands()looks goodSwitching from hard-coded
['apps', 'slashCommands']toappsQueryKeys.slashCommands()for both invalidation and the query itself centralizes the key definition and aligns with the shared queryKeys utilities; no changes needed here.Also applies to: 21-22, 48-48
32-42: Command stream filtering and explicit removal/disable handling match the desired behaviorRestricting processing to
key.startsWith('command/')ensures only command-related events trigger work, and proactively deleting entries on'command/removed'/'command/disabled'keeps theslashCommandsregistry in sync before the debounced invalidation refetch completes. Thetypeof command === 'string'guard is a reasonable safety check.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/meteor/client/hooks/useAppSlashCommands.spec.tsx (1)
37-49: QueryClient injection and invalidation assertions are well‑structured; consider reusing appsQueryKeys in testsThe tests now:
- Instantiate a
QueryClientwith retries disabled for deterministic behavior.- Inject it via
withQueryClient(queryClient)intomockAppRoot.- Spy on
invalidateQueriesand assert it is called aftercommand/removed,command/disabled, andcommand/updatedevents, matching the hook’s behavior.To avoid duplicating the query key shape and keep tests aligned with future changes, you could import and assert against
appsQueryKeys.slashCommands()instead of a hard‑coded['apps', 'slashCommands'], for example:-import { QueryClient } from '@tanstack/react-query'; +import { QueryClient } from '@tanstack/react-query'; +import { appsQueryKeys } from '../lib/queryKeys'; @@ - await waitFor(() => { - expect(queryClient.invalidateQueries).toHaveBeenCalledWith(expect.objectContaining({ queryKey: ['apps', 'slashCommands'] })); - }); + await waitFor(() => { + expect(queryClient.invalidateQueries).toHaveBeenCalledWith( + expect.objectContaining({ queryKey: appsQueryKeys.slashCommands() }), + ); + });This keeps the tests resilient if the underlying key structure changes.
Also applies to: 76-101, 170-192
🧹 Nitpick comments (2)
apps/meteor/client/lib/chats/flows/processSlashCommand.ts (1)
76-78: Fire‑and‑forget telemetry is fine; consider swallowing rejections explicitlySwitching the telemetry POST to fire‑and‑forget avoids breaking slash commands on stats failures, which is good. To avoid potential unhandled promise rejections if the endpoint errors, you could optionally add a no‑op catch:
- void sdk.rest.post('/v1/statistics.telemetry', { - params: [{ eventName: 'slashCommandsStats', timestamp: Date.now(), command: commandName }], - }); + void sdk.rest + .post('/v1/statistics.telemetry', { + params: [{ eventName: 'slashCommandsStats', timestamp: Date.now(), command: commandName }], + }) + .catch(() => undefined);packages/mock-providers/src/MockedAppRootBuilder.tsx (1)
263-275: QueryClient injection design is sound; consider caching the default clientThe new
_providedQueryClient+queryClientgetter andwithQueryClientAPI cleanly enable tests to inject and spy on a sharedQueryClient, andbuild()correctly uses that instance via destructuring.If this getter ever gets reused in more places, you might pre‑allocate and cache the default client instead of constructing it in the getter, e.g.:
- private _providedQueryClient: QueryClient | undefined; + private _providedQueryClient: QueryClient | undefined; + private _defaultQueryClient = new QueryClient({ + defaultOptions: { + queries: { retry: false }, + mutations: { retry: false }, + }, + }); @@ private get queryClient(): QueryClient { - return ( - this._providedQueryClient || - new QueryClient({ - defaultOptions: { - queries: { retry: false }, - mutations: { retry: false }, - }, - }) - ); + return this._providedQueryClient || this._defaultQueryClient; }Not required for current usage, but it future‑proofs against accidental multiple instantiations.
Also applies to: 651-670
📜 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 (6)
.changeset/fuzzy-teachers-juggle.md(1 hunks)apps/meteor/client/hooks/useAppSlashCommands.spec.tsx(7 hunks)apps/meteor/client/hooks/useAppSlashCommands.ts(4 hunks)apps/meteor/client/lib/chats/flows/processSlashCommand.ts(1 hunks)apps/meteor/client/lib/queryKeys.ts(1 hunks)packages/mock-providers/src/MockedAppRootBuilder.tsx(2 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/lib/chats/flows/processSlashCommand.tspackages/mock-providers/src/MockedAppRootBuilder.tsxapps/meteor/client/hooks/useAppSlashCommands.spec.tsxapps/meteor/client/hooks/useAppSlashCommands.tsapps/meteor/client/lib/queryKeys.ts
🧠 Learnings (16)
📓 Common learnings
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37654
File: apps/meteor/client/hooks/useAppSlashCommands.ts:32-38
Timestamp: 2025-12-02T22:23:49.593Z
Learning: In apps/meteor/client/hooks/useAppSlashCommands.ts, the `data?.forEach((command) => slashCommands.add(command))` call during render is intentional. The query is configured with `structuralSharing: false` to prevent React Query from keeping stable data references, and `slashCommands.add` is idempotent, so executing on every render is acceptable and ensures the command registry stays current.
📚 Learning: 2025-12-02T22:23:49.593Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37654
File: apps/meteor/client/hooks/useAppSlashCommands.ts:32-38
Timestamp: 2025-12-02T22:23:49.593Z
Learning: In apps/meteor/client/hooks/useAppSlashCommands.ts, the `data?.forEach((command) => slashCommands.add(command))` call during render is intentional. The query is configured with `structuralSharing: false` to prevent React Query from keeping stable data references, and `slashCommands.add` is idempotent, so executing on every render is acceptable and ensures the command registry stays current.
Applied to files:
apps/meteor/client/lib/chats/flows/processSlashCommand.tsapps/meteor/client/hooks/useAppSlashCommands.spec.tsxapps/meteor/client/hooks/useAppSlashCommands.ts.changeset/fuzzy-teachers-juggle.mdapps/meteor/client/lib/queryKeys.ts
📚 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/hooks/useAppSlashCommands.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/hooks/useAppSlashCommands.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/hooks/useAppSlashCommands.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/hooks/useAppSlashCommands.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/hooks/useAppSlashCommands.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.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests
Applied to files:
apps/meteor/client/hooks/useAppSlashCommands.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/hooks/useAppSlashCommands.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/hooks/useAppSlashCommands.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 : Implement proper wait strategies for dynamic content in Playwright tests
Applied to files:
apps/meteor/client/hooks/useAppSlashCommands.spec.tsx
📚 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/hooks/useAppSlashCommands.ts
📚 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/hooks/useAppSlashCommands.ts
📚 Learning: 2025-10-24T17:32:05.348Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
Applied to files:
apps/meteor/client/hooks/useAppSlashCommands.ts
📚 Learning: 2025-11-19T12:32:29.696Z
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.
Applied to files:
.changeset/fuzzy-teachers-juggle.md
📚 Learning: 2025-11-19T18:20:07.720Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.720Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
Applied to files:
.changeset/fuzzy-teachers-juggle.md
🧬 Code graph analysis (1)
apps/meteor/client/hooks/useAppSlashCommands.ts (1)
apps/meteor/client/lib/queryKeys.ts (1)
appsQueryKeys(122-125)
⏰ 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). (6)
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 📦 Meteor Build (coverage)
- GitHub Check: CodeQL-Build
🔇 Additional comments (5)
.changeset/fuzzy-teachers-juggle.md (1)
1-6: Changeset content and package scopes look consistentPackages and patch level align with the described fix for slashcommand disappearance in HA scenarios. Nothing to change here.
apps/meteor/client/lib/queryKeys.ts (1)
121-125: appsQueryKeys shape is consistent with existing query key patternsThe
appsQueryKeysstructure follows the same convention as other query key groups and cleanly centralizes the['apps', 'slashCommands']key for reuse.apps/meteor/client/hooks/useAppSlashCommands.ts (2)
18-26: Stream handling and invalidation now correctly cover all command eventsUsing
appsQueryKeys.slashCommands()ininvalidateQueriesand broadening the stream filter tokey.startsWith('command/')ensures everycommand/*event drives a refresh. Explicitly deletingslashCommands.commands[command]oncommand/removedandcommand/disabledkeeps the singleton in sync immediately before the refetch.Also applies to: 33-42
47-51: Query configuration and registration strategy match the intended caching behaviorThe query now:
- Uses
appsQueryKeys.slashCommands()as key.- Is gated with
enabled: !!uid, so no fetch without a logged‑in user.- Sets
structuralSharing: false, ensuring each refetch yields a new data reference.Combined with the documented, idempotent
data?.forEach((command) => slashCommands.add(command))in render, this achieves the desired behavior of always re‑registering commands while avoiding structural sharing pitfalls described in the PR and prior discussion.Also applies to: 69-81
apps/meteor/client/hooks/useAppSlashCommands.spec.tsx (1)
209-234: Batch fetch test accurately exercises paginated slash command loadingThe new test that mocks 120 commands and pages them in chunks via
offset/countis a good coverage addition. It validates thatuseAppSlashCommandskeeps fetching untiltotalis reached and thatslashCommands.commandsends up fully populated.
d-gubert
force-pushed
the
fix/faulty-slashcommand-query
branch
from
ce80521 to
72ede3a
Compare
ggazzo
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
Fixed an issue in the
useAppSlashCommandshook where slash commands could be removed from theslashCommandssingleton and never re‑added when the React Query cache considered the server response unchanged.This could occur when multiple
command/*notifications were received in a short period of time and the invalidation of the slash commands query was debounced. In that case, by the time the client refetched/v1/commands.list, the resulting list of commands was structurally equal to the cached data fromuseQuery. Because React Query applied structural sharing, it reused the same data reference and did not trigger the effect that repopulated theslashCommandssingleton, leaving some commands missing from the UI.A typical example is when an app is updated:
command/removednotification.command/added(or similarcommand/*) notification.Because these notifications can arrive within a very short window,
queryClient.invalidateQueries()may be called only once. When the client subsequently refetchescommands.list, the app’s command has already been removed and re‑added, so the final list of commands is effectively the same as before. With structural sharing enabled, React Query keeps a stable reference to the cached result, preventing the hook from re‑adding the commands to theslashCommandssingleton and causing them to disappear from the user’s UI.This change ensures that:
command/*events invalidate theappsQueryKeys.slashCommandsquery.slashCommandsforcommand/removedandcommand/disabledevents.structuralSharing: falseso that refetches always produce a new data reference, guaranteeing that the commands are re‑registered in theslashCommandssingleton, even when the server response is structurally identical.Issue(s)
ARCH-1905
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Bug Fixes
Performance
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.