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) and refined during planning for subsequent refactors (February 2026).

---

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.

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:

1. STOP immediately — do not attempt inline fixes
2. Document the failure in a dedicated file 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

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:

1. Define the interface in types.ts first (zero runtime risk)
2. Document which operations belong together
3. Commit the interface
4. Extract the component using the interface
5. 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
}

---

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

1. Search for ALL consumers before removal
2. Replace with React Context if globals serve cross-component communication
3. 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

---

Planning Template

When planning a large refactor, structure your plan with:

Phase -1: Investigation (no code changes)

• Build consumer map: each public export → list of importing files
• Diagram internal dependency chain (what calls what inside the module)
• Document current prop flows between parent and child components
• Identify circular dependencies
• List test IDs and external contracts that must be preserved
• Capture baseline metrics (typecheck time, test:ci time/count, build time, flaky tests)
• Document cross-cutting decisions with options, tradeoffs, and affected phases (resolve before Phase 1)
• Identify domain-specific invariants that become rollback triggers (beyond "tests pass")

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.

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

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

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

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

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

6. Ignoring anomalies — If you see something unexpected during extraction, document it. The next phase (or the next agent) might need to know.

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

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

1. The plan is fundamentally flawed — e.g., a circular dependency prevents the planned extraction order
2. Tests reveal bugs in existing code — fixing bugs is out of scope for a structural refactor
3. Performance regression >30% — measured by test runtime or compilation time
4. Dynamic imports break — and you can't determine the correct relative path
5. Global state initialization order differs from expected — and breaks functionality
6. Consumer code behavior must change — the refactor must be transparent to consumers
7. A domain-specific invariant is violated — any rollback trigger identified in Phase -1 fires
8. The same extraction fails twice with different approaches — re-investigate the dependency graph

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

---

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

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.