Fix/post-message-target #876
Conversation
WalkthroughExternalSource now drives Changes
Sequence Diagram(s)sequenceDiagram
participant ExternalSource
participant MessageBridge
participant IframeWindow
rect rgb(240,248,255)
Note over ExternalSource,MessageBridge: MessageBridge constructed with origin callback
ExternalSource->>MessageBridge: new MessageBridge(iframe.contentWindow, () => this.cfg.socialBaseUrl)
MessageBridge->>MessageBridge: store _getTargetOrigin
end
rect rgb(240,248,255)
Note over ExternalSource,MessageBridge: Sending messages uses computed origin
ExternalSource->>MessageBridge: send(message)
MessageBridge->>MessageBridge: targetOrigin = _getTargetOrigin()
MessageBridge->>IframeWindow: postMessage(message, targetOrigin)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
nd0ut
force-pushed
the
fix/post-message-target
branch
from
f48516e to
13aa292
Compare
![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
🧹 Nitpick comments (2)
src/blocks/ExternalSource/MessageBridge.ts (2)
18-29: Consider validating the target origin returned by the callback.The constructor now accepts a
getTargetOrigincallback to compute the target origin dynamically, which is a security improvement over using the wildcard'*'. However, there's no validation to ensure the callback returns a valid origin string.Consider adding validation in the
sendmethod to handle edge cases:send(message: OutputMessage) { const targetOrigin = this._getTargetOrigin(); + if (!targetOrigin || typeof targetOrigin !== 'string') { + console.error('Invalid target origin returned from callback:', targetOrigin); + return; + } this._context.postMessage(message, targetOrigin); }Alternatively, validate in the constructor if the callback is expected to return a constant value:
constructor(context: Window, getTargetOrigin: () => string) { this._context = context; this._getTargetOrigin = getTargetOrigin; + + // Validate that callback returns a valid origin + const testOrigin = getTargetOrigin(); + if (!testOrigin || typeof testOrigin !== 'string') { + throw new Error('getTargetOrigin must return a non-empty string'); + } window.addEventListener('message', this._handleMessage); }
57-60: Consider adding error handling for postMessage failures.The
postMessagecall can throw exceptions if thetargetOriginis malformed or if the context window is closed. Consider wrapping it in a try-catch block to handle potential failures gracefully.send(message: OutputMessage) { const targetOrigin = this._getTargetOrigin(); - this._context.postMessage(message, targetOrigin); + try { + this._context.postMessage(message, targetOrigin); + } catch (error) { + console.error('Failed to send message to iframe:', error, { targetOrigin, message }); + } }
📜 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 (3)
src/blocks/ExternalSource/ExternalSource.ts(2 hunks)src/blocks/ExternalSource/MessageBridge.ts(2 hunks)src/blocks/FileItem/FileItem.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/blocks/ExternalSource/ExternalSource.ts (1)
src/blocks/ExternalSource/MessageBridge.ts (1)
MessageBridge(17-65)
⏰ 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: build
🔇 Additional comments (3)
src/blocks/ExternalSource/ExternalSource.ts (2)
315-315: LGTM! State management simplified.The done button visibility now relies on
showSelectionStatusinstead of the removedshowDoneBtnflag. This simplifies state management and aligns with the selection status logic set on Line 189, which shows the button appropriately in multiple selection mode when items are available.
265-265: No action required.The
socialBaseUrlis a required string type with a concrete default value ('https://social.uploadcare.com') and passes through string validation. It cannot be undefined or empty due to the configuration architecture. The code already usessocialBaseUrldirectly without defensive checks on line 238 (new URL(..., socialBaseUrl)), so the callback pattern on line 265 is consistent with existing usage.Likely an incorrect or invalid review comment.
src/blocks/FileItem/FileItem.ts (1)
145-156: LGTM! Improved UX by gating the waiting hint.The addition of the
isFinishedcheck correctly prevents the "waiting-for" hint from displaying after a file upload has completed. This ensures users only see the waiting message while the upload is in progress, improving the overall user experience.
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)
src/blocks/ExternalSource/ExternalSource.ts (1)
21-42: AddshowDoneBtnto the interface definition.The
showDoneBtnproperty is used throughout the implementation (lines 65, 194, 290, 318) but is not declared in theExternalSourceInitStateinterface. This creates a type safety gap.Apply this diff to add the missing property:
interface ExternalSourceInitState extends BaseInitState { activityIcon: string; activityCaption: string; selectedList: NonNullable<InputMessageMap['selected-files-change']['selectedFiles']>; total: number; isSelectionReady: boolean; isDoneBtnEnabled: boolean; couldSelectAll: boolean; couldDeselectAll: boolean; showSelectionStatus: boolean; + showDoneBtn: boolean; counterText: string; doneBtnTextClass: string; toolbarVisible: boolean; onDone: () => void; onCancel: () => void; onSelectAll: () => void; onDeselectAll: () => void; }
🧹 Nitpick comments (1)
src/blocks/ExternalSource/ExternalSource.ts (1)
267-267: Verify thatsocialBaseUrluses only the origin for postMessage security.The protocol, port and hostname of the target window must match the targetOrigin parameter for the message to be sent. The default
socialBaseUrlvalue is correctly set to'https://social.uploadcare.com'(protocol + host only), but if someone misconfigures it with a path component, the postMessage call will fail becausetargetOriginmust exactly match the iframe's origin.Consider extracting only the origin to be defensive:
this._messageBridge = new MessageBridge( iframe.contentWindow, () => new URL(this.cfg.socialBaseUrl).origin );
📜 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)
src/blocks/ExternalSource/ExternalSource.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/blocks/ExternalSource/ExternalSource.ts (1)
src/blocks/ExternalSource/MessageBridge.ts (1)
MessageBridge(17-65)
⏰ 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: build
🔇 Additional comments (4)
src/blocks/ExternalSource/ExternalSource.ts (4)
65-65: LGTM!The
showDoneBtnproperty is correctly initialized tofalse, which is appropriate since no files are available when the external source first mounts.
186-195: Verify the UX intent for done button visibility.The done button will be visible whenever
total > 0(line 194) but only enabled whenisReady && selectedFiles.length > 0(line 189). This means users will see a disabled done button when files are available but none are selected yet.Please confirm this matches the intended UX behavior.
282-292: LGTM!The
showDoneBtnis correctly reset tofalsealong with other selection state properties, maintaining consistency when the iframe is mounted or unmounted.
315-322: LGTM!The template correctly binds both visibility (
@hidden: !showDoneBtn) and enabled state (@disabled: !isDoneBtnEnabled) to the done button, providing appropriate user feedback at different stages of the file selection process.
nd0ut
force-pushed
the
fix/post-message-target
branch
from
13aa292 to
cd9aa92
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/blocks/ExternalSource/MessageBridge.ts (1)
58-59: Security improvement: specific targetOrigin instead of wildcard.Replacing the wildcard
'*'with a specific origin fromgetTargetOrigin()is a significant security improvement that prevents messages from being intercepted by unintended origins.However, consider adding error handling in case the callback throws or returns an invalid origin:
send(message: OutputMessage) { - const targetOrigin = this._getTargetOrigin(); - this._context.postMessage(message, targetOrigin); + try { + const targetOrigin = this._getTargetOrigin(); + if (!targetOrigin || typeof targetOrigin !== 'string') { + console.error('MessageBridge: Invalid targetOrigin returned'); + return; + } + this._context.postMessage(message, targetOrigin); + } catch (error) { + console.error('MessageBridge: Failed to send message', error); + } }
📜 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 (3)
src/blocks/ExternalSource/ExternalSource.ts(2 hunks)src/blocks/ExternalSource/MessageBridge.ts(2 hunks)src/blocks/FileItem/FileItem.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/blocks/FileItem/FileItem.ts
- src/blocks/ExternalSource/ExternalSource.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). (1)
- GitHub Check: build
🔇 Additional comments (2)
src/blocks/ExternalSource/MessageBridge.ts (2)
18-22: Good encapsulation with private fields.Making
_handlerMapand_contextprivate and adding the new_getTargetOrigincallback field improves encapsulation and follows TypeScript best practices.
24-26: Constructor signature change verified—all call sites updated correctly.The single
MessageBridgeinstantiation insrc/blocks/ExternalSource/ExternalSource.ts:267properly passes both parameters: the context window and thegetTargetOrigincallback (() => this.cfg.socialBaseUrl).
Description
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Refactor