fix: iframe external commands #37829
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: 6999be3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 42 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 |
WalkthroughRefactors iframe postMessage handling from a module-level listener to a React hook. Adds a patch-level changeset for Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
yash-rajpal
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
![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
📜 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)
.changeset/odd-pigs-hang.md(1 hunks)apps/meteor/client/startup/index.ts(0 hunks)apps/meteor/client/views/root/AppLayout.tsx(2 hunks)apps/meteor/client/views/root/hooks/useIframeCommands.ts(2 hunks)
💤 Files with no reviewable changes (1)
- apps/meteor/client/startup/index.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/views/root/hooks/useIframeCommands.tsapps/meteor/client/views/root/AppLayout.tsx
🧠 Learnings (2)
📓 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/views/root/hooks/useIframeCommands.tsapps/meteor/client/views/root/AppLayout.tsx
🧬 Code graph analysis (2)
apps/meteor/client/views/root/hooks/useIframeCommands.ts (1)
packages/ui-contexts/src/index.ts (1)
useSetting(72-72)
apps/meteor/client/views/root/AppLayout.tsx (1)
apps/meteor/client/views/root/hooks/useIframeCommands.ts (1)
useIframeCommands(97-130)
⏰ 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 (4)
apps/meteor/client/views/root/hooks/useIframeCommands.ts (2)
97-130: Well-structured hook with proper lifecycle management.The refactoring from module-level initialization to a React hook is a good improvement. The effect properly:
- Guards on the feature flag
- Validates origin and command before execution
- Returns cleanup to remove the listener
17-91: Command handlers are well-implemented with proper input validation.The commands properly validate inputs before execution, and the OAuth redirect URL validation (line 57-60) provides protection against open redirect attacks.
apps/meteor/client/views/root/AppLayout.tsx (1)
24-25: Clean integration following the existing hook pattern.The placement alongside
useIframeLoginListeneris logical and the invocation follows the established pattern in this component.Also applies to: 75-75
.changeset/odd-pigs-hang.md (1)
1-5: Changeset appropriately documents the fix.The patch level and description accurately reflect the nature of this bug fix.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/client/views/root/hooks/useIframeCommands.ts (1)
120-121: Consider improving type safety for the command handler.The
anytype could be replaced with a more specific type to improve type safety.Apply this diff:
- const command: (data: any, event: MessageEvent) => void = commands[event.data.externalCommand]; + const command = commands[event.data.externalCommand];TypeScript will correctly infer the type based on the
commandsobject, eliminating the need for an explicit type annotation.
📜 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/views/root/hooks/useIframeCommands.ts(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/views/root/hooks/useIframeCommands.ts
🧠 Learnings (2)
📓 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/views/root/hooks/useIframeCommands.ts
🧬 Code graph analysis (1)
apps/meteor/client/views/root/hooks/useIframeCommands.ts (1)
packages/ui-contexts/src/index.ts (1)
useSetting(72-72)
⏰ 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)
apps/meteor/client/views/root/hooks/useIframeCommands.ts (3)
1-15: LGTM! Imports properly updated for hook-based implementation.The addition of
useSettinganduseEffectcorrectly supports the refactor from a global listener to a React hook. The relative import paths have been updated to match the new file location.
97-104: LGTM! Proper feature flag and configuration handling.The hook correctly retrieves settings and uses an early return to prevent listener registration when the feature is disabled.
124-129: LGTM! Proper listener lifecycle management.The effect correctly registers the listener and cleans it up on unmount or when dependencies change. The dependency array includes both
iframeReceiveEnabledandiframeReceiveOrigin, ensuring the listener is properly re-registered when configuration changes.
📦 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 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 23:27", "12/20 16:45", "12/20 17:11 (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, 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, 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, 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, 0.13, 0.13, 0.13, 0.13, 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, 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, 0.13, 0.13, 0.13, 0.13, 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.34, 0.34, 0.34, 0.34, 0.34, 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, 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 26 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Sat, 20 Dec 2025 17:11:41 GMT |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #37829 +/- ##
========================================
Coverage 67.68% 67.68%
========================================
Files 3476 3476
Lines 113895 113902 +7
Branches 20956 20955 -1
========================================
+ Hits 77088 77095 +7
+ Misses 34619 34617 -2
- Partials 2188 2190 +2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
kodiakhq
bot
removed
the
stat: ready to merge
|
This PR currently has a merge conflict. Please resolve this and then re-add the |
yash-rajpal
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
Iframe external commands were not working as given here
Issue(s)
Steps to test or reproduce
Further comments
CORE-1564
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.