New profiler UI by Karakatiza666 · Pull Request #6306 · feldera/feldera

Karakatiza666 · 2026-05-23T10:19:35Z

Implement new profiler UI that includes

Three-pane layout:

Dataflow graph
Readonly SQL editor
Tabs:
The new Dataflow Metrics UI
Logs
Profile Triage Suggestions

Testing: manual, added e2e smoke test that excercises "live" bundle parsing and cross-page navigation

Fix #6338
Fix #5301

mythical-fred

High-level pass on the draft — saving the full checklist for when this is ready.

Design-level concerns to think about before this leaves draft:

Cross-tab handoff for large bundles (profileBundleHandoff.ts + profile-bundle/+page.svelte). Support bundles can be hundreds of MB. BroadcastChannel does a structured clone (the comment acknowledges this), so the upload-tab handoff briefly holds 2× the bundle in memory and copies every byte across worlds. For a 500 MB bundle on a 4 GB browser tab that is a real risk, not a paper one. Two cleaner options worth considering:
- child.postMessage(buffer, '*', [buffer]) with the actual Transferable once the child posts READY — zero-copy, frees the parent's reference.
- Hand the bundle to a SharedWorker / OPFS file and just postMessage a handle.
  At minimum, please pick a max bundle size you've actually tested at and document it.
ACK_TIMEOUT_MS = 3 s is brittle. The rationale comment ("clone is microtask-fast") is only true for small payloads. A multi-hundred-MB structured clone on a mid-range laptop is measurably slower than that, and you'll see spurious "viewer tab did not acknowledge" errors with no recourse for the user. Either scale the timeout with bundle size, or pick the transfer-list approach above and drop the timeout entirely.
$effect runs an unawaited async IIFE when selectedProfile changes. If a user clicks quickly between snapshots, two processProfileFiles chains race and whichever finishes last wins — getProfileData/triageResults can flip to stale data. Capture a version counter (or AbortController) at effect start and bail out on completion if it no longer matches the current selection.
Tests. Hard rule: web-console behavior changes need tests. You deleted the existing TabProfileVisualizer coverage from DeletedPipelineState.svelte.test.ts and replaced ~360 LoC of TabProfileVisualizer.svelte with ~340 LoC of new +page.svelte, ~420 LoC of SupportBundleViewerLayout, and ~590 LoC of metrics/registry.ts — none of it tested. profileBundleHandoff in particular is pure logic (timer + state machine) and trivially Vitest-able with a fake BroadcastChannel + window.open. Please don't leave this PR's test coverage net-negative.
setup-triage.sh. Cloning into a path that's then bun add --no-saved for everyone who runs the dev server is awkward — it means the standard bun install workflow leaves you with build errors until the manual step runs. If the upstream virtual:feldera-triage-plugins module already handles "plugins absent" gracefully (the old TabProfileVisualizer uses it too), great; if not, the +page.svelte's if (triagePlugins.length > 0) guard suggests it does but the build-time virtual: import resolution still needs the plugin's resolveId to return something. Worth a sentence in the PR description about what bun run dev does on a fresh checkout without the cloud repo.
CLAUDE.md in profiler-layout. Fine in principle — there's already one at the repo root. But the new file's "Related Documentation" links use hardcoded devcontainer paths (/workspaces/feldera/…) that won't resolve on anyone's machine. Use repo-relative paths.

Not blockers; saving inline-comment noise for when you mark this ready. Looking forward to seeing it.

mythical-fred

All six concerns from the draft review are addressed; verdict APPROVE.

Follow-up on prior concerns (commit 5b6fdbb):

