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.

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, the refactor preserves behavior

Exception: Only modify existing tests if they test implementation details that necessarily changed, and only after the extraction is stable.

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.

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 a dedicated file 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.

Verification Protocol

After every extraction checkpoint, run the full verification sequence:

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

# 2. Type checking
npm run typecheck

# 3. Linting
npm run lint

# 4. Unit tests
npm run test:ci

# 5. End-to-end tests (critical - validates behavior preservation)
npm run e2e

Do not proceed to the next phase until all checks pass.

Preserving External Contracts

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.

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

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)
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 test logic must change (not just imports), 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.
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.

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.
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

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 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)

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 (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

Rule of thumb: If the refactor touches more than 5 files or involves circular dependencies, use the full protocol. If it touches 3-5 files with clear boundaries, use the lightweight variant. Otherwise, just make the change. Regardless of file count, if the module manages browser resources (Pattern F), async state machines (Pattern G), or is a static singleton (Pattern H), use the full protocol — these patterns carry structural risk independent of blast radius.

Appendix: Investigation Templates

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

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

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

Verification Protocol

Preserving External Contracts

Test IDs are API surface

Window globals and side effects

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

Timing Contract Inventory

Cross-System Invariant Map

Uh oh!

Uh oh!

Uh oh!

Clone this wiki locally