refactor: streamline core UI architecture and workflows - #461
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the UI/server integration to focus on a same-origin, prefix-aware “core workflow” (Kubeflow deployment), removing remote-target proxying and Telegram-related features while restoring core session/project UX flows and adding more robust buffering/reconnect behavior.
Changes:
- Replace global/remote proxy paths with same-origin, prefix-aware routing, improved hop-by-hop header stripping, and explicit SSE cancellation handling.
- Remove Telegram bridge/settings and other non-core UI surfaces (remote server management, hint mode, AI rename, sound notifications).
- Restore/introduce core UX utilities and persistence: scoped storage/projects, command palette utilities, follow-up queue helpers, browser notification settings/events, and related tests.
Reviewed changes
Copilot reviewed 95 out of 113 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| shared/proxy.ts | Adds reusable hop-by-hop header stripping, safe script-data serialization, and SSE proxy cancellation handling. |
| shared/base-path.ts | Introduces base-path matching/stripping helpers to avoid partial-prefix routing bugs. |
| README.md | Updates documented feature surface and security model to match same-origin/core-only focus. |
| docker/telegram-bridge.ts | Removes Telegram bridge entrypoint from the image. |
| docker/serve-ui.ts | Tightens prefix enforcement, adds same-origin mutation gating, and improves SSE/WebSocket proxy handling. |
| docker/README.md | Documents same-origin-only proxying and local-only security assumptions. |
| docker/kubeflow/s6/services.d/telegram-bridge/run | Removes Telegram bridge s6 service. |
| docker/kubeflow/README.md | Updates Kubeflow image notes to reflect same-origin proxy and removed bridge. |
| docker/kubeflow/Dockerfile | Stops copying Telegram bridge and updates permissions setup accordingly. |
| docker/Dockerfile | Stops copying Telegram bridge into the generic image. |
| app-prefixable/tests/telegram-settings-ui.test.ts | Removes Telegram settings UI tests. |
| app-prefixable/tests/telegram-guide.test.tsx | Removes Telegram setup guide tests. |
| app-prefixable/tests/storage.test.ts | Adds coverage for new scoped storage key helpers and legacy selection. |
| app-prefixable/tests/session-tree-request.test.ts | Adds coverage for descendant question selection and working-state aggregation. |
| app-prefixable/tests/session-selection.test.ts | Adds coverage for multi-select/range selection and concurrency helper behavior. |
| app-prefixable/tests/session-load.test.ts | Adds coverage for session load/result classification helpers. |
| app-prefixable/tests/server.test.ts | Removes tests for now-removed remote server auth helpers. |
| app-prefixable/tests/saved-prompts-contract.test.ts | Removes older saved-prompts response contract tests. |
| app-prefixable/tests/root-session.test.ts | Adds coverage for root-session creation behavior under concurrency. |
| app-prefixable/tests/proxy-response.test.ts | Updates proxy tests for new SSE path handling, header stripping, and script-data escaping. |
| app-prefixable/tests/providers.test.ts | Adds coverage for provider normalization without mutating sync data. |
| app-prefixable/tests/projects.test.ts | Adds coverage for project persistence, merging, and legacy migrations. |
| app-prefixable/tests/project-activity.test.ts | Adds coverage for project activity badge reduction logic. |
| app-prefixable/tests/permission.test.ts | Adds coverage for permission-response failure reset behavior. |
| app-prefixable/tests/notify.test.ts | Removes legacy per-session notify storage tests. |
| app-prefixable/tests/id.test.ts | Adds coverage for monotonic, prefixed ID generation. |
| app-prefixable/tests/followups.test.ts | Adds coverage for follow-up parsing, migration, and dispatch eligibility checks. |
| app-prefixable/tests/extended-api.test.ts | Removes Telegram-source extended-api tests. |
| app-prefixable/tests/extended-api-security.test.ts | Adds coverage for cross-origin mutation rejection and idempotent MCP deletion. |
| app-prefixable/tests/config.test.ts | Adds coverage for deep-merge behavior in config state merging. |
| app-prefixable/tests/command-palette.test.ts | Adds coverage for palette query parsing and filtering behavior. |
| app-prefixable/tests/browser-notifications.test.ts | Adds coverage for notification settings parsing and legacy migration behavior. |
| app-prefixable/tests/base-path.test.ts | Adds coverage for strict base-path matching and stripping semantics. |
| app-prefixable/tests/api-contract.test.ts | Removes extended API contract tests from the backend contract suite. |
| app-prefixable/src/utils/storage.ts | Adds server/workspace-scoped key helpers and legacy selection logic. |
| app-prefixable/src/utils/session-tree-request.ts | Updates session tree helpers to support request arrays and working-state detection. |
| app-prefixable/src/utils/session-selection.ts | Adds session selection/range selection and bounded concurrency mapping helpers. |
| app-prefixable/src/utils/session-load.ts | Adds non-throwing session request helper and route/draft key helpers. |
| app-prefixable/src/utils/root-session.ts | Simplifies root session creation flow and trace, removing in-flight joining logic. |
| app-prefixable/src/utils/path.ts | Forces SDK/extended API requests through same-origin proxy; removes injected serverUrl support. |
| app-prefixable/src/utils/notify.ts | Removes legacy per-session notify storage helpers. |
| app-prefixable/src/utils/notifications.ts | Removes synthesized sound notification utilities. |
| app-prefixable/src/utils/id.ts | Adds ascending ID generator for message/part IDs. |
| app-prefixable/src/utils/followups.ts | Adds follow-up queue parsing/migration utilities and dispatch gating. |
| app-prefixable/src/utils/event-buffer.ts | Adds bounded event buffering with optional coalescing/byte caps. |
| app-prefixable/src/utils/command-palette.ts | Adds palette query parsing and entry filtering/sorting helpers. |
| app-prefixable/src/utils/browser-notifications.ts | Adds notification settings parsing, legacy parsing, and visibility gating helpers. |
| app-prefixable/src/utils/ai-rename.ts | Removes AI-based session rename helper. |
| app-prefixable/src/types/server.ts | Removes remote server config/auth header helpers. |
| app-prefixable/src/pages/settings-tabs.ts | Narrows settings tabs to the reduced/core surface. |
| app-prefixable/src/pages/project-picker.tsx | Switches recent-project tracking to the new projects provider and SDK URL source. |
| app-prefixable/src/pages/home.tsx | Adds lifetime guarding and cleanup when session creation races with navigation/unmount. |
| app-prefixable/src/pages/home-layout.tsx | Replaces stacked providers with CoreProviders and integrates command palette + project activity. |
| app-prefixable/src/pages/directory-layout.tsx | Uses CoreProviders and migrates recent-project touch behavior to projects context. |
| app-prefixable/src/entry.tsx | Removes noisy startup logging; keeps legacy SW cleanup with a warn-level log. |
| app-prefixable/src/context/terminal.tsx | Hooks terminal state to events, restores sessions on mount, and centralizes drop logic. |
| app-prefixable/src/context/sdk.tsx | Uses server context for base URL; provides url directly (no base-path serverUrl injection). |
| app-prefixable/src/context/recent-projects.tsx | Removes legacy recent-projects provider in favor of projects context. |
| app-prefixable/src/context/projects.tsx | Adds server-scoped project persistence with legacy migration and cross-tab sync. |
| app-prefixable/src/context/layout.tsx | Scopes layout storage per server/workspace and migrates legacy layout once. |
| app-prefixable/src/context/file.tsx | Subscribes to file watcher events to refresh loaded files/dirs safely. |
| app-prefixable/src/context/events.tsx | Replaces full events provider with a compatibility projection over sync store. |
| app-prefixable/src/context/core-providers.tsx | Introduces a single “CoreProviders” wrapper for consistent provider composition. |
| app-prefixable/src/context/config.tsx | Adds deep merge helper for project config updates (partial response handling). |
| app-prefixable/src/context/command.tsx | Adds palette open/close/query handling and Ctrl/Cmd+K handler. |
| app-prefixable/src/context/browser-notifications.tsx | Adds browser notification settings storage + legacy compatibility layer. |
| app-prefixable/src/context/browser-notification-events.tsx | Adds event-driven browser notification delivery for idle/questions/permissions/errors. |
| app-prefixable/src/context/base-path.tsx | Simplifies base-path context to only provide prefixing (no serverUrl). |
| app-prefixable/src/components/tool-part.tsx | Improves “question tool” running state to detect interrupted backend restarts. |
| app-prefixable/src/components/terminal.tsx | Adds bounded reconnect logic with PTY existence checks and reset-on-stability behavior. |
| app-prefixable/src/components/telegram-setup-guide.tsx | Removes Telegram setup guide component. |
| app-prefixable/src/components/telegram-setup-guide-constants.ts | Removes Telegram setup guide constants. |
| app-prefixable/src/components/shared.tsx | Migrates project avatar badge rendering to new project-activity badge model. |
| app-prefixable/src/components/session-sidebar.tsx | Switches token usage calculations to sync-backed messages (removes per-session fetch). |
| app-prefixable/src/components/session-info.tsx | Uses router-aware <A> for internal settings navigation. |
| app-prefixable/src/components/session-header.tsx | Removes per-session alarm toggle and hint-mode target markers. |
| app-prefixable/src/components/saved-prompts-settings.tsx | Adds settings UI for saved prompts create/edit/delete. |
| app-prefixable/src/components/project-dialog.tsx | Uses SDK URL and extended-api helpers via same-origin server URL. |
| app-prefixable/src/components/message-turn.tsx | Improves child session state classification using sync status readiness. |
| app-prefixable/src/components/message-timeline.tsx | Fixes flexbox scroll behavior by adding min-h-0 to scroll containers. |
| app-prefixable/src/components/mcp-dialog.tsx | Opens OAuth popup earlier and navigates it via callback to reduce popup blocking. |
| app-prefixable/src/components/hint-mode.tsx | Removes hint-mode overlay feature. |
| app-prefixable/src/components/followup-dock.tsx | Simplifies follow-up dock UI and adds retry/resume affordances. |
| app-prefixable/src/components/browser-notifications-settings.tsx | Adds settings UI for browser notification category toggles + legacy cleanup. |
| app-prefixable/eslint.config.js | Updates ESLint config with a TSX-specific override (currently misconfigured). |
| app-prefixable/dev.ts | Aligns dev server behavior with new prefix enforcement, same-origin gating, and SSE proxy cancellation. |
| AGENTS.md | Updates repository structure references for shared modules. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 95 out of 113 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
app-prefixable/src/components/mcp-dialog.tsx:37
window.open()is used to navigate to external OAuth URLs withoutnoopener/noreferrer(or otherwise clearingwindow.opener). That enables reverse-tabnabbing: the opened page can potentially navigate the opener window. Preferwindow.open(url, "_blank", "noopener,noreferrer")(and/or setpopup.opener = null) for all external navigations.
|
Addressed the latest Copilot review cycle in two commits:
Validation: 201 full tests pass, targeted OAuth tests pass, typecheck passes, production build passes, and lint has 0 errors. |
There was a problem hiding this comment.
🔵 Needs a closer look
It’s a large architectural refactor spanning transport, security checks, persistence, UI workflows, and Docker packaging, so it needs careful human validation beyond automated review.
Review details
- Files reviewed: 98/118 changed files
- Comments generated: 1
- Review effort level: Lite
| }, { | ||
| coalesce: coalesceServerEvent, | ||
| limit: 5_000, | ||
| byteLimit: 2 * 1024 * 1024, | ||
| size: (event) => encoder.encode(JSON.stringify(event)).byteLength, | ||
| resetOnOverflow: true, | ||
| overflow: () => recover("overflow"), | ||
| run: batch, |
There was a problem hiding this comment.
Valid concern. Fixed in b978013 by recording the UTF-8 byte length of each raw SSE payload before parsing and reusing it in the scheduled buffer. Coalesced events carry the sum of their precomputed payload sizes, so the hot path no longer re-serializes parsed events while retaining conservative byte-limit accounting.
There was a problem hiding this comment.
🟡 Changes recommended
Project persistence currently has no upper bound on stored project entries, creating avoidable localStorage quota/performance risk as the list grows over time.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 106/129 changed files
- Comments generated: 1
- Review effort level: Lite
| export function ProjectsProvider(props: ParentProps) { | ||
| const serverId = LOCAL_SERVER_ID | ||
| const key = projectsStorageKey(serverId) | ||
| const storage = typeof localStorage === "undefined" ? undefined : localStorage | ||
| const [projects, setProjects] = createSignal<Project[]>(storage ? loadProjects(storage, serverId) : []) | ||
| const recent = createMemo(() => [...projects()].sort((a, b) => b.lastOpened - a.lastOpened)) | ||
|
|
||
| function save(list: Project[]) { | ||
| setProjects(list) | ||
| if (!storage) return | ||
| try { | ||
| storage.setItem(key, JSON.stringify(list)) | ||
| } catch { | ||
| // Keep in-memory project management available when storage is unavailable. | ||
| } | ||
| } |
There was a problem hiding this comment.
Valid concern. Fixed in 61ccc10 by limiting canonical project persistence to the 100 most recently opened entries on parse, migration, and save. The retained entries preserve their existing list order, and duplicate merging now uses a Map instead of repeated linear searches. Added a regression test for pruning and ordering.
There was a problem hiding this comment.
🔵 Needs a closer look
Terminal close handling can silently desynchronize UI state from backend by dropping PTYs even when the remove call fails.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
app-prefixable/src/context/terminal.tsx:116
close()removes the PTY from local state even whenclient.pty.remove()fails. This can desync UI from backend (terminal still exists server-side, but disappears client-side) and also hides the failure from the user.
Only drop the session after a successful remove (or explicitly handle an idempotent-not-found case), and surface an error message when the removal fails.
- Files reviewed: 106/129 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Addressed the suppressed terminal-close finding in a7dad35: PTYs are now dropped locally only after a confirmed successful remove; HTTP 404 is handled explicitly as idempotent success. Other failures retain the terminal and surface the existing terminal error UI. Added regression coverage. API contract sessions are also cleaned up after each suite in 14b1b12. |
There was a problem hiding this comment.
🔵 Needs a closer look
The changes span critical UI architecture, SSE/event processing, proxy/security behavior, and deployment artifacts, warranting final human review despite no specific blocking issues identified in this pass.
Review details
- Files reviewed: 106/130 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Addressed the reproduced long-session input stalls in 225aeed. The active session contained 1,723 messages, 8,879 parts, and about 8.2 MB of tool output, all loaded reactively. Sync now follows upstream Web UI behavior by loading 200 messages initially with one-item lookahead, retaining only provably continuous cached history, recovering safely across overflow/reconnect gaps, fetching a missing turn parent, and loading older history on demand. Full validation: 249 tests, typecheck, build, and lint passed. |
There was a problem hiding this comment.
🔵 Needs a closer look
There is a confirmed edge-case bug in prefixStrippedAllowed() where an empty-but-defined NB_PREFIX enables stripped-prefix mode unexpectedly, weakening base-path isolation.
Review details
- Files reviewed: 106/130 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Addressed the current-head Copilot finding in the new commit: an empty |
There was a problem hiding this comment.
🟡 Changes recommended
A newly added legacy-notification parsing helper can mis-handle array-shaped stored values, potentially enabling notifications unexpectedly, and should be hardened before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 106/130 changed files
- Comments generated: 1
- Review effort level: Lite
| try { | ||
| const parsed = JSON.parse(raw) as Record<string, unknown> | ||
| if (!parsed || typeof parsed !== "object") return {} | ||
| return Object.fromEntries(Object.entries(parsed).filter(([, enabled]) => enabled === true).map(([key]) => [key, true])) as Record<string, boolean> | ||
| } catch { | ||
| return {} | ||
| } |
There was a problem hiding this comment.
Valid concern. Fixed by rejecting array-shaped legacy notification storage before enumerating enabled session keys. Added regression coverage for [true]; focused tests, typecheck, and lint pass.
There was a problem hiding this comment.
🔵 Needs a closer look
The PR combines large architectural refactors with proxy/security-sensitive server behavior changes, warranting final human validation (especially in prefixed Kubeflow Notebook deployments).
Review details
- Files reviewed: 106/130 changed files
- Comments generated: 0 new
- Review effort level: Lite
Summary
This PR refactors the prefix-aware SolidJS frontend around a smaller core architecture, consolidates state and event handling, and restores the workflows required for daily use in Kubeflow Notebooks.
The latest update uses one app-wide
/global/eventstream for all projects, directory-scoped consumers for active state, bounded event buffering, and REST snapshots for startup, reconnect, and overflow reconciliation. Browser and proxy paths remain runtime-prefix-aware.What Changed
State And Transport
Core Workflows
Security And Reliability
Intentional Removals
Release Dependencies
1.18.23in both Dockerfiles and the image workflow.brace-expansion,browserslist,nanoid, andpostcss.Validation
bun run test: 249 tests passed across 32 files.bun run typecheck: passed.bun run build: passed.bun run lint: 0 errors and 62 existing warnings.1.18.20prompt confirmedbusyandidleevents through/global/eventand status snapshots through/session/status.git diff --check: passed.Test Image
Digest:
sha256:ae1169d494c265a0faa1265cc6b40b85306d0883cb2f30d1d6677cfd02abc263Build run: https://github.com/prokube/pk-opencode-webui/actions/runs/34135071778
This image was built successfully from current head
e38a20466with OpenCode1.18.23. A prefixed Kubeflow Notebook smoke test is still required before merging.Known Upstream Issue
OpenCode can retain server-side listeners when SSE clients disconnect. Repeated development reloads may trigger
MaxListenersExceededWarning; restarting OpenCode clears the retained listeners.Review Guide
app-prefixable/src/context/server-events.tsxfor global stream parsing, buffering, and recovery.app-prefixable/src/context/sync.tsxfor event ordering and snapshot consistency.app-prefixable/src/context/project-activity.tsxfor background activity and notification reconciliation.shared/proxy.tsand the production server for SSE cancellation and prefix-aware routing.