#	Concern	Status	Evidence
1	Structured-clone 2x memory on hundreds-of-MB bundles	Addressed	`profileBundleHandoff.ts` now uses `child.postMessage(..., [arrayBuffer])` -- Transferable, zero-copy; source ref detaches. Hybrid BroadcastChannel-for-control + window.postMessage-for-data design is clean. The header docstring even spells out the rationale and the "peak memory drops from 2x down to 1x" property.
2	3 s ACK_TIMEOUT brittle for large clones	Addressed (and tightened)	With Transferable transfer the round-trip is sub-ms, so `ACK_TIMEOUT_MS = 1_000` is well-justified by the comment ("~1000x margin").
3	`$effect` with unawaited async IIFE -> race	Addressed	`createLoadGuard` (`profiler-layout/src/lib/functions/loadGuard.ts`) serialises loads: a second call while a load is in flight returns immediately. The trigger surfaces (button / file picker / timestamp selector) are disabled while `isLoading` is true, so concurrent invocations cannot race. Profile loading runs synchronously from `+page.svelte`, not from a reactive `$effect`.
4	Behavior changes without tests (hard rule)	Addressed via integration test	`tests/supportBundleHandoff.e2e.ts` exercises: live bundle download -> viewer mount -> Logs panel populated -> bundle re-upload via cross-tab Transferable handoff -> second viewer mount -> Logs panel populated. That covers the highest-risk new code (handoff, viewer wiring, processZipBundle pipeline). Per PR_REVIEW_RULES: "Integration tests that cover the changed behavior are sufficient -- unit tests are not additionally required in that case." Acceptable.
5	`setup-triage.sh` fragility	Addressed	No `setup-triage.sh` remains in the diff.
6	Subdirectory `CLAUDE.md` with hardcoded paths	Addressed	No new `CLAUDE.md` in the diff.

Two minor nits inline. Nothing blocking.

mythical-fred · 2026-05-30T10:09:19Z

+      clearTimeout(timer)
+      window.removeEventListener('message', onMessage)
+      const buffer = event.data.buffer as ArrayBuffer
+      // ACK before resolving so the source tab's 3 s window is not consumed by


Stale comment: this says "the source tab's 3 s window" but ACK_TIMEOUT_MS is now 1000 ms. Update to "1 s window" (or just "timeout window") to match the constant.

mihaibudiu

I have only read this, but I will have to try it, it's impossible to judge UI code solely from reading.

mihaibudiu · 2026-05-30T17:45:40Z

+// for information about these interfaces
+declare global {
+	namespace App {
+		// interface Error {}


is this needed?

It is a helpful enumeration of framework interfaces, generated from the project template; it is my preference to leave it to quickly reference where I can extend an interface once it's need.

mihaibudiu · 2026-05-30T18:04:56Z

 # Only install package versions published at least 7 days ago
 minimumReleaseAge = 604800
-minimumReleaseAgeExcludes = ["svelte", "devalue"]
+minimumReleaseAgeExcludes = []


Is this needed?

mihaibudiu · 2026-05-30T23:22:37Z

+
+  type Snippet<T extends unknown[] = []> = (...params: T) => ReturnType<SvelteSnippet<T>>
+
+  export type SegmentedItem<V extends string = string> = {


I cannot guess what this is used for, maybe add some comments.
Same observation for PersistentContent, really.

mihaibudiu · 2026-05-30T23:24:41Z

@@ -0,0 +1,65 @@
+# Svelte library


Is this file relevant, or is it just copy-pasted from some place?
I don't see anything related to our UI.

mihaibudiu · 2026-05-30T23:27:50Z

+ *  (cascade re-parsing of the dual-tone value doesn't reach the data-theme scope where the
+ *  singular tones are defined). */
+export function barColor(t: number): string {
+  return lerpThemeColor(t, '--bar-lo', '--bar-hi')


where is this function? I don't see any imports.

lerpThemeColor is defined right above

mihaibudiu · 2026-05-31T00:10:33Z

@@ -0,0 +1,90 @@
+import type { DecodedBundle, TriagePlugin, TriageRuleResult } from 'triage-types'


could be in the triage PR

mihaibudiu · 2026-05-31T00:15:38Z

+  let selectedProfile: Date | null = $state(null)
+  let getProfileData:
+    | (() => {
+        profile: import('profiler-lib').JsonProfiles


This has to be done dynamically?
Won't detecting drift between types at build time be better?

This is just confusing syntax, this is still compile-time. Will adjust this

mihaibudiu · 2026-05-31T00:17:15Z

@@ -0,0 +1,329 @@
+<script lang="ts">


some of this code should have existed previously

mihaibudiu · 2026-05-31T00:19:51Z

+    lookupQuery = ''
+  })
+
+  // Panel-B (SQL) tab state. Single-tab; bound state is required by TabsPanel.


Why name them Panel-A, B, C instead of PanelGraph, PanelSQL, etc?

Renamed more appropriately

mihaibudiu · 2026-05-31T01:03:12Z

+  ])
+</script>
+
+<!-- ── Panel A (dataflow graph) ────────────────────────────────────────── -->


could the code after this point be moved to a separate file?

mythical-fred

Re-reviewed after the force-push to aef0805. Beyond the rebase pickup, the PR-only delta is coherent polish on the same architecture I approved at 5b6fdbb: Monaco worker setup hoisted out of MonacoEditorRunes into @feldera/vite-plugin-monaco-editor (consumers now own languages/features/inlineWorkers); processZipBundle gained logText + pipelineName extraction; the loadGuard pattern is threaded through both profiler-app and the web-console viewer route, replacing the prior ad-hoc isLoading/try-catch blocks; DownloadSupportBundle was rewritten around the new openUploadBundleTab / openRemoteBundleTab flow with a two-step picked-file confirmation so the popup gesture survives the async file read; TriageResultsView + lookup coordinator added. Prior inline nit (the stale '3 s window' comment on profileBundleHandoff.ts) is resolved -- header now consistently references ~1 s. E2E supportBundleHandoff.e2e.ts still covers download -> view -> upload -> view. Still LGTM.

mythical-fred

Re-review of 636fee3: rebased onto a new base; the only substantive change vs the previously approved aef0805 is one line in js-packages/web-console/vite.config.ts — adding 'fancy-ansi > escape-html' to testOptimizeDepsInclude. The remaining 111 files are byte-identical to the prior tip. LGTM.

Includes: Dataflow diagram Readonly SQL code editor Panel with the new Metrics visualization Logs panel Support triage issues and suggestions Signed-off-by: Karakatiza666 <bulakh.96@gmail.com>

mythical-fred reviewed May 23, 2026

View reviewed changes

Karakatiza666 force-pushed the new-profiler-ui branch 7 times, most recently from fa70de4 to 5b6fdbb Compare May 30, 2026 08:55

Karakatiza666 requested a review from mihaibudiu May 30, 2026 08:55

Karakatiza666 marked this pull request as ready for review May 30, 2026 08:55

Karakatiza666 requested a review from mythical-fred May 30, 2026 09:04

mythical-fred approved these changes May 30, 2026

View reviewed changes

mihaibudiu approved these changes May 31, 2026

View reviewed changes

Karakatiza666 force-pushed the new-profiler-ui branch 2 times, most recently from 7bc4a99 to aef0805 Compare June 2, 2026 17:22

Karakatiza666 enabled auto-merge June 2, 2026 17:22

Karakatiza666 added this pull request to the merge queue Jun 2, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 2, 2026

mythical-fred approved these changes Jun 2, 2026

View reviewed changes

Karakatiza666 force-pushed the new-profiler-ui branch from aef0805 to 636fee3 Compare June 2, 2026 19:20

Karakatiza666 enabled auto-merge June 2, 2026 19:20

Karakatiza666 added this pull request to the merge queue Jun 2, 2026

mythical-fred approved these changes Jun 2, 2026

View reviewed changes

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 2, 2026

Karakatiza666 force-pushed the new-profiler-ui branch from 636fee3 to 2b1a48b Compare June 3, 2026 14:11

Implement the new Profile Viewer layout

539ac20

Includes: Dataflow diagram Readonly SQL code editor Panel with the new Metrics visualization Logs panel Support triage issues and suggestions Signed-off-by: Karakatiza666 <bulakh.96@gmail.com>

Karakatiza666 force-pushed the new-profiler-ui branch from 2b1a48b to 539ac20 Compare June 3, 2026 14:12

Karakatiza666 enabled auto-merge June 3, 2026 14:12

Karakatiza666 added this pull request to the merge queue Jun 3, 2026

Merged via the queue into main with commit 2dd8f4c Jun 3, 2026
1 check passed

Karakatiza666 deleted the new-profiler-ui branch June 3, 2026 15:49


		type Snippet<T extends unknown[] = []> = (...params: T) => ReturnType<SvelteSnippet<T>>

		export type SegmentedItem<V extends string = string> = {

		@@ -0,0 +1,90 @@
		import type { DecodedBundle, TriagePlugin, TriageRuleResult } from 'triage-types'

Conversation

Karakatiza666 commented May 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mythical-fred left a comment

Choose a reason for hiding this comment

Uh oh!

mythical-fred left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mihaibudiu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mythical-fred left a comment

Choose a reason for hiding this comment

Uh oh!

mythical-fred left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Karakatiza666 commented May 23, 2026 •

edited

Loading