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:

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

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

---

Extraction Patterns

Standard patterns (A-E)

Use these by default for ordinary refactors. They do not need long treatment here.

Pattern	Use when	Main move	Must test
A. Pure utility	Logic is data-in/data-out	Extract directly	Unit tests and edge cases
B. Simple hook	State is self-contained and uses built-in hooks only	Extract to a focused custom hook	renderHook state transitions
C. DI hook	Hook needs other hooks/services	Inject dependencies and callbacks	Hook behavior plus seam contract
D. Three-layer split	Circular dependency exists	Split into state, persistence, actions (or equivalent)	Cycle removed, same behavior preserved
E. Interface-first component	Component has too many props	Define interface first, then extract	Prop surface unchanged

If a refactor matches one of the harder patterns below, switch to the full protocol immediately.

Pattern F: Imperative resource manager extraction

Use when: The module manually owns browser resources such as timers, RAF handles, observers, DOM refs, or event listeners outside normal React cleanup.

What matters:

• Inventory every managed resource and what releases it.
• Keep ownership groups intact. If two resources must start and stop together, extract them together.
• Extract selectors first if the module is tightly coupled to Grafana DOM structure.
• Preserve cleanup order in the orchestrator.

Tripwires:

• Selector health tests
• start() / stop() or create() / destroy() lifecycle tests
• Cleanup-order assertions when order affects behavior

Stop if: Cleanup order changes or resources leak after extraction.

Pattern G: Async state machine decomposition

Use when: The module has queues, debounce timers, retry loops, or async guard flags such as isProcessing or hasSynced.

What matters:

• Draw the state machine first.
• Write down interleaving invariants: no lost items, no duplicate processing, latest-write-wins, re-check after async gaps, and so on.
• Extract queue or buffer logic separately from scheduler and processor logic.
• Re-test the whole pipeline with realistic interleavings, not just leaf units.

Tripwires:

• Fake-timer state-machine tests
• Interleaving tests: enqueue during processing, dispose during processing, multiple schedules in one debounce window
• Invariant tests for queue drain behavior

Stop if: The same input sequence produces a different number of processing calls, loses queued work, or changes drain behavior.

Pattern H: Singleton-to-injectable migration

Use when: A static class or module-level singleton with mutable state needs to be decomposed.

What matters:

• Convert static state to an instance first without changing consumers.
• Add DI next.
• Migrate consumers incrementally.
• Remove the static facade only after all consumers move.
• If the singleton subscribes to external events, move that lifecycle to explicit subscribe() / unsubscribe() methods.

Tripwires:

• Instance behaves the same as the old static API
• Consumers can use a test-specific instance
• No remaining static-style calls in source after migration

Stop if: The static-to-instance step reveals hidden init-order dependencies or shared-state regressions.

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

Use when: The module depends on module-scope counters, Map or Set registries, static booleans, or restore-once guards.

What matters:

• Inventory every registry, guard, reset point, and ordering assumption.
• Extract the owner of the registry or guard before splitting consumers.
• Preserve reset moments and remount semantics exactly.

Tripwires:

• Register -> reset -> re-register behavior
• No duplicate initialization across remounts
• Stable order-sensitive counters or offsets

Stop if: Remounts, reopen flows, or restore-once behavior change.

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

Use when: The module owns storage keys, CustomEvent names, data-testid or data-test-* attributes, fallback order, or source-owned contract tests.

What matters:

• Inventory the contract surface, its consumers, and the semantics to preserve.
• Pin those semantics with characterization tests before moving ownership.
• Move ownership deliberately and update source-owned contract tests in the same commit if needed.
• Do not rename or clean up contract surfaces during a structural refactor.

Tripwires:

• Storage and event integration tests
• Contract tests for data-testid / data-test-*
• Fallback-order tests

Stop if: A contract test needs a behavioral assertion change, not just an ownership-path update.

Pattern K: Bootstrap and entry-point orchestration extraction

Use when: A root-level file or startup orchestrator uses top-level side effects, top-level await, early listeners, URL mutation, lazy imports, or window globals.

What matters:

• Write the startup timeline first: module load, async init, plugin init, mount, replay.
• Classify every side effect: early event capture, storage restore, URL mutation, global assignment, lazy import boundary, mount trigger.
• Add startup tripwires before extraction.
• Extract pre-mount, mount-time, and post-mount logic in separate steps.
• Treat new root-level dependencies as design changes, not temporary shortcuts.

Tripwires:

• Cold-start behavior
• Buffered-event replay
• URL cleanup and panel-mode restore behavior
• Window-global assignment timing
• No duplicate startup listeners across remounts or mode switches

Stop if: The same cold-start sequence produces different replay, URL state, restore behavior, or init side effects.

---

Routing, verification, and stop conditions

Choose the protocol

Case	Use
1-2 file rename or tiny extraction, no shared state or contracts	Just make the change
3-5 files, clear boundaries, no F-K pattern applies	Lightweight protocol
More than 5 files, circular deps, shared state, orchestration, or any Pattern F-K	Full protocol

Shortcut:

• If the file lives at src/ root, runs before first render, or controls startup order, use the full protocol.
• If the module manages timers, queues, singletons, registries, or externally observed contracts, use the full protocol.

Investigation checklist

Before Phase 0, collect only the information that will actually steer the refactor:

• Consumer map for public exports
• Internal dependency chain and any circular deps
• Current tier plus any ALLOWED_VERTICAL_VIOLATIONS / ALLOWED_LATERAL_VIOLATIONS
• Manual import audit if the file lives at src/ root
• Contract Surface Inventory if Pattern J or K applies
• Timing Contract Inventory if Pattern F applies
• Cross-System Invariant Map if Pattern G or J applies
• Startup timeline if Pattern K applies
• One candidate tripwire per high-risk seam

Minimal full protocol

1. Phase 0: Add baseline smoke tests and seam tripwires.
2. For each extraction: pre-test -> extract -> post-test -> checkpoint.
3. Migrate consumers from lightest to heaviest.
4. Delete legacy modules or facades last, only after git grep shows no remaining source references.

Checkpoint verification

# Every checkpoint
npm run prettier
npm run typecheck
npm run lint
npm run test:ci -- <targeted test slice>

# At least once per phase
npm run test:ci

# After orchestration/storage/event/bootstrap changes and before landing
npm run e2e

Hard gates

Stop and redesign or revert if any of these happen:

• npm run test:ci fails
• TypeScript no longer compiles
• A new vertical or lateral architecture allowlist entry is required
• A root-level file grows new dependencies without explicit design review
• A high-risk seam has no tripwire
• Permanent durable coverage is not net-positive by the end of the refactor
• Cleanup order, startup order, or async interleaving changes
• A contract surface changes names, payloads, storage keys, or fallback order
• The same extraction fails twice with different implementations
• Consumer-visible behavior must change to complete the refactor

Do not

• Improve behavior while refactoring
• Batch unrelated structural moves into one commit
• Break the architecture fence "temporarily"
• Rename contract surfaces "while you're here"
• Assume instead of checking the real dependency graph

Keep a short execution log

You do not need a long template in the hot path. Record only:

• findings not obvious from the plan
• decisions and deviations
• invariants validated
• anomalies for the next phase

---

Appendix: Investigation templates

Complete only the templates that match the active pattern.

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

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

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