Skip to content

implementation-notes-quickwins.md

Latest commit

 

History

History
113 lines (79 loc) · 11.6 KB

implementation-notes-quickwins.md

File metadata and controls

113 lines (79 loc) · 11.6 KB
Raw

Implementation Notes — Quick-Wins Batch (2026-06-21)

Running log of decisions/changes/tradeoffs while building the 5 quick-wins (plan: docs/research-2026-06/quick-wins-build-plan.md). Branch: quick-wins-batch.

Conventions reused across all 5

  • Persisted UI prefs follow the agentLogEnabled pattern in uiStore.ts (localStorage key + load*/persist* helpers + state field + setter).
  • Settings toggles copy the .settings__toggle-row markup in Settings.tsx "Interface" section.
  • Slava is the only maintained skin — theme there first; default/hermes minimal.
  • No new dependencies (verified React Flow already exports MiniMap/Controls/Panel/useStore).

P0.1 — Large-graph canvas performance

Status: code-complete; tsc -b --noEmit clean; eslint clean on changed files. Browser verify pending the consolidated pass.

Note: npm run lint fails on a PRE-EXISTING check:inline-styles violation in BrandShowcaseView.tsx (not in this changeset; brand surface is deprecated per memory). eslint passes on all files I touched.

Decisions:

  • Two persisted toggles, both default ON: canvasPerfMode (nebula:canvas:perfMode) = onlyRenderVisibleElements + MiniMap + Controls; canvasLowDetail (nebula:canvas:lowDetail) = hide node preview media past zoom 0.4.
  • LOD via a <ZoomLodController> child of <ReactFlow> that reads zoom with useStore((s) => s.transform[2]) (re-renders only on zoom number change, not pan; component returns null so no DOM diff) and writes data-lod on the .canvas-wrapper via a ref. Avoids re-rendering every node.
  • LOD hides the whole .model-node__preview block (heaviest = images/video) + port labels; keeps handles/edges/header visible so topology stays readable.
  • Confirmed Canvas is mounted inside <ReactFlowProvider> (App.tsx:184) so useStore works.
  • Reused the orphaned .react-flow__controls CSS already in canvas.css; added MiniMap theming (default + Slava) and a small node-count <Panel>.

Open/!flagged: MiniMap+Controls change default canvas chrome (taste call surfaced to user; defaulting ON per plan).

P0.2 — Friendly moderation / safety errors

Status: code-complete; 43 backend tests pass (22 new classifier + 21 engine); tsc -b --noEmit clean; my eslint clean.

Decisions:

  • New backend/execution/error_classifier.py — pure, never-raises classify_error(raw) -> (category, friendly, retryable). Categories: blocked/auth/quota/rate_limit/timeout/network/invalid_input/unknown. blocked checked before generic 4xx (a 400 with a moderation marker → blocked). unknown → truncated raw (140 chars). retryable=True for rate_limit/timeout/network.
  • ErrorEvent gains optional category/friendly (raw error unchanged). main.py camelize is generic → no edit needed (single-word keys pass through).
  • engine.py:743 except block classifies and attaches; one edit covers all providers.
  • Shared frontend/.../nodes/NodeError.tsx replaces the raw-error <div> in BOTH DynamicNode and ModelNode; shows friendly message + raw in a collapsed <details> (nothing lost). blocked gets a calmer amber/muted treatment (still an error state, not a red failure).
  • CSS: --blocked modifier + details styling in nodes.css (default) and slava-restraint.css (active skin). Skipped hermes.css (deprecated; base error rule still applies).

Found (out of scope, flagged via spawn_task): ModelNode.tsx has 3 PRE-EXISTING react-hooks/rules-of-hooks violations (conditional useGraphStore/useMemo after an early return). Not introduced by me (confirmed on committed version); latent because npm run lint bails on the inline-styles check before eslint runs.

P0.3 — Cmd+K command palette

