fix: improve Yjs reconnect recovery after file rename#255
fix: improve Yjs reconnect recovery after file rename#255YASHcode-IIITV wants to merge 6 commits into
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
👋 Thanks for opening a PR, @YASHcode-IIITV!Your PR has entered the 🚦 PR Review Pipeline.
What happens next
A pipeline status comment will appear below and update automatically as your PR progresses. While you wait
This comment is posted only once. |
|
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:
WalkthroughYjs provider dispatches a ChangesYjs Reconnection Preview Refresh
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (4)
modules/webcontainers/components/webcontainer-preview.tsx (2)
373-373: 💤 Low valueRemove or reduce verbosity of debug logs.
The all-caps console.log statements appear to be temporary debug logging. Consider removing them or converting to a proper logging utility before merging.
🧹 Optional cleanup
- console.log("SERVER READY EVENT", port, url); + // Server ready event handled- console.log("FORCING PREVIEW REFRESH AFTER YJS RECONNECT"); - setRefreshKey((k) => k + 1);Also applies to: 784-784
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/webcontainers/components/webcontainer-preview.tsx` at line 373, The console.log call printing "SERVER READY EVENT" with variables port and url is a noisy debug statement; remove it or replace it with the project's logging utility at an appropriate level (e.g., debug/info) inside the handler where the server-ready event is emitted (the spot referencing port and url in webcontainer-preview component). Ensure any retained log includes structured context (port and url) and uses the centralized logger API rather than console.log to control verbosity.
378-378: Preview refreshes on both server-ready and yjs-reconnected.Note that
refreshKeyis now incremented in two scenarios:
- Line 378: When WebContainer server becomes ready
- Line 786: When Yjs reconnects (dispatched from
lib/yjs.ts)This means if both events fire in quick succession (e.g., during reconnection after a file rename), the iframe will remount twice. This is likely intentional to handle different reconnection scenarios, but be aware of potential brief flickering if events overlap.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/webcontainers/components/webcontainer-preview.tsx` at line 378, The refreshKey is incremented in two separate handlers (the server-ready handler calling setRefreshKey and the Yjs reconnection dispatch from lib/yjs.ts), causing duplicate iframe remounts when both fire quickly; fix it by centralizing/coalescing the increments: create a single helper (e.g., incrementRefreshKeyDebounced) in the webcontainer-preview component that debounces or coalesces multiple calls within a short window (or checks a recent timestamp flag) and use that helper instead of calling setRefreshKey directly from the server-ready handler and the Yjs reconnection listener; replace direct calls to setRefreshKey with calls to incrementRefreshKeyDebounced so overlapping events only trigger one remount.lib/yjs.ts (2)
69-69: 💤 Low valueConsider removing debug console.logs or use proper logging.
The console.log statements are helpful during development but should either be removed for production or replaced with a proper logging framework that supports log levels.
Also applies to: 73-73
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/yjs.ts` at line 69, Replace the debug console.log calls in lib/yjs.ts (e.g., the statement printing `[Yjs] Room ${roomId} status:` using roomId and status, and the similar call at the nearby line) with the project's logging utility or remove them for production; specifically, locate the console.log(...) usages that reference roomId and status and either call logger.debug/info with the same message and structured context (logger.debug("Yjs room status", { roomId, status })) or delete the lines entirely so no console output remains in production code. Ensure you import/use the existing logger instance used elsewhere in the codebase rather than creating a new console-based log.
76-78: 💤 Low valueAdd defensive window check for SSR safety.
While the provider is typically instantiated client-side, adding a
typeof window !== "undefined"check before dispatching the event would make this code more resilient to future refactoring or SSR scenarios.🛡️ Optional defensive check
// Attempt recovery after reconnect if (status === "connected") { console.log("[Yjs] Reconnected successfully"); // Force preview refresh after reconnect - window.dispatchEvent( - new CustomEvent("yjs-reconnected") - ); + if (typeof window !== "undefined") { + window.dispatchEvent( + new CustomEvent("yjs-reconnected") + ); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/yjs.ts` around lines 76 - 78, The dispatch of the "yjs-reconnected" event should be guarded for SSR safety: wrap the window.dispatchEvent(new CustomEvent("yjs-reconnected")) call in a runtime check such as typeof window !== "undefined" (and optionally typeof window.dispatchEvent === "function") so the event is only dispatched when window exists; update the code around the CustomEvent creation/dispatch (the new CustomEvent("yjs-reconnected") call site) to perform this defensive check before invoking dispatchEvent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/yjs.ts`:
- Around line 71-79: The dispatched CustomEvent name "yjs-reconnected" is
misleading because the code in the (status === "connected") block fires on
initial connect as well (see the status variable and window.dispatchEvent(new
CustomEvent("yjs-reconnected"))), so fix by either renaming the event to
"yjs-connected" (and update any listeners) or add a persisted connection flag
(e.g., module/closure-level hasConnectedOnce) and only dispatch
"yjs-reconnected" when hasConnectedOnce is true, otherwise set the flag (and
optionally dispatch "yjs-connected" on first connect); update the corresponding
event listeners to match the chosen name/behavior.
---
Nitpick comments:
In `@lib/yjs.ts`:
- Line 69: Replace the debug console.log calls in lib/yjs.ts (e.g., the
statement printing `[Yjs] Room ${roomId} status:` using roomId and status, and
the similar call at the nearby line) with the project's logging utility or
remove them for production; specifically, locate the console.log(...) usages
that reference roomId and status and either call logger.debug/info with the same
message and structured context (logger.debug("Yjs room status", { roomId, status
})) or delete the lines entirely so no console output remains in production
code. Ensure you import/use the existing logger instance used elsewhere in the
codebase rather than creating a new console-based log.
- Around line 76-78: The dispatch of the "yjs-reconnected" event should be
guarded for SSR safety: wrap the window.dispatchEvent(new
CustomEvent("yjs-reconnected")) call in a runtime check such as typeof window
!== "undefined" (and optionally typeof window.dispatchEvent === "function") so
the event is only dispatched when window exists; update the code around the
CustomEvent creation/dispatch (the new CustomEvent("yjs-reconnected") call site)
to perform this defensive check before invoking dispatchEvent.
In `@modules/webcontainers/components/webcontainer-preview.tsx`:
- Line 373: The console.log call printing "SERVER READY EVENT" with variables
port and url is a noisy debug statement; remove it or replace it with the
project's logging utility at an appropriate level (e.g., debug/info) inside the
handler where the server-ready event is emitted (the spot referencing port and
url in webcontainer-preview component). Ensure any retained log includes
structured context (port and url) and uses the centralized logger API rather
than console.log to control verbosity.
- Line 378: The refreshKey is incremented in two separate handlers (the
server-ready handler calling setRefreshKey and the Yjs reconnection dispatch
from lib/yjs.ts), causing duplicate iframe remounts when both fire quickly; fix
it by centralizing/coalescing the increments: create a single helper (e.g.,
incrementRefreshKeyDebounced) in the webcontainer-preview component that
debounces or coalesces multiple calls within a short window (or checks a recent
timestamp flag) and use that helper instead of calling setRefreshKey directly
from the server-ready handler and the Yjs reconnection listener; replace direct
calls to setRefreshKey with calls to incrementRefreshKeyDebounced so overlapping
events only trigger one remount.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c6675905-2e70-4af0-ba07-ded72713618d
📒 Files selected for processing (2)
lib/yjs.tsmodules/webcontainers/components/webcontainer-preview.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/yjs.ts`:
- Line 77: The emitted event name was changed to "yjs-connected"
(CustomEvent("yjs-connected")) but downstream still listens for
"yjs-reconnected" (see webcontainer-preview.tsx listener), breaking reconnect
behavior; fix by restoring the original event name or emit both for
compatibility: dispatch CustomEvent("yjs-reconnected") (or dispatch both
"yjs-reconnected" and "yjs-connected") and update any listeners accordingly so
emitter/listener names stay in sync.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7824490f-fb84-454d-af31-f2186880eb75
📒 Files selected for processing (1)
lib/yjs.ts
|
@piyushdotcomm plz review and if possible could you give me the critical tag in this one . |
|
now no issues i think it is ready for merge |
|
every thing is done now , ready to merge |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
modules/webcontainers/components/webcontainer-preview.tsx (1)
113-123: ⚡ Quick winRemove the unnecessary
(inst as any)cast inbindServerReady:@webcontainer/api’s TypeScript types forWebContainer.on('server-ready', ...)expectlistener: (port: number, url: string) => voidand return anUnsubscribefunction (() => void), which matchesserverReadyCleanupRef—soinst.on("server-ready", handler)should type-check and keep compile-time event/listener checking inmodules/webcontainers/components/webcontainer-preview.tsx(lines 113-123).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/webcontainers/components/webcontainer-preview.tsx` around lines 113 - 123, Remove the unnecessary cast by calling inst.on directly: in bindServerReady, replace (inst as any).on("server-ready", handler) with inst.on("server-ready", handler) and assign its return to serverReadyCleanupRef.current (since WebContainer.on returns an Unsubscribe () => void), ensuring serverReadyCleanupRef and the handler signature (port: number, url: string) remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@modules/webcontainers/components/webcontainer-preview.tsx`:
- Around line 113-123: Remove the unnecessary cast by calling inst.on directly:
in bindServerReady, replace (inst as any).on("server-ready", handler) with
inst.on("server-ready", handler) and assign its return to
serverReadyCleanupRef.current (since WebContainer.on returns an Unsubscribe ()
=> void), ensuring serverReadyCleanupRef and the handler signature (port:
number, url: string) remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 888d5e7c-2ce5-49dd-9d0d-aa09eb55df03
📒 Files selected for processing (1)
modules/webcontainers/components/webcontainer-preview.tsx
this is the outdated comment this is already fixed , |
|
@piyushdotcomm plz merge this pr bro |
piyushdotcomm
left a comment
piyushdotcomm
left a comment
There was a problem hiding this comment.
Breaking compilation and risking HMR loops. Fixes required.
| import { Label } from "@/components/ui/label"; | ||
| import type { TemplateCategory } from "@/lib/templates/types"; | ||
| import { getTemplateSummaries } from "@/lib/templates/actions"; | ||
| import type { TemplateKey } from "@/lib/template"; |
There was a problem hiding this comment.
You removed the import but the function is still called on line 68. This breaks compilation.
| serverReadyCleanupRef.current?.(); | ||
| serverReadyCleanupRef.current = inst.on("server-ready", handler); | ||
|
|
||
| serverReadyCleanupRef.current = (inst as any).on( |
There was a problem hiding this comment.
Drop the \as any\ cast. Use @ts-expect-error\ if the types from @webcontainer/api\ are truly mismatched.
|
|
||
| const isCommonFrontendPort = [ | ||
| 3000, 5173, 8080, 4200, 8000, | ||
| ].includes(port); |
There was a problem hiding this comment.
Unconditional iframe remounts on \server-ready\ will destroy HMR. Only increment
efreshKey\ if the URL actually changed.
|
|
||
| // Force preview refresh after reconnect | ||
| window.dispatchEvent( | ||
| new CustomEvent("yjs-reconnected") |
There was a problem hiding this comment.
Bypassing React state with global \window\ events is brittle. Pass a callback or use Zustand for connection state.
2 participants
Summary
Type of change
Related issue
Closes #237
Validation
npm run lintnpm testnpm run buildList any additional manual verification you performed:
Screenshots or recordings
Checklist
Summary by CodeRabbit
Bug Fixes
Chores