Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit 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. WalkthroughIntroduces two hooks for frame reloads and sandbox connection timeout, refactors Frame view to use a single Penpal connection attempt with explicit success/failure callbacks and a configurable timeout, and rewires the index component to use the new hooks and props. Debounce/toast usages are shifted from the index into dedicated hooks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Index as Frame Index (index.tsx)
participant View as Frame View (view.tsx)
participant IFrame as Iframe
participant Penpal as Penpal Bridge
participant Reload as useFrameReload
participant Sandbox as useSandboxTimeout
participant Engine as Editor Engine
User->>Index: Open project / show frame
Index->>Reload: init hook
Index->>Sandbox: init hook with frame, onTimeout
Index->>View: mount with {reloadIframe, onConnectionSuccess/Failed, penpalTimeoutMs}
Note over View,Penpal: Attempt single Penpal connection<br/>with timeout (penpalTimeoutMs)
View->>IFrame: obtain contentWindow
View->>Penpal: connect()
View-->>View: race(connect, timeout)
alt Connected
View->>Index: onConnectionSuccess()
Index->>Reload: handleConnectionSuccess()
else Timeout or Failure
View->>Index: onConnectionFailed()
Index->>Reload: handleConnectionFailed() (debounced)
Reload-->>Reload: schedule progressive reload
Reload->>Index: reloadKey updated
Index->>View: rerender with new key
end
rect rgba(230,240,255,0.5)
Note over Sandbox,Engine: Monitor sandbox "connecting"
Sandbox->>Engine: check isConnecting
Engine-->>Sandbox: state
alt Still connecting after 30s
Sandbox-->>Index: onTimeout()
Index->>Reload: schedule reload
else Not connecting
Sandbox-->>Sandbox: reset hasTimedOut
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
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 |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
| }, reloadDelay); | ||
| }; | ||
|
|
||
| const handleConnectionFailed = debounce(() => { |
There was a problem hiding this comment.
The debounced 'handleConnectionFailed' is created on every render. Wrap the debounce call in useMemo or useCallback and cancel it on unmount to maintain stable debounce state.
| console.error('No iframe found'); | ||
| throw new Error('No iframe found'); | ||
| console.error(`${PENPAL_PARENT_CHANNEL} (${frame.id}) - No iframe found`); | ||
| onConnectionFailed; |
There was a problem hiding this comment.
onConnectionFailed is referenced without invoking it; change to onConnectionFailed() to execute the callback.
| onConnectionFailed; | |
| onConnectionFailed(); |
| console.log(`${PENPAL_PARENT_CHANNEL} (${frame.id}) - Penpal connection set `); | ||
|
|
||
| // Notify parent of successful connection | ||
| onConnectionSuccess; |
There was a problem hiding this comment.
onConnectionSuccess is referenced without being invoked; modify to onConnectionSuccess() to properly execute the callback.
| onConnectionSuccess; | |
| onConnectionSuccess(); |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| console.error('No iframe found'); | ||
| throw new Error('No iframe found'); | ||
| console.error(`${PENPAL_PARENT_CHANNEL} (${frame.id}) - No iframe found`); | ||
| onConnectionFailed; |
There was a problem hiding this comment.
The function reference onConnectionFailed is not being invoked. This needs to be changed to onConnectionFailed() to properly execute the callback function. This issue appears in multiple places throughout the file and should be fixed in all occurrences to ensure proper error handling.
| onConnectionFailed; | |
| onConnectionFailed(); |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
This comment came from an experimental review—please leave feedback if it was helpful/unhelpful. Learn more about experimental comments here.
| console.log(`${PENPAL_PARENT_CHANNEL} (${frame.id}) - Penpal connection set `); | ||
|
|
||
| // Notify parent of successful connection | ||
| onConnectionSuccess; |
There was a problem hiding this comment.
The function reference onConnectionSuccess is not being invoked. This needs to be changed to onConnectionSuccess() to properly execute the callback function. The same issue appears with onConnectionFailed in several places throughout this file.
| onConnectionSuccess; | |
| onConnectionSuccess(); |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
This comment came from an experimental review—please leave feedback if it was helpful/unhelpful. Learn more about experimental comments here.
| `${PENPAL_PARENT_CHANNEL} (${frame.id}) - Connection failed: child is null`, | ||
| ); | ||
| retrySetupPenpalConnection(error); | ||
| onConnectionFailed; |
There was a problem hiding this comment.
The function reference onConnectionFailed is not being executed here. This should be changed to onConnectionFailed() to properly invoke the callback function. This issue appears in multiple places throughout the file where the callback is referenced but not called.
| onConnectionFailed; | |
| onConnectionFailed(); |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
This comment came from an experimental review—please leave feedback if it was helpful/unhelpful. Learn more about experimental comments here.
| error, | ||
| ); | ||
| retrySetupPenpalConnection(error); | ||
| onConnectionFailed; |
There was a problem hiding this comment.
The function reference onConnectionFailed is not being executed here. This should be changed to onConnectionFailed() to properly invoke the function when a connection failure occurs. This issue appears in multiple places throughout the file where the callback is referenced but not called.
| onConnectionFailed; | |
| onConnectionFailed(); |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
This comment came from an experimental review—please leave feedback if it was helpful/unhelpful. Learn more about experimental comments here.
| const timeoutPromise = new Promise<never>((_, reject) => { | ||
| setTimeout(() => { | ||
| reject(new Error(`Penpal connection timeout after ${penpalTimeoutMs}ms`)); | ||
| }, penpalTimeoutMs); | ||
| }); |
There was a problem hiding this comment.
The timeout promise could trigger even after a successful connection. Consider adding a cleanup mechanism to prevent this race condition:
let timeoutId: NodeJS.Timeout;
const timeoutPromise = new Promise<never>((_, reject) => {
timeoutId = setTimeout(() => {
reject(new Error(`Penpal connection timeout after ${penpalTimeoutMs}ms`));
}, penpalTimeoutMs);
});
// Then in the connection.promise.then handler:
clearTimeout(timeoutId);Alternatively, an AbortController could provide a cleaner solution for canceling the timeout when the connection succeeds.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
This comment came from an experimental review—please leave feedback if it was helpful/unhelpful. Learn more about experimental comments here.
1 participant
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Important
Fixes iframe refresh bug by introducing hooks for managing connection retries and timeouts, improving iframe handling logic.
useFrameReloadanduseSandboxTimeouthooks to manage iframe connection retries and timeouts.FrameComponentinview.tsxnow uses these hooks to handle connection failures and successes, improving iframe refresh logic.index.tsxandview.tsx.useFrameReload: Manages reload timing and connection state, with debounced connection failure handling.useSandboxTimeout: Handles sandbox connection timeout, triggering retries and displaying toast notifications.FrameComponentto use new hooks, simplifying connection logic.FrameViewinindex.tsxto integrate with new hooks.This description was created by
for 72ee150. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor