feat: Search rooms by cyrillic characters in channel names #36861
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: 0922022 The changes in this PR will be included in the next version bump. This PR includes changesets to release 39 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
yash-rajpal
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #36861 +/- ##
===========================================
- Coverage 66.26% 66.24% -0.03%
===========================================
Files 3339 3340 +1
Lines 113726 113726
Branches 21170 21140 -30
===========================================
- Hits 75365 75342 -23
- Misses 35673 35698 +25
+ Partials 2688 2686 -2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
@abhinavkrin I think there's a setting that needs to be toggled |
ricardogarim
left a comment
ricardogarim
left a comment
There was a problem hiding this comment.
hey @yash-rajpal, noticed during testing that if a room name starts with cyrillic characters (e.g. тест) it shows up fine in search. but if the name mixes character types (e.g. test тест), searching just for тест doesn’t return it. might be related to the regex being used in search.
These conditions for skipping archived and federated channels were already there, I just merged the previous $and condition with my code for matching For the search only working if initials of room name matching, check the regex here. |
I'm almost sure it was a "perf thing". |
yash-rajpal
removed
the
stat: ready to merge
yash-rajpal
added
the
stat: ready to merge
dionisio-bot
bot
removed
the
stat: ready to merge
WalkthroughAdds a changeset bumping @rocket.chat/models and @rocket.chat/meteor to patch versions. Modifies Rooms query to match name or fname with a regex while excluding federated/archived rooms. Introduces end-to-end API tests validating Cyrillic channel-name autocomplete; includes setup/teardown and duplicated test blocks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API as rooms.autocomplete.channelAndPrivate
participant Service as Rooms Service
participant DB as Rooms Collection
Client->>API: GET autocomplete (selector with query text)
API->>Service: request rooms by query
Service->>DB: findRoomsWithoutDiscussionsByRoomIds({<br/> ids/t filters,<br/> t in [c,p],<br/> $and: [ { $or: [name~regex, fname~regex] }, {federated: {$ne: true}}, {archived: {$ne: true}} ],<br/> team/prid conditions })
DB-->>Service: matching rooms
Service-->>API: rooms list
API-->>Client: response (supports Cyrillic matches)
note over API,Service: New behavior: regex applies to both name and fname
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
✨ Finishing Touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/models/src/models/Rooms.ts (1)
333-358: Ensure parity: paginated variant should also match fname and exclude archived/federated
findPaginatedRoomsWithoutDiscussionsByRoomIdsstill filters bynameonly and doesn’t excludearchived. This can make/rooms.autocomplete.channelAndPrivate.withPaginationbehave differently from the non-paginated endpoint for Cyrillic names and archived rooms.Suggested refactor:
- const nameRegex = new RegExp(`^${escapeRegExp(name).trim()}`, 'i'); + const nameRegex = new RegExp(`^${escapeRegExp(name).trim()}`, 'i'); const query: Filter<IRoom> = { _id: { $in: roomIds, }, t: { $in: ['c', 'p'], }, - name: nameRegex, + $and: [ + { $or: [{ name: nameRegex }, { fname: nameRegex }] }, + { federated: { $ne: true } }, + { archived: { $ne: true } }, + ], $or: [ { teamId: { $exists: false, }, }, { teamId: { $exists: true, }, _id: { $in: roomIds, }, }, ], prid: { $exists: false }, - $and: [{ $or: [{ federated: { $exists: false } }, { federated: false }] }], };
♻️ Duplicate comments (1)
apps/meteor/tests/end-to-end/api/rooms.ts (1)
2161-2175: Add negative cases: archived and federated rooms should not appearPast feedback asked for this—adding assertions will protect against regressions.
Example additions (archive case; adjust API as needed for federated mark):
it('should return the rooms with cyrillic characters in channel name', (done) => { … }); + +it('should not return archived rooms', async () => { + await request.post(api('channels.archive')).set(credentials).send({ roomId: testChannel._id }).expect(200); + await request + .get(api('rooms.autocomplete.channelAndPrivate')) + .query({ selector: '{ "name": "тест" }' }) + .set(credentials) + .expect(200) + .expect((res) => { + expect(res.body.items).to.have.lengthOf(0); + }); +});I can also draft a federated-room setup if there’s a supported API/path in this suite.
🧹 Nitpick comments (4)
packages/models/src/models/Rooms.ts (1)
295-296: Optional: add Unicode flag to regex for robust case-insensitive matchingUsing
iucan improve correctness for non-Latin scripts’ case-folding. Low risk, but keep if consistent with the rest of the codebase.-const nameRegex = new RegExp(`^${escapeRegExp(name).trim()}`, 'i'); +const nameRegex = new RegExp(`^${escapeRegExp(name).trim()}`, 'iu');apps/meteor/tests/end-to-end/api/rooms.ts (2)
2130-2133: Restore original setting value to avoid suite cross-talkCapture the prior value in
beforeand restore it inafterinstead of hardcodingtrue.- before(async () => { - await updateSetting('UI_Allow_room_names_with_special_chars', true); - testChannel = (await createRoom({ type: 'c', name: 'тест' })).body.channel; - }); + let originalSpecialCharsSetting: SettingValue; + before(async () => { + originalSpecialCharsSetting = await getSettingValueById('UI_Allow_room_names_with_special_chars'); + await updateSetting('UI_Allow_room_names_with_special_chars', true); + testChannel = (await createRoom({ type: 'c', name: 'тест' })).body.channel; + }); - after(async () => { - await updateSetting('UI_Allow_room_names_with_special_chars', true); - await deleteRoom({ type: 'c', roomId: testChannel._id }); - }); + after(async () => { + await updateSetting('UI_Allow_room_names_with_special_chars', Boolean(originalSpecialCharsSetting)); + await deleteRoom({ type: 'c', roomId: testChannel._id }); + });
2178-2224: Also verify Cyrillic search in the paginated endpointOnce the model parity is in place, add the same Cyrillic assertion here to keep endpoints consistent.
it('should return the rooms to fill auto complete', (done) => { … }); + +it('should return rooms with cyrillic names (with pagination)', (done) => { + void request + .get(api('rooms.autocomplete.channelAndPrivate.withPagination')) + .query({ selector: '{ "name": "тест" }' }) + .set(credentials) + .expect(200) + .expect((res) => { + expect(res.body).to.have.property('success', true); + expect(res.body).to.have.property('items').and.to.be.an('array'); + expect(res.body.items.find((i: any) => i.fname === 'тест')).to.exist; + }) + .end(done); +});.changeset/angry-apes-double.md (1)
6-6: Good, concise changesetOptional: reference SUP-819 in the note to aid traceability.
-Enable room search by Cyrillic characters in channel names (e.g. "тест"). +Enable room search by Cyrillic characters in channel names (e.g., “тест”). Related: SUP-819.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/angry-apes-double.md(1 hunks)apps/meteor/tests/end-to-end/api/rooms.ts(2 hunks)packages/models/src/models/Rooms.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/tests/end-to-end/api/rooms.ts (3)
apps/meteor/app/lib/server/functions/createRoom.ts (1)
createRoom(120-278)apps/meteor/tests/e2e/utils/create-target-channel.ts (1)
deleteRoom(48-50)apps/meteor/tests/data/api-data.ts (1)
credentials(39-42)
⏰ 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)
packages/models/src/models/Rooms.ts (1)
304-304: Cyrillic match + archived/federated filters look goodMatching on both name/fname with a start-anchored regex and excluding federated/archived rooms aligns with the intended behavior and keeps the “first token only” optimization.
apps/meteor/tests/end-to-end/api/rooms.ts (1)
2123-2133: Great targeted coverage for Cyrillic searchEnables the setting, creates a Cyrillic-named room, and exercises the endpoint—nice.
yash-rajpal
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
Currently room names having Cyrillic characters (e.g., тест) in channel name are not searchable.
Issue(s)
Steps to test or reproduce
Before
After
Further comments
SUP-819
Summary by CodeRabbit
New Features
Tests
Chores