-
Notifications
You must be signed in to change notification settings - Fork 156
[feature] Refactor labextension React components from class to functional with hooks #648
Description
Feature Area
What is the problem?
The labextension uses React class components throughout. The largest — KubeflowKaleLeftPanel (~700 lines) — is a concrete example of why this is a problem: it manages all sidebar state in one monolithic IState blob, causing real bugs and making the code hard to reason about. But the same patterns (manual signal wiring, interleaved state, no effect cleanup) exist across InlineCellsMetadata, CellMetadataEditor, DeployProgress, and others.
Concrete issues today
-
Mutable external state read during render (#647).
getActiveNotebook()readthis.props.tracker.currentWidget— a mutable JupyterLab object — inrender(). React can't track this, so the UI stayed stuck on the disabled toggle after opening a notebook. On clusters, the sequential RPC awaits insetNotebookPanel(kernel startup + 6 backend calls) stacked up to 30+ seconds of latency before anysetStatefired. Locally the calls fail fast, which is why nobody caught it. We patched it by addingactiveNotebookto state, but auseActiveNotebook(tracker)hook would have prevented this class of bug entirely. -
Giant interleaved state. 11 state fields with different lifecycles share one
setStatepath.resetStatehas to manually preserveisEnabledandactiveNotebook— each new field that needs preservation is a landmine. -
Async state management without cleanup.
setNotebookPanelmakes multiplesetStatecalls across 7+awaitpoints with no cancellation. Switching notebooks mid-flight leaves the old chain running and writing stale data. No try/catch either — aKernelErrorbecomes an unhandled rejection and shows an error toast. -
No separation of concerns. Deploy logic, metadata persistence, experiment fetching, notebook lifecycle, and UI rendering all live in one component. Testing any concern requires instantiating the entire panel.
What would a refactor look like?
Migrate to functional components with hooks:
useActiveNotebook(tracker)— hook that subscribes totracker.currentChangedand returns the currentNotebookPanel | null. Replaces the manual signal wiring + state tracking.useNotebookMetadata(notebook)— hook that reads/writes Kale metadata from the active notebook. Handles the async session-ready wait internally with proper cleanup on notebook change.useKaleBackend(notebook, kernel)— hook that fetches namespace, base image, experiments, KFP host. Returns loading/error/data states. Cancels in-flight requests on notebook switch.useDeploys()— hook that manages deploy progress state independently.- Split the render into focused components:
KaleHeader,EnableToggle,PipelineMetadataForm,DeploySection,DeployProgress.
Each hook owns its own lifecycle and cleanup. No more 11-field IState. No more resetState manually preserving fields. No more mutable external reads in render.
Why this matters
- #647 — stuck toggle went unnoticed for months because the state management is opaque and only manifests on clusters with real network latency
- The async race condition in
setNotebookPanelis a latent bug waiting to surface - Adding any feature to the sidebar requires understanding all 700 lines
- Class components don't support effect cleanup or cancellation
Migration path
This doesn't need to happen all at once. React supports mixing class and functional components. A reasonable order:
- Extract
useActiveNotebookhook andEnableTogglecomponent (smallest, most self-contained) - Extract
useNotebookMetadataand the metadata form - Extract deploy logic
- Convert the shell to a functional component
- Delete the class component
Each step is independently shippable and testable.
What is the use case or pain point?
Developer productivity and bug prevention. The current architecture produced a real bug (stuck toggle), has a latent race condition (notebook switching during async setup), and makes every sidebar change riskier than it needs to be.
Is there a workaround currently?
We patch individual symptoms as they appear (e.g., adding activeNotebook to state). This works but accumulates complexity rather than reducing it.
Love this idea? Give it a 👍.