Status: code-complete; tsc -b --noEmit clean; eslint clean on changed files.

New files: lib/commandPalette.ts (PaletteCommand type + buildCommands(ctx) + filterCommands), components/CommandPalette.tsx (portal overlay + global hotkey), styles/command-palette.css (default + Slava). Modified: App.tsx (mount inside ReactFlowProvider, gated !isBrandShowcase).

Decisions / deviations from the scoper spec:

  • DEVIATION: scoper said "palette wins globally" over the editor's Cmd+K. I chose the opposite — the palette yields in editor/remotion-editor views (the editor's Cmd+K = cut-clip-at-playhead is core muscle memory). The hotkey handler checks useUIStore.getState().viewMode and bails in those views. Cleaner than capture-phase fighting and preserves editor UX.
  • Commands: all 138 nodes (insert at viewport center via screenToFlowPosition), Actions (Run/Save/Load/Fit — Save/Load reuse the existing nebula:save/nebula:load CustomEvents the Toolbar already listens for), View (Create view, panel toggles, skins), Agent ("Ask the agent…" → sub-mode → dispatches nebula:chat-send, which auto-sends; correct here since the user explicitly typed a query).
  • Did NOT extract Toolbar's handleClear/handleImportCLI/handleResetLayout (scoper's "may defer") — avoids touching working Toolbar flows for v1. So no Toolbar/NodeLibrary edits at all.
  • eslint react-hooks rules forced two refactors: precomputed indexById map instead of a mutated render counter; derived safeSelected at render + resets moved into the keydown handler instead of setState-in-effect.
  • v1 ordering is alphabetical-by-group (Actions/View/Agent/Nodes); MRU deferred.

P0.4 — Job notifications

Status: code-complete; tsc -b --noEmit clean; eslint clean; 8 vitest tests pass.

New: lib/jobNotifications.ts (pure glue — prefs, permission, shouldNotifyFor, OS Notification, tab-title + favicon badge, WebAudio beep), tests/jobNotifications.test.ts. Modified: graphStore.ts, uiStore.ts, Settings.tsx, App.tsx.

Decisions:

  • Default OFF — enabling is the user gesture that requests Notification permission (setNotificationPrefs calls ensureNotificationPermission() + primeAudio() on enable). Never prompts unprompted.
  • Failure detection (the tricky bit — no terminal "failed" event): module-level currentRunHadError in graphStore, reset in resetExecution() (the single shared entry point all execute* methods call), set true in the error and validationError cases. graphComplete notifies ok: !currentRunHadError; validationError notifies ok:false directly (it ends the run with no following graphComplete). Exactly one notification per run.
  • shouldNotify = tab hidden || duration >= 30s (fixed 30s constant). Whole-graph completion only (not per-node).
  • Working-badge coordinator lives in an App.tsx effect subscribing to isExecuting transitions (keeps the concern out of the store); badge only shows while document.hidden; restores on focus/visible.
  • Favicon dot drawn on a runtime canvas, fully try/catch-guarded → title-only fallback if the SVG taints the canvas. All Notification/document/AudioContext access feature-detected + guarded.
  • No import cycle: jobNotifications is a leaf (imported by both graphStore and uiStore; imports neither).

P0.5 — Onboarding / first-run experience

Status: code-complete; tsc -b --noEmit clean; eslint clean; slava-scope clean; full frontend build OK; 355 frontend tests pass.

New: constants/sampleGraph.ts, components/onboarding/OnboardingOverlay.tsx, components/onboarding/onboarding.css. Modified: uiStore.ts (onboarding state/actions + ONBOARDED_KEY), graphStore.ts (loadSampleGraph), App.tsx (mount + first-run trigger), Canvas.tsx (suppress splash while active), Settings.tsx + panels.css ("Show onboarding again").

