refactor: modularize playground page orchestration#231
refactor: modularize playground page orchestration#231HimasreeKolathur24 wants to merge 3 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, @HimasreeKolathur24!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:
WalkthroughSplits the monolithic playground page into focused hooks and components: dialog/keyboard hooks, save/download/wrapped file handlers, a PlaygroundEditorArea component, and PlaygroundModals; the main page composes these pieces and reorders side effects and rendering. ChangesPlayground Component Decomposition
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@modules/playground/components/playground-editor-area.tsx`:
- Around line 117-124: The call to WebContainerPreview is using a non-null
assertion serverUrl! which hides the actual string | null type; change the prop
to pass a nullable-safe value (e.g., serverUrl ?? "") when invoking
WebContainerPreview in playground-editor-area.tsx, or alternatively update
WebContainerPreviewProps to accept string | null/optional and propagate that
change through its props; locate the invocation of WebContainerPreview and
replace serverUrl! with a safe expression (serverUrl ?? "") or update the prop
type accordingly.
In `@modules/playground/components/playground-modals.tsx`:
- Line 64: The command palette's "close all files" action is currently a no-op
because onCloseAllFiles is set to () => {} in playground-modals.tsx; replace the
noop by passing a real handler prop from the parent/page: add an onCloseAllFiles
prop to the PlaygroundModals component (or the component that renders the
command palette) and wire that prop through to where onCloseAllFiles is used,
ensuring the parent page supplies the actual closeAllFiles handler (preserving
existing behavior) and update any relevant prop types/interfaces (e.g., the
PlaygroundModalsProps) so the handler is required.
In `@modules/playground/hooks/useKeyboardShortcuts.ts`:
- Around line 25-63: The keyboard handler handleKeyDown in
useKeyboardShortcuts.ts should ignore global shortcuts when the event target is
an editable element; add an early return at the top of handleKeyDown that checks
if e.target (or e.composedPath()[0]) is an INPUT, TEXTAREA or has
contentEditable not "false" (or otherwise matches editable elements) and if so
do nothing, then keep the existing shortcut logic (handleSave, handleSaveAll,
setIsCommandPaletteOpen, sidebar.toggleSidebar, setIsPreviewVisible,
useAI.getState().toggleChat, closeFile using activeFileId). Apply the identical
guard to the other handleKeyDown in usePlaygroundActions.ts so neither global
handler runs while focus is inside editable controls.
In `@modules/playground/hooks/useSaveHandlers.ts`:
- Around line 51-66: The current updateContentInTree routine matches files by
filename + fileExtension (in updateContentInTree, comparing
item.filename/item.fileExtension to fileToSave) which can hit duplicates across
folders; modify the logic to match a stable unique identifier (e.g.,
fileToSave.resolvedPath or fileToSave.id) instead of name+extension. Add or use
a fullPath/id property on TemplateFile (or compute the path while recursing by
carrying the parent path into updateContentInTree), compare that fullPath/id to
fileToSave.fullPath or fileToSave.id when deciding to replace content, and only
update the single matching node in updatedTemplateData.items to avoid updating
multiple files.
In `@modules/playground/hooks/useWrappedFileHandlers.ts`:
- Around line 25-28: The wrapped add-file handler passes writeFileSync (via
wrappedHandleAddFile in useWrappedFileHandlers) unguarded into handleAddFile,
but writeFileSync from useWebContainer throws if instance is not ready; change
the call to only pass writeFileSync when instance is non-null (or otherwise
guard the sync inside wrappedHandleAddFile), e.g. check instance before invoking
handleAddFile or pass null/undefined for writeFileSync until ready so
handleAddFile’s existing if (writeFileSync) check is reliable; update error
handling/message in handleAddFile to avoid surfacing a “Failed to create file”
for failures that only affect remote sync while local template update succeeded.
🪄 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: 36e241e5-128f-49c8-8959-a42138280f6f
📒 Files selected for processing (8)
app/playground/[id]/page.tsxmodules/playground/components/playground-editor-area.tsxmodules/playground/components/playground-modals.tsxmodules/playground/hooks/useDownload.tsmodules/playground/hooks/useKeyboardShortcuts.tsmodules/playground/hooks/usePlaygroundDialogs.tsmodules/playground/hooks/useSaveHandlers.tsmodules/playground/hooks/useWrappedFileHandlers.ts
6d24b15 to
13b7623
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
modules/playground/hooks/useSaveHandlers.ts (1)
51-66:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a unique path-based match when updating the tree.
At Line 58, matching by
filename+fileExtensioncan update multiple files in different folders with the same name. Use the resolved path (or a stable id) to update exactly one node.Suggested fix
- const updateContentInTree = ( - items: (TemplateFile | TemplateFolder)[] - ): (TemplateFile | TemplateFolder)[] => - items.map((item) => { - if ("folderName" in item) { - return { ...item, items: updateContentInTree(item.items) }; - } else if ( - item.filename === fileToSave.filename && - item.fileExtension === fileToSave.fileExtension - ) { - return { ...item, content: fileToSave.content }; - } - return item; - }); + const normalizedTargetPath = filePath.replace(/^\/+/, ""); + const updateContentInTree = ( + items: (TemplateFile | TemplateFolder)[], + currentPath = "" + ): (TemplateFile | TemplateFolder)[] => + items.map((item) => { + if ("folderName" in item) { + const nextPath = currentPath + ? `${currentPath}/${item.folderName}` + : item.folderName; + return { ...item, items: updateContentInTree(item.items, nextPath) }; + } + + const itemName = item.fileExtension + ? `${item.filename}.${item.fileExtension}` + : item.filename; + const itemPath = `${currentPath}/${itemName}`.replace(/^\/+/, ""); + if (itemPath === normalizedTargetPath) { + return { ...item, content: fileToSave.content }; + } + return item; + }); - updatedTemplateData.items = updateContentInTree(updatedTemplateData.items); + updatedTemplateData.items = updateContentInTree(updatedTemplateData.items);🤖 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/playground/hooks/useSaveHandlers.ts` around lines 51 - 66, The current updateContentInTree function matches files by filename + fileExtension which can collide across folders; change it to match a unique path/id: modify updateContentInTree to accept a currentPath string (or build a path as you recurse), construct each node's resolvedPath (e.g., `${currentPath}/${folderName}` for folders and `${currentPath}/${filename}.${fileExtension}` for files), and compare that resolvedPath to fileToSave.resolvedPath (or fileToSave.path/id) when deciding to replace content in updatedTemplateData.items; ensure recursion passes the updated currentPath so only the exact node is updated.
🤖 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 `@app/playground/`[id]/page.tsx:
- Around line 69-71: The useEffect currently fires toast.success when isSuccess
and templateData change, causing repeated toasts when templateData mutates;
change it so the load toast runs only once by tracking whether the toast has
already been shown (e.g., add a ref or state like hasShownLoadToast) and only
call toast.success in the useEffect when isSuccess is true, templateData exists,
and hasShownLoadToast is false, then set hasShownLoadToast to true; update the
useEffect dependencies to avoid including templateData changes (use isSuccess
and the ref/state) so the toast triggers only on the initial successful load in
the useEffect that references useEffect, isSuccess, templateData, and
toast.success.
- Around line 74-92: The effect opens a default file whenever activeFileId is
falsy, not only on the transition when the preview is opened; modify the
useEffect that contains findDefaultFile/openFile so it only runs when
dialogs.isPreviewVisible transitions from false to true (and activeFileId is
falsy and templateData exists). Implement this by tracking the previous
dialogs.isPreviewVisible (e.g., a useRef prevIsPreviewVisible) and inside the
effect require prevIsPreviewVisible === false && dialogs.isPreviewVisible ===
true before running findDefaultFile/openFile, then update the prev ref at the
end; keep the existing helper findDefaultFile and openFile references.
In `@modules/playground/hooks/useKeyboardShortcuts.ts`:
- Around line 36-67: Normalize the keyboard event's key once at the top of the
shortcut handler (e.g., const key = e.key.toLowerCase()) and use that lowercase
variable for all letter comparisons so Caps Lock doesn't break shortcuts; update
checks that currently test e.key === "s"/"S"/"k"/"b"/"w"/"A"/"P" to use key ===
"s"/"k"/"b"/"w"/"a"/"p" respectively while retaining existing ctrl/shift logic
and special-key checks (e.g., keep the backslash check for e.key === "\\" as-is
if needed), ensuring handlers like handleSave, handleSaveAll,
setIsCommandPaletteOpen, sidebar.toggleSidebar, setIsPreviewVisible, and
useAI.getState().toggleChat are invoked unchanged.
In `@modules/playground/hooks/useSaveHandlers.ts`:
- Around line 44-47: The code currently swallows unresolved-path failures by
calling toast.error and returning (e.g., the block checking filePath for
fileToSave in useSaveHandlers.ts), which lets higher-level "Save All" treat the
save as successful; change these early returns to throw a consistent Error
(include the same descriptive message used in the toast) so the promise rejects
and Save All can detect failure. Update every similar pattern in this file (the
blocks around filePath/fileToSave referenced at the other occurrences) to both
toast.error(...) and then throw new Error(<same message>), and ensure the caller
of the save function will propagate/reject accordingly.
---
Duplicate comments:
In `@modules/playground/hooks/useSaveHandlers.ts`:
- Around line 51-66: The current updateContentInTree function matches files by
filename + fileExtension which can collide across folders; change it to match a
unique path/id: modify updateContentInTree to accept a currentPath string (or
build a path as you recurse), construct each node's resolvedPath (e.g.,
`${currentPath}/${folderName}` for folders and
`${currentPath}/${filename}.${fileExtension}` for files), and compare that
resolvedPath to fileToSave.resolvedPath (or fileToSave.path/id) when deciding to
replace content in updatedTemplateData.items; ensure recursion passes the
updated currentPath so only the exact node is updated.
🪄 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: 5415d1f2-6034-4d27-a42e-5fd1730e5043
📒 Files selected for processing (8)
app/playground/[id]/page.tsxmodules/playground/components/playground-editor-area.tsxmodules/playground/components/playground-modals.tsxmodules/playground/hooks/useDownload.tsmodules/playground/hooks/useKeyboardShortcuts.tsmodules/playground/hooks/usePlaygroundDialogs.tsmodules/playground/hooks/useSaveHandlers.tsmodules/playground/hooks/useWrappedFileHandlers.ts
✅ Files skipped from review due to trivial changes (1)
- modules/playground/hooks/usePlaygroundDialogs.ts
238657d to
5e002cb
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
modules/playground/hooks/useSaveHandlers.ts (2)
57-61:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a unique identifier/path for tree updates.
Line 58-60 matches by
filename + fileExtension, which can update multiple files when names repeat in different folders.🤖 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/playground/hooks/useSaveHandlers.ts` around lines 57 - 61, The update currently matches items by filename and fileExtension (in the block comparing item.filename === fileToSave.filename && item.fileExtension === fileToSave.fileExtension) which can collide across folders; change the comparison to use a unique identifier such as item.path, item.fullPath or an explicit id (e.g., item.id === fileToSave.id or item.fullPath === fileToSave.fullPath) and update any call sites that set fileToSave so the unique field is populated; modify the update branch in useSaveHandlers.ts to match on that unique key and then return the patched item with updated content to avoid accidental updates of files with the same name.
44-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate unresolved-path failures instead of returning silently.
Line 46 returns without rejecting, so batch save can still appear successful after a file failed to resolve.
Suggested patch
if (!filePath) { - toast.error(`Could not find path for file: ${fileToSave.filename}.${fileToSave.fileExtension}`); - return; + const msg = `Could not find path for file: ${fileToSave.filename}.${fileToSave.fileExtension}`; + toast.error(msg); + throw new Error(msg); }🤖 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/playground/hooks/useSaveHandlers.ts` around lines 44 - 47, The current early return in useSaveHandlers (when filePath is falsy) swallows failures causing batch saves to appear successful; instead propagate the error by rejecting the save operation: inside the save handler where fileToSave and filePath are checked, replace the plain return with throwing an Error or returning a rejected Promise that includes the filename and extension so callers (e.g., batchSave) can detect and surface the failure. Ensure the thrown/rejected error is descriptive (e.g., "Could not find path for file: ...") so upstream logic can handle or aggregate failures correctly.modules/playground/hooks/useKeyboardShortcuts.ts (1)
42-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix unreachable Ctrl+Shift shortcuts after key normalization.
Line 26 lowercases
e.key, but Line 42/47/62 compare against"S","P", and"A". Those branches never match, so those shortcuts won’t fire.Suggested patch
- if (e.ctrlKey && e.shiftKey && key === "S") { + if (e.ctrlKey && e.shiftKey && key === "s") { e.preventDefault(); handleSaveAll(); } @@ - if ((e.ctrlKey && key === "k") || (e.ctrlKey && e.shiftKey && key === "P")) { + if ((e.ctrlKey && key === "k") || (e.ctrlKey && e.shiftKey && key === "p")) { e.preventDefault(); setIsCommandPaletteOpen(true); } @@ - if (e.ctrlKey && e.shiftKey && key === "A") { + if (e.ctrlKey && e.shiftKey && key === "a") { e.preventDefault(); useAI.getState().toggleChat(); }🤖 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/playground/hooks/useKeyboardShortcuts.ts` around lines 42 - 63, In useKeyboardShortcuts, the handler lowercases e.key but then compares to uppercase letters, so Ctrl+Shift shortcuts (handleSaveAll, setIsCommandPaletteOpen, AI chat) never match; fix by comparing against lowercase keys (e.g., "s", "p", "a", "k", "b", "\\") or stop normalizing e.key — update the comparisons in the keydown handler for handleSaveAll, setIsCommandPaletteOpen, sidebar.toggleSidebar, setIsPreviewVisible and the AI chat branch to use the normalized lowercase values and preserve the existing ctrl/shift boolean checks.
🤖 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 `@modules/playground/hooks/useDownload.ts`:
- Around line 26-46: The download function handleDownloadZip currently creates
an object URL and a temporary anchor but only cleans them up inside the try
block, risking leaks if an exception is thrown; refactor handleDownloadZip so
url and link are declared in outer scope before the try, perform the JSZip
generation and link.click() inside try/catch, and move
document.body.removeChild(link) and window.URL.revokeObjectURL(url) into a
finally block that checks for non-null link and url (created via
window.URL.createObjectURL) to ensure removal and revocation always run;
reference handleDownloadZip, addFilesToZip, window.URL.createObjectURL,
window.URL.revokeObjectURL and the temporary link element when making the
changes.
In `@modules/playground/hooks/useSaveHandlers.ts`:
- Line 17: The setOpenFiles type and its usage cause lost updates when saves run
concurrently: change the declaration of setOpenFiles from "setOpenFiles: (files:
any[]) => void" to a React state dispatcher type (e.g.,
Dispatch<SetStateAction<any[]>>) so callers can use the functional updater, and
update handleSaveAll (the function around lines 90-100) to call
setOpenFiles(prev => { /* derive new state from prev */ }) instead of reading a
stale openFiles snapshot and setting it directly; make sure all updates to
openFiles inside save completion handlers use the functional form to avoid
races.
---
Duplicate comments:
In `@modules/playground/hooks/useKeyboardShortcuts.ts`:
- Around line 42-63: In useKeyboardShortcuts, the handler lowercases e.key but
then compares to uppercase letters, so Ctrl+Shift shortcuts (handleSaveAll,
setIsCommandPaletteOpen, AI chat) never match; fix by comparing against
lowercase keys (e.g., "s", "p", "a", "k", "b", "\\") or stop normalizing e.key —
update the comparisons in the keydown handler for handleSaveAll,
setIsCommandPaletteOpen, sidebar.toggleSidebar, setIsPreviewVisible and the AI
chat branch to use the normalized lowercase values and preserve the existing
ctrl/shift boolean checks.
In `@modules/playground/hooks/useSaveHandlers.ts`:
- Around line 57-61: The update currently matches items by filename and
fileExtension (in the block comparing item.filename === fileToSave.filename &&
item.fileExtension === fileToSave.fileExtension) which can collide across
folders; change the comparison to use a unique identifier such as item.path,
item.fullPath or an explicit id (e.g., item.id === fileToSave.id or
item.fullPath === fileToSave.fullPath) and update any call sites that set
fileToSave so the unique field is populated; modify the update branch in
useSaveHandlers.ts to match on that unique key and then return the patched item
with updated content to avoid accidental updates of files with the same name.
- Around line 44-47: The current early return in useSaveHandlers (when filePath
is falsy) swallows failures causing batch saves to appear successful; instead
propagate the error by rejecting the save operation: inside the save handler
where fileToSave and filePath are checked, replace the plain return with
throwing an Error or returning a rejected Promise that includes the filename and
extension so callers (e.g., batchSave) can detect and surface the failure.
Ensure the thrown/rejected error is descriptive (e.g., "Could not find path for
file: ...") so upstream logic can handle or aggregate failures correctly.
🪄 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: b1ec1db1-2e53-46cf-a3fc-15378ff077eb
📒 Files selected for processing (8)
app/playground/[id]/page.tsxmodules/playground/components/playground-editor-area.tsxmodules/playground/components/playground-modals.tsxmodules/playground/hooks/useDownload.tsmodules/playground/hooks/useKeyboardShortcuts.tsmodules/playground/hooks/usePlaygroundDialogs.tsmodules/playground/hooks/useSaveHandlers.tsmodules/playground/hooks/useWrappedFileHandlers.ts
d696297 to
aad92ca
Compare
piyushdotcomm
left a comment
There was a problem hiding this comment.
@coderabbitai full review
piyushdotcomm
left a comment
There was a problem hiding this comment.
resolve the merge conflict
@piyushdotcomm, resolved the merge conflicts as suggested. Please let me know if any additional changes or improvements are needed. |
piyushdotcomm
left a comment
There was a problem hiding this comment.
there is still merge conflict
c847cfe to
e662be5
Compare
|
@piyushdotcomm, re-verified and resolved the conflicts as suggested. Please let me know if any additional changes are needed. |
piyushdotcomm
left a comment
There was a problem hiding this comment.
Breaking compilation errors introduced by the refactor. See inline comments.
| error: containerError, | ||
| instance, | ||
| writeFileSync, | ||
| } = useWebContainer({ templateData }); |
There was a problem hiding this comment.
\useWebContainer\ expects 0 arguments. Remove { templateData }.
|
|
||
| // Bootstrap | ||
| useEffect(() => { | ||
| resetUI(); |
There was a problem hiding this comment.
esetUI\ is called but not imported or defined.
| <TooltipProvider> | ||
| <> | ||
| <PlaygroundSidebar | ||
| templateData={templateData} |
There was a problem hiding this comment.
\PlaygroundSidebar\ does not accept these props. Update its interface.
| className="flex-1 w-auto min-w-0 transition-all ease-linear duration-300 relative bg-background" | ||
| > | ||
| <PlaygroundHeader | ||
| id={id} |
There was a problem hiding this comment.
\PlaygroundHeader\ does not accept these props. Update its interface.
| <WebContainerPreview | ||
| templateData={templateData} | ||
| instance={instance} | ||
| writeFileSync={writeFileSync} |
There was a problem hiding this comment.
\writeFileSync\ can be null. Provide a fallback like \writeFileSync || (async () => {}).
Summary
Refactored
app/playground/[id]/page.tsxby extracting orchestration logic and UI sections into modular hooks and components.Changes made
playground-editor-area.tsxplayground-modals.tsxuseSaveHandlers.tsuseKeyboardShortcuts.tsuseDownload.tsuseWrappedFileHandlers.tsusePlaygroundDialogs.tsWhy
The original
page.tsxacted as a large orchestrator with heavy prop drilling and centralized logic. This refactor improves readability, modularity, and maintainability while reducing component complexity.Type of change
Related issue
Closes #31
Validation
npm run lintnpm testnpm run buildManual verification
Checklist
Summary by CodeRabbit
New Features
Improvements