Skip to content

Conversation

@abhinavkrin
Copy link
Member

@abhinavkrin abhinavkrin commented Oct 24, 2025

Proposed changes (including videos or screenshots)

This pull request fixes a regression introduced in v7.7.1 where the Directory search stopped returning private channels that the user is a member of.

The query used by the Directory API relies on the user.__rooms field to identify which rooms a user has joined.
However, this field was not being populated by the authentication handler, resulting in an empty list and causing private channels to be excluded from the Directory search results.

Issue(s)

Steps to test or reproduce

  1. Create a private channel (e.g., Private_Channel) and ensure the test user is a member.
  2. Call:
    curl -X GET "https://<server>/api/v1/directory?text=Private_Channel&type=channels" \
      -H "X-Auth-Token: <token>" \
      -H "X-User-Id: <userid>" \
      -H "Content-type: application/json"
  • Actual (before fix): Returns []
  • Expected (after fix): Returns the private channel object.

Further comments

SUP-913

Summary by CodeRabbit

  • Bug Fixes
    • Directory search now correctly includes private groups a user belongs to, improving channel discovery.
  • Refactor
    • Server directory listing now supplies a restricted view of the current user (limited membership data) when generating results.
  • Tests
    • Added end-to-end test coverage for private group discovery in directory searches and cleanup.
  • Chores
    • Added a patch changeset documenting the directory/private-channel fix.

Copilot AI review requested due to automatic review settings October 24, 2025 11:13
@abhinavkrin abhinavkrin requested a review from a team as a code owner October 24, 2025 11:13
@dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Oct 24, 2025

Looks like this PR is not ready to merge, because of the following issues:

  • This PR is targeting the wrong base branch. It should target 7.13.0, but it targets 7.12.0

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

@changeset-bot
Copy link

changeset-bot bot commented Oct 24, 2025

🦋 Changeset detected

Latest commit: af3f3bb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 41 packages
Name Type
@rocket.chat/meteor Patch
@rocket.chat/core-typings Patch
@rocket.chat/rest-typings Patch
@rocket.chat/uikit-playground Patch
@rocket.chat/api-client Patch
@rocket.chat/apps Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/ddp-client Patch
@rocket.chat/freeswitch Patch
@rocket.chat/fuselage-ui-kit Patch
@rocket.chat/gazzodown Patch
@rocket.chat/http-router Patch
@rocket.chat/livechat Patch
@rocket.chat/model-typings Patch
@rocket.chat/ui-avatar Patch
@rocket.chat/ui-client Patch
@rocket.chat/ui-contexts Patch
@rocket.chat/web-ui-registration Patch
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/stream-hub-service Patch
@rocket.chat/federation-matrix Patch
@rocket.chat/license Patch
@rocket.chat/media-calls Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/models Patch
@rocket.chat/network-broker Patch
@rocket.chat/omni-core-ee Patch
@rocket.chat/mock-providers Patch
@rocket.chat/ui-video-conf Patch
@rocket.chat/ui-voip Patch
@rocket.chat/instance-status Patch
@rocket.chat/omni-core Patch

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

@abhinavkrin abhinavkrin added this to the 7.12.0 milestone Oct 24, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request fixes a regression in v7.7.1 where the Directory search API stopped returning private channels that users are members of. The root cause was the user.__rooms field not being populated by the authentication handler, which is required by the Directory API query to identify joined rooms.

Key Changes:

  • Populates the user.__rooms field with a fallback database query if not present
  • Adds test coverage for private channel directory search functionality

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
apps/meteor/server/methods/browseChannels.ts Ensures user.__rooms is populated before executing directory search logic
apps/meteor/tests/end-to-end/api/miscellaneous.ts Adds test case verifying private channels are returned in directory search results

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@coderabbitai
Copy link

coderabbitai bot commented Oct 24, 2025

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Narrowed public user parameter types in browseChannels.ts to AtLeast<IUser,'_id'|'__rooms'> and removed an unused IRoom import; the directory route now fetches a projected user (only __rooms) before calling the browse method; tests add a private group, parallelize setup, and extend cleanup.

Changes

