feat: redesign the web ui and fix preview/run/stop#2
Conversation
|
Warning Review limit reached
Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIntroduces shared workspace app state on the server, updates preview routing and proxying, adds history event plumbing, and refreshes the web UI icons, chat, projects, and styling. ChangesServer: AppHub, Session, Preview, Runner
Web UI: icons, chat, theme, store
CI Coverage Formatting
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ 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 |
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
packages/web/src/styles.css (1)
62-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor token inconsistency: a few hardcoded hex colors bypass the new CSS variable system.
Scrollbar thumb colors,
.terminalbackground, and.preview-bar button.stop:hoveruse raw hex values instead of the new--bg-*/--badtokens, unlike the rest of the theme.♻️ Example alignment with tokens
::-webkit-scrollbar-thumb { - background: `#2c2c31`; + background: var(--bg-3); border: 2px solid transparent; background-clip: padding-box; border-radius: 8px; } ::-webkit-scrollbar-thumb:hover { - background: `#3a3a41`; + background: var(--border-strong); background-clip: padding-box; } ... .terminal { display: flex; flex-direction: column; height: 100%; - background: `#0d1117`; + background: var(--bg-0); } ... .preview-bar button.stop:hover { - background: `#ff7373`; + background: var(--bad); }Also applies to: 665-670, 722-724
🤖 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 `@packages/web/src/styles.css` around lines 62 - 75, The theme styling in styles.css still uses hardcoded hex colors in the scrollbar thumb rules and related UI states, which bypasses the new CSS token system. Update the scrollbar thumb/hover styles, the .terminal background, and the .preview-bar button.stop:hover state to use the existing --bg-* and --bad variables instead of raw hex values, keeping the token usage consistent with the rest of the stylesheet.packages/server/src/session.ts (1)
210-220: 🩺 Stability & Availability | 🔵 TrivialOrphaned dev servers when all sessions disconnect.
unmount()intentionally leaves the workspace app running in the hub so a reconnecting tab still sees it. However, nothing stops the hub's app once every session for that workspace is gone. On a long-lived server, repeatedly opening/running different projects (or closing all tabs) can accumulate orphaned dev-server processes holding ports. Consider a hub-level cleanup for workspaces with no attached sessions after an idle window, or stopping the app when the last session for adircloses.🤖 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 `@packages/server/src/session.ts` around lines 210 - 220, The unmount flow in session.ts only closes the tab mount and workspace handle, but it never stops the hub-level app when the last session for a workspace disappears, which can leave orphaned dev servers running. Update Session.unmount() and related session/workspace tracking so that when the final session for a given dir/workspace closes, the hub app is cleaned up after an idle window or stopped immediately if no sessions remain. Use the existing activeRun, mount, and workspace/session lifecycle logic to locate the cleanup point and ensure the workspace-level process is terminated only when it is no longer attached to any session.packages/server/test/app-hub.test.ts (1)
1-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssertions are appropriately specific; consider extending coverage for stop/restart edge cases.
Test assertions correctly target specific values (
toBe('running'),toBe(5173),toBe(true)) rather than generic substrings, consistent with path instructions for*.test.tsfiles. As a follow-up, consider adding coverage for the scenarios flagged inapp-hub.ts: (a)runningPreview()when one workspace is stopped and a second, different workspace is running, and (b) restarting the same app on the same port to confirmpreview_readystill re-broadcasts.As per path instructions,
packages/**/*.test.ts: "Prefer asserting on specific values (paths, tool names, model tiers) over generic substrings."🤖 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 `@packages/server/test/app-hub.test.ts` around lines 1 - 59, Extend the AppHub test coverage to include the edge cases called out in app-hub.ts: add a case that verifies runningPreview() returns the active preview for a different workspace after one workspace has been stopped, and another case that exercises restarting the same app on the same port to confirm notePort() still emits a preview_ready event again. Use the existing AppHub, stop, run, preview, and notePort helpers so the new assertions stay specific and aligned with the current test style.Source: Path instructions
🤖 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 `@packages/server/src/app-hub.ts`:
- Around line 92-114: Wrap both `CommandRunner.run()` in the install branch and
`WorkflowManager.start()` in `app-hub.ts` with error handling so rejected
promises do not leave the app stuck in `installing` or `starting`. In the
`app.installer` flow and the `app.mgr` flow, catch thrown errors, clear the
active runner/manager state, and call `setStatus` with `type: 'app_status'`,
`state: 'error'`, and a message that includes the failure context. Use the
existing `app.installer`, `app.mgr`, `setStatus`, and `broadcast` paths to keep
the state consistent and allow retry after failure.
- Around line 78-116: Serialize the AppHub run/stop lifecycle per directory so
overlapping calls cannot interleave the await this.stop(dir) → detectWorkflows()
→ app.mgr.start(wf) sequence. Add a per-dir lock or queue inside AppHub.run and
make stop(dir) participate in the same guard, using the existing appFor,
app.mgr, and app.activeWorkflow flow to ensure one workflow launch/teardown
completes before the next begins and status stays consistent.
- Around line 103-113: The workflow exit handling in app-hub currently marks the
whole app as stopped on every child process exit, which causes premature
teardown while sibling steps are still running. Update the
WorkflowManager.start() wiring in app.mgr initialization to track active child
processes per workflow, and only clear app.mgr and app.activeWorkflow after the
final child exits. Use the existing callbacks and symbols in AppHub,
WorkflowManager.start, notePort, and setStatus to keep status updates accurate
for each step without resetting the workflow too early.
- Around line 58-70: Update the preview lifecycle in app-hub so a stopped
workspace does not keep advertising a live preview. The issue is that stop() is
leaving PreviewManager’s port set, which lets runningPreview() keep returning a
dead app and can prevent a fresh preview refresh on restart. Fix this by
clearing the stored port when stopping a preview, or by making runningPreview()
skip apps whose mgr is null, using the existing runningPreview(), notePort(),
and PreviewManager state to keep /__preview in sync.
In `@packages/server/src/preview.ts`:
- Around line 45-71: The HTML rewrite path in preview handling can corrupt
compressed responses because `proxyReq` forwards `accept-encoding` and
`proxyRes` is later decoded with `toString('utf8')` in the `rewritePreviewHtml`
flow. Update the proxy request in `packages/server/src/preview.ts` so HTML is
either requested uncompressed by forcing `accept-encoding: identity` or
explicitly decompressed before rewriting, then continue stripping
`content-encoding` and adjusting `content-length` before sending the response.
---
Nitpick comments:
In `@packages/server/src/session.ts`:
- Around line 210-220: The unmount flow in session.ts only closes the tab mount
and workspace handle, but it never stops the hub-level app when the last session
for a workspace disappears, which can leave orphaned dev servers running. Update
Session.unmount() and related session/workspace tracking so that when the final
session for a given dir/workspace closes, the hub app is cleaned up after an
idle window or stopped immediately if no sessions remain. Use the existing
activeRun, mount, and workspace/session lifecycle logic to locate the cleanup
point and ensure the workspace-level process is terminated only when it is no
longer attached to any session.
In `@packages/server/test/app-hub.test.ts`:
- Around line 1-59: Extend the AppHub test coverage to include the edge cases
called out in app-hub.ts: add a case that verifies runningPreview() returns the
active preview for a different workspace after one workspace has been stopped,
and another case that exercises restarting the same app on the same port to
confirm notePort() still emits a preview_ready event again. Use the existing
AppHub, stop, run, preview, and notePort helpers so the new assertions stay
specific and aligned with the current test style.
In `@packages/web/src/styles.css`:
- Around line 62-75: The theme styling in styles.css still uses hardcoded hex
colors in the scrollbar thumb rules and related UI states, which bypasses the
new CSS token system. Update the scrollbar thumb/hover styles, the .terminal
background, and the .preview-bar button.stop:hover state to use the existing
--bg-* and --bad variables instead of raw hex values, keeping the token usage
consistent with the rest of the stylesheet.
🪄 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: bb8a08de-3dad-4caf-907f-8fc1eed1d613
📒 Files selected for processing (18)
packages/server/src/app-hub.tspackages/server/src/index.tspackages/server/src/preview.tspackages/server/src/runner.tspackages/server/src/session.tspackages/server/test/app-hub.test.tspackages/server/test/preview.test.tspackages/server/test/runner.test.tspackages/shared/src/protocol.tspackages/web/index.htmlpackages/web/src/App.tsxpackages/web/src/icons.tsxpackages/web/src/panes/Chat.tsxpackages/web/src/panes/FilesEditor.tsxpackages/web/src/panes/Preview.tsxpackages/web/src/panes/Projects.tsxpackages/web/src/store.tspackages/web/src/styles.css
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/server/src/app-hub.ts (2)
177-192: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
stopLockedleaves state inconsistent ifmgr.stop()rejects.There's no try/catch around
await app.mgr.stop(). If it throws,app.mgrstays set,activeWorkflowand the preview port are never cleared, andsetStatusnever fires — so the UI shows a running app pointing at a dead server. Sincedetachcallsvoid this.stop(dir), the rejection also becomes unhandled and can crash the process.🔧 Proposed fix
private async stopLocked(dir: string): Promise<void> { const app = this.apps.get(dir); if (app) { app.installer?.stopRuns(); app.installer = null; if (app.mgr) { - await app.mgr.stop(); + try { await app.mgr.stop(); } + catch { /* best-effort — still clean up state below */ } app.mgr = null; } app.activeWorkflow = null; app.preview.clearPort(); } this.setStatus(dir, { type: 'app_status', state: 'stopped' }); }🤖 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 `@packages/server/src/app-hub.ts` around lines 177 - 192, The stopLocked flow can leave AppHub state half-updated if app.mgr.stop() rejects, so update stopLocked to handle failures from app.mgr.stop() while still cleaning up app.installer, app.mgr, activeWorkflow, and app.preview consistently. Wrap the stop sequence in a try/finally (or equivalent) so cleanup and setStatus in AppHub always run even when mgr.stop() fails, and make sure the rejection is observed/logged so detach calling stop(dir) does not produce an unhandled rejection.
162-168: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winTear down the app before clearing references
WorkflowManager.start()can leave already-spawned detached children behind if a later step throws. Run the existing cleanup before nullingapp.mgr/app.installer, so the failure path doesn’t orphan processes or keep ports busy.🤖 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 `@packages/server/src/app-hub.ts` around lines 162 - 168, The failure path in app-hub’s app startup catch block clears app.installer and app.mgr before cleanup, which can leave detached children running and ports occupied. In the catch around WorkflowManager.start(), call the existing teardown/cleanup logic first on the current app state, then null out app.mgr and app.installer, and finally set the error status so the process tree is reliably stopped before references are dropped.
🤖 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.
Outside diff comments:
In `@packages/server/src/app-hub.ts`:
- Around line 177-192: The stopLocked flow can leave AppHub state half-updated
if app.mgr.stop() rejects, so update stopLocked to handle failures from
app.mgr.stop() while still cleaning up app.installer, app.mgr, activeWorkflow,
and app.preview consistently. Wrap the stop sequence in a try/finally (or
equivalent) so cleanup and setStatus in AppHub always run even when mgr.stop()
fails, and make sure the rejection is observed/logged so detach calling
stop(dir) does not produce an unhandled rejection.
- Around line 162-168: The failure path in app-hub’s app startup catch block
clears app.installer and app.mgr before cleanup, which can leave detached
children running and ports occupied. In the catch around
WorkflowManager.start(), call the existing teardown/cleanup logic first on the
current app state, then null out app.mgr and app.installer, and finally set the
error status so the process tree is reliably stopped before references are
dropped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 45d8d88d-4fff-49f7-96b8-be1695ff6219
📒 Files selected for processing (4)
packages/server/src/app-hub.tspackages/server/src/session.tspackages/server/test/app-hub.test.tspackages/web/src/styles.css
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/server/src/session.ts
- packages/web/src/styles.css
|
@coderabbitai review Addressed all 8 comments from the previous review (5 actionable + 3 nitpicks):
Please confirm the threads are resolved. |
|
✅ Action performedReview finished.
|
|
@coderabbitai review Fixed the two follow-up comments on app-hub.ts:
|
🧩 Analysis chain🏁 Script executed: #!/bin/bash
fd app-hub.ts
echo "---"
cat -n packages/server/src/app-hub.tsLength of output: 9876
Also nice touch: the exit callback's ✅ Action performedReview finished.
|
1 participant
Summary
Redesigns the OpenREPL web UI into a modern AI-IDE look (Replit-inspired) and fixes several functional issues surfaced while testing the preview/run/stop flow end-to-end.
UI redesign
#ff3c00), Inter font, larger display type, rounded panels.Functional fixes (found via real testing)
pickPreview), so a second tab/reconnect can't steal it.AppHub), so any tab sees the same status/Stop button and Stop always reaches the process./__preview, so a server-rendered app's links/forms/assets don't escape to OpenREPL's SPA fallback.Testing
app-hub.test.ts,pickPreview+rewritePreviewHtmlinpreview.test.ts,stopRunsinrunner.test.ts.Session Context
Session transcript
Summary by CodeRabbit