Measure page load and shader compile times #8996
Conversation
Disable interpolation in case of WebGL context losses, to try again with a more simple shader. Compile shader code asynchronously to avoid lag during page load.
📝 WalkthroughWalkthroughAdds a Window augmentation flag, collects WebGL vendor/renderer analytics, defers/gates rendering until async shader compilation to measure time‑to‑first‑render, renames readiness actions/state from "ready" → "initialized" and propagates those renames across sagas, tests, and UI plumbing. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/javascripts/viewer/view/layouting/tracing_layout_view.tsx (1)
127-136: Fix leaked window listeners on early return (superuser path).Early return in componentWillUnmount prevents removing resize/touch/mouseover listeners, causing duplicates after re-mounts.
Apply this diff to always detach listeners before branching:
componentWillUnmount() { document.removeEventListener("paste", this.onPaste); UrlManager.stopUrlUpdater(); Model.reset(); destroySceneController(); Store.dispatch(resetStoreAction()); Store.dispatch(cancelSagaAction()); + // Always remove window listeners to avoid leaks, regardless of reload behavior. + window.removeEventListener("resize", this.debouncedOnLayoutChange); + window.removeEventListener("touchstart", this.handleTouch); + window.removeEventListener("mouseover", this.handleMouseOver); + const { activeUser } = Store.getState(); if (activeUser?.isSuperUser) { // For super users, we don't enforce a page reload. // They'll act as a guinea pig for this performance // improvement for now. // Since the page is not reloaded, the performance navigation // timings are no longer accurate and should not be used. window.measuredTimeToFirstRender = true; return; } // Enforce a reload to absolutely ensure a clean slate. @@ - window.removeEventListener("resize", this.debouncedOnLayoutChange); - window.removeEventListener("touchstart", this.handleTouch); - window.removeEventListener("mouseover", this.handleMouseOver);
🧹 Nitpick comments (3)
frontend/javascripts/viewer/controller/renderer.ts (1)
47-71: Consider cleaning up the temporary canvas element.The function creates a temporary canvas element to query WebGL capabilities but never removes it from memory. While this is likely called infrequently for analytics (minimizing the impact), explicitly cleaning up the canvas would prevent a minor memory leak.
Apply this diff to clean up the canvas:
const canvas = document.createElement("canvas"); const gl = canvas.getContext("webgl2"); if (gl != null) { const debugInfo = gl.getExtension("WEBGL_debug_renderer_info"); if (debugInfo != null) { info.vendor = gl.getParameter(debugInfo.UNMASKED_VENDOR_WEBGL); info.renderer = gl.getParameter(debugInfo.UNMASKED_RENDERER_WEBGL); } } + // Clean up the temporary canvas + canvas.width = 0; + canvas.height = 0; return info; }Note: The function is otherwise well-structured with defensive null checks for the WebGL context and debug extension.
frontend/javascripts/libs/window.ts (1)
79-79: LGTM on window augmentation.Optional: add a brief comment describing the flag’s semantics (set once after first-render is measured or intentionally skipped).
frontend/javascripts/viewer/view/tracing_view.tsx (1)
16-17: Type the canvas ref for clarity (optional).Use HTMLCanvasElement | null and WebGL-specific event types to drop ts-expect-error.
Example:
const registerWebGlCrashHandler = (canvas: HTMLCanvasElement | null) => { /* ... */ }; // and inside listeners: // (e: Event) => { /* ... */ } // or custom type if available
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/javascripts/libs/window.ts(1 hunks)frontend/javascripts/viewer/controller/renderer.ts(3 hunks)frontend/javascripts/viewer/view/arbitrary_view.ts(3 hunks)frontend/javascripts/viewer/view/layouting/tracing_layout_view.tsx(2 hunks)frontend/javascripts/viewer/view/plane_view.ts(9 hunks)frontend/javascripts/viewer/view/tracing_view.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/javascripts/viewer/view/tracing_view.tsx (3)
frontend/javascripts/admin/rest_api.ts (1)
sendAnalyticsEvent(117-128)frontend/javascripts/viewer/controller/renderer.ts (1)
getWebGlAnalyticsInformation(47-71)frontend/javascripts/viewer/model/actions/settings_actions.ts (1)
updateDatasetSettingAction(65-73)
frontend/javascripts/viewer/controller/renderer.ts (2)
frontend/javascripts/viewer/store.ts (1)
WebknossosState(575-595)frontend/javascripts/libs/window.ts (2)
location(60-60)document(19-34)
frontend/javascripts/viewer/view/plane_view.ts (2)
frontend/javascripts/admin/rest_api.ts (1)
sendAnalyticsEvent(117-128)frontend/javascripts/viewer/controller/renderer.ts (1)
getWebGlAnalyticsInformation(47-71)
🔇 Additional comments (6)
frontend/javascripts/viewer/controller/renderer.ts (2)
2-2: LGTM!The import additions for
locationandWebknossosStateappropriately support the new analytics gathering functionality.Also applies to: 5-5
18-18: LGTM!Exporting
getRendererenables external modules to access the cached WebGL renderer instance, supporting the analytics and context-loss handling features added in this PR.frontend/javascripts/viewer/view/arbitrary_view.ts (1)
87-91: Skip first-render measurement in non-orthogonal modes — good.Setting window.measuredTimeToFirstRender avoids misleading timings when starting in arbitrary/flight/oblique.
frontend/javascripts/viewer/view/tracing_view.tsx (2)
32-32: Context-loss analytics — good coverage.sendAnalyticsEvent with getWebGlAnalyticsInformation provides actionable signals for debugging GPU/driver issues.
42-49: Auto-disable interpolation on restore — pragmatic recovery.Nice mitigation to reduce shader complexity after a loss; user-informed via Toast.
Consider emitting a small analytics event (e.g., interpolation_auto_disabled) to quantify frequency/impact. Optional.
frontend/javascripts/viewer/view/plane_view.ts (1)
140-143: One-shot timing trigger — good placement.Gating by window.measuredTimeToFirstRender ensures single analytics emission post-first successful viewport render.
…ag and white viewports
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
frontend/javascripts/viewer/view/arbitrary_view.ts (1)
113-119: Handle compileAsync rejection to prevent UI stallIf compileAsync rejects, animate never starts and UI_READY is never dispatched, leaving a permanent spinner/hang.
Apply this patch to add a safe fallback:
- renderer.compileAsync(scene, this.camera).then(() => { - // Counter-intuitively this is not the moment where the webgl program is fully compiled. - // There is another stall once render or getProgramInfoLog is called, since not all work is done yet. - // Only once that is done, the compilation process is fully finished, see `renderFunction`. - Store.dispatch(uiReadyAction()); - this.animate(); - }); + renderer + .compileAsync(scene, this.camera) + .then(() => { + // Counter-intuitively this is not the moment where the webgl program is fully compiled. + // There is another stall once render or getProgramInfoLog is called. + Store.dispatch(uiReadyAction()); + this.animate(); + }) + .catch((err) => { + console.warn("renderer.compileAsync failed; continuing", err); + Store.dispatch(uiReadyAction()); + this.animate(); + });Optional: consider dispatching UI_READY after the first successful render (in renderFunction) to avoid removing the cover right before the initial compile stall on render.
frontend/javascripts/viewer/view/plane_view.ts (2)
339-345: Guard compileAsync: add catch/fallback and always dispatch UI ready + start animationIf compileAsync rejects or is undefined, UI can hang (no UI_READY, no animate). Add a fallback and ensure cleanup in finally.
Apply this diff:
- // The shader is the same for all three viewports, so it doesn't matter which camera is used. - renderer.compileAsync(scene, this.cameras[OrthoViews.PLANE_XY]).then(() => { - // Counter-intuitively this is not the moment where the webgl program is fully compiled. - // There is another stall once render or getProgramInfoLog is called, since not all work is done yet. - // Only once that is done, the compilation process is fully finished, see `renderFunction`. - Store.dispatch(uiReadyAction()); - this.animate(); - }); + // The shader is the same for all three viewports, so it doesn't matter which camera is used. + const compilePromise: Promise<unknown> = + typeof (renderer as any).compileAsync === "function" + ? renderer.compileAsync(scene, this.cameras[OrthoViews.PLANE_XY]) + : Promise.resolve(); + + compilePromise + .catch((err) => { + console.warn("renderer.compileAsync failed; continuing", err); + }) + .finally(() => { + // Counter-intuitively this is not the moment where the webgl program is fully compiled. + // There is another stall once render or getProgramInfoLog is called, since not all work is done yet. + // Only once that is done, the compilation process is fully finished, see `renderFunction`. + Store.dispatch(uiReadyAction()); + this.animate(); + });Based on learnings
393-408: Harden performance API usage (older browsers/tests) and compute compile time robustlyDirect calls to performance.getEntriesByType()[0] and performance.measure can throw or be undefined. Use window.performance, guard calls, and derive compile duration from the last shader_compile_start mark. Optionally clear marks after.
Apply this diff:
- measureTimeToFirstRender() { - const timeToFirstRenderInMs = Math.round( - performance.now() - performance.getEntriesByType("navigation")[0].startTime, - ); - const timeToCompileShaderInMs = Math.round( - performance.measure("shader_compile_duration", "shader_compile_start").duration, - ); - console.log(`Time to compile shaders was ${timeToCompileShaderInMs} ms.`); - console.log(`Time to first render was ${timeToFirstRenderInMs} ms.`); - sendAnalyticsEvent("time_to_first_render", { - ...getWebGlAnalyticsInformation(Store.getState()), - timeToFirstRenderInMs, - timeToCompileShaderInMs, - }); - window.measuredTimeToFirstRender = true; - } + measureTimeToFirstRender() { + const perf = window.performance ?? performance; + const now = + typeof perf?.now === "function" ? perf.now() : Date.now(); + const navEntries = + typeof perf?.getEntriesByType === "function" + ? perf.getEntriesByType("navigation") + : []; + const navStart = (navEntries[0] as PerformanceNavigationTiming | undefined)?.startTime ?? 0; + const timeToFirstRenderInMs = Math.round(now - navStart); + + // Compute compile time from the most recent start mark if available. + let timeToCompileShaderInMs: number | null = null; + const startMarks = + typeof perf?.getEntriesByName === "function" + ? perf.getEntriesByName("shader_compile_start") + : []; + const lastStartMark = (startMarks[startMarks.length - 1] as PerformanceMark | undefined); + if (lastStartMark) { + timeToCompileShaderInMs = Math.round(now - lastStartMark.startTime); + } + + console.log(`Time to compile shaders was ${timeToCompileShaderInMs} ms.`); + console.log(`Time to first render was ${timeToFirstRenderInMs} ms.`); + sendAnalyticsEvent("time_to_first_render", { + ...getWebGlAnalyticsInformation(Store.getState()), + timeToFirstRenderInMs, + timeToCompileShaderInMs, + }); + // Keep the timeline clean (best-effort). + perf?.clearMarks?.("shader_compile_start"); + perf?.clearMeasures?.("shader_compile_duration"); + window.measuredTimeToFirstRender = true; + }Based on learnings
🧹 Nitpick comments (1)
frontend/javascripts/viewer/controller.tsx (1)
336-341: Guard against blank UI if cover is nullRare but possible mismatch could return null when !isWkReady. Safer to default to a spinner.
Example:
- if (!isWkReady) { - return cover; - } + if (!isWkReady) { + return cover ?? <BrainSpinner />; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
frontend/javascripts/viewer/controller.tsx(6 hunks)frontend/javascripts/viewer/default_state.ts(1 hunks)frontend/javascripts/viewer/model/actions/actions.ts(2 hunks)frontend/javascripts/viewer/model/reducers/ui_reducer.ts(1 hunks)frontend/javascripts/viewer/store.ts(1 hunks)frontend/javascripts/viewer/view/arbitrary_view.ts(4 hunks)frontend/javascripts/viewer/view/plane_view.ts(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/javascripts/viewer/view/plane_view.ts (3)
frontend/javascripts/viewer/model/actions/actions.ts (1)
uiReadyAction(50-53)frontend/javascripts/admin/rest_api.ts (1)
sendAnalyticsEvent(117-128)frontend/javascripts/viewer/controller/renderer.ts (1)
getWebGlAnalyticsInformation(47-71)
frontend/javascripts/viewer/controller.tsx (1)
frontend/javascripts/components/brain_spinner.tsx (2)
BrainSpinner(14-46)BrainSpinnerWithError(48-99)
frontend/javascripts/viewer/view/arbitrary_view.ts (1)
frontend/javascripts/viewer/model/actions/actions.ts (1)
uiReadyAction(50-53)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: frontend-tests
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (12)
frontend/javascripts/viewer/default_state.ts (1)
265-266: Initialize UI readiness flag in default state — looks goodDefaulting isUiReady to false is correct and consistent with the new readiness flow.
frontend/javascripts/viewer/store.ts (1)
510-511: UiInformation: add isUiReady — OKType addition aligns with reducer and default state.
frontend/javascripts/viewer/model/reducers/ui_reducer.ts (1)
153-155: Handle UI_READY in reducer — OKSets uiInformation.isUiReady to true as intended.
frontend/javascripts/viewer/model/actions/actions.ts (2)
38-39: Action union update — OKIncluding ReturnType is correct.
50-54: Add uiReadyAction — OKSimple const action creator is fine and matches reducer.
frontend/javascripts/viewer/view/arbitrary_view.ts (3)
109-111: Scene wiring via rootGroup — OKReparenting camera under group and attaching to rootGroup is fine.
156-163: Animation loop scheduling — OKrequestAnimationFrame recursion is correct; animationRequestId is updated and canceled in stop().
88-92: No action needed:libs/window.tsalready exports awindowwithmeasuredTimeToFirstRender?: boolean, so the type is declared.frontend/javascripts/viewer/controller.tsx (1)
309-316: Readiness gating and cover layering — looks good
- Cover shown until UI is ready.
- Controllers mount only after WK is ready; cover overlays them.
Also applies to: 355-367, 378-380
frontend/javascripts/viewer/view/plane_view.ts (3)
102-104: Animation gating via isRunning looks goodEarly-return in animate and flag reset in stop prevent runaway RAF loops.
Also applies to: 292-307
1-1: Analytics wiring LGTMImports for sendAnalyticsEvent, getWebGlAnalyticsInformation, and uiReadyAction are correct and used appropriately.
Also applies to: 16-16, 24-24
141-143: No action needed:measuredTimeToFirstRenderalready declared
frontend/javascripts/libs/window.tsaugments the globalWindowinterface withmeasuredTimeToFirstRender?: boolean.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/javascripts/viewer/view/plane_view.ts (1)
140-143: Consider moving the measurement outside the plane loop.The first-render measurement is currently called after each plane renders in the loop. While
window.measuredTimeToFirstRenderprevents duplicate calls, moving the check outside the loop (after line 145) would be clearer and avoid the redundant guard checks on subsequent plane iterations.Apply this diff to move the measurement after all planes render:
for (const plane of OrthoViewValues) { sceneController.updateSceneForCam(plane); const { left, top, width, height } = viewport[plane]; if (width > 0 && height > 0) { setupRenderArea(renderer, left, top, width, height, OrthoViewColors[plane]); renderer.render(scene, this.cameras[plane]); - - if (!window.measuredTimeToFirstRender) { - this.measureTimeToFirstRender(); - } } } + + if (!window.measuredTimeToFirstRender) { + this.measureTimeToFirstRender(); + } this.needsRerender = false;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/javascripts/router/route_wrappers.tsx(2 hunks)frontend/javascripts/viewer/view/layouting/tracing_layout_view.tsx(2 hunks)frontend/javascripts/viewer/view/plane_view.ts(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/viewer/view/layouting/tracing_layout_view.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/javascripts/viewer/view/plane_view.ts (3)
frontend/javascripts/viewer/model/actions/actions.ts (1)
uiReadyAction(50-53)frontend/javascripts/admin/rest_api.ts (1)
sendAnalyticsEvent(117-128)frontend/javascripts/viewer/controller/renderer.ts (1)
getWebGlAnalyticsInformation(47-71)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
- GitHub Check: frontend-tests
🔇 Additional comments (4)
frontend/javascripts/router/route_wrappers.tsx (2)
172-172: Good placement for the performance mark.The mark is correctly positioned after parameter extraction and before rendering the tracing view, which will accurately capture the start of the tracing view load for annotation URLs.
272-272: Verify timing accuracy for legacy URL redirects.For the main path (non-legacy URLs), the mark placement is correct. However, for legacy URLs (lines 273-288), the mark is recorded before the
AsyncRedirectcompletes, potentially making the timing measurement inaccurate since the actual tracing view load happens after the redirect.Consider whether legacy URLs are common enough to warrant placing the mark after the legacy URL check, or if the current placement is acceptable given that legacy URLs are edge cases.
frontend/javascripts/viewer/view/plane_view.ts (2)
1-1: LGTM: Analytics imports are appropriate.The new imports support the timing measurement and analytics reporting functionality.
Also applies to: 16-16, 24-24
64-64: LGTM: Property rename improves clarity.The
isRunningname follows boolean naming conventions.
There was a problem hiding this comment.
great stuff! 🥇
are we positive that interpolation-related problems (almost) always lead to a context loss? for example, if the shader compilation would fail with an INVALID_OPERATION error, WK wouldn't do anything currently. however, we can also change this later if it turns out to be relevant..
I ran the screenshot tests on this branch and unfortunately they failed:
https://github.com/scalableminds/webknossos/actions/runs/18769953664/job/53552503914
the default zoom in the 3d viewport seems to have changed? and test-agglomerate-file_with_mapping_link is completely blank (maybe a rerun would help with that?).
I have only ever seen context losses in that case. However, one can certainly imagine cases where the shader code is faulty for some other reason, may it be a bug, which would lead to an error during compilation. I think now that we're calling Regarding the screenshot tests, I really cannot see a possible cause for the changed zoom value in the 3d viewport looking at the PR diff 😕 We have had this happen in the past and also couldn't figure out a reason, which makes me think it might be flaky somehow. Or I slightly changed the timing when exactly the 3d viewport is rendered 🤔 I'll also have a look. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
frontend/javascripts/viewer/view/arbitrary_view.ts (1)
113-119: Add rejection handler to prevent UI stall.Past reviews flagged that
compileAsyncrejection is not handled, which can cause the UI to stall if shader compilation fails. Additionally, based on the discussion in past reviews,animate()should be called before dispatchinguiReadyAction().Apply this diff:
- renderer.compileAsync(scene, this.camera).then(() => { - // Counter-intuitively this is not the moment where the webgl program is fully compiled. - // There is another stall once render or getProgramInfoLog is called, since not all work is done yet. - // Only once that is done, the compilation process is fully finished, see `renderFunction`. - this.animate(); - Store.dispatch(uiReadyAction()); - }); + renderer + .compileAsync(scene, this.camera) + .then(() => { + // Counter-intuitively this is not the moment where the webgl program is fully compiled. + // There is another stall once render or getProgramInfoLog is called, since not all work is done yet. + // Only once that is done, the compilation process is fully finished, see `renderFunction`. + this.animate(); + Store.dispatch(uiReadyAction()); + }) + .catch((err) => { + console.error("Shader compilation failed; continuing with degraded performance:", err); + this.animate(); + Store.dispatch(uiReadyAction()); + });frontend/javascripts/viewer/view/plane_view.ts (2)
337-345: Add rejection handler and use enum for camera key.Past reviews flagged that
compileAsyncrejection is not handled (critical issue), which can cause the UI to stall. Additionally, the camera key should use the enumOrthoViews.PLANE_XYinstead of a string literal for type safety.Apply this diff:
performance.mark("shader_compile_start"); // The shader is the same for all three viewports, so it doesn't matter which camera is used. - renderer.compileAsync(scene, this.cameras[OrthoViews.PLANE_XY]).then(() => { - // Counter-intuitively this is not the moment where the webgl program is fully compiled. - // There is another stall once render or getProgramInfoLog is called, since not all work is done yet. - // Only once that is done, the compilation process is fully finished, see `renderFunction`. - this.animate(); - Store.dispatch(uiReadyAction()); - }); + renderer + .compileAsync(scene, this.cameras[OrthoViews.PLANE_XY]) + .then(() => { + // Counter-intuitively this is not the moment where the webgl program is fully compiled. + // There is another stall once render or getProgramInfoLog is called, since not all work is done yet. + // Only once that is done, the compilation process is fully finished, see `renderFunction`. + this.animate(); + Store.dispatch(uiReadyAction()); + }) + .catch((err) => { + console.error("Shader compilation failed; continuing with degraded performance:", err); + this.animate(); + Store.dispatch(uiReadyAction()); + });
393-413: Guard performance measurements against missing marks.Past reviews flagged that
performance.measure()throws aDOMExceptionif marks don't exist (critical issue). This can occur with direct navigation, browser back/forward, tests, or older browsers. The exception breaks rendering and analytics.Apply this diff:
measureTimeToFirstRender() { - // We cannot use performance.getEntriesByType("navigation")[0].startTime, because the page might be loaded - // much earlier. It is not reloaded when opening a dataset or annotation from the dashboard and also might - // not be reloaded when navigating from the tracing view back to the dashboard. - // Therefore, we use performance.mark in the router to mark the start time ourselves. The downside of that - // is that the time for the intitial resource loading is not included, then. - const timeToFirstRenderInMs = Math.round( - performance.measure("tracing_view_load_duration", "tracing_view_load_start").duration, - ); - const timeToCompileShaderInMs = Math.round( - performance.measure("shader_compile_duration", "shader_compile_start").duration, - ); - console.log(`Time to compile shaders was ${timeToCompileShaderInMs} ms.`); - console.log(`Time to first render was ${timeToFirstRenderInMs} ms.`); + let timeToFirstRenderInMs: number | null = null; + let timeToCompileShaderInMs: number | null = null; + + try { + // We cannot use performance.getEntriesByType("navigation")[0].startTime, because the page might be loaded + // much earlier. It is not reloaded when opening a dataset or annotation from the dashboard and also might + // not be reloaded when navigating from the tracing view back to the dashboard. + // Therefore, we use performance.mark in the router to mark the start time ourselves. The downside of that + // is that the time for the intitial resource loading is not included, then. + timeToFirstRenderInMs = Math.round( + performance.measure("tracing_view_load_duration", "tracing_view_load_start").duration, + ); + } catch (err) { + console.warn("Could not measure time to first render:", err); + } + + try { + timeToCompileShaderInMs = Math.round( + performance.measure("shader_compile_duration", "shader_compile_start").duration, + ); + } catch (err) { + console.warn("Could not measure shader compile time:", err); + } + + if (timeToCompileShaderInMs != null) { + console.log(`Time to compile shaders was ${timeToCompileShaderInMs} ms.`); + } + if (timeToFirstRenderInMs != null) { + console.log(`Time to first render was ${timeToFirstRenderInMs} ms.`); + } + sendAnalyticsEvent("time_to_first_render", { ...getWebGlAnalyticsInformation(Store.getState()), timeToFirstRenderInMs, timeToCompileShaderInMs, }); window.measuredTimeToFirstRender = true; }
🧹 Nitpick comments (2)
frontend/javascripts/viewer/model/sagas/dataset_saga.ts (1)
155-159: Avoid fixed sleep; wait for first “maximum zoom computed” or timeoutSleeping 1s may be flaky on slow/fast devices. Prefer racing on the first SET_MAXIMUM_ZOOM_FOR_ALL_MAGS_FOR_LAYER (or similar) with a fallback timeout.
Apply within this block:
- yield* call(ensureWkInitialized); - // Once WK is initialized, a correct maximumZoomForAllMags attribute is computed - // asynchronously. During that time, we don't want to show any warning. - // Therefore, we sleep a little bit here. - yield* call(sleep, 1000); + yield* call(ensureWkInitialized); + // Wait for first recomputation or fall back after 1s. + const { _computed } = yield* race({ + _computed: take("SET_MAXIMUM_ZOOM_FOR_ALL_MAGS_FOR_LAYER"), + _timeout: call(sleep, 1000), + });Also add missing imports at top:
- import { call, put, takeEvery, takeLatest } from "typed-redux-saga"; + import { call, put, takeEvery, takeLatest, take, race } from "typed-redux-saga";frontend/javascripts/viewer/model/sagas/skeletontracing_saga.ts (1)
493-494: Ensure tree-name fixups run even if already initializedIf WK_INITIALIZED fired before this saga attaches, takeEvery won’t trigger and watchTreeNames won’t run. Call it once after ensureWkInitialized, then subscribe for future restarts.
- yield* takeEvery("WK_INITIALIZED", watchTreeNames); + // Run once if already initialized, then also on future restarts. + yield* call(ensureWkInitialized); + yield* call(watchTreeNames); + yield* takeEvery("WK_INITIALIZED", watchTreeNames);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
frontend/javascripts/app.ts(1 hunks)frontend/javascripts/router/route_wrappers.tsx(4 hunks)frontend/javascripts/test/api/api_skeleton_latest.spec.ts(1 hunks)frontend/javascripts/test/helpers/apiHelpers.ts(3 hunks)frontend/javascripts/test/sagas/annotation_saga.spec.ts(3 hunks)frontend/javascripts/test/sagas/save_saga.spec.ts(4 hunks)frontend/javascripts/viewer/api/api_latest.ts(2 hunks)frontend/javascripts/viewer/api/api_loader.ts(1 hunks)frontend/javascripts/viewer/controller.tsx(7 hunks)frontend/javascripts/viewer/controller/scene_controller.ts(2 hunks)frontend/javascripts/viewer/default_state.ts(1 hunks)frontend/javascripts/viewer/model/actions/actions.ts(1 hunks)frontend/javascripts/viewer/model/actions/ui_actions.ts(3 hunks)frontend/javascripts/viewer/model/reducers/ui_reducer.ts(1 hunks)frontend/javascripts/viewer/model/sagas/annotation_saga.tsx(3 hunks)frontend/javascripts/viewer/model/sagas/annotation_tool_saga.ts(2 hunks)frontend/javascripts/viewer/model/sagas/dataset_saga.ts(3 hunks)frontend/javascripts/viewer/model/sagas/flycam_info_cache_saga.ts(2 hunks)frontend/javascripts/viewer/model/sagas/load_histogram_data_saga.ts(1 hunks)frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts(2 hunks)frontend/javascripts/viewer/model/sagas/meshes/common_mesh_saga.ts(3 hunks)frontend/javascripts/viewer/model/sagas/meshes/precomputed_mesh_saga.ts(2 hunks)frontend/javascripts/viewer/model/sagas/prefetch_saga.ts(1 hunks)frontend/javascripts/viewer/model/sagas/ready_sagas.ts(1 hunks)frontend/javascripts/viewer/model/sagas/root_saga.ts(2 hunks)frontend/javascripts/viewer/model/sagas/saving/save_queue_draining.ts(2 hunks)frontend/javascripts/viewer/model/sagas/saving/save_queue_filling.ts(3 hunks)frontend/javascripts/viewer/model/sagas/saving/save_saga.ts(2 hunks)frontend/javascripts/viewer/model/sagas/settings_saga.ts(2 hunks)frontend/javascripts/viewer/model/sagas/skeletontracing_saga.ts(3 hunks)frontend/javascripts/viewer/model/sagas/split_boundary_mesh_saga.ts(2 hunks)frontend/javascripts/viewer/model/sagas/task_saga.tsx(3 hunks)frontend/javascripts/viewer/model/sagas/undo_saga.ts(2 hunks)frontend/javascripts/viewer/model/sagas/volume/mapping_saga.ts(2 hunks)frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts(2 hunks)frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx(4 hunks)frontend/javascripts/viewer/store.ts(1 hunks)frontend/javascripts/viewer/view/arbitrary_view.ts(4 hunks)frontend/javascripts/viewer/view/layouting/tracing_layout_view.tsx(2 hunks)frontend/javascripts/viewer/view/plane_view.ts(9 hunks)frontend/javascripts/viewer/view/right-border-tabs/connectome_tab/connectome_view.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/javascripts/app.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/javascripts/viewer/store.ts
- frontend/javascripts/viewer/model/reducers/ui_reducer.ts
- frontend/javascripts/router/route_wrappers.tsx
- frontend/javascripts/viewer/default_state.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-22T17:18:04.217Z
Learnt from: dieknolle3333
PR: scalableminds/webknossos#8168
File: frontend/javascripts/oxalis/model/sagas/proofread_saga.ts:1039-1039
Timestamp: 2024-11-22T17:18:04.217Z
Learning: In `frontend/javascripts/oxalis/model/sagas/proofread_saga.ts`, when calling `getMagInfo`, the use of `volumeTracingLayer.resolutions` is intentional and should not be changed to `volumeTracingLayer.mags`.
Applied to files:
frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsxfrontend/javascripts/viewer/model/sagas/volume/proofread_saga.tsfrontend/javascripts/viewer/model/sagas/volume/mapping_saga.ts
🧬 Code graph analysis (31)
frontend/javascripts/viewer/model/sagas/annotation_saga.tsx (1)
frontend/javascripts/viewer/model/sagas/ready_sagas.ts (1)
ensureWkInitialized(20-26)
frontend/javascripts/viewer/model/sagas/undo_saga.ts (1)
frontend/javascripts/viewer/model/sagas/ready_sagas.ts (1)
ensureWkInitialized(20-26)
frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx (1)
frontend/javascripts/viewer/model/sagas/ready_sagas.ts (1)
ensureWkInitialized(20-26)
frontend/javascripts/test/api/api_skeleton_latest.spec.ts (1)
frontend/javascripts/test/helpers/apiHelpers.ts (1)
setupWebknossosForTesting(318-415)
frontend/javascripts/viewer/view/plane_view.ts (3)
frontend/javascripts/viewer/model/actions/actions.ts (1)
uiReadyAction(55-58)frontend/javascripts/admin/rest_api.ts (1)
sendAnalyticsEvent(117-128)frontend/javascripts/viewer/controller/renderer.ts (1)
getWebGlAnalyticsInformation(47-71)
frontend/javascripts/viewer/model/sagas/meshes/precomputed_mesh_saga.ts (1)
frontend/javascripts/viewer/model/sagas/ready_sagas.ts (2)
ensureSceneControllerInitialized(28-31)ensureWkInitialized(20-26)
frontend/javascripts/test/sagas/annotation_saga.spec.ts (3)
frontend/javascripts/viewer/model/actions/actions.ts (1)
wkInitializedAction(47-50)frontend/javascripts/test/helpers/sagaHelpers.ts (1)
expectValueDeepEqual(9-13)frontend/javascripts/viewer/model/sagas/ready_sagas.ts (1)
ensureWkInitialized(20-26)
frontend/javascripts/viewer/model/sagas/saving/save_queue_draining.ts (1)
frontend/javascripts/viewer/model/sagas/ready_sagas.ts (1)
ensureWkInitialized(20-26)
frontend/javascripts/viewer/controller/scene_controller.ts (1)
frontend/javascripts/viewer/model/actions/actions.ts (1)
sceneControllerInitializedAction(65-68)
frontend/javascripts/viewer/model/sagas/split_boundary_mesh_saga.ts (1)
frontend/javascripts/viewer/model/sagas/ready_sagas.ts (1)
ensureWkInitialized(20-26)
frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts (1)
frontend/javascripts/viewer/model/sagas/ready_sagas.ts (1)
ensureWkInitialized(20-26)
frontend/javascripts/viewer/model/sagas/ready_sagas.ts (1)
frontend/javascripts/viewer/store.ts (1)
WebknossosState(576-596)
frontend/javascripts/viewer/model/sagas/saving/save_queue_filling.ts (1)
frontend/javascripts/viewer/model/sagas/ready_sagas.ts (1)
ensureWkInitialized(20-26)
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts (1)
frontend/javascripts/viewer/model/sagas/ready_sagas.ts (2)
ensureSceneControllerInitialized(28-31)ensureWkInitialized(20-26)
frontend/javascripts/viewer/model/sagas/load_histogram_data_saga.ts (1)
frontend/javascripts/viewer/model/sagas/ready_sagas.ts (1)
ensureWkInitialized(20-26)
frontend/javascripts/viewer/model/sagas/saving/save_saga.ts (1)
frontend/javascripts/viewer/model/sagas/ready_sagas.ts (1)
ensureWkInitialized(20-26)
frontend/javascripts/viewer/model/sagas/root_saga.ts (1)
frontend/javascripts/viewer/model/actions/ui_actions.ts (1)
setIsWkInitializedAction(169-173)
frontend/javascripts/viewer/model/sagas/prefetch_saga.ts (1)
frontend/javascripts/viewer/model/sagas/ready_sagas.ts (1)
ensureWkInitialized(20-26)
frontend/javascripts/viewer/model/sagas/meshes/common_mesh_saga.ts (1)
frontend/javascripts/viewer/model/sagas/ready_sagas.ts (2)
ensureWkInitialized(20-26)ensureSceneControllerInitialized(28-31)
frontend/javascripts/viewer/view/arbitrary_view.ts (1)
frontend/javascripts/viewer/model/actions/actions.ts (1)
uiReadyAction(55-58)
frontend/javascripts/viewer/model/sagas/annotation_tool_saga.ts (1)
frontend/javascripts/viewer/model/sagas/ready_sagas.ts (1)
ensureWkInitialized(20-26)
frontend/javascripts/viewer/controller.tsx (2)
frontend/javascripts/viewer/model/actions/actions.ts (1)
wkInitializedAction(47-50)frontend/javascripts/components/brain_spinner.tsx (2)
BrainSpinner(14-46)BrainSpinnerWithError(48-99)
frontend/javascripts/viewer/model/sagas/settings_saga.ts (1)
frontend/javascripts/viewer/model/sagas/ready_sagas.ts (1)
ensureWkInitialized(20-26)
frontend/javascripts/viewer/model/sagas/skeletontracing_saga.ts (2)
frontend/javascripts/viewer/model/sagas/saga_helpers.ts (1)
takeWithBatchActionSupport(148-160)frontend/javascripts/viewer/model/sagas/ready_sagas.ts (1)
ensureWkInitialized(20-26)
frontend/javascripts/viewer/model/sagas/flycam_info_cache_saga.ts (1)
frontend/javascripts/viewer/model/sagas/ready_sagas.ts (1)
ensureWkInitialized(20-26)
frontend/javascripts/viewer/model/sagas/task_saga.tsx (1)
frontend/javascripts/viewer/model/sagas/ready_sagas.ts (1)
ensureWkInitialized(20-26)
frontend/javascripts/test/helpers/apiHelpers.ts (2)
frontend/javascripts/viewer/api/wk_dev.ts (1)
api(74-80)frontend/javascripts/viewer/model/actions/actions.ts (1)
wkInitializedAction(47-50)
frontend/javascripts/viewer/model/sagas/dataset_saga.ts (1)
frontend/javascripts/viewer/model/sagas/ready_sagas.ts (1)
ensureWkInitialized(20-26)
frontend/javascripts/test/sagas/save_saga.spec.ts (2)
frontend/javascripts/test/helpers/sagaHelpers.ts (1)
expectValueDeepEqual(9-13)frontend/javascripts/viewer/model/sagas/ready_sagas.ts (1)
ensureWkInitialized(20-26)
frontend/javascripts/viewer/api/api_latest.ts (1)
frontend/javascripts/viewer/model/actions/actions.ts (1)
wkInitializedAction(47-50)
frontend/javascripts/viewer/model/sagas/volume/mapping_saga.ts (1)
frontend/javascripts/viewer/model/sagas/ready_sagas.ts (1)
ensureWkInitialized(20-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
- GitHub Check: frontend-tests
🔇 Additional comments (44)
frontend/javascripts/viewer/model/actions/ui_actions.ts (1)
18-18: LGTM! Clean and consistent rename from "ready" to "initialized".The rename from
SetIsWkReadyActiontoSetIsWkInitializedActionand the corresponding action creator update improve naming clarity by distinguishing initialization state from UI readiness.Also applies to: 62-62, 169-173
frontend/javascripts/viewer/view/right-border-tabs/connectome_tab/connectome_view.tsx (1)
262-269: LGTM! Consistent property rename with updated comments.The guard logic remains functionally unchanged while the naming is updated to match the broader refactor.
frontend/javascripts/viewer/model/sagas/volume/mapping_saga.ts (1)
76-76: LGTM! Import, comment, and usage all consistently updated.The saga now correctly references the renamed initialization helper.
Also applies to: 143-145
frontend/javascripts/viewer/model/sagas/flycam_info_cache_saga.ts (1)
23-23: LGTM! Saga initialization properly updated.Both the import and call site are updated consistently.
Also applies to: 82-82
frontend/javascripts/test/api/api_skeleton_latest.spec.ts (1)
47-47: LGTM! Test helper flag updated consistently.The test setup now uses the renamed initialization flag.
frontend/javascripts/viewer/api/api_loader.ts (1)
17-17: LGTM! Event listener updated to match new initialization event.The API loader now listens for the renamed
webknossos:initializationevent, which aligns with the broader terminology shift.frontend/javascripts/test/sagas/save_saga.spec.ts (1)
6-6: LGTM! Test assertions updated consistently.All saga test expectations now correctly reference
ensureWkInitialized.Also applies to: 97-97, 251-251, 279-279
frontend/javascripts/viewer/model/sagas/prefetch_saga.ts (1)
25-25: LGTM! Prefetch saga correctly updated.Both the import and usage are consistently renamed.
Also applies to: 33-33
frontend/javascripts/viewer/model/sagas/saving/save_queue_draining.ts (1)
27-27: LGTM! Consistent initialization rename.The import and usage have been correctly updated from
ensureWkReadytoensureWkInitialized, aligning with the broader codebase refactoring to use "Initialized" terminology.Also applies to: 40-40
frontend/javascripts/viewer/model/sagas/undo_saga.ts (1)
45-45: LGTM! Initialization helper correctly renamed.The readiness saga import and invocation have been updated consistently with the new naming convention.
Also applies to: 162-162
frontend/javascripts/viewer/model/sagas/annotation_saga.tsx (1)
50-50: LGTM! Multiple call sites updated consistently.All usages of the initialization saga have been correctly renamed from
ensureWkReadytoensureWkInitializedthroughout the file.Also applies to: 195-195, 241-241
frontend/javascripts/viewer/model/sagas/annotation_tool_saga.ts (1)
14-14: LGTM! Initialization renamed appropriately.The saga initialization has been updated to use the new
ensureWkInitializedhelper.Also applies to: 132-132
frontend/javascripts/viewer/model/sagas/settings_saga.ts (1)
19-19: LGTM! Consistent renaming applied.The initialization helper import and usage correctly reflect the new naming convention.
Also applies to: 122-122
frontend/javascripts/viewer/model/sagas/split_boundary_mesh_saga.ts (1)
10-10: LGTM! Initialization updated correctly.The saga correctly delegates to
ensureWkInitialized. Note that this file uses direct generator delegation (yield* ensureWkInitialized()) while most other files useyield* call(ensureWkInitialized). Both patterns are valid in typed-redux-saga, though consistency across the codebase would be ideal.Also applies to: 89-89
frontend/javascripts/viewer/model/sagas/task_saga.tsx (1)
29-29: LGTM! All initialization calls updated.The saga initialization helper has been correctly renamed across all usages in this file.
Also applies to: 134-134, 205-205
frontend/javascripts/viewer/view/layouting/tracing_layout_view.tsx (2)
7-7: LGTM! Window import added for measurement feature.The default import of
windowis correctly added to support the newmeasuredTimeToFirstRenderflag assignment.
132-132: LGTM! Measurement flag reset for super users.The assignment of
window.measuredTimeToFirstRender = falsecorrectly resets the timing measurement flag when the component unmounts. This is part of the PR's instrumentation to measure page load and shader compile times, and appropriately placed in the super user experimental path that avoids forced page reloads.frontend/javascripts/viewer/model/sagas/saving/save_saga.ts (1)
15-15: LGTM! Consistent rename from "ready" to "initialized".The import and usage have been correctly updated to use the new
ensureWkInitializednaming, which better reflects the initialization semantics described in the PR objectives.Also applies to: 157-157
frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx (1)
86-86: LGTM! Comprehensive and consistent rename throughout.All references to ready-related terminology have been correctly updated to "initialized" across imports, saga calls, and action patterns. The changes align with the updated action types (
WK_INITIALIZED,SCENE_CONTROLLER_INITIALIZED) and saga helpers.Also applies to: 94-94, 680-680, 733-733, 742-742
frontend/javascripts/viewer/controller/scene_controller.ts (1)
68-68: LGTM! Action rename properly applied.The scene controller now dispatches
sceneControllerInitializedAction()instead of the previous ready action, which is consistent with the initialization lifecycle changes across the codebase.Also applies to: 797-797
frontend/javascripts/viewer/model/sagas/saving/save_queue_filling.ts (1)
25-25: LGTM! Initialization guards updated correctly.Both
setupSavingForAnnotationandsetupSavingForTracingTypenow properly wait for webKnossos initialization using the renamed helper, ensuring the saving logic only starts after the application is fully initialized.Also applies to: 41-41, 88-88
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts (1)
54-54: LGTM! Mesh saga initialization properly updated.The ad-hoc mesh saga now waits for both scene controller and webKnossos initialization using the renamed helpers. The comment has also been updated to reflect the new
WK_INITIALIZEDaction name, maintaining consistency.Also applies to: 701-705
frontend/javascripts/viewer/model/sagas/meshes/precomputed_mesh_saga.ts (1)
50-50: LGTM! Precomputed mesh saga initialization aligned.The precomputed mesh saga follows the same initialization pattern as the ad-hoc mesh saga, with both scene controller and webKnossos initialization guards properly renamed. The comment update maintains consistency with the new action naming.
Also applies to: 493-499
frontend/javascripts/viewer/model/sagas/meshes/common_mesh_saga.ts (1)
28-28: LGTM! Common mesh saga initialization correctly updated.Both
handleAdditionalCoordinateUpdateandcommonMeshSaganow use the renamed initialization helpers, ensuring mesh operations only begin after the application and scene controller are fully initialized.Also applies to: 128-128, 210-211
frontend/javascripts/viewer/model/sagas/root_saga.ts (1)
22-22: LGTM! Root saga restart logic updated.The root saga now correctly resets the initialization state using
setIsWkInitializedAction(false)during restart, which aligns with the renamed action and maintains proper initialization lifecycle management.Also applies to: 42-42
frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts (1)
90-90: Rename to ensureWkInitialized — LGTMImport and call updates are correct and keep the gating semantics unchanged.
Also applies to: 109-109
frontend/javascripts/viewer/model/sagas/dataset_saga.ts (2)
28-28: Initialization import rename — LGTM
54-55: Warning watcher also on WK_INITIALIZED — LGTMGood to show/hide the “too many layers” toast after initialization.
frontend/javascripts/viewer/model/sagas/load_histogram_data_saga.ts (1)
13-16: Gate histogram loading on initialization — LGTMfrontend/javascripts/viewer/model/sagas/skeletontracing_saga.ts (2)
91-91: Initialization import rename — LGTM
260-265: Buffer until WK_INITIALIZED — LGTMAction channels plus waiting for initialization avoid pre-init races for agglomerate loading.
Also applies to: 267-271
frontend/javascripts/test/sagas/annotation_saga.spec.ts (1)
13-15: Test updates to INITIALIZED terminology — LGTMAssertions now reflect ensureWkInitialized/wkInitializedAction; flow remains clear.
Also applies to: 42-43, 54-57
frontend/javascripts/test/helpers/apiHelpers.ts (2)
395-395: Initialization event and action alignment verified — LGTMVerified that
webknossos:initializationis properly emitted (line 394) andwkInitializedActionis dispatched (line 404). Confirmed no remaining references to the oldwebknossos:readyevent exist in the codebase. The changes align correctly with the application initialization flow.
321-322: Option rename verified: all usages updatedThe old flag
dontDispatchWkReadyhas been completely removed from the codebase, and the new flagdontDispatchWkInitializedis properly implemented in the function signature (line 321) and conditional check (line 402) within apiHelpers.ts. No remaining usages of the old flag exist in tests or elsewhere.frontend/javascripts/viewer/api/api_latest.ts (1)
1164-1165: No idempotency issues found—code is safe as-isRe-dispatching WK_INITIALIZED on restart is intentional.
watchTreeNamesis idempotent by design: it only renames trees with empty names (if (tree.name === "")), so on re-dispatch, already-named trees skip the condition. Other WK_INITIALIZED listeners usetake()(one-time triggers) ortakeLatest(inherently safe), preventing re-execution issues. The pattern is correct.frontend/javascripts/viewer/model/sagas/ready_sagas.ts (1)
28-31: Review comment is incorrect. The current implementation already prevents deadlocks.The module boolean
isSceneControllerInitializedcorrectly handles both timing scenarios:
- If
ensureSceneControllerInitialized()is called after theSCENE_CONTROLLER_INITIALIZEDaction fires, the boolean is alreadytrueand the saga returns immediately.- If called before the action fires, the saga waits via
take().No deadlock can occur. Additionally, the proposed store field
state.uiInformation.isSceneControllerInitializeddoes not exist in the codebase—no such field appears in the reducer or store definitions. The suggested fix assumes infrastructure that isn't present.Likely an incorrect or invalid review comment.
frontend/javascripts/viewer/view/arbitrary_view.ts (2)
88-91: Good context flag for timing measurements.The flag and explanatory comment clearly indicate when timing measurements are valid. This prevents inaccurate analytics from non-orthogonal modes.
156-162: LGTM!The
animate()method is correctly implemented with properisRunningchecks and clean callback handling.frontend/javascripts/viewer/controller.tsx (2)
179-181: LGTM!The initialization sequence correctly emits the event, dispatches the action, and sets the controller status. This allows dependent components and sagas to react to the initialization milestone.
312-340: Well-structured initialization gating.The rendering logic correctly gates UI visibility behind initialization milestones:
- Spinner shown during loading and until UI is ready
- Controllers only mount after WK initialization
- Cover overlays controllers during the transition
This prevents premature rendering and ensures a smooth loading experience.
frontend/javascripts/viewer/view/plane_view.ts (3)
141-143: Good placement for timing measurement.Measuring after the first actual render ensures accurate timing. The flag guard prevents duplicate measurements.
64-64: LGTM!The
isRunningproperty is properly declared and used consistently throughout the class to manage the animation lifecycle.
1-1: LGTM!The new imports for analytics and UI readiness support the timing measurement and initialization flow.
frontend/javascripts/viewer/model/actions/actions.ts (1)
37-68: Well-structured initialization actions with clear separation of concerns.The renaming from "ready" to "initialized" is more precise, and the new
uiReadyActionappropriately separates UI readiness from WebKnossos initialization. The descriptive comments clearly explain the distinction between initialization milestones.
Unfortunately not that easy as |
… setTimeout for setControllerStatus. Removing the second setTimeout fixed the issue.
This turned out to be caused by a changed initialization order due to the setTimeout I removed for |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/javascripts/viewer/view/plane_view.ts (1)
318-360: Ensure UI readiness and animation even if compileAsync rejectsIf compileAsync ever rejects (unexpected errors, context issues), UI can hang behind the cover since uiReadyAction/animate are only in then(). Move them to finally() to always proceed; keep catch for telemetry/toast.
Apply this diff:
performance.mark(PerformanceMarkEnum.SHADER_COMPILE); // The shader is the same for all three viewports, so it doesn't matter which camera is used. renderer .compileAsync(scene, this.cameras[OrthoViews.PLANE_XY]) - .then(() => { - // Counter-intuitively this is not the moment where the webgl program is fully compiled. - // There is another stall once render or getProgramInfoLog is called, since not all work is done yet. - // Only once that is done, the compilation process is fully finished, see `renderFunction`. - this.animate(); - Store.dispatch(uiReadyAction()); - }) + .then(() => { + // Counter-intuitively this is not the moment where the webgl program is fully compiled. + // There is another stall once render or getProgramInfoLog is called, since not all work is done yet. + // Only once that is done, the compilation process is fully finished, see `renderFunction`. + }) .catch((error) => { // This code will not be hit if there are shader compilation errors. To react to those, see https://github.com/mrdoob/three.js/pull/25679 Toast.error(`An unexpected error occurred while compiling the WebGL shaders: ${error}`); console.error(error); ErrorHandling.notify(error); - }); + }) + .finally(() => { + // Continue initialization regardless of compile outcome. + this.animate(); + Store.dispatch(uiReadyAction()); + });
🧹 Nitpick comments (2)
frontend/javascripts/viewer/constants.ts (1)
512-515: Centralized performance marks look good; consider cleanup post-measure.Enum names/values align with usage in plane_view.ts and router. Optionally clear marks/measures after measuring to avoid accumulation across navigations.
frontend/javascripts/viewer/view/plane_view.ts (1)
147-151: First-render measurement trigger placementTriggering after the first successful render is fine. Note: the measurement guard is implemented in measureTimeToFirstRender; see fix below to avoid thrown DOMExceptions when marks are missing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
frontend/javascripts/app.ts(1 hunks)frontend/javascripts/router/route_wrappers.tsx(3 hunks)frontend/javascripts/test/helpers/apiHelpers.ts(3 hunks)frontend/javascripts/viewer/api/api_loader.ts(1 hunks)frontend/javascripts/viewer/constants.ts(1 hunks)frontend/javascripts/viewer/controller.tsx(7 hunks)frontend/javascripts/viewer/controller/camera_controller.ts(1 hunks)frontend/javascripts/viewer/view/plane_view.ts(9 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/javascripts/app.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/router/route_wrappers.tsx
🧰 Additional context used
🧬 Code graph analysis (4)
frontend/javascripts/viewer/controller/camera_controller.ts (1)
frontend/javascripts/viewer/api/wk_dev.ts (1)
api(74-80)
frontend/javascripts/test/helpers/apiHelpers.ts (1)
frontend/javascripts/viewer/model/actions/actions.ts (1)
wkInitializedAction(47-50)
frontend/javascripts/viewer/controller.tsx (2)
frontend/javascripts/viewer/model/actions/actions.ts (1)
wkInitializedAction(47-50)frontend/javascripts/components/brain_spinner.tsx (2)
BrainSpinner(14-46)BrainSpinnerWithError(48-99)
frontend/javascripts/viewer/view/plane_view.ts (3)
frontend/javascripts/viewer/model/actions/actions.ts (1)
uiReadyAction(55-58)frontend/javascripts/admin/rest_api.ts (1)
sendAnalyticsEvent(117-128)frontend/javascripts/viewer/controller/renderer.ts (1)
getWebGlAnalyticsInformation(47-71)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-smoketest-push
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
🔇 Additional comments (14)
frontend/javascripts/test/helpers/apiHelpers.ts (2)
46-50: LGTM! Import updated to match action rename.The import change from
wkReadyActiontowkInitializedActionis correct and consistent with the broader refactoring.
321-321: Verification complete: all action usages are consistent.The script confirms that the refactoring is complete and consistent:
- No remaining references to the old
wkReadyActionwkInitializedActionis used consistently across production code (controller.tsx, api_latest.ts), action definitions, and test files- The parameter rename in apiHelpers.ts aligns with the event emission and action dispatch changes
All changes work together correctly with no breaking changes or incomplete migration.
frontend/javascripts/viewer/api/api_loader.ts (1)
17-17: Event name consistency verified across the codebase.All references to the old
"webknossos:ready"event have been removed, and the new"webknossos:initialized"event is used consistently across listeners and emitters (controller.tsx, test helpers). The change is safe to merge.frontend/javascripts/viewer/controller/camera_controller.ts (1)
113-124: No timing issues detected—initialization sequence is safe to execute synchronously.The verification confirms that
rotate3DViewToDiagonal(false)executes synchronously when animation is disabled:
- When
animate=false, the function immediately callsupdateCameraTDView(to, 1)(line 368 in camera_controller.ts) rather than initiating a TWEEN animation.updateCameraTDViewdispatchessetTDCameraWithoutTimeTrackingActionsynchronously to update store state.- Redux dispatch is synchronous, so
Store.getState()at line 122 correctly reflects the merged state from both the dispatch at line 115 and the internal dispatch withinrotate3DViewTo.The removal of
setTimeout(0)is safe—all operations complete synchronously without race conditions or stale state.frontend/javascripts/viewer/controller.tsx (6)
25-25: Import of wkInitializedActionMatches the new initialization flow; no issues.
45-47: New UI readiness flags wired into propsAdding isUiReady/isWkInitialized is correct and keeps render logic explicit. LGTM.
179-182: Emit initialized event, dispatch WK_INITIALIZED, then mark loadedOrder makes sense and avoids unnecessary delay. LGTM.
312-341: Cover logic handles loading vs. failed vs. loaded-but-not-UI-readyCondition resolves the prior suggestion while preserving failedLoading branches. Please verify back/forward navigation still shows the spinner until UI is ready.
355-367: Render controllers behind cover after initializationOverlay pattern is sound and avoids unrendered viewports flashing. LGTM.
378-380: mapStateToProps includes readiness flagsState wiring is correct.
frontend/javascripts/viewer/view/plane_view.ts (4)
1-5: New imports for analytics, error handling, and enumsAll are used below and consistent with the new flow. LGTM.
Also applies to: 16-21, 23-23, 31-31
71-71: Introduce isRunning flagClear, simple gating for RAF loop. LGTM.
109-115: Animation guardEarly-return when stopped prevents extra frames. LGTM.
299-299: Stop sets isRunning = falsePrevents further RAF scheduling. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/javascripts/viewer/view/plane_view.ts (1)
344-360: Ensure UI continues even if shader compilation fails.While the
.catch()block now exists, it doesn't callanimate()or dispatchuiReadyAction(), leaving the UI frozen ifcompileAsync()rejects unexpectedly (e.g., during WebGL context loss or resource exhaustion). Although shader compilation errors won't trigger this path, other unexpected failures will.Apply this diff to ensure the UI remains responsive:
.catch((error) => { // This code will not be hit if there are shader compilation errors. To react to those, see https://github.com/mrdoob/three.js/pull/25679 Toast.error(`An unexpected error occurred while compiling the WebGL shaders: ${error}`); console.error(error); ErrorHandling.notify(error); + // Ensure UI continues even if compilation fails + this.animate(); + Store.dispatch(uiReadyAction()); });
🧹 Nitpick comments (1)
frontend/javascripts/viewer/view/plane_view.ts (1)
408-435: Consider wrapping performance.measure() in try/catch for robustness.While the
.length > 0checks help,performance.measure()can still throwDOMExceptionin edge cases (e.g., marks cleared between check and measure). Adding try/catch blocks would make the measurement more resilient and allow optional cleanup of marks/measures.Example pattern for each measurement block:
if (performance.getEntriesByName(PerformanceMarkEnum.TRACING_VIEW_LOAD, "mark").length > 0) { + try { timeToFirstRenderInMs = Math.round( performance.measure("tracing_view_load_duration", PerformanceMarkEnum.TRACING_VIEW_LOAD) .duration, ); console.log(`Time to first render was ${timeToFirstRenderInMs} ms.`); + } catch (err) { + console.warn("Could not measure time to first render:", err); + } }Optionally add
performance.clearMeasures?.("tracing_view_load_duration")after each measurement to prevent accumulation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/javascripts/viewer/view/plane_view.ts(9 hunks)unreleased_changes/8996.md(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/javascripts/viewer/view/plane_view.ts (3)
frontend/javascripts/viewer/model/actions/actions.ts (1)
uiReadyAction(55-58)frontend/javascripts/admin/rest_api.ts (1)
sendAnalyticsEvent(117-128)frontend/javascripts/viewer/controller/renderer.ts (1)
getWebGlAnalyticsInformation(47-71)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
- GitHub Check: frontend-tests
🔇 Additional comments (4)
unreleased_changes/8996.md (1)
1-3: LGTM! Clear changelog entry.The changelog entry accurately describes the two main user-facing improvements: automatic interpolation disabling on WebGL context loss and asynchronous shader compilation to reduce page load lag.
frontend/javascripts/viewer/view/plane_view.ts (3)
1-1: LGTM! Necessary import additions.All new imports support the performance measurement and error handling functionality added in this PR.
Also applies to: 3-4, 16-21, 23-23, 31-31
71-71: LGTM! Consistent property initialization and usage.The explicit initialization of
isRunningtofalseand consistent usage throughout the file improves code clarity.Also applies to: 109-109, 299-299, 342-342
148-150: LGTM! Appropriate placement for first-render measurement.The flag-guarded call to
measureTimeToFirstRender()ensures timing is captured once after the first successful render.
|
The screenshot tests found an actual bug 🎉 Fixed now. Would be great if you could have another look. |
| if (status === "loading") { | ||
| return <BrainSpinner />; | ||
| let cover = null; | ||
| // Show the brain spinner during loading and until the UI is ready | ||
| if (status === "loading" || (status === "loaded" && !isUiReady)) { | ||
| cover = <BrainSpinner />; | ||
| } else if (status === "failedLoading" && user != null) { | ||
| return ( | ||
| cover = ( | ||
| <BrainSpinnerWithError | ||
| gotUnhandledError={gotUnhandledError} | ||
| organizationToSwitchTo={organizationToSwitchTo} | ||
| /> | ||
| ); | ||
| } else if (status === "failedLoading") { | ||
| return ( |
There was a problem hiding this comment.
Here is the cause for bug noticed here: https://scm.slack.com/archives/C5AKLAV0B/p1761766742395699.
The issue seems to be that the status is still loading but wk is already initialized. In that case the previous logic just rendered the brain spinner. The new code now also tries to render the PlaneController when status === "loading" && isWkInitialized === true. And PlaneController tries to access the scene controller which is not yet re-initialized.
Reverting this code seems to fix the bug. But not sure whether this is actually hiding a bug 🤔
URL of deployed dev instance (used for testing):
Steps to test:
window.testContextLoss(). The context should be recovered, the interpolation setting should be disabled, and there should be toasts indicating so.TODOs:
(Please delete unneeded items, merge only when none are left open)
$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)