Cohort / File(s) Summary
Browse channels method — signature and imports
apps/meteor/server/methods/browseChannels.ts
Changed user parameter types to `AtLeast<IUser, '_id'
Directory route — use projected user
apps/meteor/app/api/server/v1/misc.ts
Fetch current user with projection { __rooms: 1 } via Users.findOneById(this.userId, { projection: { __rooms: 1 } }) and pass that projected user to browseChannelsMethod instead of this.user.
End-to-end tests — add private group and parallelize setup
apps/meteor/tests/end-to-end/api/miscellaneous.ts
Added testGroup (private group) creation alongside testChannel and team, parallelized setup with Promise.all, added directory search assertion for the private group, and extended cleanup to remove the group.
Changeset
.changeset/old-cobras-serve.md
Added changeset describing a patch release that fixes directory search not returning private channels the user belongs to.

Sequence Diagram(s)

sequenceDiagram
    actor Client
    participant API as /directory route
    participant UsersDB as Users DB
    participant Browse as browseChannelsMethod
    participant Filter as Channel/Group Filter

    Client->>API: Request directory
    API->>UsersDB: findOneById(userId, projection: { __rooms: 1 })
    UsersDB-->>API: user (includes _id and __rooms)
    API->>Browse: call browseChannelsMethod(user: AtLeast<IUser,'_id'|'__rooms'>)
    Browse->>Browse: use user.__rooms to filter memberships
    alt user.__rooms missing
        Browse->>UsersDB: optionally fetch/populate __rooms
        UsersDB-->>Browse: __rooms populated
    end
    Browse->>Filter: filter channels/groups using membership data
    Filter-->>Browse: results
    Browse-->>API: directory results
    API-->>Client: return results
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Verify all callers of the updated functions supply the required projected user shape (or adjust call sites).
  • Confirm no remaining references to IRoom in the file or related imports.
  • Check tests' Promise.all setup and cleanup for flakiness and proper fixture removal.

Suggested reviewers

  • scuciatto
  • ggazzo

Poem

🐇 I hopped through code with tiny paws,
Found hidden groups behind closed doors,
I kept just keys — light and neat,
Now directory and tests all meet,
A joyful thump — the fix applause! 🎉

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "fix: directory search does not return private channels" directly reflects the primary objective of this changeset. It is concise, specific, and clearly communicates the core issue being resolved—that the Directory API was not returning private channels for users who are members of those channels. The title avoids generic language and noise, making it immediately meaningful to someone reviewing pull request history.
Linked Issues Check ✅ Passed The code changes directly implement all objectives from SUP-913. The PR addresses the root cause by ensuring the user.__rooms field is properly populated and used: browseChannels.ts narrows type definitions to require the __rooms field, misc.ts explicitly fetches the user with a projection including __rooms and passes it to browseChannelsMethod, and miscellaneous.ts adds tests that validate private groups are correctly returned in Directory search results. Together, these changes restore the expected behavior where private channels joined by the requesting user are included in Directory API search results.
Out of Scope Changes Check ✅ Passed All code changes are tightly scoped to fixing Directory search for private channels. The type narrowing in browseChannels.ts makes the __rooms requirement explicit, the user projection in misc.ts ensures the field is populated, the test additions in miscellaneous.ts validate the fix, and the changeset documents the bug fix. No unrelated modifications, refactoring, or feature additions are present that would fall outside the stated objectives of resolving the Directory search regression.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch regression/directory-search-private-channels

📜 Recent 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.

📥 Commits

Reviewing files that changed from the base of the PR and between c426d27 and af3f3bb.

📒 Files selected for processing (4)
  • .changeset/old-cobras-serve.md (1 hunks)
  • apps/meteor/app/api/server/v1/misc.ts (2 hunks)
  • apps/meteor/server/methods/browseChannels.ts (5 hunks)
  • apps/meteor/tests/end-to-end/api/miscellaneous.ts (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/meteor/app/api/server/v1/misc.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
PR: RocketChat/Rocket.Chat#37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.

Applied to files:

  • apps/meteor/server/methods/browseChannels.ts
🧬 Code graph analysis (2)
apps/meteor/tests/end-to-end/api/miscellaneous.ts (3)
packages/core-typings/src/IRoom.ts (1)
  • IRoom (21-95)
apps/meteor/tests/e2e/utils/create-target-channel.ts (2)
  • deleteTeam (66-68)
  • deleteRoom (48-50)
apps/meteor/tests/data/api-data.ts (2)
  • request (10-10)
  • credentials (39-42)
apps/meteor/server/methods/browseChannels.ts (2)
apps/meteor/client/startup/iframeCommands.ts (1)
  • user (80-89)
packages/core-typings/src/IUser.ts (1)
  • IUser (186-255)
⏰ 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 Unit / Unit Tests
  • GitHub Check: 🔨 Test Storybook / Test Storybook
  • GitHub Check: 🔎 Code Check / Code Lint
  • GitHub Check: 🔎 Code Check / TypeScript
  • GitHub Check: 📦 Meteor Build - coverage
  • GitHub Check: CodeQL-Build
  • GitHub Check: CodeQL-Build
🔇 Additional comments (10)
apps/meteor/server/methods/browseChannels.ts (6)

2-2: LGTM! Import changes align with type refactoring.

The import change from IRoom to AtLeast is appropriate given that the user parameter types have been narrowed and IRoom is no longer directly used in this file.


48-48: Appropriate type narrowing for required fields.

The parameter type has been correctly narrowed to AtLeast<IUser, '_id' | '__rooms'>, clearly documenting that only these fields are required by this function. This aligns with the PR objective to ensure __rooms is available for directory search.


65-65: Good defensive coding with nullish coalescing.

The fallback to an empty array is appropriate since __rooms is optional on the base IUser interface and could be undefined at runtime, even though the type signature requires the field to exist.


122-122: Verify if __rooms is required for this function.

While the signature change is consistent with other functions, I notice that getTeams doesn't appear to use the __rooms field directly—it only uses user._id (line 134). Consider whether AtLeast<IUser, '_id'> would be more accurate, unless __rooms is required for consistency or future use.


250-250: Verify if __rooms is required for this function.

Similar to getTeams, the getUsers function only uses user._id (lines 259, 263) and doesn't appear to use __rooms. Consider whether AtLeast<IUser, '_id'> would be more accurate unless __rooms is needed for consistency.


302-302: LGTM! Signature accommodates all downstream functions.

The parameter type correctly requires both _id and __rooms since this function passes the user to getChannelsAndGroups (which uses __rooms), getTeams, and getUsers.

apps/meteor/tests/end-to-end/api/miscellaneous.ts (3)

218-218: LGTM! Added variable for private group test fixture.

The addition of testGroup is necessary for the new test case that verifies private groups appear in directory search results.


227-231: Excellent optimization with parallel setup.

Converting the sequential room and team creation to use Promise.all improves test execution time since these entities can be created independently.


239-239: LGTM! Proper cleanup for private group.

The cleanup correctly deletes the newly created private group with the appropriate type 'p'.

.changeset/old-cobras-serve.md (1)

1-5: LGTM! Changeset accurately describes the fix.

The changeset correctly documents this as a patch-level fix for the Directory search regression affecting private channels.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link

codecov bot commented Oct 24, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.92%. Comparing base (eb631f6) to head (af3f3bb).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #37290      +/-   ##
===========================================
- Coverage    67.92%   67.92%   -0.01%     
===========================================
  Files         3356     3356              
  Lines       114887   114886       -1     
  Branches     20758    20752       -6     
===========================================
- Hits         78036    78033       -3     
- Misses       34163    34165       +2     
  Partials      2688     2688              
Flag Coverage Δ
e2e 57.43% <ø> (+0.03%) ⬆️
unit 71.99% <ø> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@pierre-lehnen-rc pierre-lehnen-rc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These small changes are only because we have an utilitary type (AtLeast) that is better suited for this situation than the standard Pick; otherwise the PR is fine now.

@abhinavkrin abhinavkrin force-pushed the regression/directory-search-private-channels branch from c426d27 to de6a118 Compare October 27, 2025 16:37
@abhinavkrin abhinavkrin requested review from a team as code owners October 27, 2025 16:37
@abhinavkrin abhinavkrin changed the base branch from release-7.12.0 to develop October 27, 2025 16:37
@abhinavkrin abhinavkrin changed the title regression: directory search does not return private channels fix: directory search does not return private channels Oct 27, 2025
Signed-off-by: Abhinav Kumar <[email protected]>
Signed-off-by: Abhinav Kumar <[email protected]>
Signed-off-by: Abhinav Kumar <[email protected]>
@abhinavkrin abhinavkrin force-pushed the regression/directory-search-private-channels branch from de6a118 to 05f4bb4 Compare October 27, 2025 16:40
@pierre-lehnen-rc pierre-lehnen-rc removed request for a team October 27, 2025 22:04
@scuciatto scuciatto removed this from the 7.12.0 milestone Oct 28, 2025
@scuciatto scuciatto added this to the 7.13.0 milestone Oct 28, 2025
@abhinavkrin abhinavkrin added the stat: QA assured Means it has been tested and approved by a company insider label Oct 29, 2025
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Oct 29, 2025
@kodiakhq kodiakhq bot merged commit 1acf6a3 into develop Oct 29, 2025
49 checks passed
@kodiakhq kodiakhq bot deleted the regression/directory-search-private-channels branch October 29, 2025 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stat: QA assured Means it has been tested and approved by a company insider stat: ready to merge PR tested and approved waiting for merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants