feat(search): enable click-to-jump on search result messages (#39079)#39081
feat(search): enable click-to-jump on search result messages (#39079)#39081TechByAnshuu wants to merge 17 commits intoRocketChat:developfrom
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: b73993e 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughModify RoomMessage click handling to support jump-to-message: clicks on non-interactive parts set a query-string parameter to locate the message, while preserving selection mode behavior and ignoring clicks on buttons, links, checkboxes, and the message toolbar. Added a changeset documenting the patch. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
bot
added
the
type: feature
…hat#39079) Added onClick handler to RoomMessage that calls setMessageJumpQueryStringParameter when context is truthy. Guards clicks on inner interactive elements (buttons, links, toolbar) so they continue to work correctly. Added cursor: pointer when context is active. Works for search, pinned, starred and mentions automatically. Zero new hooks, zero backend changes. Existing pipeline handles cross-room navigation, unloaded history, and highlight. Closes RocketChat#39079
TechByAnshuu
force-pushed
the
feat/search-result-click-navigation
branch
from
323be8c to
b7aa701
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/meteor/client/components/message/variants/RoomMessage.tsx (1)
23-23: Consider grouping imports by path depth.This import goes up 3 levels (
../../../lib/utils/...) but is placed after shallower imports (../...on lines 19-22). Moving it near lines 10-17 would align with the existing depth-based ordering pattern.Suggested import reorganization
import { useIsSelecting, useToggleSelect, useIsSelectedMessage, useCountSelected, } from '../../../views/room/MessageList/contexts/SelectedMessagesContext'; import { useJumpToMessage } from '../../../views/room/MessageList/hooks/useJumpToMessage'; +import { setMessageJumpQueryStringParameter } from '../../../lib/utils/setMessageJumpQueryStringParameter'; import Emoji from '../../Emoji'; import IgnoredContent from '../IgnoredContent'; import MessageHeader from '../MessageHeader'; import MessageToolbarHolder from '../MessageToolbarHolder'; import StatusIndicators from '../StatusIndicators'; -import { setMessageJumpQueryStringParameter } from '../../../lib/utils/setMessageJumpQueryStringParameter'; import RoomMessageContent from './room/RoomMessageContent';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/components/message/variants/RoomMessage.tsx` at line 23, The import ordering is inconsistent: move the import of setMessageJumpQueryStringParameter into the block of deeper-level imports so it groups with other imports of similar ../../.. path depth in RoomMessage.tsx; update the import ordering so setMessageJumpQueryStringParameter appears alongside other deep relative imports (keeping the existing alphabetical/organizational convention) to match the project's depth-based grouping.apps/meteor/tests/end-to-end/api/incoming-integrations.ts (1)
170-315: Consider extracting a shared request builder for alias testsThese cases duplicate the same
integrations.createpayload shape many times. A small helper would reduce noise and make future rule updates easier.♻️ Example refactor direction
+const createIncoming = (alias?: string) => + request + .post(api('integrations.create')) + .set(credentials) + .send({ + type: 'webhook-incoming', + name: 'Incoming test', + enabled: true, + ...(alias !== undefined ? { alias } : {}), + username: 'rocket.cat', + scriptEnabled: false, + channel: '#general', + }) + .expect('Content-Type', 'application/json');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/end-to-end/api/incoming-integrations.ts` around lines 170 - 315, Tests repeatedly build nearly identical payloads for the integrations.create endpoint (see calls to api('integrations.create') that set alias/name/username/channel and subsequent removeIntegration cleanup), making the suite noisy and harder to update; extract a small helper (e.g., buildIncomingIntegrationPayload or createIncomingIntegrationRequest) that accepts overrides (alias, name, enabled, scriptEnabled, etc.) and use it across the tests instead of duplicating the .send({...}) objects, then update each spec (including those referencing ALIAS_MAX_LENGTH and calling removeIntegration) to call the helper and only pass the differing fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/client/components/message/variants/RoomMessage.tsx`:
- Line 23: The import ordering is inconsistent: move the import of
setMessageJumpQueryStringParameter into the block of deeper-level imports so it
groups with other imports of similar ../../.. path depth in RoomMessage.tsx;
update the import ordering so setMessageJumpQueryStringParameter appears
alongside other deep relative imports (keeping the existing
alphabetical/organizational convention) to match the project's depth-based
grouping.
In `@apps/meteor/tests/end-to-end/api/incoming-integrations.ts`:
- Around line 170-315: Tests repeatedly build nearly identical payloads for the
integrations.create endpoint (see calls to api('integrations.create') that set
alias/name/username/channel and subsequent removeIntegration cleanup), making
the suite noisy and harder to update; extract a small helper (e.g.,
buildIncomingIntegrationPayload or createIncomingIntegrationRequest) that
accepts overrides (alias, name, enabled, scriptEnabled, etc.) and use it across
the tests instead of duplicating the .send({...}) objects, then update each spec
(including those referencing ALIAS_MAX_LENGTH and calling removeIntegration) to
call the helper and only pass the differing fields.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/meteor/client/components/message/variants/RoomMessage.tsxapps/meteor/client/views/admin/integrations/incoming/IncomingWebhookForm.tsxapps/meteor/tests/end-to-end/api/incoming-integrations.tspackages/core-typings/src/index.tspackages/core-typings/src/integrations/constants.tspackages/core-typings/tsconfig.jsonpackages/i18n/src/locales/en.i18n.jsonpackages/rest-typings/src/v1/integrations/IntegrationsCreateProps.tspackages/rest-typings/src/v1/integrations/IntegrationsUpdateProps.ts
📜 Review details
⏰ 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). (1)
- GitHub Check: cubic · AI code reviewer
🧰 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/core-typings/src/index.tsapps/meteor/client/components/message/variants/RoomMessage.tsxapps/meteor/client/views/admin/integrations/incoming/IncomingWebhookForm.tsxpackages/core-typings/src/integrations/constants.tsapps/meteor/tests/end-to-end/api/incoming-integrations.tspackages/rest-typings/src/v1/integrations/IntegrationsUpdateProps.tspackages/rest-typings/src/v1/integrations/IntegrationsCreateProps.ts
🧠 Learnings (9)
📓 Common learnings
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82
Timestamp: 2026-02-24T19:36:55.089Z
Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
📚 Learning: 2026-02-24T19:36:55.089Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82
Timestamp: 2026-02-24T19:36:55.089Z
Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').
Applied to files:
apps/meteor/client/components/message/variants/RoomMessage.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 **/*.{ts,tsx,js} : Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Applied to files:
packages/core-typings/tsconfig.json
📚 Learning: 2026-02-25T20:10:07.657Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:07.657Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
packages/core-typings/tsconfig.json
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
packages/core-typings/tsconfig.jsonapps/meteor/tests/end-to-end/api/incoming-integrations.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:
packages/i18n/src/locales/en.i18n.json
📚 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/tests/end-to-end/api/incoming-integrations.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 : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/tests/end-to-end/api/incoming-integrations.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/tests/end-to-end/api/incoming-integrations.ts
🧬 Code graph analysis (3)
apps/meteor/client/views/admin/integrations/incoming/IncomingWebhookForm.tsx (1)
packages/core-typings/src/integrations/constants.ts (2)
ALIAS_MAX_LENGTH(1-1)ALIAS_ALLOWED_PATTERN(3-3)
packages/rest-typings/src/v1/integrations/IntegrationsUpdateProps.ts (1)
packages/core-typings/src/integrations/constants.ts (2)
ALIAS_MAX_LENGTH(1-1)ALIAS_ALLOWED_PATTERN(3-3)
packages/rest-typings/src/v1/integrations/IntegrationsCreateProps.ts (1)
packages/core-typings/src/integrations/constants.ts (2)
ALIAS_MAX_LENGTH(1-1)ALIAS_ALLOWED_PATTERN(3-3)
🔇 Additional comments (10)
apps/meteor/client/components/message/variants/RoomMessage.tsx (1)
73-86: Clean implementation of click-to-jump with appropriate guards.The handler correctly:
- Prioritizes selection mode with early return.
- Guards interactive elements (
button,a,.rcx-checkbox,.rcx-message-toolbar) to preserve their native behavior.- Conditionally sets cursor to indicate interactivity.
The
target.closest()approach is idiomatic and handles nested interactive elements properly.packages/core-typings/src/index.ts (1)
26-26: LGTM!The new export is well-placed alongside related integration exports (
IIntegration,IIntegrationHistory) and follows the existing barrel export pattern.packages/core-typings/tsconfig.json (2)
11-15: LGTM on the typia transform plugin.The plugin configuration for typia's transform is correctly formatted. This enables compile-time generation of runtime type validators.
4-7:⚠️ Potential issue | 🔴 CriticalRemove DOM type references or restore DOM lib configuration.
The
libconfiguration removes"dom"and"dom.iterable", but the package source contains unresolvedDOM type references that depend on these libraries:
packages/core-typings/src/IUser.tsusesBlobandFormData(lines 296, 302)packages/core-typings/src/IOembed.tsusesURL(line 11)packages/core-typings/src/omnichannel/sms.tsusesRequest(line 29)These types are not explicitly imported and rely on
lib.domfor resolution. Compilation will fail with the current configuration. Either restore the DOM libraries or add explicit imports for these types.⛔ Skipped due to learnings
Learnt from: ahmed-n-abdeltwab Repo: RocketChat/Rocket.Chat PR: 38913 File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34 Timestamp: 2026-02-25T20:10:07.657Z Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.Learnt from: tassoevan Repo: RocketChat/Rocket.Chat PR: 38219 File: packages/core-typings/src/cloud/Announcement.ts:5-6 Timestamp: 2026-01-17T01:51:47.764Z Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.Learnt from: KevLehman Repo: RocketChat/Rocket.Chat PR: 37091 File: ee/packages/abac/jest.config.ts:4-7 Timestamp: 2025-12-10T21:00:54.909Z Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.packages/i18n/src/locales/en.i18n.json (1)
483-483: LGTM: alias validation copy is clear and correctly placed.The new translation key/value is concise and fits the existing alias-related messaging pattern.
Based on learnings: new translation keys should be added to
packages/i18n/src/locales/en.i18n.jsononly.packages/core-typings/src/integrations/constants.ts (1)
1-6: Good centralization of alias constraintsThis is a clean source-of-truth addition and helps keep client/server validation aligned.
packages/rest-typings/src/v1/integrations/IntegrationsCreateProps.ts (1)
89-93: Alias validation is correctly enforced in both create schemasApplying the same max-length and character-pattern checks to incoming and outgoing create payloads keeps API behavior consistent.
Also applies to: 195-199
packages/rest-typings/src/v1/integrations/IntegrationsUpdateProps.ts (1)
95-99: Nice parity between create and update alias rulesBringing the same alias constraints to update schemas prevents drift between creation and edit flows.
Also applies to: 209-213
apps/meteor/tests/end-to-end/api/incoming-integrations.ts (1)
170-315: Strong alias-validation coverage addedThe new cases hit key boundaries and acceptance/rejection paths (length, charset, Unicode, optional/empty alias) well.
apps/meteor/client/views/admin/integrations/incoming/IncomingWebhookForm.tsx (1)
233-267: Alias field validation and UX updates look solidClient-side rules now match backend constraints, and the inline counter + error state wiring improves form feedback.
coderabbitai
bot
added
community
and removed
type: feature
|
Addressed the two bot findings:
Ready for review. |
coderabbitai
bot
added
type: feature
|
Addressed coderabbitai feedback: Fixed import ordering in RoomMessage.tsx — moved Ready for review. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/client/components/message/variants/RoomMessage.tsx`:
- Around line 78-84: The click-guard in RoomMessage.tsx that checks
target.closest('button, a, .rcx-checkbox, .rcx-message-toolbar') is missing
elements using role="button" (e.g., MessageAvatar), so add [role="button"] to
that selector used in the click handler (the block that calls
setMessageJumpQueryStringParameter(message._id)) so clicks on non-button
elements with role="button" (like MessageAvatar) are ignored and won't trigger
the jump navigation.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/client/components/message/variants/RoomMessage.tsx
📜 Review details
⏰ 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). (1)
- GitHub Check: cubic · AI code reviewer
🧰 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/components/message/variants/RoomMessage.tsx
🧠 Learnings (2)
📓 Common learnings
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82
Timestamp: 2026-02-24T19:36:55.089Z
Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
📚 Learning: 2026-02-24T19:36:55.089Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82
Timestamp: 2026-02-24T19:36:55.089Z
Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').
Applied to files:
apps/meteor/client/components/message/variants/RoomMessage.tsx
🧬 Code graph analysis (1)
apps/meteor/client/components/message/variants/RoomMessage.tsx (1)
packages/core-services/src/types/ServiceClass.ts (1)
context(96-98)
🔇 Additional comments (1)
apps/meteor/client/components/message/variants/RoomMessage.tsx (1)
73-86: Overall implementation approach looks solid.The click-to-jump logic correctly:
- Preserves selection mode behavior by checking
selectingfirst- Guards against clicks on interactive elements before triggering navigation
- Uses the existing jump pipeline via
setMessageJumpQueryStringParameter- Applies cursor styling conditionally
Aside from the
[role="button"]selector gap noted above, this is a clean, minimal change that leverages existing infrastructure well.
|
Fixed — added [role="button"] to the click guard selector. MessageAvatar uses role="button" but is not a native button Full guard selector is now: All child components of RoomMessage verified — no other |
fix(ci): fix CI pipeline failuresWhat does this PR do?Fixes CI pipeline failures blocking all jobs. Changes
NoteDocker jobs need |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/ci.yml">
<violation number="1" location=".github/workflows/ci.yml:835">
P2: Deploy job no longer publishes/signed build artifacts because the only upload/signing step is fully commented out, so release/develop runs can succeed without uploading `rocket.chat-*.tgz` and its signature.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/ci.yml">
<violation number="1" location=".github/workflows/ci.yml:843">
P2: `Publish assets` uses `ROCKET_DEPLOY_DIR` and `ARTIFACT_NAME` without defining them in the deploy job, so `mkdir -p $ROCKET_DEPLOY_DIR` and the filename construction can fail with empty values, breaking the deploy step and downstream jobs.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/ci.yml">
<violation number="1" location=".github/workflows/ci.yml:841">
P1: Misindented `GPG_PASSWORD`/`run` lines are outside the `Publish assets` step, breaking the workflow structure and removing the publish/sign commands.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
The reported issue is valid. Since this PR is coming from a fork, GitHub Actions does not expose repository secrets (AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, GPG_PASSWORD) to forked workflows. As a result, the publish step fails with: fatal error: Unable to locate credentials if: github.repository == 'RocketChat/Rocket.Chat' This ensures: The deploy/signing step runs only in the upstream repository. @cubic-dev-ai re-review |
@TechByAnshuu I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/ci.yml">
<violation number="1" location=".github/workflows/ci.yml:841">
P1: `Publish assets` step is malformed: `GPG_PASSWORD` and `run` are outdented to the job level, leaving the step without a `run`/`uses` and breaking workflow validation.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/ci.yml">
<violation number="1" location=".github/workflows/ci.yml:842">
P2: `Publish assets` runs an empty script, so the deploy job can succeed without publishing build artifacts.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/ci.yml">
<violation number="1" location=".github/workflows/ci.yml:836">
P2: The repository guard was added only to the `Publish assets` step, so on forks the `deploy` job can succeed (step skipped) and still triggers `docker-image-publish`, which runs unguarded and tries to use DockerHub/GHCR secrets. This creates a new failure path/inconsistent release flow for non-RocketChat repositories. Consider applying the repository guard at the job level or gating downstream publish jobs as well.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
The issue is valid. Previously, only the Publish assets step was guarded, which allowed the deploy job to succeed in forks and still trigger downstream jobs (docker-image-publish, notify-services, docs-update) that require repository secrets. I have applied the repository guard at the job level for all publish-related jobs to ensure forks skip the entire release pipeline and avoid accessing unavailable secrets. This ensures a consistent and secure release flow while preserving full functionality in the main repository. |
|
I'm sorry, but this PR + the comments looks very spammish. I'll keep the issue open but this is gonna be closed. |
|
Sorry if this came across as spam — that wasn’t my intention. |
|
what CI issue? and what does the CI issue has to do with your click-to-jump pr? |
|
You’re right the click-to-jump change itself isn’t related to CI. |
2 participants
Problem
In Rocket.Chat's contextual search bar, when a user clicks on a
search result row, nothing happens. The only way to jump to a
message is by clicking the tiny "Jump" button inside the message
toolbar — which is unintuitive and inconsistent with how modern
messaging apps like WhatsApp and Telegram work, where tapping
any part of a search result navigates directly to that message.
This creates three specific UX problems:
and Mentions contextual bars
Investigation
Before writing any code I did a full audit of the existing
codebase to understand what infrastructure already exists.
Rocket.Chat already has a complete and robust jump pipeline:
setMessageJumpQueryStringParameter
→ sets ?msg=[id] URL parameter
→ picked up by MessageListProvider
→ triggers useLoadSurroundingMessages
→ calls legacyJumpToMessage
→ uses RoomHistoryManager to load unloaded message history
→ finally triggers useJumpToMessage
→ scrolls to message element and applies CSS highlight animation
This pipeline already handles:
The gap was simply that the search result row itself was not
wired up to trigger this pipeline on click.
Solution
Modified RoomMessage.tsx — the component used to render messages
in all contextual bars (Search, Pinned, Starred, Mentions).
Added an onClick handler on the outer Message component:
clicking the message row calls setMessageJumpQueryStringParameter
which triggers the entire existing pipeline automatically
target.closest('button, a, .rcx-checkbox, .rcx-message-toolbar')
so buttons, links, and toolbar actions still work correctly
to signal to the user that the row is clickable
toggles selection as before, jump does not trigger
Why This Approach
Instead of reinventing new hooks or manually calling
RoomHistoryManager, this implementation leans entirely into the
existing reactive architecture. The existing pipeline is already
battle-tested, handles all edge cases, and is the canonical way
to trigger jump behavior in this codebase.
This is the cleanest and most native implementation path:
Bonus
Because the fix targets RoomMessage.tsx which is used by ALL
contextual bars, this automatically brings click-to-jump
functionality to Pinned, Starred, and Mentions tabs as well —
not just Search. This provides broad usability improvements
across the entire app from a single small change.
Issue(s):
Closes #39079
Steps to test or reproduce:
Setup: Run Rocket.Chat locally, create a channel with 20-30 messages.
Further comments:
Rocket.Chat's jump architecture is already complete and production-ready.
setMessageJumpQueryStringParameter is the canonical entry point for
triggering jump behavior — calling it automatically handles cross-room
navigation, unloaded history, and CSS highlight animation.
Alternatives rejected:
The chosen approach is the most consistent with existing codebase patterns
and requires the least new code — one file, 15 lines.
Summary by CodeRabbit
New Features
Documentation