High Risk Refactor Guidelines

High-Risk Refactoring Guidelines

This document captures proven techniques for executing complex, high-risk refactors in this codebase. These guidelines were derived from the BlockEditor refactor (January 2026), refined during planning for subsequent refactors (February 2026), and expanded in April 2026 with patterns for imperative DOM code, async state machines, and singleton services — informed by analysis of navigation-manager.ts, user-storage.ts, and context.service.ts.

Core Principles

1. Risk stratification drives phase ordering

Not all extractions carry equal risk. Structure refactors into phases ordered by increasing risk:

Phase	Risk Level	What to Extract
First	Low	Pure utilities (no dependencies), simple hooks (minimal state), trivial components
Middle	Medium	Hooks with moderate interdependencies, components with clear boundaries
Last	High	Complex hooks with circular dependencies, components with extensive prop threading

Rationale: Low-risk extractions establish testing infrastructure and build confidence before tackling dangerous changes.

2. Investigation before implementation

Before writing any extraction code, conduct a thorough investigation phase:

Map consumers: Build a table of each public export → its consumers. This tells you the blast radius of every extraction.
Map internal dependencies: Diagram how the module's internals chain together (what calls what, what depends on what).
Document prop flows between parent and child components.
Identify circular dependencies that will require architectural solutions.
Capture baseline metrics: Record typecheck time, test:ci time/pass count, build time, and known flaky tests. Without baselines, you can't detect the soft metric regressions defined in Success Metrics.
Surface cross-cutting decisions: Investigation often reveals design choices that affect multiple phases (e.g., mocking strategy, import style, interface changes). Document each as a named decision with options, tradeoffs, and which phases it gates. Resolve all decisions before Phase 1.
Identify domain-specific invariants: Beyond "tests pass," identify behavioral contracts the refactor must preserve (e.g., "event X fires with payload Y," "function Z is idempotent"). These become rollback triggers — if violated, stop and revert.
Catalog timing contracts: For code that uses setTimeout, setInterval, requestAnimationFrame, polling loops, or debounce/throttle patterns, document each timing dependency: what triggers it, what duration it expects, what happens if it fires early/late/never, and what cleanup is required. These contracts are invisible to type checking and most unit tests — they must be explicitly identified and tested. See Timing Contract Inventory for the investigation template.
Map cross-system invariants: For code that maintains consistency between two or more systems (e.g., localStorage + server storage, DOM state + React state, event producer + consumer), document the consistency model: what guarantees does it provide (eventual consistency, strong consistency, last-write-wins)? What happens during partial failure (one system writes, the other doesn't)? What is the recovery path? These invariants require integration-style tests — standard unit tests with mocked backends won't catch divergence bugs. See Cross-System Invariant Map for the investigation template.

Repo-specific investigation overlay

In this repository, the investigation phase must also inventory the repo's mechanically enforced contracts. These are not optional niceties — they are part of the behavior a refactor must preserve.

Architecture fence: Record the module's tier from src/validation/import-graph.ts and any existing exceptions in src/validation/architecture.test.ts. A refactor should preserve or reduce these exceptions, not casually add new ones.
Root-level files need manual fence review: Files that live directly under src/ (for example module.tsx) are only allowlisted today and are not fully covered by the tier ratchet. For these files, manually inventory every relative import and justify why it belongs in bootstrap code rather than a lower tier.
Contract surfaces: Inventory all data-testid and data-test-* attributes, storage keys, CustomEvent names, window globals, URL/fallback ordering rules, and lazy imports that consumers or tests depend on.
Module-level registries and guards: Identify module-scope counters, Map/Set registries, static booleans, restore guards, and singleton initialization flags. These often encode ordering assumptions that are not obvious from the call graph alone.
Scene/model lifecycle: If the code uses Grafana Scenes or lazy-loaded panels/components, document what is expected to survive remounts, what must only initialize once, and what must reset when display mode or content changes.
Bootstrap ordering and startup side effects: For entry points and plugin init code, document top-level awaits, early event listeners and buffers, URL/query-param mutation, storage restore behavior, lazy import boundaries, and window-global assignment in exact execution order. In these files, order is part of behavior.
Cross-subsystem seams: For orchestration modules, list every imported subsystem and the behavior that crosses that seam. Example seams: docs panel ↔ docs retrieval, interactive section ↔ requirements manager, context engine ↔ completion storage, content renderer ↔ assistant integration.

This investigation phase should produce zero code changes but inform all subsequent design decisions.

3. Tests are safety rails, not refactoring targets

For existing tests:

Do NOT modify existing unit tests during the refactor
Do NOT modify e2e tests at all
Existing tests (especially e2e) are your contract validators
If e2e tests pass, major user-visible behavior is likely preserved, but storage/event/fence contracts still need explicit tripwires

Exceptions:

Modify existing tests if they test implementation details that necessarily changed, and only after the extraction is stable.
In this repo, some tests intentionally assert source ownership of a contract (for example, which file holds a specific testIds reference). If a refactor moves that ownership to a new file without changing the runtime contract, updating the test to point at the new owner is allowed.
By the end of the refactor, permanent tests must be net-positive. Do not delete disposable safety-net tests until permanent replacements exist and durable coverage is stronger than before.

4. Per-phase testing sandwich

Every phase that changes code must follow a pre-test / extract / post-test sandwich:

Phase N:
  Pre-test  → Targeted contract/smoke tests for the specific code being extracted
  Extract   → Perform the extraction
  Post-test → Unit tests for the newly extracted modules
  Checkpoint → Baseline + new tests all pass

Pre-testing (contract, smoke): Verify behaviors of the overall parts from the outside before the extraction begins. These tests are scoped to the specific behaviors at risk in the current phase, not the entire system. Pre-tests are disposable safety nets — they exist to catch regressions during the refactor and can be deleted once permanent post-tests provide equivalent coverage.

Post-testing (unit): Verify behaviors of the final modules in isolation after extraction. These tests are permanent — they provide long-term coverage for the extracted modules and will be maintained after the refactor.

Baseline behavioral tests (Phase 0): A broad, shallow smoke suite established before any code changes. This suite runs at every checkpoint across all phases as a regression gate. Baseline tests are disposable — they can be deleted after the refactor lands if permanent post-tests provide better coverage. Per-phase pre-tests supplement the baseline — they do not replace it.

Label test lifecycle intent: Mark each test file or describe block as disposable or permanent. This prevents test debt accumulation and ensures cleanup happens at the end.

Seam tripwires: For orchestration modules that bridge two or more subsystems, add at least one black-box characterization test per high-risk seam before extraction. Do not rely only on leaf tests. Good tripwires check things like:

storage/event semantics stay unchanged
data-testid / data-test-* contracts still appear
fallback order stays the same
completion/analytics side effects still happen once, with the same triggering conditions
initialization and cleanup order stay the same
early-event buffering and replay still work
URL cleanup, window-global assignment, and auto-open side effects still happen in the same order for bootstrap files
architecture boundaries and existing ratchets still hold

Rationale: Front-loading all contract testing into Phase 0 has a weakness — you're writing behavioral tests for risks you haven't fully characterized yet. By the time you reach high-risk phases, Phase 0 tests may not cover the specific interaction boundaries at risk. Per-phase pre-tests scale testing effort with risk: Phase 1 might need only a quick smoke test confirming the component renders, while Phase 3 needs targeted behavioral tests for the specific hook interactions being decomposed.

5. Atomic commits with explicit rollback points

Every extraction gets its own commit:

✅ "refactor: extract useModalManager hook"
✅ "refactor: extract BlockEditorFooter component"
❌ "refactor: extract useModalManager and useBlockSelection hooks"  // Too many changes

Backup branches: Create explicit backup branches before highest-risk phases:

git checkout -b feature-refactor-pre-phase-3
git checkout -  # Return to working branch

6. Separate adding functionality from moving code

If an extraction requires new functionality (a new method, interface, or abstraction layer), add it first in the existing structure, test it, then extract in a separate step. Never combine behavioral additions with structural moves.

This often manifests as a "preparation phase" inserted between risk tiers — e.g., adding a keys() method to an interface while code is still in the monolith, then extracting the module that uses it in the next phase. Each step is independently testable and revertible.

Rationale: Mixing "add" with "move" makes it impossible to tell whether a failure is caused by the new behavior or the structural change.

7. Hard stop on test failures

If e2e tests fail after an extraction:

STOP immediately — do not attempt inline fixes
Document the failure in the execution log, refactor plan, or PR notes with:
- Which phase failed
- Full error output
- Analysis of likely cause
Consider reverting to the last passing checkpoint
Re-analyze before retrying

Heroic debugging during a refactor often makes things worse. A clean rollback is faster than a broken fix.

Extraction Patterns

Pattern A: Pure utility extraction

When to use: Logic that performs calculations or transformations without side effects.

Characteristics:

Takes data in, returns data out
No React hooks, no state, no DOM
Easy to unit test in isolation

Example:

// Before: inline in component
const processedSteps = steps.map(step => { /* complex logic */ });

// After: extracted utility
import { processSteps } from './utils/step-processor';
const processedSteps = processSteps(steps);

Testing: Write comprehensive unit tests covering edge cases. This is low-risk and high-value test coverage.

Pattern B: Simple hook extraction

When to use: State management that doesn't depend on other custom hooks.

Characteristics:

Uses only built-in React hooks (useState, useCallback, useMemo)
Self-contained state with clear boundaries
No external service dependencies

Example:

// Before: scattered state in component
const [isOpen, setIsOpen] = useState(false);
const [selectedId, setSelectedId] = useState<string | null>(null);

// After: consolidated in hook
const selection = useSelection();
// selection.isOpen, selection.selectedId, selection.open(), selection.close()

Testing: Use renderHook from @testing-library/react for isolated hook testing.

Pattern C: Complex hook with dependency injection

When to use: Hooks that need access to other hooks or services but shouldn't create them.

Characteristics:

Receives dependencies via parameters
Returns actions that operate on injected dependencies
Avoids circular dependency by receiving callbacks

Example:

// Hook receives its dependencies
export function useRecordingActions(deps: {
  state: RecordingState;
  editor: EditorActions;
  onClear: () => void;  // Callback breaks circular dependency
}) {
  const { state, editor, onClear } = deps;
  
  const stopRecording = useCallback(() => {
    // Use injected dependencies
    editor.addBlocks(state.recordedSteps);
    onClear();  // Call injected callback
  }, [state, editor, onClear]);
  
  return { stopRecording };
}

Pattern D: Three-layer architecture for circular dependencies

When to use: When Hook A needs Hook B's clear function, but Hook B needs Hook A's state.

Solution: Split into three layers where each layer only depends on the previous:

┌─────────────────────────────────────────────────────┐
│  Layer 1: State (no dependencies)                   │
│  const state = useRecordingState();                 │
├─────────────────────────────────────────────────────┤
│  Layer 2: Persistence (reads state)                 │
│  const persistence = usePersistence({               │
│    state: state,                                    │
│    onRestore: state.restore,                        │
│  });                                                │
├─────────────────────────────────────────────────────┤
│  Layer 3: Actions (receives state + clear callback) │
│  const actions = useActions({                       │
│    state: state,                                    │
│    onClear: persistence.clear,                      │
│  });                                                │
└─────────────────────────────────────────────────────┘

Key insight: The cycle is broken because Layer 3 receives persistence.clear as a callback, rather than importing the persistence hook directly.

Pattern E: Interface-first component extraction

When to use: Before extracting a component that receives many props.

Process:

Define the interface in types.ts first (zero runtime risk)
Document which operations belong together
Commit the interface
Extract the component using the interface
Update consumers to pass the consolidated object

Example:

// Step 1: Define interface (commit this alone)
export interface BlockOperations {
  onEdit: (block: Block) => void;
  onDelete: (id: string) => void;
  onMove: (from: number, to: number) => void;
  // ... 25 more operations grouped logically
}

// Step 2: Extract component using interface
interface BlockListProps {
  blocks: Block[];
  operations: BlockOperations;  // Single prop replaces 29 individual props
}

Pattern F: Imperative resource manager extraction

When to use: Classes or modules that directly manage browser resources — requestAnimationFrame handles, setTimeout/setInterval IDs, ResizeObserver instances, MutationObserver instances, or DOM element references — outside of React's lifecycle.

Why this is different from Patterns A-E: React components and hooks have lifecycle guarantees (unmount cleanup via useEffect return, dependency tracking). Imperative resource managers have manual lifecycles — the code itself is responsible for allocation, tracking, and cleanup. Extracting a piece of such a manager can split resource ownership across modules, introducing cleanup ordering bugs that didn't exist before.

Characteristics:

Holds handles/IDs that must be explicitly released (RAF IDs, timeout IDs, observer instances)
Cleanup ordering matters: canceling a timeout before disconnecting an observer may produce different behavior than the reverse
Often depends on external DOM structure (selectors, aria-* attributes, class names) that can change independently

Process:

Inventory all managed resources: List every resource the class allocates, what triggers allocation, and what triggers release. Build a table:

Resource	Type	Allocated when	Released when	Depends on
`driftDetectionRafHandle`	RAF ID	`setupPositionTracking()`	`stopDriftDetection()`	DOM element ref
`dotRemovalTimeout`	Timeout ID	`showHighlight()`	`clearAllHighlights()` or timeout fires	highlight element

Identify resource ownership groups: Resources that must be allocated and released together form an ownership group. Never split an ownership group across modules. For example, if a RAF loop reads from a DOM element reference and writes to a highlight element, all three (RAF handle, source ref, target ref) must stay in the same module.

Extract the selector layer first: If the class queries the DOM using selectors (querySelector, querySelectorAll, closest), extract all selectors into a dedicated module before extracting any logic. This isolates the fragile coupling to external DOM structure.

// selectors.ts — extracted first, tested independently
export const NAV_SELECTORS = {
  menuToggle: '#mega-menu-toggle',
  dockButton: '#dock-menu-button',
  navItem: 'a[data-testid="data-testid Nav menu item"]',
  expandSection: 'button[aria-label*="Expand section"]',
} as const;

// Selector health check (run in pre-tests)
export function validateSelectors(document: Document): string[] {
  const missing: string[] = [];
  for (const [name, selector] of Object.entries(NAV_SELECTORS)) {
    if (!document.querySelector(selector)) {
      missing.push(name);
    }
  }
  return missing;
}

Extract resource lifecycle as a unit: Each ownership group becomes its own module with explicit start()/stop() or create()/destroy() methods. The parent manager composes these lifecycle units.

// drift-detector.ts — one ownership group
export class DriftDetector {
  private rafHandle: number | null = null;
  private element: HTMLElement | null = null;
  private highlight: HTMLElement | null = null;

  start(element: HTMLElement, highlight: HTMLElement): void { /* ... */ }
  stop(): void {
    if (this.rafHandle !== null) {
      cancelAnimationFrame(this.rafHandle);
      this.rafHandle = null;
    }
    this.element = null;
    this.highlight = null;
  }
}

Preserve cleanup ordering in the orchestrator: The parent class calls stop() on extracted lifecycle units in the same order as the original cleanup sequence. Document the required ordering with a comment.

Testing:

Selector tests: Verify selectors against a minimal DOM fixture. These tests are the early warning system for Grafana UI changes.
Lifecycle tests: Use jest.useFakeTimers() and manual RAF simulation. Verify that stop() after start() releases all resources. Verify that stop() when not started is a no-op.
Ordering tests: Verify the cleanup sequence — e.g., stopping drift detection before removing highlight elements, not after.

Rollback trigger: If extracting a lifecycle unit causes cleanup to fire in a different order (detectable via mock call order assertions), stop and reconsider the ownership group boundaries.

Pattern G: Async state machine decomposition

When to use: Code that manages concurrent async operations with explicit guards (isProcessing flags), queues, debounce timers, or retry logic. Common in storage layers, sync engines, and background task managers.

Why this is different from Patterns A-C: Pure utilities and simple hooks have deterministic execution — given the same inputs, they produce the same outputs. Async state machines have interleaving-dependent behavior — the result depends on the order in which async operations complete, which operations are in flight when a new one starts, and whether guards/queues correctly serialize access. Extracting a piece of such a machine can change interleaving semantics.

Characteristics:

Uses flags like isProcessing, hasSynced, isInitialized to guard concurrent access
Maintains a queue or buffer of pending operations
Has debounce/throttle logic that batches rapid operations
Contains "re-check after await" patterns (tail recursion after async gaps)

Process:

Draw the state machine: Before extracting anything, diagram the states and transitions. Use the actual flag/guard variables as state names:

IDLE ──[scheduleWrite]──▶ DEBOUNCING ──[timer fires]──▶ PROCESSING
  ▲                                                         │
  │                    ┌──[queue empty]──────────────────────┘
  │                    │
  └────────────────────┘
  
PROCESSING ──[new item added during processing]──▶ PROCESSING (re-check tail)

Identify the interleaving invariants: These are the properties that must hold regardless of timing:
- "Every item added to the queue is eventually processed or explicitly dropped"
- "At most one processQueue() executes at a time"
- "Items added during processing are not lost (re-check fires)"
- "The debounce timer is reset on each new write (latest value wins)"
Document each invariant. These become your pre-test assertions.

Extract the queue/buffer as a standalone data structure first (Pattern A — pure utility):

// write-queue.ts — no async, no timers, just data structure
export class WriteQueue<T> {
  private items: T[] = [];
  
  enqueue(item: T): void { this.items.push(item); }
  dequeueAll(): T[] { const batch = this.items; this.items = []; return batch; }
  get isEmpty(): boolean { return this.items.length === 0; }
  get size(): number { return this.items.length; }
}

Extract the scheduling logic separately from the processing logic: The scheduler (debounce timer, scheduling decisions) and the processor (what to do with queued items) are different concerns. Extract the scheduler as a unit that calls an injected processFn:

// debounced-processor.ts
export class DebouncedProcessor {
  private timer: ReturnType<typeof setTimeout> | null = null;
  private isProcessing = false;

  constructor(
    private readonly processFn: () => Promise<void>,
    private readonly debounceMs: number
  ) {}

  schedule(): void { /* debounce logic */ }
  async flush(): Promise<void> { /* immediate processing */ }
  dispose(): void { /* cleanup timer */ }
}

Reunify with integration tests: After extracting queue + scheduler + processor, write integration tests that exercise the full pipeline with realistic interleaving:

it('items added during processing are not lost', async () => {
  const processed: string[] = [];
  const processor = new DebouncedProcessor(async () => {
    const batch = queue.dequeueAll();
    processed.push(...batch);
    // Simulate: new item arrives during processing
    queue.enqueue('late-arrival');
  }, 100);
  
  queue.enqueue('first');
  processor.schedule();
  await jest.advanceTimersByTimeAsync(100);
  // Verify re-check picked up the late arrival
  await jest.advanceTimersByTimeAsync(100);
  expect(processed).toContain('late-arrival');
});

Testing:

State machine tests: For each transition in the state diagram, verify the guard conditions and resulting state. Use jest.useFakeTimers() to control timing deterministically.
Interleaving tests: Specifically test the dangerous interleavings: item arrives during processing, dispose() called during processing, two schedule() calls within the debounce window, flush() called while debounce timer is pending.
Invariant tests: For each interleaving invariant documented in step 2, write a test that would fail if the invariant were violated.

Rollback trigger: If an extraction changes the number of times processFn is called for the same input sequence (detectable via mock call counts), the interleaving semantics have changed. Stop and re-examine.

Pattern H: Singleton-to-injectable migration

When to use: Static classes or module-level singletons with mutable state that need to be decomposed. Common in service layers (ContextService), storage managers, and event systems.

Why this is different from other patterns: Singletons create implicit coupling — every consumer imports the module and calls static methods, relying on shared mutable state without declaring that dependency. You can't extract pieces of a singleton using standard patterns because the consumers don't pass the singleton in — they reach for it globally. The refactor must first make the dependency explicit before decomposition is possible.

Characteristics:

static methods and fields on a class, or module-level let/const variables with mutation
Consumers import and call directly: ContextService.getContextData()
State is shared implicitly: one consumer's write affects another consumer's read
Often has initialization guards (isInitialized, hasSubscribed) that assume single-instance semantics

Process:

Make the singleton an instance (no consumer changes yet):

// BEFORE: static class
export class ContextService {
  private static currentDatasourceType: string | null = null;
  static getContextData(): Promise<ContextData> { /* uses this.currentDatasourceType */ }
}

// AFTER: instance class + module-level default instance
export class ContextService {
  private currentDatasourceType: string | null = null;
  getContextData(): Promise<ContextData> { /* uses this.currentDatasourceType */ }
}

// Preserve backward compatibility during migration
export const defaultContextService = new ContextService();

// Re-export static-style accessors (will be removed in Phase 4)
export const getContextData = () => defaultContextService.getContextData();

Commit this alone. All consumers continue working via the re-exported functions. No behavior change.

Add the instance to a React Context (or other DI mechanism):

// context-service.provider.tsx
const ContextServiceContext = createContext<ContextService>(defaultContextService);

export function ContextServiceProvider({ service, children }: {
  service?: ContextService;
  children: React.ReactNode;
}) {
  return (
    <ContextServiceContext.Provider value={service ?? defaultContextService}>
      {children}
    </ContextServiceContext.Provider>
  );
}

export function useContextService(): ContextService {
  return useContext(ContextServiceContext);
}

Commit this alone. No consumers migrated yet. The provider wraps the app root with the default instance.

Migrate consumers incrementally (blast-radius order, lightest first):

// BEFORE
import { ContextService } from '../context-engine/context.service';
const data = await ContextService.getContextData();

// AFTER
import { useContextService } from '../context-engine/context-service.provider';
const contextService = useContextService();
const data = await contextService.getContextData();

Checkpoint after each consumer tier.

Remove the static facade: Once all consumers use the injected instance, delete the defaultContextService export and the re-exported static-style functions. The singleton is now injectable.
Decompose the instance: With the singleton converted to an injectable instance, standard extraction patterns (A-E) now apply. Extract sub-services, split state, etc.

Testing:

Step 1 tests: Verify the instance-based class passes the same tests as the static class. Run the existing test suite — if it was testing via static methods, update imports to use defaultContextService but don't change assertions.
Step 2 tests: Render a component with a test-specific instance (not the default). Verify it uses the injected instance, not the global. This confirms DI is wired correctly.
Step 3 tests: After each consumer migration, run npm run test:ci and verify no consumer accidentally still uses the static path.
Step 4 tests: git grep 'ContextService\.' (static-style calls) should return zero hits in source code.

Rollback trigger: If step 1 (static → instance) causes any test failure, the class has hidden static-initialization-order dependencies. Document them and resolve before proceeding.

Note on event subscriptions: If the singleton subscribes to external events (e.g., EchoSrv), the subscription lifecycle must transfer cleanly to the instance. Add explicit subscribe()/unsubscribe() methods rather than relying on module-load-time side effects. This also enables testing with isolated instances that don't pollute global event state.

Pattern I: Module-level registry and lifecycle-guard extraction

When to use: Modules that rely on module-scope counters, registries, static booleans, or one-time restore guards. Common in orchestration-heavy components and Scene-backed models.

Why this matters: These variables often look like harmless implementation details, but they encode ordering and lifecycle assumptions. Refactoring around them without making ownership explicit can change behavior even when the call graph looks the same.

Characteristics:

Module-scope let counters, Map/Set registries, or static class booleans
Reset functions whose timing matters
"Run once" guards that exist to survive remounts or recreated model instances
Hidden dependencies on registration order or content load order

Process:

Inventory every registry/guard: What owns it, what mutates it, and what resets it?
Document ordering semantics: Is this preserving document order, remount safety, restore-once semantics, or deduplication?
Add characterization tests first: Verify reset behavior, re-registration behavior, and remount/reopen behavior before extraction.
Extract ownership before decomposition: Move the registry/guard behind a dedicated API or lifecycle object before splitting its consumers.
Preserve reset points: The extracted owner must reset at the same moments as the original implementation.

Testing:

Verify register → reset → re-register behavior
Verify multiple mounts/remounts do not duplicate initialization
Verify order-sensitive offsets or counters remain stable for the same render/input sequence

Rollback trigger: If the same mount/reopen sequence produces different offsets, registration order, or "restore only once" behavior after extraction, stop and re-evaluate ownership.

Pattern J: Contract-surface extraction for storage, events, and test contracts

When to use: Modules whose behavior is externally observed through storage keys, CustomEvent names, fallback order, data-testid / data-test-* attributes, or source-owned contract tests.

Why this matters: These are part of the repo's de facto API surface. They are not optional cleanup details. A structurally "successful" refactor that changes one of these silently has still failed.

Characteristics:

Writes to localStorage/sessionStorage or storage helpers with specific key names
Dispatches CustomEvents that other modules subscribe to
Exposes stable data-testid or data-test-* attributes used by tests and tooling
Implements ordering rules like content.json before unstyled.html
Has contract tests that verify a specific file owns a reference

Process:

Inventory names and semantics: Not just the identifier string, but what it means and who consumes it.
Pin the behavior with characterization tests: Verify the emitted event name, the chosen storage backend/key, the DOM attribute presence, or the fallback order.
Extract the contract owner deliberately: If the contract should move to a new file, move ownership in one step and update any source-owned contract tests in the same commit.
Do not rename "while you're here": Contract cleanup is a separate change.

Testing:

Storage/event integration tests for each invariant
Contract tests for data-testid / data-test-*
Tests for fallback ordering when content can be resolved multiple ways

Rollback trigger: If a contract test must change its behavioral assertion rather than just its owning file path or import location, you are no longer doing a pure refactor.

Pattern K: Bootstrap and entry-point orchestration extraction

When to use: Root-level entry points and startup orchestrators such as src/module.tsx, plugin init() paths, Scene bootstrap code, or any file with top-level side effects, top-level await, early event listeners, lazy imports, URL mutation, or window-global assignment.

Why this matters: In bootstrap files, execution order is part of the contract. These files often buffer events before React mounts, mutate URL state before components render, set globals other modules read during startup, or preserve lazy import boundaries for bundle and lifecycle reasons. A refactor can keep the same functions yet still break the app by shifting when they run. This risk is higher in this repo because root-level files are not fully covered by the tier ratchet today.

Characteristics:

Lives directly under src/ or runs before the first React render
Uses top-level await, plugin.init, or module-load-time side effects
Registers early listeners or buffers events before async init completes
Reads or mutates URL params, localStorage restore state, or panel mode before mount
Assigns window globals or dispatches startup CustomEvents
Uses lazy imports to keep bundle size or init sequencing under control

Process:

Write the startup timeline first: List each step in exact order: module load, top-level async init, plugin init, sidebar/panel mount, post-mount replay. Treat this as an invariant document, not nice-to-have notes.
Classify every side effect: Mark each step as one of: early event capture, storage restore, URL mutation, global assignment, lazy import boundary, analytics or telemetry binding, or mount trigger.
Add bootstrap tripwires before extraction: Cover at least: early buffered events replay, URL param stripping, panel mode restore behavior, window-global assignment, and one-time initialization across remounts or display-mode switches.
Extract one lifecycle boundary at a time: Move pre-mount logic, mount-time logic, and post-mount replay in separate steps. Keep the top-level orchestrator thin but intact until the end.
Preserve lazy import boundaries deliberately: If a lazy import exists for bundle or startup-order reasons, keep it on the same side of the boundary unless the refactor explicitly treats bundle behavior as part of the change.
Treat new root-level dependencies as design changes: If the easiest extraction path adds more direct imports into a root-level file, stop and ask whether the dependency should instead move down into a tiered module.

Testing:

Cold-start tests: same initial URL, same startup side effects, same auto-open behavior
Buffered-event tests: events fired before async init still replay once after init
URL/state tests: the same params are stripped or retained in the same order
One-time init tests: no duplicate listener registration or restore-once regressions across remounts or panel-mode switches
Import audit: root-level imports remain minimal and intentional

Rollback trigger: If the same cold-start sequence produces different buffered-event replay, URL state, dock/restore behavior, window-global state, or plugin-init side effects after extraction, stop and re-evaluate. You have changed startup behavior, not just structure.

Verification Protocol

After every extraction checkpoint, run verification in layers. Not every tiny extraction needs the most expensive suite, but every phase needs real guardrails.

# 1. Format code (prevents CI failures)
npm run prettier

# 2. Type checking
npm run typecheck

# 3. Linting
npm run lint

# 4. Targeted tests for the changed module + its contract tests
#    Examples: architecture ratchets, docs panel contract tests,
#    interactive data-attribute contract tests, storage/event tests
npm run test:ci -- <targeted test slice>

# 5. Full unit/integration suite at least once per phase
npm run test:ci

# 6. End-to-end tests after major orchestration changes and before landing
npm run e2e

Cadence guidance:

Run steps 1-4 after every extraction checkpoint.
Run step 5 at the end of each phase and before deleting the old module/facade.
Run step 6 after phases that change orchestration, DOM contracts, storage/event semantics, interactive behavior, or cross-subsystem lifecycle — and always before landing.

Do not proceed to the next phase until the verification appropriate for that checkpoint passes.

Preserving External Contracts

Architecture fences are API surface for refactors

This repo has a mechanically enforced import fence via src/validation/architecture.test.ts and ESLint no-restricted-imports.

Record the current tier of the module before starting.
Record any current vertical or lateral allowlist entries that touch the module.
Default expectation: a refactor keeps the same fence shape or improves it.
Adding a new allowlist entry is not routine cleanup. Treat it as a design decision requiring explicit justification.
If the file already sits behind an allowlist entry, add at least one seam tripwire for the behavior that forced the exception. Do not "stack" another exception to make the extraction easier.

Root-level files are a special case

src/validation/architecture.test.ts currently documents that root-level files such as module.tsx are effectively outside tier enforcement because their topLevelDir is null.

For these files, run a manual import audit during investigation.
Prefer moving logic down into tiered modules over adding new root-level imports.
Treat any new root-level dependency as a design review moment, not a temporary shortcut.
If a refactor changes startup ordering in one of these files, require bootstrap tripwires (Pattern K), not just generic unit tests.

Test IDs are API surface

Document all data-testid attributes that e2e tests depend on:

Test ID	Purpose	Must Preserve
`data-testid="block-editor"`	Main container	Yes
`data-testid="block-palette"`	Footer palette	Yes
`data-testid="view-mode-toggle"`	Preview toggle	Yes

When extracting components, ensure these IDs remain in the DOM at the same structural level.

`data-test-*`, storage keys, event names, and fallback order are also API surface

In this repository, behavior can be externally depended on even when users never see it directly. Preserve:

data-test-* attributes used by E2E and orchestration tooling
storage key names and which storage backend is used for which content type
CustomEvent names and payload shapes
URL validation and fetch/fallback order
lazy import boundaries that exist for bundle or lifecycle reasons

If you need to change one of these, it is a behavior change, not a pure refactor.

Window globals and side effects

If the codebase uses window globals (a code smell, but reality):

Search for ALL consumers before removal
Replace with React Context if globals serve cross-component communication
If globals are internal to one component, simple state replacement suffices

Source-owned contract tests may move with ownership

Some tests in this repo intentionally assert that a particular file owns a contract reference. If ownership moves during a refactor but the runtime contract is preserved:

Move the test assertion to the new owning file in the same commit
Do not change the asserted runtime contract at the same time
Treat any required behavioral assertion change as a stop signal

Troubleshooting Common Issues

Symptom	Likely Cause	Solution
TypeScript: Property 'X' does not exist	Missing prop in interface	Add to interface
E2E: Cannot find element	Test ID not preserved	Check data-testid in extracted component
Circular dependency error	Hook A imports Hook B which imports Hook A	Use three-layer architecture
Modal doesn't open	Event handler not wired	Verify callback prop names match
State not clearing	Cleanup function missing dependency	Check useCallback dependency array
Infinite loop	Object/array in useEffect deps	Use useMemo or extract primitives
Highlight stuck on screen	Cleanup handler not called or called in wrong order	Check resource ownership groups (Pattern F); verify cleanup ordering matches original
Data lost after page refresh	Write queue item dequeued but not persisted	Check interleaving invariants (Pattern G); verify re-check fires after async gap
Storage state diverges between tabs	Conflict resolution branch not exercised	Add integration test for the specific timestamp scenario; check cross-system invariant map
RAF/timeout fires after element removed	Stale DOM reference in closure	Verify lifecycle `stop()` is called before element removal; add `isConnected` guard
Wrong recommendations shown	Static singleton state stale from prior navigation	Check event subscription lifecycle; verify `subscribe()`/`unsubscribe()` on instance (Pattern H)
Selector returns null after Grafana upgrade	External DOM structure changed	Run selector health check (Pattern F step 3); update selector constants

Planning Template

When planning a large refactor, structure your plan with:

Phase -1: Investigation (no code changes)

Phase 0: Baseline behavioral tests (additive tests only; no code changes)

Investigate existing contract/behavioral test coverage
Create broad, shallow smoke tests for overall component behavior (mark as disposable)
Add at least one black-box tripwire per high-risk cross-subsystem seam
For bootstrap/storage/singleton modules, add tripwires for init order, event replay, and contract surfaces before any extraction
Mark which Phase 0 tests are disposable and which later permanent tests will replace or surpass them
Perform a principal engineer level code review of the resulting tests, with an eye towards maintainability. Ensure tests focus on behavior, not implementation
Meta-validate: Introduce a deliberate bug (e.g., break a key function), verify at least one baseline test fails, revert. Document which test caught it. If no test fails, the baseline has gaps — fix before proceeding.
Update risk estimates in the planning document, and produce an assessment: do these tests reduce the risk of a refactor? Where and how?
This baseline suite runs at every checkpoint across all subsequent phases

Phase 1: Low-risk extractions

Pre-test: Smoke tests for specific utilities/hooks being extracted (lightweight — confirm renders, basic behavior)
Extract pure utilities
Extract simple hooks
Extract trivial components
Post-test: Unit tests for each newly extracted module
Checkpoint: Baseline + new tests all passing

Phase 2: Medium-risk extractions

Pre-test: Contract tests for the specific interaction boundaries being split
Create backup branch before highest-risk item
Extract hooks with moderate dependencies
Extract components with clear prop boundaries
Post-test: Unit tests for each newly extracted module
Checkpoint: Baseline + new tests all passing

Phase 3: High-risk extractions

Pre-test: Targeted behavioral tests for specific hook interactions and state flows being decomposed
Create backup branch before starting
Extract complex interdependent hooks (use dependency injection)
Extract components with extensive prop threading (use interface consolidation)
Post-test: Unit tests for each newly extracted module
Checkpoint: Baseline + new tests all passing

Phase 4: Consumer migration and cleanup

Update baseline test imports to new paths. If a source-owned contract test must point at a new file but the runtime contract is unchanged, update it in the same commit. If test logic must change, STOP — this indicates a behavioral regression.
Migrate consumers in blast-radius order: lightest consumers first (fewest imports), heaviest last. Checkpoint after each tier.
Pre-deletion verification: Search for all remaining references to the old module (git grep). Should return zero results in source code before you delete anything.
Delete the old module. No backward-compat facade — consumers are already migrated.
Remove dead code and unused imports.
Final verification: all baseline + phase tests passing.

Common Pitfalls

These are the most frequent causes of refactor failures. Treat them as hard rules, not suggestions.

Improving while refactoring — This is the single biggest source of refactor failures. When you see a bug or a better pattern during extraction, the temptation to fix it inline is strong. Don't. Extract code as-is. Document the improvement opportunity as an accepted tradeoff — a named note with rationale for why you're leaving it imperfect — and move on. Without this record, the next person can't tell whether an imperfection was intentional or accidental. Mixing improvements with structural changes destroys the ability to isolate what went wrong.
Skipping checkpoints — Every checkpoint must pass. Never proceed with "I'll fix it later." A failing checkpoint means the current extraction introduced a problem. Fix it now or revert.
Batching unrelated changes — Each logical extraction should be a separate commit. This allows precise rollback if needed. If you can't describe the commit in one sentence, it's too big.
Assuming instead of verifying — If unsure whether a function is called from another module, check it. Don't assume. The investigation phase exists to build a map of reality, not to confirm assumptions.
Overconfidence in tests — Tests written in Phase 0 are for previously-untested code. They might be wrong. If behavior seems off, stop and investigate — the test may be asserting incorrect behavior.
Ignoring anomalies — If you see something unexpected during extraction, document it. The next phase (or the next agent) might need to know.
Breaking the architecture fence "temporarily" — In this repo, temporary upward-tier imports and new lateral engine imports tend to stick. If a step requires a new ratchet allowlist entry, treat that as a design review moment, not a casual intermediate state.
Refactoring an orchestrator with only leaf tests — A sea of unit tests around helpers does not protect modules like docs panels, content renderers, or section orchestrators. Add seam-level tripwires first.
Changing contract names during extraction — Renaming storage keys, CustomEvents, data-test-*, or fallback semantics as part of a refactor is behavior change. Split it out.
Force-fitting the plan — If reality diverges from the plan, document the divergence and stop if it's significant. Don't bulldoze through obstacles. A plan that doesn't match the code is worse than no plan.
Retrying the same approach — If the same extraction fails twice with different implementations, stop. This usually indicates a misunderstanding of the dependency graph, not an implementation problem. Re-investigate before trying again.
Treating bootstrap like a normal module — Root entry points and plugin init code often hide ordering contracts that ordinary extraction patterns do not protect. If you have not written a startup timeline and bootstrap tripwires, you are guessing.

Success Metrics

Track these metrics in the execution log for each phase.

Hard gates (must pass or stop)

Metric	Threshold	Action on failure
`test:ci` pass rate	100%	Stop. Do not proceed.
TypeScript compilation	Zero errors	Stop. Fix before continuing.
Circular dependencies	Zero	Stop. Redesign extraction.
Architecture ratchet entries	No new vertical/lateral allowlist entries	Stop. Rework the extraction or explicitly treat it as a design change.
High-risk seam tripwires	At least one passing characterization test per high-risk seam	Stop. Add the missing tripwire before continuing.
Permanent test count	More permanent tests than before, with no net loss of durable coverage	Stop. Add post-tests before deleting disposable safety nets.
e2e tests	All passing	Stop. Revert and re-analyze.

Soft metrics (track and flag if degraded)

Metric	Warning threshold	Notes
`test:ci` runtime	>30% increase	Acceptable if new tests were added; investigate if not.
TypeScript compilation time	>20% increase	May indicate excessive re-exports or type complexity.
Lines of code per file	>250 lines	Target, not hard limit. Flag files that grew during refactor.
Total line count	>10% increase	Refactors should be roughly line-neutral. Large increases suggest duplication.

Execution Log Template

Append a log entry to the refactor plan after completing each phase. This structured format ensures knowledge transfers between phases (and between agents/sessions).

## Phase [N] Execution Log

**Timestamp**: [ISO 8601]
**Status**: ✅ PASSED | ❌ FAILED | ⚠️ COMPLETED WITH WARNINGS

### Findings
- Architectural details not obvious from the plan
- Unexpected dependencies or coupling

### Decisions made
- Why approach X over Y; deviations from plan and rationale

### Invariants validated
- ✓ [specific behavioral contract you verified]

### Anomalies & warnings
- ⚠️ [anything surprising — the next phase needs to know]

### Accepted tradeoffs
- [imperfection left intentionally, with rationale]

### Checkpoint
- [ ] `npm run typecheck` ✅
- [ ] `npm run test:ci` ✅
- [ ] `npm run lint` ✅
- [ ] Git status clean

### Files modified
- Created: [list]
- Modified: [list]
- Deleted: [list]

### Next phase recommendations
- [specific guidance for the next phase or agent]

Emergency Stops

Stop immediately and consult the user if:

The plan is fundamentally flawed — e.g., a circular dependency prevents the planned extraction order
Tests reveal bugs in existing code — fixing bugs is out of scope for a structural refactor
Performance regression >30% — measured by test runtime or compilation time
Dynamic imports break — and you can't determine the correct relative path
Global state initialization order differs from expected — and breaks functionality
Consumer code behavior must change — the refactor must be transparent to consumers
A domain-specific invariant is violated — any rollback trigger identified in Phase -1 fires
The same extraction fails twice with different approaches — re-investigate the dependency graph
Resource cleanup ordering changed — if mock call order assertions show cleanup happening in a different sequence after extraction, the ownership group boundaries are wrong (Pattern F)
Async interleaving semantics changed — if the same input sequence produces a different number of processFn calls or different queue drain behavior, the state machine decomposition broke an invariant (Pattern G)
Cross-system invariant violated — if integration tests show the two systems diverging (e.g., localStorage has data that server storage doesn't, or vice versa), the extraction split a consistency boundary
Architecture ratchet grew — if the refactor requires a new vertical or lateral allowlist entry, stop and decide whether the design still counts as an improvement
A source-owned contract test now requires a behavioral assertion change — moving the test to a new owning file is okay; changing what it asserts is a signal that the runtime contract changed

When stopping:

Preserve the current state (don't rollback yet)
Document the issue in the execution log with a clear status marker
Provide diagnosis and recommendation
Wait for user guidance

Lightweight Variant (Medium-Complexity Refactors)

Not every refactor needs the full protocol. For medium-complexity refactors (3-5 files, clear boundaries, no circular dependencies), use this streamlined version:

What to keep:

Phase -1 investigation (abbreviated — focus on consumer search and dependency mapping)
Post-testing for every extraction (unit tests for new modules are always required)
Verification protocol after each extraction
Emergency stops (these always apply)
Common pitfalls (these always apply)

What to skip:

Per-phase pre-testing — baseline smoke tests from Phase 0 are sufficient
Backup branches — a single backup before starting is enough
Success metrics — track informally, don't log formally
Phase 0 as a separate phase — write baseline tests as part of Phase 1 pre-work

When to escalate to the full protocol:

You discover circular dependencies during investigation
The extraction touches shared state or global singletons
More than 5 files are affected
You hit an emergency stop condition
The module is a root-level bootstrap or startup orchestrator (Pattern K applies)
The module manages browser resources with manual lifecycle (Pattern F applies)
The module contains async guards, queues, or debounce logic (Pattern G applies)
The module is a static singleton with mutable state (Pattern H applies)
The module relies on module-level registries, counters, or one-time lifecycle guards (Pattern I applies)
The module owns storage/event/test contracts or fallback-order semantics (Pattern J applies)

When NOT to Use These Guidelines

These guidelines exist on a spectrum:

Complexity	Files touched	Characteristics	Protocol
Trivial	1-2	Rename, add prop, extract small utility	Just make the change
Medium	3-5	Clear boundaries, no circular deps	Lightweight variant (above)
High	6+	State/hook extraction, circular deps, shared globals	Full protocol (Patterns A-E)
High (bootstrap)	Any	Top-level await, startup ordering, URL/init side effects, early listeners	Full protocol + Pattern K
High (imperative)	Any	Manual resource lifecycle, DOM manipulation, timers/RAF	Full protocol + Pattern F
High (concurrent)	Any	Async guards, queues, debounce, cross-system sync	Full protocol + Pattern G
High (singleton)	Any + many dependents	Static mutable state, implicit global coupling	Full protocol + Pattern H

Decision shortcut:

If the module lives at src/ root, runs before first render, or manages startup order, use the full protocol.
If the module manages timers, queues, singletons, registries, or externally observed contracts, use the full protocol.
If it touches 3-5 files with clear boundaries and none of the above apply, use the lightweight variant.
Otherwise, just make the change.

Appendix: Investigation Templates

These templates are referenced by the investigation phase checklist items. Complete the relevant templates during Phase -1.

Contract Surface Inventory

For each externally observed contract the module owns:

Surface type	Identifier	Consumer(s)	Semantics to preserve	Current owner	Tripwire test
Storage key	`StorageKeys.TABS`	docs panel restore flow	Same key, same serialization, same restore timing	`user-storage.ts`	Tabs restore after refresh with same active tab
`CustomEvent`	`pathfinder-auto-open-docs`	docs panel auto-open listener	Same event name, payload shape, dispatch timing	`content-renderer.tsx`	Clicking interactive-learning link dispatches once with same detail
Window global	`__pathfinderPluginConfig`	bootstrap + docs panel	Same assignment timing and shape	`module.tsx`	Cold start exposes config before dependent code reads it
Fallback order	`content.json` before `unstyled.html`	docs fetch flow	Same resolution order and stop conditions	`content-fetcher.ts`	Same input URL resolves to same resource in tests
Test contract	`data-testid="docs-panel-container"`	E2E + contract tests	Same DOM presence and ownership or documented move	`docs-panel.tsx`	Existing contract test still passes

How to use this table: Fill one row per contract surface the module owns. If you cannot name the consumer or the exact semantics to preserve, the refactor plan is underspecified. Pattern J and Pattern K work best when this table is completed before any extraction begins.

Timing Contract Inventory

For each timer, RAF, polling loop, or debounce/throttle in the module:

ID	Mechanism	Trigger	Duration/Interval	Fires when	Cleanup	What breaks if late	What breaks if never fires
T1	`setTimeout`	`showHighlight()`	`dotDurationMs`	Auto-cleanup enabled	Stored in `activeCleanupHandlers`	Highlight lingers	Highlight stuck permanently
T2	`requestAnimationFrame`	`setupPositionTracking()`	Every frame	Drift detection enabled	`cancelAnimationFrame` in `stopDriftDetection()`	Highlight position drifts	Position never updates
T3	Polling loop	`pollForDockButton()`	`pollIntervalMs` × `pollMaxAttempts`	After mega-menu toggle click	Loop exits on match or max attempts	Button not found, operation fails	Same — graceful null return

How to use this table: Each row is a timing contract. During extraction, if a resource from row T1 and a resource from row T2 are split into different modules, verify that their cleanup still fires in the same relative order. If T1's cleanup previously happened inside the same method as T2's cleanup, and extraction puts them in separate stop() methods, the orchestrator must call them in the original order.

Cross-System Invariant Map

For each pair of systems that the module keeps in sync:

Invariant	System A	System B	Consistency model	Failure mode	Recovery	Test strategy
"Most recent write wins"	localStorage	Grafana user storage	Last-write-wins (timestamp)	Clock skew between tabs	Higher timestamp wins; equal = no-op	Integration test: simulate write to A, delay, write to B, verify B wins
"Deletions propagate"	localStorage	Grafana user storage	Tombstone (timestamp without data)	Crash between localStorage delete and Grafana write	Next sync sees tombstone timestamp > data timestamp, applies delete	Integration test: delete in A, crash before B sync, restart, verify B deletes
"Queue items not lost"	In-memory write queue	Grafana user storage	At-least-once (re-check after drain)	Item enqueued during `processQueue()` await	Tail recursive re-check processes stragglers	Unit test: enqueue during mock async processing, verify processed

How to use this table: Each row defines a consistency contract between two systems. During extraction, if the code that writes to System A and the code that writes to System B are split into different modules, the invariant must be preserved by the orchestrator. Write an integration test for each invariant before the extraction (Phase 0 or per-phase pre-test), and verify it still passes after.

Uh oh!

High Risk Refactor Guidelines

High-Risk Refactoring Guidelines

Core Principles

1. Risk stratification drives phase ordering

2. Investigation before implementation

Repo-specific investigation overlay

3. Tests are safety rails, not refactoring targets

4. Per-phase testing sandwich

5. Atomic commits with explicit rollback points

6. Separate adding functionality from moving code

7. Hard stop on test failures

Extraction Patterns

Pattern A: Pure utility extraction

Pattern B: Simple hook extraction

Pattern C: Complex hook with dependency injection

Pattern D: Three-layer architecture for circular dependencies

Pattern E: Interface-first component extraction

Pattern F: Imperative resource manager extraction

Pattern G: Async state machine decomposition

Pattern H: Singleton-to-injectable migration

Pattern I: Module-level registry and lifecycle-guard extraction

Pattern J: Contract-surface extraction for storage, events, and test contracts

Pattern K: Bootstrap and entry-point orchestration extraction

Verification Protocol

Preserving External Contracts

Architecture fences are API surface for refactors

Root-level files are a special case

Test IDs are API surface

data-test-*, storage keys, event names, and fallback order are also API surface

Window globals and side effects

Source-owned contract tests may move with ownership

Troubleshooting Common Issues

Planning Template

Phase -1: Investigation (no code changes)

Phase 0: Baseline behavioral tests (additive tests only; no code changes)

Phase 1: Low-risk extractions

Phase 2: Medium-risk extractions

Phase 3: High-risk extractions

Phase 4: Consumer migration and cleanup

Common Pitfalls

Success Metrics

Hard gates (must pass or stop)

Soft metrics (track and flag if degraded)

Execution Log Template

Emergency Stops

Lightweight Variant (Medium-Complexity Refactors)

When NOT to Use These Guidelines

Appendix: Investigation Templates

Contract Surface Inventory

Timing Contract Inventory

Cross-System Invariant Map

Uh oh!

Uh oh!

Uh oh!

Clone this wiki locally

`data-test-*`, storage keys, event names, and fallback order are also API surface