Verified-in-code before building the fixture (the scoper's flagged risk): text-input (output port text, param value) and imagen-4-generate (input port prompt) both exist and are NOT in DYNAMIC_NODE_IDS, so both render as model-node; fixture shape matches addNode's local fallback exactly. Edge type typed-edge.

Decisions:

  • Welcome card (step 0) → Take the tour / Load a sample graph / Describe what you want / Skip. Tour = spotlight (box-shadow "hole" + outline) over the always-mounted .panel-launcher--* buttons (nodes/create/chat/moodboard/character); recomputes rect on resize; centered-tooltip fallback if a target is missing (no crash, no auto-advance setState).
  • "Describe what you want" opens+focuses chat but does not auto-send (avoids spending an API call on first run).
  • Trigger fires only in GraphHydrator's empty-canvas branch (data.empty + catch), guarded !hasOnboarded && !onboardingActive → idempotent under StrictMode; never on a non-empty graph.
  • Sample graph is frontend-only (loadGraph path) until a backend sync — acceptable for a demo (matches addNode's documented local fallback).
  • eslint forced: removed setRect(null) from the effect body (stale rect never renders since the component returns null when inactive) — measurement setRect lives in the measure callback, which the rule allows.

Post-build code review fixes (adversarial reviewer pass)

A code-reviewer subagent reviewed the full branch diff. 5 real findings, all fixed:

  1. [critical] Inverted silent in notifyJobComplete — silent: prefs.sound meant the OS dinged when the user had sound OFF, and (with the custom beep) risked double-sound when ON. Fixed to silent: true always (the beep() owns the sound-on case; sound-off = silent).
  2. [leak] armRestore stacked visibilitychange listeners — only the focus listener was {once}; the visibility one was never removed. Now stored in visibilityHandler and removed in restoreBadges.
  3. [bug] currentRunHadError leaked into the concurrent Create path — executeClusterConcurrent skips resetExecution, so a prior failed run could mark a clean generation "failed". Now resets the flag at the start of that path too.
  4. [mis-classify] over-broad "billing" quota marker — replaced with specific phrases (billing hard limit, check your plan and billing, …); added a regression test that "invalid billing address" ≠ quota.
  5. [robustness] onboarding tooltip could overflow the bottom edge — added a vertical clamp/flip (below → above when it wouldn't fit). (Low real risk since launchers sit top-left, but cheap correctness.)

Re-verified after fixes: 23 classifier + 8 notification tests pass, tsc clean, eslint clean.

Pre-existing issues found (NOT mine — flagged separately)

  1. ModelNode.tsx — 3 react-hooks/rules-of-hooks violations (spawn_task task_b9b86004).
  2. test_node_contracts.py::test_generated_model_reference_matches_committed_file — FAILS on main too (MODEL_REFERENCE.md drifted from node_definitions.json). I touched neither file. Needs node scripts/check-node-contracts.mjs regen.
  3. npm run lint bails on a pre-existing check:inline-styles failure in BrandShowcaseView.tsx before eslint runs.

Verification status

All 5: tsc -b --noEmit clean, eslint clean on changed files, slava-scope clean, full frontend build OK, 355 frontend + 1002 backend tests pass (the 1 backend failure is pre-existing #2 above).

Browser smoke pass: DONE (dev servers + Playwright, real Chromium):

  • P0.1 — MiniMap (bottom-right) + Controls (bottom-left) render with default ON. ✓
  • P0.3 — ⌘K/Ctrl+K opens the palette; "imagen" filters to "Imagen 4 · image-gen · google"; grouped. ✓
  • P0.5 — onboarding welcome card renders; "Take the tour" → spotlight ring on the Node Library launcher + tooltip "1 / 5". ✓ (triggered via window.__nebulaUIStore.getState().startOnboarding() since the test backend had a non-empty cli_graph — which also confirmed onboarding correctly does NOT auto-fire on a non-empty canvas.)
  • P0.2 / P0.4 — covered by unit tests (22 classifier + 8 notifications); both need a live provider rejection / backgrounded long-job to see in-browser.