formualizer_eval/engine/graph/editor/

vertex_editor.rs

use crate::SheetId;
use crate::engine::addr::GridAddr;
use crate::engine::graph::DependencyGraph;
use crate::engine::graph::editor::change_log::{FormulaRunAdjusted, MutationCapture};
use crate::engine::graph::editor::reference_adjuster::{
    MoveReferenceAdjuster, ReferenceAdjuster, ReferenceContext, RelativeReferenceAdjuster,
    ShiftOperation,
};
use crate::engine::named_range::{NameScope, NamedDefinition};
use crate::engine::{ChangeEvent, ChangeLogger, VertexId, VertexKind};
use crate::reference::{CellRef, Coord};
use formualizer_common::{ExcelError, ExcelErrorKind, LiteralValue};
use formualizer_parse::parser::ASTNode;
use rustc_hash::FxHashMap;
use std::sync::atomic::{AtomicU64, Ordering};

/// Metadata for creating a new vertex
#[derive(Debug, Clone)]
pub struct VertexMeta {
    pub coord: GridAddr,
    pub sheet_id: SheetId,
    pub kind: VertexKind,
    pub flags: u8,
}

impl VertexMeta {
    pub fn new(row: u32, col: u32, sheet_id: SheetId, kind: VertexKind) -> Self {
        Self {
            coord: GridAddr::new(row, col),
            sheet_id,
            kind,
            flags: 0,
        }
    }

    pub fn with_flags(mut self, flags: u8) -> Self {
        self.flags = flags;
        self
    }

    pub fn dirty(mut self) -> Self {
        self.flags |= 0x01;
        self
    }

    pub fn volatile(mut self) -> Self {
        self.flags |= 0x02;
        self
    }
}

/// Patch for updating vertex metadata
#[derive(Debug, Clone)]
pub struct VertexMetaPatch {
    pub kind: Option<VertexKind>,
    pub coord: Option<GridAddr>,
    pub dirty: Option<bool>,
    pub volatile: Option<bool>,
}

/// Patch for updating vertex data
#[derive(Debug, Clone)]
pub struct VertexDataPatch {
    pub value: Option<LiteralValue>,
    pub formula: Option<ASTNode>,
}

/// Summary of metadata update
#[derive(Debug, Clone, Default)]
pub struct MetaUpdateSummary {
    pub coord_changed: bool,
    pub kind_changed: bool,
    pub flags_changed: bool,
}

/// Summary of data update
#[derive(Debug, Clone, Default)]
pub struct DataUpdateSummary {
    pub value_changed: bool,
    pub formula_changed: bool,
    pub dependents_marked_dirty: Vec<VertexId>,
}

/// Summary of shift operations (row/column insert/delete)
#[derive(Debug, Clone, Default)]
pub struct ShiftSummary {
    pub vertices_moved: Vec<VertexId>,
    pub vertices_deleted: Vec<VertexId>,
    pub references_adjusted: usize,
    pub formulas_updated: usize,
    #[cfg(test)]
    pub(crate) structural_dependents_dirtied: Vec<VertexId>,
}

/// Summary of range operations
#[derive(Debug, Clone, Default)]
pub struct RangeSummary {
    pub cells_affected: usize,
    pub vertices_created: Vec<VertexId>,
    pub vertices_updated: Vec<VertexId>,
    pub cells_moved: usize,
}

/// Transaction ID for tracking active transactions
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub struct TransactionId(u64);

impl TransactionId {
    fn new() -> Self {
        static COUNTER: AtomicU64 = AtomicU64::new(0);
        TransactionId(COUNTER.fetch_add(1, Ordering::Relaxed))
    }
}

/// Represents an active transaction
#[derive(Debug)]
struct Transaction {
    id: TransactionId,
    start_index: usize, // Index in change_log where transaction started
}

/// Custom error type for vertex editor operations
#[derive(Debug, Clone)]
pub enum EditorError {
    TargetOccupied { cell: CellRef },
    OutOfBounds { row: u32, col: u32 },
    InvalidName { name: String, reason: String },
    TransactionFailed { reason: String },
    TransactionUnsupported { reason: String },
    NoActiveTransaction,
    VertexNotFound { id: VertexId },
    Excel(ExcelError),
}

impl From<ExcelError> for EditorError {
    fn from(e: ExcelError) -> Self {
        EditorError::Excel(e)
    }
}

impl std::fmt::Display for EditorError {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        match self {
            EditorError::TargetOccupied { cell } => {
                write!(
                    f,
                    "Target cell occupied at row {}, col {}",
                    cell.coord.row(),
                    cell.coord.col()
                )
            }
            EditorError::OutOfBounds { row, col } => {
                write!(f, "Cell position out of bounds: row {row}, col {col}")
            }
            EditorError::InvalidName { name, reason } => {
                write!(f, "Invalid name '{name}': {reason}")
            }
            EditorError::TransactionFailed { reason } => {
                write!(f, "Transaction failed: {reason}")
            }
            EditorError::TransactionUnsupported { reason } => {
                write!(f, "Transaction unsupported: {reason}")
            }
            EditorError::NoActiveTransaction => {
                write!(f, "No active transaction")
            }
            EditorError::VertexNotFound { id } => {
                write!(f, "Vertex not found: {id:?}")
            }
            EditorError::Excel(e) => write!(f, "Excel error: {e:?}"),
        }
    }
}

impl std::error::Error for EditorError {}

/// Builder/controller object that provides exclusive access to the dependency graph
/// for all mutation operations. This ensures consistency and proper change tracking.
/// # Example Usage
///
/// ```rust
/// use formualizer_eval::engine::{DependencyGraph, VertexEditor, VertexMeta, VertexKind};
/// use formualizer_common::LiteralValue;
/// use formualizer_eval::reference::{CellRef, Coord};
///
/// let mut graph = DependencyGraph::new();
/// let mut editor = VertexEditor::new(&mut graph);
///
/// // Batch operations for better performance
/// editor.begin_batch();
///
/// // Create a new cell vertex
/// let meta = VertexMeta::new(1, 1, 0, VertexKind::Cell).dirty();
/// let vertex_id = editor.add_vertex(meta);
///
/// // Set cell values
/// let cell_ref = CellRef {
///     sheet_id: 0,
///     coord: Coord::new(2, 3, true, true)
/// };
/// editor.set_cell_value(cell_ref, LiteralValue::Number(42.0));
///
/// // Commit batch operations
/// editor.commit_batch();
///
/// ```
/// Optional hook for reading Arrow-truth spill values for ChangeLog snapshots.
///
/// VertexEditor is structure-only; in canonical mode, callers should provide this
/// reader so spill undo/redo uses Arrow overlays rather than graph value caches.
pub trait SpillValueReader {
    fn read_cell_value(&self, sheet: &str, row: u32, col: u32) -> Option<LiteralValue>;
}

/// Whether a run the block shift moved holds vertex `v`.
fn moved_run_holds(runs: &[crate::engine::graph::ShiftedRun], v: VertexId) -> bool {
    runs.iter()
        .any(|r| r.moved && v.0 >= r.first.0 && v.0 - r.first.0 < r.len)
}

/// The editor's change sink: a caller's logger, or the engine's mutation
/// capture, which also keeps run records (Program 2) unexpanded.
enum EditorLogger<'g> {
    Dyn(&'g mut dyn ChangeLogger),
    Capture(&'g mut MutationCapture),
}

impl EditorLogger<'_> {
    #[inline]
    fn record(&mut self, event: ChangeEvent) {
        match self {
            EditorLogger::Dyn(l) => l.record(event),
            EditorLogger::Capture(c) => c.record(event),
        }
    }

    #[inline]
    fn begin_compound(&mut self, description: String) {
        match self {
            EditorLogger::Dyn(l) => l.begin_compound(description),
            EditorLogger::Capture(c) => c.begin_compound(description),
        }
    }

    #[inline]
    fn end_compound(&mut self) {
        match self {
            EditorLogger::Dyn(l) => l.end_compound(),
            EditorLogger::Capture(c) => c.end_compound(),
        }
    }

    /// A run's per-member `FormulaAdjusted` events: kept as one record by
    /// the engine's capture, expanded now for any other logger.
    fn record_formula_run(&mut self, run: FormulaRunAdjusted) {
        match self {
            EditorLogger::Dyn(l) => {
                for event in run.events() {
                    l.record(event);
                }
            }
            EditorLogger::Capture(c) => c.record_lazy(run),
        }
    }
}

pub struct VertexEditor<'g> {
    graph: &'g mut DependencyGraph,
    change_logger: Option<EditorLogger<'g>>,
    spill_value_reader: Option<&'g dyn SpillValueReader>,
    structural_occupancy: Option<crate::engine::graph::StructuralOccupancy>,
    batch_mode: bool,
}

impl<'g> VertexEditor<'g> {
    /// Create a new vertex editor without change logging
    pub fn new(graph: &'g mut DependencyGraph) -> Self {
        Self {
            graph,
            change_logger: None,
            spill_value_reader: None,
            structural_occupancy: None,
            batch_mode: false,
        }
    }

    /// Supply the conservative union of graph and Arrow occupancy for structural edits.
    pub(crate) fn with_structural_occupancy(
        mut self,
        occupancy: crate::engine::graph::StructuralOccupancy,
    ) -> Self {
        self.structural_occupancy = Some(occupancy);
        self
    }

    pub(crate) fn set_structural_occupancy(
        &mut self,
        occupancy: crate::engine::graph::StructuralOccupancy,
    ) {
        self.structural_occupancy = Some(occupancy);
    }

    /// Create a new vertex editor with change logging
    pub fn with_logger<L: ChangeLogger + 'g>(
        graph: &'g mut DependencyGraph,
        logger: &'g mut L,
    ) -> Self {
        Self {
            graph,
            change_logger: Some(EditorLogger::Dyn(logger as &'g mut dyn ChangeLogger)),
            spill_value_reader: None,
            structural_occupancy: None,
            batch_mode: false,
        }
    }

    /// Create a new vertex editor with change logging and an Arrow-truth spill reader
    pub fn with_logger_and_spill_reader<L: ChangeLogger + 'g>(
        graph: &'g mut DependencyGraph,
        logger: &'g mut L,
        spill_value_reader: &'g dyn SpillValueReader,
    ) -> Self {
        Self {
            graph,
            change_logger: Some(EditorLogger::Dyn(logger as &'g mut dyn ChangeLogger)),
            spill_value_reader: Some(spill_value_reader),
            structural_occupancy: None,
            batch_mode: false,
        }
    }

    /// An editor logging into the engine's mutation capture (run records
    /// stay unexpanded), with an Arrow-truth spill reader.
    pub(crate) fn with_capture_and_spill_reader(
        graph: &'g mut DependencyGraph,
        capture: &'g mut MutationCapture,
        spill_value_reader: &'g dyn SpillValueReader,
    ) -> Self {
        Self {
            graph,
            change_logger: Some(EditorLogger::Capture(capture)),
            spill_value_reader: Some(spill_value_reader),
            structural_occupancy: None,
            batch_mode: false,
        }
    }

    /// Start batch mode to defer expensive operations until commit
    pub fn begin_batch(&mut self) {
        if !self.batch_mode {
            self.graph.begin_batch();
            self.batch_mode = true;
        }
    }

    /// End batch mode and commit all deferred operations
    pub fn commit_batch(&mut self) {
        if self.batch_mode {
            self.graph.end_batch();
            self.batch_mode = false;
        }
    }

    /// Helper method to log a change event
    fn log_change(&mut self, event: ChangeEvent) {
        if let Some(logger) = &mut self.change_logger {
            logger.record(event);
        }
    }

    fn snapshot_spill_for_anchor(
        &self,
        anchor: VertexId,
    ) -> Option<crate::engine::graph::editor::change_log::SpillSnapshot> {
        let cells = self.graph.spill_cells_for_anchor(anchor)?.to_vec();
        if cells.is_empty() {
            return None;
        }

        // Defensive bound for log payloads.
        let max = self.graph.get_config().spill.max_spill_cells as usize;
        let mut cells = cells;
        if cells.len() > max {
            cells.truncate(max);
        }

        let first = *cells.first().expect("non-empty spill cells");
        let sheet_name = self.graph.sheet_name(first.sheet_id).to_string();
        let row0 = first.coord.row();
        let col0 = first.coord.col();

        let mut max_row = row0;
        let mut max_col = col0;
        let mut by_coord: FxHashMap<(u32, u32), LiteralValue> = FxHashMap::default();
        for cell in &cells {
            max_row = max_row.max(cell.coord.row());
            max_col = max_col.max(cell.coord.col());
            let v = if let Some(reader) = self.spill_value_reader {
                reader
                    .read_cell_value(&sheet_name, cell.coord.row() + 1, cell.coord.col() + 1)
                    .unwrap_or(LiteralValue::Empty)
            } else {
                self.graph
                    .get_cell_value(&sheet_name, cell.coord.row() + 1, cell.coord.col() + 1)
                    .unwrap_or(LiteralValue::Empty)
            };
            by_coord.insert((cell.coord.row(), cell.coord.col()), v);
        }

        let rows = (max_row - row0 + 1) as usize;
        let cols = (max_col - col0 + 1) as usize;
        let mut values: Vec<Vec<LiteralValue>> = Vec::with_capacity(rows);
        for r in 0..rows {
            let mut row: Vec<LiteralValue> = Vec::with_capacity(cols);
            for c in 0..cols {
                row.push(
                    by_coord
                        .get(&(row0 + r as u32, col0 + c as u32))
                        .cloned()
                        .unwrap_or(LiteralValue::Empty),
                );
            }
            values.push(row);
        }

        Some(crate::engine::graph::editor::change_log::SpillSnapshot {
            target_cells: cells,
            values,
        })
    }

    /// Commit a spill region and log it for replay/undo.
    pub fn commit_spill_region(
        &mut self,
        anchor: VertexId,
        target_cells: Vec<CellRef>,
        values: Vec<Vec<LiteralValue>>,
    ) -> Result<(), EditorError> {
        let old = self.snapshot_spill_for_anchor(anchor);
        self.graph
            .commit_spill_region_atomic_with_fault(
                anchor,
                target_cells.clone(),
                values.clone(),
                None,
            )
            .map_err(EditorError::Excel)?;
        self.log_change(ChangeEvent::SpillCommitted {
            anchor,
            old,
            new: crate::engine::graph::editor::change_log::SpillSnapshot {
                target_cells,
                values,
            },
        });
        Ok(())
    }

    /// Clear a spill region (if any) and log it for replay/undo.
    pub fn clear_spill_region(&mut self, anchor: VertexId) {
        let Some(old) = self.snapshot_spill_for_anchor(anchor) else {
            return;
        };
        self.graph.clear_spill_region(anchor);
        self.log_change(ChangeEvent::SpillCleared { anchor, old });
    }

    /// Check if change logging is enabled
    pub fn has_logger(&self) -> bool {
        self.change_logger.is_some()
    }

    fn get_formula_ast(&self, id: VertexId) -> Option<ASTNode> {
        self.graph.get_formula(id)
    }

    fn snapshot_named_definitions(&self) -> FxHashMap<(NameScope, String), NamedDefinition> {
        let mut out: FxHashMap<(NameScope, String), NamedDefinition> = FxHashMap::default();
        for (name, nr) in self.graph.named_ranges_iter() {
            out.insert((NameScope::Workbook, name.clone()), nr.definition.clone());
        }
        for ((sheet_id, name), nr) in self.graph.sheet_named_ranges_iter() {
            out.insert(
                (NameScope::Sheet(*sheet_id), name.clone()),
                nr.definition.clone(),
            );
        }
        out
    }

    // Transaction support

    // Transaction support has been moved to TransactionContext
    // which coordinates ChangeLog, TransactionManager, and VertexEditor

    /// Backward replay reached a compound's end marker; `description` is
    /// its start marker's. A structural edit's extent record goes back to
    /// the pre-edit frame here, before the replay of its events
    /// (`DependencyGraph::undo_structural_extent`).
    pub(crate) fn inverse_compound_end(&mut self, description: &str) {
        self.graph.undo_structural_extent(description);
    }

    /// Apply the inverse of a change event (used by TransactionContext for rollback)
    pub fn apply_inverse(&mut self, change: ChangeEvent) -> Result<(), EditorError> {
        match change {
            ChangeEvent::SetValue {
                addr,
                old_value,
                old_formula,
                new: _,
            } => {
                // Restore previous state. Setting a value can overwrite a formula.
                if let Some(old_formula) = old_formula {
                    self.set_cell_formula(addr, old_formula);
                } else if let Some(old_value) = old_value {
                    self.set_cell_value(addr, old_value);
                } else {
                    // No prior state the graph holds: the cell is a value or
                    // empty cell again (Arrow restores its value). Its formula
                    // id retires at the cell (decision 27), so replaying the
                    // formula back revives it.
                    self.graph.retire_cell_for_replay(addr);
                }
            }
            ChangeEvent::SetFormula {
                addr,
                old_value,
                old_formula,
                new: _,
            } => {
                // Restore previous state. Setting a formula can overwrite a value.
                if let Some(old_formula) = old_formula {
                    self.set_cell_formula(addr, old_formula);
                } else if let Some(old_value) = old_value {
                    self.set_cell_value(addr, old_value);
                } else {
                    // No prior state the graph holds: the cell is a value or
                    // empty cell again (Arrow restores its value). Its formula
                    // id retires at the cell (decision 27), so replaying the
                    // formula back revives it.
                    self.graph.retire_cell_for_replay(addr);
                }
            }
            ChangeEvent::SetRowVisibility { .. } => {
                // Engine-level sidecar metadata; handled by Engine replay/rollback paths.
            }
            ChangeEvent::AddVertex { id, .. } => {
                // Inverse of AddVertex is removal
                let _ = self.remove_vertex(id); // ignore errors for now
            }
            ChangeEvent::RemoveVertex {
                id,
                old_value,
                old_formula,
                old_dependencies,
                old_dependents,
                coord,
                sheet_id,
                kind,
                ..
            } => {
                if let (Some(c), Some(sid)) = (coord, sheet_id) {
                    let meta =
                        VertexMeta::new(c.row(), c.col(), sid, kind.unwrap_or(VertexKind::Cell));
                    // Decision 9 (as amended in Program 2): the removed
                    // vertex itself comes back, so the cell's formula keeps
                    // its id. Legacy re-created the cell on a new vertex;
                    // that stays the fallback when the cell is occupied.
                    let new_id = if self.graph.revive_vertex(id, sid, c) {
                        if self.has_logger() {
                            self.log_change(ChangeEvent::AddVertex {
                                id,
                                coord: meta.coord,
                                sheet_id: meta.sheet_id,
                                value: Some(LiteralValue::Empty),
                                formula: None,
                                kind: Some(meta.kind),
                                flags: Some(meta.flags),
                            });
                        }
                        id
                    } else {
                        self.try_add_vertex(meta)?
                    };
                    if let Some(v) = old_value {
                        let cell_ref = self.graph.make_cell_ref_internal(sid, c.row(), c.col());
                        self.set_cell_value(cell_ref, v);
                    }
                    if let Some(f) = old_formula {
                        let cell_ref = self.graph.make_cell_ref_internal(sid, c.row(), c.col());
                        self.set_cell_formula(cell_ref, f);
                    }
                    // Legacy's edges are rebuilt from the restored formula
                    // (the authority reads formulas, not recorded edges).
                    #[cfg(any(test, feature = "legacy_oracle"))]
                    {
                        for dep in old_dependencies {
                            self.graph.add_dependency_edge(new_id, dep)?;
                        }
                        for parent in old_dependents {
                            self.graph.add_dependency_edge(parent, new_id)?;
                        }
                    }
                    #[cfg(not(any(test, feature = "legacy_oracle")))]
                    let _ = (old_dependencies, old_dependents, new_id);
                }
            }
            ChangeEvent::DefineName { name, scope, .. } => {
                // Inverse is delete name
                self.graph.delete_name(&name, scope)?;
            }
            ChangeEvent::UpdateName {
                name,
                scope,
                old_definition,
                ..
            } => {
                // Restore old definition
                self.graph.update_name(&name, old_definition, scope)?;
            }
            ChangeEvent::DeleteName {
                name,
                scope,
                old_definition,
            } => {
                if let Some(def) = old_definition {
                    self.graph.define_name(&name, def, scope)?;
                } else {
                    return Err(EditorError::TransactionFailed {
                        reason: "Missing old definition for name deletion rollback".to_string(),
                    });
                }
            }
            ChangeEvent::SpillCommitted { anchor, old, .. } => {
                // Restore previous spill region.
                if let Some(old) = old {
                    self.graph
                        .commit_spill_region_atomic_with_fault(
                            anchor,
                            old.target_cells,
                            old.values,
                            None,
                        )
                        .map_err(EditorError::Excel)?;
                } else {
                    self.graph.clear_spill_region(anchor);
                }
            }
            ChangeEvent::SpillCleared { anchor, old } => {
                // Re-commit the previous spill region.
                self.graph
                    .commit_spill_region_atomic_with_fault(
                        anchor,
                        old.target_cells,
                        old.values,
                        None,
                    )
                    .map_err(EditorError::Excel)?;
            }
            ChangeEvent::StagedFormulaCellChanged { .. } => {
                // Workbook-level deferred state is replayed by Engine undo/redo wrappers.
            }
            // Granular events for compound operations
            ChangeEvent::CompoundStart { description, .. } => {
                // A marker; a structural edit's marker also shifts the
                // retired-id side table back (decision 27).
                self.graph.replay_structural_marker(&description, false);
            }
            ChangeEvent::CompoundEnd { .. } => {
                // A marker: backward replay loops call
                // `inverse_compound_end` with its compound's description.
            }
            ChangeEvent::VertexMoved {
                id,
                sheet_id: _,
                old_coord,
                ..
            } => {
                // Move back to old position
                self.move_vertex(id, old_coord)?;
            }
            ChangeEvent::FormulaAdjusted { id, old_ast, .. } => {
                // Restore old formula directly by vertex id.
                self.graph
                    .update_vertex_formula(id, old_ast)
                    .map_err(EditorError::Excel)?;
                self.graph.mark_vertex_dirty(id);
            }
            ChangeEvent::NamedRangeAdjusted {
                name,
                scope,
                old_definition,
                ..
            } => {
                // Restore old definition
                self.graph.update_name(&name, old_definition, scope)?;
            }
            ChangeEvent::EdgeAdded { from, to } => {
                // Remove the edge
                // TODO: Need specific edge removal method
                return Err(EditorError::TransactionFailed {
                    reason: "Cannot rollback edge addition yet".to_string(),
                });
            }
            ChangeEvent::EdgeRemoved { from, to } => {
                // Re-add the edge
                // TODO: Need specific edge addition method
                return Err(EditorError::TransactionFailed {
                    reason: "Cannot rollback edge removal yet".to_string(),
                });
            }
        }
        Ok(())
    }

    /// Add a vertex to the graph.
    ///
    /// This compatibility API preserves the historical sentinel return on failure. New
    /// transactional callers should use [`Self::try_add_vertex`] to retain typed admission errors.
    pub fn add_vertex(&mut self, meta: VertexMeta) -> VertexId {
        self.try_add_vertex(meta)
            .unwrap_or_else(|_| VertexId::new(0))
    }

    pub fn try_add_vertex(&mut self, meta: VertexMeta) -> Result<VertexId, EditorError> {
        // An explicitly requested vertex: an empty one at the cell (value
        // cells otherwise have none, decision 27; a value set later removes
        // it again). VertexMeta uses internal 0-based coordinates.
        let id = self
            .graph
            .add_empty_vertex(meta.sheet_id, meta.coord.row(), meta.coord.col())
            .map_err(EditorError::Excel)?;

        if self.has_logger() && id.0 != 0 {
            self.log_change(ChangeEvent::AddVertex {
                id,
                coord: meta.coord,
                sheet_id: meta.sheet_id,
                value: Some(LiteralValue::Empty),
                formula: None,
                kind: Some(meta.kind),
                flags: Some(meta.flags),
            });
        }
        Ok(id)
    }

    /// Remove a vertex from the graph with proper cleanup
    pub fn remove_vertex(&mut self, id: VertexId) -> Result<(), EditorError> {
        // Check if vertex exists
        if !self.graph.vertex_exists(id) {
            return Err(EditorError::Excel(
                ExcelError::new(ExcelErrorKind::Ref).with_message("Vertex does not exist"),
            ));
        }
        self.graph.materialize_vertex(id);

        // If this vertex anchors a spill, clear ownership + spilled children first.
        // This keeps the spill registry consistent even if the anchor is removed.
        let spill_snapshot = self.snapshot_spill_for_anchor(id);
        let did_spill_clear = spill_snapshot.is_some();
        if let Some(old_spill) = spill_snapshot {
            if let Some(logger) = &mut self.change_logger {
                logger.begin_compound(format!("RemoveVertexWithSpillClear id={}", id.0));
            }
            self.graph.clear_spill_region(id);
            self.log_change(ChangeEvent::SpillCleared {
                anchor: id,
                old: old_spill,
            });
        }

        // Its direct readers (legacy's in-edges) before anything changes.
        let dependents = self.graph.authority_in_edge_readers(id);

        // Capture old state (dependencies & dependents) BEFORE edge removal
        let (
            old_value,
            old_formula,
            old_dependencies,
            old_dependents,
            coord,
            sheet_id_opt,
            kind,
            flags,
        ) = if self.has_logger() {
            let coord = self.graph.get_grid_addr(id);
            let sheet_id = self.graph.get_sheet_id(id);
            let kind = self.graph.get_vertex_kind(id);
            // flags not publicly exposed; set to 0 for now (future: expose getter)
            let flags = 0u8;
            (
                self.graph.get_value(id),
                self.get_formula_ast(id),
                // Outgoing edges: legacy's, in oracle builds only.
                {
                    #[cfg(any(test, feature = "legacy_oracle"))]
                    let deps = self.graph.get_dependencies(id);
                    #[cfg(not(any(test, feature = "legacy_oracle")))]
                    let deps = Vec::new();
                    deps
                },
                dependents.clone(), // captured earlier
                coord,
                Some(sheet_id),
                Some(kind),
                Some(flags),
            )
        } else {
            (None, None, vec![], vec![], None, None, None, None)
        };

        // A formula leaving its cell is journaled (replay that brings it
        // back revives this vertex).
        self.graph.journal_formula_left(id);
        // Remove from cell mapping if it exists
        if let Some(cell_ref) = self.graph.get_cell_ref_for_vertex(id) {
            self.graph.remove_cell_mapping(&cell_ref);
            // Legacy's cell leaves the used extent with its vertex.
            let (r, c) = (cell_ref.coord.row(), cell_ref.coord.col());
            self.graph
                .forget_extent_cells(cell_ref.sheet_id, (r, r), (c, c));
        }

        // Remove all formula/value payloads owned by this vertex.  Tombstoned vertices remain in
        // the SoA store for stable IDs/debugging, but they must not continue to participate in
        // formula evaluation through `vertex_formulas`.
        self.graph.vertex_formulas.remove(&id);
        self.graph.vertex_values.remove(&id);
        self.graph.clear_formula_vertex_dirty(id);
        self.graph.mark_volatile(id, false);
        self.graph.store.set_kind(id, VertexKind::Empty);
        self.graph.store.set_dynamic(id, false);

        // Remove all edges
        self.graph.remove_all_edges(id);

        // Mark all dependents as having #REF! error
        for dep_id in &dependents {
            self.graph.mark_as_ref_error(*dep_id);
        }

        // Mark as deleted in store (tombstone)
        self.graph.mark_deleted(id, true);

        // Log change event
        self.log_change(ChangeEvent::RemoveVertex {
            id,
            old_value,
            old_formula,
            old_dependencies,
            old_dependents,
            coord,
            sheet_id: sheet_id_opt,
            kind,
            flags,
        });

        if did_spill_clear && let Some(logger) = &mut self.change_logger {
            logger.end_compound();
        }

        Ok(())
    }

    /// Convenience: remove vertex at a given cell ref if exists
    pub fn remove_vertex_at(&mut self, cell: CellRef) -> Result<(), EditorError> {
        if let Some(id) = self.graph.get_vertex_for_cell(&cell) {
            self.remove_vertex(id)
        } else {
            // A value or referenced cell (legacy's vertex) leaves the used
            // extent.
            let (r, c) = (cell.coord.row(), cell.coord.col());
            self.graph
                .forget_extent_cells(cell.sheet_id, (r, r), (c, c));
            Ok(())
        }
    }

    /// Move a vertex to a new position
    ///
    /// The `GridAddr` argument says where the vertex is going, but the `VertexId` says
    /// nothing about whether it is somewhere to begin with. A symbol has no position, so
    /// moving one is meaningless: it is what turned a default-sheet insert into a
    /// name-hijacked cell (#304). Every in-tree caller iterates `grid_vertices_in_sheet`
    /// and so cannot reach this, but the method is public, so refuse explicitly.
    pub fn move_vertex(&mut self, id: VertexId, new_coord: GridAddr) -> Result<(), EditorError> {
        self.move_vertex_inner(id, new_coord, true)
    }

    /// [`Self::move_vertex`]; `map_cell` false leaves the cell map alone
    /// (the destination keeps the vertex a block shift moved there).
    fn move_vertex_inner(
        &mut self,
        id: VertexId,
        new_coord: GridAddr,
        map_cell: bool,
    ) -> Result<(), EditorError> {
        self.graph.authority_note_structural(false);
        // A compressed member's formula is relative to its cell: it keeps
        // its own AST across the move (Program 2).
        self.graph.own_formula_id(id);
        // Check if vertex exists
        if !self.graph.vertex_exists(id) {
            return Err(EditorError::Excel(
                ExcelError::new(ExcelErrorKind::Ref).with_message("Vertex does not exist"),
            ));
        }
        if self.graph.get_grid_addr(id).is_none() {
            return Err(EditorError::Excel(
                ExcelError::new(ExcelErrorKind::Ref).with_message(
                    "Symbol vertices have no position and cannot be moved onto the grid",
                ),
            ));
        }

        // Get old cell reference
        let old_cell_ref = self.graph.get_cell_ref_for_vertex(id);

        // Create new cell reference
        let sheet_id = self.graph.get_sheet_id(id);
        let new_cell_ref = CellRef::new(
            sheet_id,
            Coord::new(new_coord.row(), new_coord.col(), true, true),
        );

        // Update coordinate in store
        self.graph.set_grid_addr(id, new_coord);

        // Update edge cache coordinate if needed
        self.graph.update_edge_grid_addr(id, new_coord);

        // Update cell mapping
        if map_cell {
            self.graph
                .update_cell_mapping(id, old_cell_ref, new_cell_ref);
        }

        // Mark dependents as dirty
        self.graph.mark_dependents_dirty(id);

        Ok(())
    }

    /// Update vertex metadata
    pub fn patch_vertex_meta(
        &mut self,
        id: VertexId,
        patch: VertexMetaPatch,
    ) -> Result<MetaUpdateSummary, EditorError> {
        if !self.graph.vertex_exists(id) {
            return Err(EditorError::Excel(
                ExcelError::new(ExcelErrorKind::Ref).with_message("Vertex does not exist"),
            ));
        }

        let mut summary = MetaUpdateSummary::default();

        if let Some(coord) = patch.coord {
            // Same reasoning as `move_vertex`: a symbol has no position to patch.
            if self.graph.get_grid_addr(id).is_none() {
                return Err(EditorError::Excel(
                    ExcelError::new(ExcelErrorKind::Ref).with_message(
                        "Symbol vertices have no position and cannot be moved onto the grid",
                    ),
                ));
            }
            // A compressed member's formula is relative to its cell.
            let _ = self.graph.own_formula_id(id);
            self.graph.set_grid_addr(id, coord);
            self.graph.update_edge_grid_addr(id, coord);
            summary.coord_changed = true;
        }

        if let Some(kind) = patch.kind {
            self.graph.set_kind(id, kind);
            summary.kind_changed = true;
        }

        if let Some(dirty) = patch.dirty {
            self.graph.set_dirty(id, dirty);
            summary.flags_changed = true;
        }

        if let Some(volatile) = patch.volatile {
            self.graph.mark_volatile(id, volatile);
            summary.flags_changed = true;
        }

        Ok(summary)
    }

    /// Update vertex data (value or formula)
    pub fn patch_vertex_data(
        &mut self,
        id: VertexId,
        patch: VertexDataPatch,
    ) -> Result<DataUpdateSummary, EditorError> {
        if !self.graph.vertex_exists(id) {
            return Err(EditorError::Excel(
                ExcelError::new(ExcelErrorKind::Ref).with_message("Vertex does not exist"),
            ));
        }

        let mut summary = DataUpdateSummary::default();

        if let Some(value) = patch.value {
            self.graph.update_vertex_value(id, value);
            summary.value_changed = true;

            // Mark dependents as dirty. get_dependents is delta-aware, so no
            // CSR rebuild is required even when edits are pending (#125).
            let dependents = self.graph.authority_in_edge_readers(id);
            for dep in &dependents {
                self.graph.set_dirty(*dep, true);
            }
            summary.dependents_marked_dirty = dependents;
        }

        if let Some(_formula) = patch.formula {
            // This would need proper formula update implementation
            // For now, we'll mark as changed
            summary.formula_changed = true;
        }

        Ok(summary)
    }

    /// No-op kept for journal replay of old `EdgeAdded` events: the
    /// dependency authority derives every edge from formulas.
    pub fn add_edge(&mut self, from: VertexId, to: VertexId) -> bool {
        if from == to {
            return false; // Prevent self-loops
        }

        // TODO: Add edge through proper API when available
        // For now, return true to indicate intent
        true
    }

    /// No-op kept for journal replay of old `EdgeRemoved` events.
    pub fn remove_edge(&mut self, _from: VertexId, _to: VertexId) -> bool {
        // TODO: Remove edge through proper API when available
        true
    }

    /// Insert rows at the specified position, shifting existing rows down
    pub fn insert_rows(
        &mut self,
        sheet_id: SheetId,
        before: u32,
        count: u32,
    ) -> Result<ShiftSummary, EditorError> {
        let result = self.insert_rows_impl(sheet_id, before, count);
        self.graph.authority_end_structural();
        result
    }

    fn insert_rows_impl(
        &mut self,
        sheet_id: SheetId,
        before: u32,
        count: u32,
    ) -> Result<ShiftSummary, EditorError> {
        self.graph.authority_note_structural_shift();
        if count == 0 {
            return Ok(ShiftSummary::default());
        }

        let mut summary = ShiftSummary::default();

        // Begin batch for efficiency
        self.begin_batch();
        let conservative = crate::engine::graph::StructuralOccupancy::conservative();
        let occupancy = self.structural_occupancy.as_ref().unwrap_or(&conservative);
        let range_dependents = self.graph.authority_structural_band_readers(
            sheet_id,
            crate::engine::graph::StructuralEdit::InsertRows { before },
            occupancy,
        );
        #[cfg(test)]
        {
            summary.structural_dependents_dirtied = range_dependents.clone();
        }
        self.graph.mark_dirty_many(&range_dependents);

        // 1. Collect vertices to shift (those at or after the insert point)
        let vertices_to_shift: Vec<(VertexId, GridAddr)> = self
            .graph
            .grid_vertices_in_sheet(sheet_id)
            .filter(|(_, coord)| coord.row() >= before)
            .collect();

        if let Some(logger) = &mut self.change_logger {
            logger.begin_compound(format!(
                "InsertRows sheet={sheet_id} before={before} count={count}"
            ));
        }
        // 2-3. Shift vertices (emit VertexMoved) and adjust formulas
        // (emit FormulaAdjusted).
        let op = ShiftOperation::InsertRows {
            sheet_id,
            before,
            count,
        };
        self.shift_and_adjust(
            sheet_id,
            &op,
            vertices_to_shift,
            |old_coord| GridAddr::new(old_coord.row() + count, old_coord.col()),
            &mut summary,
        )?;

        // 4. Adjust named ranges
        let old_names = if self.has_logger() {
            Some(self.snapshot_named_definitions())
        } else {
            None
        };
        self.graph.adjust_named_ranges(&op)?;
        if let Some(old_names) = old_names {
            let new_names = self.snapshot_named_definitions();
            for ((scope, name), old_definition) in old_names {
                if let Some(new_definition) = new_names.get(&(scope, name.clone()))
                    && *new_definition != old_definition
                {
                    self.log_change(ChangeEvent::NamedRangeAdjusted {
                        name,
                        scope,
                        old_definition,
                        new_definition: new_definition.clone(),
                    });
                }
            }
        }

        // 5. Log change event
        if let Some(logger) = &mut self.change_logger {
            logger.end_compound();
        }

        self.commit_batch();

        Ok(summary)
    }

    /// Delete rows at the specified position, shifting remaining rows up
    pub fn delete_rows(
        &mut self,
        sheet_id: SheetId,
        start: u32,
        count: u32,
    ) -> Result<ShiftSummary, EditorError> {
        let result = self.delete_rows_impl(sheet_id, start, count);
        self.graph.authority_end_structural();
        result
    }

    fn delete_rows_impl(
        &mut self,
        sheet_id: SheetId,
        start: u32,
        count: u32,
    ) -> Result<ShiftSummary, EditorError> {
        if count == 0 {
            self.graph.authority_note_structural_shift();
            return Ok(ShiftSummary::default());
        }
        // The deleted band's readers (and theirs) are dirty, in the pre-edit
        // frame: value cells have no vertex whose removal would dirty them
        // (decision 27). Only occupied columns when the caller knows them
        // (an empty column's readers see no change).
        let end = start.saturating_add(count - 1);
        let rects: Vec<(SheetId, u32, u32, u32, u32)> = match self
            .structural_occupancy
            .as_ref()
            .and_then(|o| o.occupied_column_runs())
        {
            Some(runs) => runs
                .into_iter()
                .map(|(c0, c1)| (sheet_id, start, end, c0, c1))
                .collect(),
            None => vec![(sheet_id, start, end, 0, 16_383)],
        };
        let _ = self.graph.mark_dirty_rects(&rects);
        self.graph.authority_note_structural_shift();

        let mut summary = ShiftSummary::default();

        self.begin_batch();

        if let Some(logger) = &mut self.change_logger {
            logger.begin_compound(format!(
                "DeleteRows sheet={sheet_id} start={start} count={count}"
            ));
        }

        // 1. Delete vertices in the range
        let vertices_to_delete: Vec<VertexId> = self
            .graph
            .grid_vertices_in_sheet(sheet_id)
            .filter(|(_, coord)| coord.row() >= start && coord.row() < start + count)
            .map(|(id, _)| id)
            .collect();
        let conservative = crate::engine::graph::StructuralOccupancy::conservative();
        let occupancy = self.structural_occupancy.as_ref().unwrap_or(&conservative);
        let range_dependents = self.graph.authority_structural_band_readers(
            sheet_id,
            crate::engine::graph::StructuralEdit::DeleteRows {
                start,
                end: start.saturating_add(count).saturating_sub(1).max(start),
            },
            occupancy,
        );
        #[cfg(test)]
        {
            summary.structural_dependents_dirtied = range_dependents.clone();
        }
        self.graph.mark_dirty_many(&range_dependents);

        for id in vertices_to_delete {
            self.remove_vertex(id)?;
            summary.vertices_deleted.push(id);
        }
        // 2-3. Shift the remaining vertices (emit VertexMoved) and adjust
        // formulas (emit FormulaAdjusted).
        let vertices_to_shift: Vec<(VertexId, GridAddr)> = self
            .graph
            .grid_vertices_in_sheet(sheet_id)
            .filter(|(_, coord)| coord.row() >= start + count)
            .collect();
        let op = ShiftOperation::DeleteRows {
            sheet_id,
            start,
            count,
        };
        self.shift_and_adjust(
            sheet_id,
            &op,
            vertices_to_shift,
            |old_coord| GridAddr::new(old_coord.row() - count, old_coord.col()),
            &mut summary,
        )?;

        // 4. Adjust named ranges
        let old_names = if self.has_logger() {
            Some(self.snapshot_named_definitions())
        } else {
            None
        };
        self.graph.adjust_named_ranges(&op)?;
        if let Some(old_names) = old_names {
            let new_names = self.snapshot_named_definitions();
            for ((scope, name), old_definition) in old_names {
                if let Some(new_definition) = new_names.get(&(scope, name.clone()))
                    && *new_definition != old_definition
                {
                    self.log_change(ChangeEvent::NamedRangeAdjusted {
                        name,
                        scope,
                        old_definition,
                        new_definition: new_definition.clone(),
                    });
                }
            }
        }

        // 5. Log change event
        if let Some(logger) = &mut self.change_logger {
            logger.end_compound();
        }

        self.commit_batch();

        Ok(summary)
    }

    /// Insert columns at the specified position, shifting existing columns right
    pub fn insert_columns(
        &mut self,
        sheet_id: SheetId,
        before: u32,
        count: u32,
    ) -> Result<ShiftSummary, EditorError> {
        let result = self.insert_columns_impl(sheet_id, before, count);
        self.graph.authority_end_structural();
        result
    }

    fn insert_columns_impl(
        &mut self,
        sheet_id: SheetId,
        before: u32,
        count: u32,
    ) -> Result<ShiftSummary, EditorError> {
        self.graph.authority_note_structural_shift();
        if count == 0 {
            return Ok(ShiftSummary::default());
        }

        let mut summary = ShiftSummary::default();

        // Begin batch for efficiency
        self.begin_batch();
        let conservative = crate::engine::graph::StructuralOccupancy::conservative();
        let occupancy = self.structural_occupancy.as_ref().unwrap_or(&conservative);
        let range_dependents = self.graph.authority_structural_band_readers(
            sheet_id,
            crate::engine::graph::StructuralEdit::InsertColumns { before },
            occupancy,
        );
        #[cfg(test)]
        {
            summary.structural_dependents_dirtied = range_dependents.clone();
        }
        self.graph.mark_dirty_many(&range_dependents);

        // 1. Collect vertices to shift (those at or after the insert point)
        let vertices_to_shift: Vec<(VertexId, GridAddr)> = self
            .graph
            .grid_vertices_in_sheet(sheet_id)
            .filter(|(_, coord)| coord.col() >= before)
            .collect();

        if let Some(logger) = &mut self.change_logger {
            logger.begin_compound(format!(
                "InsertColumns sheet={sheet_id} before={before} count={count}"
            ));
        }
        // 2-3. Shift vertices (emit VertexMoved) and adjust formulas
        // (emit FormulaAdjusted).
        let op = ShiftOperation::InsertColumns {
            sheet_id,
            before,
            count,
        };
        self.shift_and_adjust(
            sheet_id,
            &op,
            vertices_to_shift,
            |old_coord| GridAddr::new(old_coord.row(), old_coord.col() + count),
            &mut summary,
        )?;

        // 4. Adjust named ranges
        let old_names = if self.has_logger() {
            Some(self.snapshot_named_definitions())
        } else {
            None
        };
        self.graph.adjust_named_ranges(&op)?;
        if let Some(old_names) = old_names {
            let new_names = self.snapshot_named_definitions();
            for ((scope, name), old_definition) in old_names {
                if let Some(new_definition) = new_names.get(&(scope, name.clone()))
                    && *new_definition != old_definition
                {
                    self.log_change(ChangeEvent::NamedRangeAdjusted {
                        name,
                        scope,
                        old_definition,
                        new_definition: new_definition.clone(),
                    });
                }
            }
        }

        // 5. Log change event
        if let Some(logger) = &mut self.change_logger {
            logger.end_compound();
        }

        self.commit_batch();

        Ok(summary)
    }

    /// Steps 2 and 3 of a row/column insert or delete: move the vertices
    /// at or past the edit (logging `VertexMoved`), then adjust every
    /// formula (logging `FormulaAdjusted`).
    ///
    /// Program 2 (contract decision 20.5): virtual family member runs are
    /// shifted and adjusted as one first (`shift_virtual_runs`); their
    /// members are not moved or rewritten one by one, and their
    /// `FormulaAdjusted` events go to the log as one run record, expanded
    /// only when read. Runs the block transform cannot express come back
    /// to the per-cell maps and take the per-vertex path below. Events are
    /// logged in vertex-id order (moves, then adjustments), the per-vertex
    /// order legacy logged moves in.
    fn shift_and_adjust(
        &mut self,
        sheet_id: SheetId,
        op: &ShiftOperation,
        vertices_to_shift: Vec<(VertexId, GridAddr)>,
        shift: impl Fn(GridAddr) -> GridAddr,
        summary: &mut ShiftSummary,
    ) -> Result<(), EditorError> {
        let journal = self.has_logger();
        self.graph.shift_retired_ids(op, journal);
        let runs = self.graph.shift_virtual_runs(op);

        for (id, old_coord) in vertices_to_shift {
            let new_coord = shift(old_coord);
            if self.has_logger() {
                self.log_change(ChangeEvent::VertexMoved {
                    id,
                    sheet_id,
                    old_coord,
                    new_coord,
                });
            }
            if self.graph.is_virtual_member(id) {
                // Moved with its run.
                debug_assert_eq!(self.graph.get_grid_addr(id), Some(new_coord));
            } else {
                // Legacy moved vertices one at a time in id order: of two
                // vertices shifted onto one cell (a tombstone or stale
                // vertex, and a live one) the later id kept the mapping. A
                // member its run moved there already keeps the cell when
                // its id is later.
                let cell = CellRef::new(
                    sheet_id,
                    Coord::new(new_coord.row(), new_coord.col(), true, true),
                );
                let member_moves_later = self
                    .graph
                    .virtual_member_at(&cell)
                    .is_some_and(|m| m.0 > id.0 && moved_run_holds(&runs, m));
                self.move_vertex_inner(id, new_coord, !member_moves_later)?;
            }
            summary.vertices_moved.push(id);
        }
        self.graph.after_shift_moves(&runs);

        let adjuster = ReferenceAdjuster::new();
        let mut adjusted_runs: Vec<&crate::engine::graph::ShiftedRun> =
            runs.iter().filter(|r| r.adjusted.is_some()).collect();
        adjusted_runs.sort_unstable_by_key(|r| r.first.0);
        let mut next_run = 0;
        let formula_vertices = self.graph.materialized_formula_vertices_sorted();
        for id in formula_vertices {
            while next_run < adjusted_runs.len() && adjusted_runs[next_run].first.0 < id.0 {
                self.log_adjusted_run(adjusted_runs[next_run], summary);
                next_run += 1;
            }
            if let Some(ast) = self.get_formula_ast(id)
                && let Some(adjusted) = adjuster.adjust_ast_if_changed_in_context(
                    &ast,
                    op,
                    &ReferenceContext::new(self.graph.get_sheet_id(id), self.graph.sheet_reg()),
                )
            {
                if self.has_logger() {
                    self.log_change(ChangeEvent::FormulaAdjusted {
                        id,
                        addr: self.graph.get_cell_ref_for_vertex(id),
                        old_ast: ast.clone(),
                        new_ast: adjusted.clone(),
                    });
                }
                self.graph.update_vertex_formula(id, adjusted)?;
                self.graph.mark_vertex_dirty(id);
                summary.formulas_updated += 1;
            }
        }
        for run in &adjusted_runs[next_run..] {
            self.log_adjusted_run(run, summary);
        }
        Ok(())
    }

    /// A run the block transform adjusted: its members' `FormulaAdjusted`
    /// events as one record (the transform already rewrote and dirtied
    /// them).
    fn log_adjusted_run(
        &mut self,
        run: &crate::engine::graph::ShiftedRun,
        summary: &mut ShiftSummary,
    ) {
        summary.formulas_updated += run.len as usize;
        let Some((old_first, new_first)) = &run.adjusted else {
            return;
        };
        if let Some(logger) = &mut self.change_logger {
            logger.record_formula_run(FormulaRunAdjusted {
                first: run.first,
                len: run.len,
                sheet_id: run.sheet,
                col: run.col,
                row0: run.row0,
                old_first: old_first.clone(),
                new_first: new_first.clone(),
            });
        }
    }

    /// Delete columns at the specified position, shifting remaining columns left
    pub fn delete_columns(
        &mut self,
        sheet_id: SheetId,
        start: u32,
        count: u32,
    ) -> Result<ShiftSummary, EditorError> {
        let result = self.delete_columns_impl(sheet_id, start, count);
        self.graph.authority_end_structural();
        result
    }

    fn delete_columns_impl(
        &mut self,
        sheet_id: SheetId,
        start: u32,
        count: u32,
    ) -> Result<ShiftSummary, EditorError> {
        if count == 0 {
            self.graph.authority_note_structural_shift();
            return Ok(ShiftSummary::default());
        }
        // The deleted band's readers (and theirs) are dirty, in the pre-edit
        // frame: value cells have no vertex whose removal would dirty them
        // (decision 27).
        let _ = self.graph.mark_dirty_rects(&[(
            sheet_id,
            0,
            1_048_575,
            start,
            start.saturating_add(count - 1),
        )]);
        self.graph.authority_note_structural_shift();

        let mut summary = ShiftSummary::default();

        self.begin_batch();

        if let Some(logger) = &mut self.change_logger {
            logger.begin_compound(format!(
                "DeleteColumns sheet={sheet_id} start={start} count={count}"
            ));
        }

        // 1. Delete vertices in the range
        let vertices_to_delete: Vec<VertexId> = self
            .graph
            .grid_vertices_in_sheet(sheet_id)
            .filter(|(_, coord)| coord.col() >= start && coord.col() < start + count)
            .map(|(id, _)| id)
            .collect();
        let conservative = crate::engine::graph::StructuralOccupancy::conservative();
        let occupancy = self.structural_occupancy.as_ref().unwrap_or(&conservative);
        let range_dependents = self.graph.authority_structural_band_readers(
            sheet_id,
            crate::engine::graph::StructuralEdit::DeleteColumns {
                start,
                end: start.saturating_add(count).saturating_sub(1).max(start),
            },
            occupancy,
        );
        #[cfg(test)]
        {
            summary.structural_dependents_dirtied = range_dependents.clone();
        }
        self.graph.mark_dirty_many(&range_dependents);

        for id in vertices_to_delete {
            self.remove_vertex(id)?;
            summary.vertices_deleted.push(id);
        }
        // 2-3. Shift the remaining vertices (emit VertexMoved) and adjust
        // formulas (emit FormulaAdjusted).
        let vertices_to_shift: Vec<(VertexId, GridAddr)> = self
            .graph
            .grid_vertices_in_sheet(sheet_id)
            .filter(|(_, coord)| coord.col() >= start + count)
            .collect();
        let op = ShiftOperation::DeleteColumns {
            sheet_id,
            start,
            count,
        };
        self.shift_and_adjust(
            sheet_id,
            &op,
            vertices_to_shift,
            |old_coord| GridAddr::new(old_coord.row(), old_coord.col() - count),
            &mut summary,
        )?;

        // 4. Adjust named ranges
        let old_names = if self.has_logger() {
            Some(self.snapshot_named_definitions())
        } else {
            None
        };
        self.graph.adjust_named_ranges(&op)?;
        if let Some(old_names) = old_names {
            let new_names = self.snapshot_named_definitions();
            for ((scope, name), old_definition) in old_names {
                if let Some(new_definition) = new_names.get(&(scope, name.clone()))
                    && *new_definition != old_definition
                {
                    self.log_change(ChangeEvent::NamedRangeAdjusted {
                        name,
                        scope,
                        old_definition,
                        new_definition: new_definition.clone(),
                    });
                }
            }
        }

        // 5. Log change event
        if let Some(logger) = &mut self.change_logger {
            logger.end_compound();
        }

        self.commit_batch();

        Ok(summary)
    }

    /// Shift rows down/up within a sheet (Excel's insert/delete rows)
    pub fn shift_rows(&mut self, sheet_id: SheetId, start_row: u32, delta: i32) {
        if delta == 0 {
            return;
        }

        // Log change event for undo/redo
        let change_event = ChangeEvent::SetValue {
            addr: CellRef {
                sheet_id,
                coord: Coord::new(start_row, 0, true, true),
            },
            old_value: None,
            old_formula: None,
            new: LiteralValue::Text(format!("Row shift: start={start_row}, delta={delta}")),
        };
        self.log_change(change_event);

        // TODO: Implement actual row shifting logic
        // This would require coordination with the vertex store and dependency tracking
    }

    /// Shift columns left/right within a sheet (Excel's insert/delete columns)
    pub fn shift_columns(&mut self, sheet_id: SheetId, start_col: u32, delta: i32) {
        if delta == 0 {
            return;
        }

        // Log change event
        let change_event = ChangeEvent::SetValue {
            addr: CellRef {
                sheet_id,
                coord: Coord::new(0, start_col, true, true),
            },
            old_value: None,
            old_formula: None,
            new: LiteralValue::Text(format!("Column shift: start={start_col}, delta={delta}")),
        };
        self.log_change(change_event);

        // TODO: Implement actual column shifting logic
        // This would require coordination with the vertex store and dependency tracking
    }

    /// Set a cell value. A value cell has no vertex (decision 27): returns
    /// `VertexId(0)`; a formula it replaces retires its id.
    pub fn set_cell_value(&mut self, cell_ref: CellRef, value: LiteralValue) -> VertexId {
        self.set_cell_value_with_old_state(cell_ref, value, None, None)
    }

    /// Like [`set_cell_value`](Self::set_cell_value), but lets the caller
    /// supply old state captured from an external source of truth (e.g. the
    /// Arrow store, whose values are invisible here when the graph value cache
    /// is disabled) for the change-log event.
    ///
    /// Precedence matches the historical append-then-patch flow
    /// (`ChangeLog::patch_last_cell_event_old_state`): state the editor
    /// captures from the graph wins; caller-supplied state only fills fields
    /// the graph left `None`.
    pub fn set_cell_value_with_old_state(
        &mut self,
        cell_ref: CellRef,
        value: LiteralValue,
        fallback_old_value: Option<LiteralValue>,
        fallback_old_formula: Option<ASTNode>,
    ) -> VertexId {
        let sheet_name = self.graph.sheet_name(cell_ref.sheet_id).to_string();

        // Capture old state before modification (value + formula); fall back
        // to caller-supplied state for anything the graph cannot see.
        let old_id = self.graph.get_vertex_id_for_address(&cell_ref);
        let old_value = old_id
            .and_then(|id| self.graph.get_value(id))
            .or(fallback_old_value);
        let old_formula = old_id
            .and_then(|id| self.get_formula_ast(id))
            .or(fallback_old_formula);

        // If this cell currently anchors a spill, clear the spill first and log it.
        // This keeps spill ownership maps and children consistent under undo/redo.
        let spill_snapshot =
            old_id.and_then(|id| self.snapshot_spill_for_anchor(id).map(|s| (id, s)));
        let did_spill_clear = spill_snapshot.is_some();
        if let Some((anchor, old_spill)) = spill_snapshot {
            if let Some(logger) = &mut self.change_logger {
                logger.begin_compound(format!(
                    "SetValueWithSpillClear sheet={} row={} col={}",
                    cell_ref.sheet_id,
                    cell_ref.coord.row(),
                    cell_ref.coord.col()
                ));
            }
            self.graph.clear_spill_region(anchor);
            self.log_change(ChangeEvent::SpillCleared {
                anchor,
                old: old_spill,
            });
        }

        // Use the existing DependencyGraph API
        // VertexEditor operates on internal 0-based coords; graph APIs are 1-based.
        match self.graph.set_cell_value(
            &sheet_name,
            cell_ref.coord.row() + 1,
            cell_ref.coord.col() + 1,
            value.clone(),
        ) {
            Ok(_) => {
                // Log change event
                let change_event = ChangeEvent::SetValue {
                    addr: cell_ref,
                    old_value,
                    old_formula,
                    new: value,
                };
                self.log_change(change_event);

                if did_spill_clear && let Some(logger) = &mut self.change_logger {
                    logger.end_compound();
                }

                // A value cell has no vertex (decision 27).
                VertexId::new(0)
            }
            Err(_) => VertexId::new(0),
        }
    }

    /// Set a cell formula, creating the vertex if it doesn't exist.
    ///
    /// Legacy compatibility API: failures return vertex zero. Prefer
    /// [`Self::try_set_cell_formula`] when failure must be observable.
    pub fn set_cell_formula(&mut self, cell_ref: CellRef, formula: ASTNode) -> VertexId {
        self.set_cell_formula_with_old_state(cell_ref, formula, None, None)
    }

    /// Set a formula, reporting binding/admission failures without clearing its old spill.
    pub fn try_set_cell_formula(
        &mut self,
        cell_ref: CellRef,
        formula: ASTNode,
    ) -> Result<VertexId, ExcelError> {
        self.try_set_cell_formula_with_old_state(cell_ref, formula, None, None)
    }

    /// Like [`set_cell_formula`](Self::set_cell_formula), but lets the caller
    /// supply old state captured from an external source of truth (e.g. the
    /// Arrow store) for the change-log event. Same precedence as
    /// [`set_cell_value_with_old_state`](Self::set_cell_value_with_old_state):
    /// graph-captured state wins, caller state only fills `None` fields.
    pub fn set_cell_formula_with_old_state(
        &mut self,
        cell_ref: CellRef,
        formula: ASTNode,
        fallback_old_value: Option<LiteralValue>,
        fallback_old_formula: Option<ASTNode>,
    ) -> VertexId {
        self.try_set_cell_formula_with_old_state(
            cell_ref,
            formula,
            fallback_old_value,
            fallback_old_formula,
        )
        .unwrap_or_else(|_| VertexId::new(0))
    }

    /// Fallible counterpart to [`Self::set_cell_formula_with_old_state`].
    /// Graph-captured old state takes precedence over caller-provided state.
    pub fn try_set_cell_formula_with_old_state(
        &mut self,
        cell_ref: CellRef,
        formula: ASTNode,
        fallback_old_value: Option<LiteralValue>,
        fallback_old_formula: Option<ASTNode>,
    ) -> Result<VertexId, ExcelError> {
        self.set_cell_formula_with_old_state_and_plan(
            cell_ref,
            formula,
            fallback_old_value,
            fallback_old_formula,
            None,
        )
    }

    pub(crate) fn set_cell_formula_with_prepared_plan(
        &mut self,
        cell_ref: CellRef,
        formula: ASTNode,
        fallback_old_value: Option<LiteralValue>,
        fallback_old_formula: Option<ASTNode>,
        ast_id: crate::engine::arena::AstNodeId,
        plan: crate::engine::ingest_pipeline::DependencyPlanRow,
    ) -> VertexId {
        self.set_cell_formula_with_old_state_and_plan(
            cell_ref,
            formula,
            fallback_old_value,
            fallback_old_formula,
            Some((ast_id, plan)),
        )
        .unwrap_or_else(|_| VertexId::new(0))
    }

    fn set_cell_formula_with_old_state_and_plan(
        &mut self,
        cell_ref: CellRef,
        formula: ASTNode,
        fallback_old_value: Option<LiteralValue>,
        fallback_old_formula: Option<ASTNode>,
        prepared: Option<(
            crate::engine::arena::AstNodeId,
            crate::engine::ingest_pipeline::DependencyPlanRow,
        )>,
    ) -> Result<VertexId, ExcelError> {
        let sheet_name = self.graph.sheet_name(cell_ref.sheet_id).to_string();

        // Capture old state before modification (value + formula); fall back
        // to caller-supplied state for anything the graph cannot see.
        let old_id = self.graph.get_vertex_id_for_address(&cell_ref);
        let old_value = old_id
            .and_then(|id| self.graph.get_value(id))
            .or(fallback_old_value);
        let old_formula = old_id
            .and_then(|id| self.get_formula_ast(id))
            .or(fallback_old_formula);

        // Snapshot old spill values before updating, but do not clear or log anything
        // until the fallible binding/admission path succeeds.
        let spill_snapshot =
            old_id.and_then(|id| self.snapshot_spill_for_anchor(id).map(|s| (id, s)));
        let did_spill_clear = spill_snapshot.is_some();

        // VertexEditor operates on internal 0-based coords; graph APIs are 1-based.
        let result = if let Some((ast_id, plan)) = prepared {
            self.graph.set_cell_formula_with_plan(
                &sheet_name,
                cell_ref.coord.row() + 1,
                cell_ref.coord.col() + 1,
                ast_id,
                &plan,
                plan.volatile,
                plan.dynamic,
            )
        } else {
            self.graph.set_cell_formula(
                &sheet_name,
                cell_ref.coord.row() + 1,
                cell_ref.coord.col() + 1,
                formula.clone(),
            )
        };
        match result {
            Ok(summary) => {
                if let Some((anchor, old_spill)) = spill_snapshot {
                    if let Some(logger) = &mut self.change_logger {
                        logger.begin_compound(format!(
                            "SetFormulaWithSpillClear sheet={} row={} col={}",
                            cell_ref.sheet_id,
                            cell_ref.coord.row(),
                            cell_ref.coord.col()
                        ));
                    }
                    self.graph.clear_spill_region(anchor);
                    self.log_change(ChangeEvent::SpillCleared {
                        anchor,
                        old: old_spill,
                    });
                }
                // Log change event
                let change_event = ChangeEvent::SetFormula {
                    addr: cell_ref,
                    old_value,
                    old_formula,
                    new: formula,
                };
                self.log_change(change_event);

                if did_spill_clear && let Some(logger) = &mut self.change_logger {
                    logger.end_compound();
                }

                Ok(summary
                    .affected_vertices
                    .into_iter()
                    .next()
                    .unwrap_or(VertexId::new(0)))
            }
            Err(error) => Err(error),
        }
    }

    // Range operations

    /// Set values for a rectangular range of cells
    pub fn set_range_values(
        &mut self,
        sheet_id: SheetId,
        start_row: u32,
        start_col: u32,
        values: &[Vec<LiteralValue>],
    ) -> Result<RangeSummary, EditorError> {
        let mut summary = RangeSummary::default();

        self.begin_batch();
        // One multi-source dirty propagation for the whole rectangle instead
        // of a full BFS per cell (the loop body cannot error, so the scope
        // always closes before returning).
        self.graph.begin_deferred_dirty();

        for (row_offset, row_values) in values.iter().enumerate() {
            for (col_offset, value) in row_values.iter().enumerate() {
                let row = start_row + row_offset as u32;
                let col = start_col + col_offset as u32;
                let cell_ref = self.graph.make_cell_ref_internal(sheet_id, row, col);
                let existing_id = self.graph.get_vertex_id_for_address(&cell_ref);

                let id = self.set_cell_value(cell_ref, value.clone());
                match existing_id {
                    Some(existing_id) => summary.vertices_updated.push(existing_id),
                    None if id.0 != 0 => summary.vertices_created.push(id),
                    None => {}
                }
                summary.cells_affected += 1;
            }
        }

        let _ = self.graph.end_deferred_dirty();
        self.commit_batch();

        Ok(summary)
    }

    /// Clear all cells in a rectangular range
    pub fn clear_range(
        &mut self,
        sheet_id: SheetId,
        start_row: u32,
        start_col: u32,
        end_row: u32,
        end_col: u32,
    ) -> Result<RangeSummary, EditorError> {
        let mut summary = RangeSummary::default();

        self.begin_batch();

        // Collect vertices in range
        let vertices_in_range: Vec<_> = self
            .graph
            .grid_vertices_in_sheet(sheet_id)
            .filter(|(_, coord)| {
                let row = coord.row();
                let col = coord.col();
                row >= start_row && row <= end_row && col >= start_col && col <= end_col
            })
            .collect();

        let mut vertex_cells = rustc_hash::FxHashSet::default();
        for (id, coord) in vertices_in_range {
            self.remove_vertex(id)?;
            vertex_cells.insert((coord.row(), coord.col()));
            summary.cells_affected += 1;
        }
        // Value and referenced cells (legacy's vertices) leave the used
        // extent too, and count as cleared cells, as their vertices did.
        for (col, r0, r1) in
            self.graph
                .forget_extent_cells(sheet_id, (start_row, end_row), (start_col, end_col))
        {
            summary.cells_affected += (r0..=r1)
                .filter(|&row| !vertex_cells.contains(&(row, col)))
                .count();
        }

        self.commit_batch();

        Ok(summary)
    }

    /// Copy a range to a new location
    pub fn copy_range(
        &mut self,
        sheet_id: SheetId,
        from_start_row: u32,
        from_start_col: u32,
        from_end_row: u32,
        from_end_col: u32,
        to_sheet_id: SheetId,
        to_row: u32,
        to_col: u32,
    ) -> Result<RangeSummary, EditorError> {
        let row_offset = to_row as i32 - from_start_row as i32;
        let col_offset = to_col as i32 - from_start_col as i32;

        let mut summary = RangeSummary::default();
        let mut cell_data = Vec::new();

        // Collect source data
        let vertices_in_range: Vec<_> = self
            .graph
            .grid_vertices_in_sheet(sheet_id)
            .filter(|(_, coord)| {
                let row = coord.row();
                let col = coord.col();
                row >= from_start_row
                    && row <= from_end_row
                    && col >= from_start_col
                    && col <= from_end_col
            })
            .collect();

        for (id, coord) in vertices_in_range {
            let row = coord.row();
            let col = coord.col();

            // Get value or formula
            if let Some(formula) = self.get_formula_ast(id) {
                cell_data.push((
                    row - from_start_row,
                    col - from_start_col,
                    CellData::Formula(formula),
                ));
            } else if let Some(value) = self.graph.get_value(id) {
                cell_data.push((
                    row - from_start_row,
                    col - from_start_col,
                    CellData::Value(value),
                ));
            }
        }

        self.begin_batch();

        // Apply to destination with relative adjustment
        for (row_idx, col_idx, data) in cell_data {
            let dest_row = (to_row as i32 + row_idx as i32) as u32;
            let dest_col = (to_col as i32 + col_idx as i32) as u32;

            match data {
                CellData::Value(value) => {
                    let cell_ref =
                        self.graph
                            .make_cell_ref_internal(to_sheet_id, dest_row, dest_col);

                    if let Some(existing_id) = self.graph.get_vertex_id_for_address(&cell_ref) {
                        self.graph.update_vertex_value(existing_id, value);
                        self.graph.mark_vertex_dirty(existing_id);
                        summary.vertices_updated.push(existing_id);
                    } else {
                        let meta =
                            VertexMeta::new(dest_row, dest_col, to_sheet_id, VertexKind::Cell);
                        let id = self.try_add_vertex(meta)?;
                        self.graph.update_vertex_value(id, value);
                        summary.vertices_created.push(id);
                    }
                }
                CellData::Formula(formula) => {
                    // Adjust relative references in formula
                    let adjuster = RelativeReferenceAdjuster::new(row_offset, col_offset);
                    let adjusted = adjuster.adjust_formula(&formula);

                    let cell_ref =
                        self.graph
                            .make_cell_ref_internal(to_sheet_id, dest_row, dest_col);

                    if let Some(existing_id) = self.graph.get_vertex_id_for_address(&cell_ref) {
                        self.graph.update_vertex_formula(existing_id, adjusted)?;
                        summary.vertices_updated.push(existing_id);
                    } else {
                        let meta = VertexMeta::new(
                            dest_row,
                            dest_col,
                            to_sheet_id,
                            VertexKind::FormulaScalar,
                        );
                        let id = self.try_add_vertex(meta)?;
                        self.graph.update_vertex_formula(id, adjusted)?;
                        summary.vertices_created.push(id);
                    }
                }
            }

            summary.cells_affected += 1;
        }

        self.commit_batch();

        Ok(summary)
    }

    /// Move a range to a new location (copy + clear source)
    pub fn move_range(
        &mut self,
        sheet_id: SheetId,
        from_start_row: u32,
        from_start_col: u32,
        from_end_row: u32,
        from_end_col: u32,
        to_sheet_id: SheetId,
        to_row: u32,
        to_col: u32,
    ) -> Result<RangeSummary, EditorError> {
        let result = self.move_range_impl(
            sheet_id,
            from_start_row,
            from_start_col,
            from_end_row,
            from_end_col,
            to_sheet_id,
            to_row,
            to_col,
        );
        self.graph.authority_end_structural();
        result
    }

    fn move_range_impl(
        &mut self,
        sheet_id: SheetId,
        from_start_row: u32,
        from_start_col: u32,
        from_end_row: u32,
        from_end_col: u32,
        to_sheet_id: SheetId,
        to_row: u32,
        to_col: u32,
    ) -> Result<RangeSummary, EditorError> {
        self.graph.authority_note_structural(true);
        // First copy the range
        let mut summary = self.copy_range(
            sheet_id,
            from_start_row,
            from_start_col,
            from_end_row,
            from_end_col,
            to_sheet_id,
            to_row,
            to_col,
        )?;

        // Then clear the source range
        let clear_summary = self.clear_range(
            sheet_id,
            from_start_row,
            from_start_col,
            from_end_row,
            from_end_col,
        )?;

        summary.cells_moved = clear_summary.cells_affected;

        // Update external references to moved cells
        let row_offset = to_row as i32 - from_start_row as i32;
        let col_offset = to_col as i32 - from_start_col as i32;

        // Find all formulas that reference the moved range
        let all_formula_vertices: Vec<_> = self.graph.vertices_with_formulas().collect();

        let from_sheet_name = self.graph.sheet_name(sheet_id).to_string();
        let to_sheet_name = self.graph.sheet_name(to_sheet_id).to_string();
        let adjuster = MoveReferenceAdjuster::new(
            sheet_id,
            from_sheet_name,
            from_start_row,
            from_start_col,
            from_end_row,
            from_end_col,
            to_sheet_id,
            to_sheet_name,
            row_offset,
            col_offset,
        );

        for formula_id in all_formula_vertices {
            if let Some(formula) = self.get_formula_ast(formula_id) {
                let formula_sheet_id = self.graph.get_vertex_sheet_id(formula_id);
                if let Some(adjusted) = adjuster.adjust_if_references(&formula, formula_sheet_id) {
                    self.graph.update_vertex_formula(formula_id, adjusted)?;
                }
            }
        }

        Ok(summary)
    }

    /// Define a named range
    pub fn define_name(
        &mut self,
        name: &str,
        definition: NamedDefinition,
        scope: NameScope,
    ) -> Result<(), EditorError> {
        self.graph.define_name(name, definition.clone(), scope)?;

        self.log_change(ChangeEvent::DefineName {
            name: name.to_string(),
            scope,
            definition,
        });

        Ok(())
    }

    /// Helper to create definitions from coordinates for a single cell
    pub fn define_name_for_cell(
        &mut self,
        name: &str,
        sheet_name: &str,
        row: u32,
        col: u32,
        scope: NameScope,
    ) -> Result<(), EditorError> {
        let sheet_id = self
            .graph
            .sheet_id(sheet_name)
            .ok_or_else(|| EditorError::InvalidName {
                name: sheet_name.to_string(),
                reason: "Sheet not found".to_string(),
            })?;
        let cell_ref = CellRef::new(sheet_id, Coord::from_excel(row, col, true, true));
        self.define_name(name, NamedDefinition::Cell(cell_ref), scope)
    }

    /// Helper to create definitions from coordinates for a range
    pub fn define_name_for_range(
        &mut self,
        name: &str,
        sheet_name: &str,
        start_row: u32,
        start_col: u32,
        end_row: u32,
        end_col: u32,
        scope: NameScope,
    ) -> Result<(), EditorError> {
        let sheet_id = self
            .graph
            .sheet_id(sheet_name)
            .ok_or_else(|| EditorError::InvalidName {
                name: sheet_name.to_string(),
                reason: "Sheet not found".to_string(),
            })?;
        let start = CellRef::new(
            sheet_id,
            Coord::from_excel(start_row, start_col, true, true),
        );
        let end = CellRef::new(sheet_id, Coord::from_excel(end_row, end_col, true, true));
        let range_ref = crate::reference::RangeRef::new(start, end);
        self.define_name(name, NamedDefinition::Range(range_ref), scope)
    }

    /// Update an existing named range definition
    pub fn update_name(
        &mut self,
        name: &str,
        new_definition: NamedDefinition,
        scope: NameScope,
    ) -> Result<(), EditorError> {
        // Get the old definition for the change log
        let old_definition = self
            .graph
            .resolve_name(
                name,
                match scope {
                    NameScope::Sheet(id) => id,
                    NameScope::Workbook => 0,
                },
            )
            .cloned();

        self.graph
            .update_name(name, new_definition.clone(), scope)?;

        if let Some(old_def) = old_definition {
            self.log_change(ChangeEvent::UpdateName {
                name: name.to_string(),
                scope,
                old_definition: old_def,
                new_definition,
            });
        }

        Ok(())
    }

    /// Delete a named range
    pub fn delete_name(&mut self, name: &str, scope: NameScope) -> Result<(), EditorError> {
        // Capture old definition *before* deletion so undo can restore it.
        let old_def = if self.has_logger() {
            self.graph
                .resolve_name(
                    name,
                    match scope {
                        NameScope::Sheet(id) => id,
                        NameScope::Workbook => 0,
                    },
                )
                .cloned()
        } else {
            None
        };

        self.graph.delete_name(name, scope)?;
        self.log_change(ChangeEvent::DeleteName {
            name: name.to_string(),
            scope,
            old_definition: old_def,
        });

        Ok(())
    }
}

/// Helper enum for cell data
enum CellData {
    Value(LiteralValue),
    Formula(ASTNode),
}

impl<'g> Drop for VertexEditor<'g> {
    fn drop(&mut self) {
        // Ensure batch operations are committed when the editor is dropped
        if self.batch_mode {
            self.commit_batch();
        }
    }
}

#[cfg(test)]
mod tests {
    use super::*;
    use crate::engine::graph::editor::change_log::{ChangeEvent, ChangeLog};
    use crate::reference::Coord;

    fn create_test_graph() -> DependencyGraph {
        DependencyGraph::new()
    }

    #[test]
    fn test_vertex_editor_creation() {
        let mut graph = create_test_graph();
        let editor = VertexEditor::new(&mut graph);
        assert!(!editor.has_logger());
        assert!(!editor.batch_mode);
    }

    #[test]
    fn test_vertex_editor_with_logger() {
        let mut graph = create_test_graph();
        let mut log = ChangeLog::new();
        let editor = VertexEditor::with_logger(&mut graph, &mut log);
        assert!(editor.has_logger());
        assert!(!editor.batch_mode);
    }

    #[test]
    fn test_add_vertex() {
        let mut graph = create_test_graph();
        let mut editor = VertexEditor::new(&mut graph);

        let meta = VertexMeta::new(5, 10, 0, VertexKind::Cell).dirty();
        let vertex_id = editor.add_vertex(meta);

        // Verify vertex was created (simplified check)
        assert!(vertex_id.0 > 0);
    }

    #[test]
    fn test_batch_operations() {
        let mut graph = create_test_graph();
        let mut editor = VertexEditor::new(&mut graph);

        assert!(!editor.batch_mode);
        editor.begin_batch();
        assert!(editor.batch_mode);

        // Add multiple vertices in batch mode
        let meta1 = VertexMeta::new(1, 1, 0, VertexKind::Cell);
        let meta2 = VertexMeta::new(2, 2, 0, VertexKind::Cell);

        let id1 = editor.add_vertex(meta1);
        let id2 = editor.add_vertex(meta2);

        // Add edge between them
        assert!(editor.add_edge(id1, id2));

        editor.commit_batch();
        assert!(!editor.batch_mode);
    }

    #[test]
    fn test_remove_vertex() {
        let mut graph = create_test_graph();
        let mut editor = VertexEditor::new(&mut graph);

        let meta = VertexMeta::new(3, 4, 0, VertexKind::Cell).dirty();
        let vertex_id = editor.add_vertex(meta);

        // Now removal returns Result
        assert!(editor.remove_vertex(vertex_id).is_ok());
    }

    #[test]
    fn test_remove_vertex_clears_spill_registry_for_anchor() {
        let mut graph = create_test_graph();
        let sheet_id = graph.sheet_id_mut("Sheet1");

        // Create anchor vertex at A1 (0-based internal coord 0,0).
        let anchor_cell = CellRef::new(sheet_id, Coord::new(0, 0, true, true));
        let anchor_vid = {
            let mut editor = VertexEditor::new(&mut graph);
            // A formula anchors a spill (value cells have no vertex,
            // decision 27; this used a value cell's vertex).
            editor.set_cell_formula(anchor_cell, formualizer_parse::parser::parse("=0").unwrap())
        };

        let target_cells = vec![
            CellRef::new(sheet_id, Coord::new(0, 0, true, true)),
            CellRef::new(sheet_id, Coord::new(0, 1, true, true)),
            CellRef::new(sheet_id, Coord::new(1, 0, true, true)),
            CellRef::new(sheet_id, Coord::new(1, 1, true, true)),
        ];
        let values = vec![
            vec![LiteralValue::Number(1.0), LiteralValue::Number(2.0)],
            vec![LiteralValue::Number(3.0), LiteralValue::Number(4.0)],
        ];

        graph
            .commit_spill_region_atomic_with_fault(anchor_vid, target_cells.clone(), values, None)
            .unwrap();

        assert!(graph.spill_registry_has_anchor(anchor_vid));
        for cell in &target_cells {
            assert_eq!(
                graph.spill_registry_anchor_for_cell(*cell),
                Some(anchor_vid)
            );
        }

        {
            let mut editor = VertexEditor::new(&mut graph);
            editor.remove_vertex(anchor_vid).unwrap();
        }

        assert!(!graph.spill_registry_has_anchor(anchor_vid));
        for cell in &target_cells {
            assert_eq!(graph.spill_registry_anchor_for_cell(*cell), None);
        }
        assert_eq!(graph.spill_registry_counts(), (0, 0));
    }

    #[test]
    fn test_edge_operations() {
        let mut graph = create_test_graph();
        let mut editor = VertexEditor::new(&mut graph);

        let meta1 = VertexMeta::new(1, 1, 0, VertexKind::Cell);
        let meta2 = VertexMeta::new(2, 2, 0, VertexKind::FormulaScalar);

        let id1 = editor.add_vertex(meta1);
        let id2 = editor.add_vertex(meta2);

        // Add edge
        assert!(editor.add_edge(id1, id2));

        // Prevent self-loop
        assert!(!editor.add_edge(id1, id1));

        // Remove edge
        assert!(editor.remove_edge(id1, id2));
    }

    #[test]
    fn test_set_cell_value() {
        let mut graph = create_test_graph();
        let mut log = ChangeLog::new();

        let cell_ref = CellRef {
            sheet_id: 0,
            coord: Coord::new(2, 3, true, true),
        };
        let value = LiteralValue::Number(42.0);

        let vertex_id = {
            let mut editor = VertexEditor::with_logger(&mut graph, &mut log);
            editor.set_cell_value(cell_ref, value.clone())
        };

        // A value cell has no vertex (decision 27): the sentinel id.
        assert_eq!(vertex_id.0, 0);

        // Verify change log
        assert_eq!(log.len(), 1);
        match &log.events()[0] {
            ChangeEvent::SetValue { addr, new, .. } => {
                assert_eq!(addr.sheet_id, cell_ref.sheet_id);
                assert_eq!(addr.coord.row(), cell_ref.coord.row());
                assert_eq!(addr.coord.col(), cell_ref.coord.col());
                assert_eq!(new, &value);
            }
            _ => panic!("Expected SetValue event"),
        }
    }

    #[test]
    fn test_set_cell_formula() {
        let mut graph = create_test_graph();
        let mut log = ChangeLog::new();

        let cell_ref = CellRef {
            sheet_id: 0,
            coord: Coord::new(1, 1, true, true),
        };

        use formualizer_parse::parser::ASTNodeType;
        let formula = formualizer_parse::parser::ASTNode {
            node_type: ASTNodeType::Literal(LiteralValue::Number(100.0)),
            source_token: None,
            contains_volatile: false,
        };

        let vertex_id = {
            let mut editor = VertexEditor::with_logger(&mut graph, &mut log);
            editor.set_cell_formula(cell_ref, formula.clone())
        };

        // Verify vertex was created (simplified check)
        assert!(vertex_id.0 > 0);

        // Verify change log
        assert_eq!(log.len(), 1);
        match &log.events()[0] {
            ChangeEvent::SetFormula { addr, .. } => {
                assert_eq!(addr.sheet_id, cell_ref.sheet_id);
                assert_eq!(addr.coord.row(), cell_ref.coord.row());
                assert_eq!(addr.coord.col(), cell_ref.coord.col());
            }
            _ => panic!("Expected SetFormula event"),
        }
    }

    #[test]
    fn test_shift_rows() {
        let mut graph = create_test_graph();
        let mut log = ChangeLog::new();

        {
            let mut editor = VertexEditor::with_logger(&mut graph, &mut log);

            // Create vertices at different rows
            let cell1 = CellRef {
                sheet_id: 0,
                coord: Coord::new(5, 1, true, true),
            };
            let cell2 = CellRef {
                sheet_id: 0,
                coord: Coord::new(10, 1, true, true),
            };
            let cell3 = CellRef {
                sheet_id: 0,
                coord: Coord::new(15, 1, true, true),
            };

            editor.set_cell_value(cell1, LiteralValue::Number(1.0));
            editor.set_cell_value(cell2, LiteralValue::Number(2.0));
            editor.set_cell_value(cell3, LiteralValue::Number(3.0));
        }

        // Clear change log to focus on shift operation
        log.clear();

        {
            let mut editor = VertexEditor::with_logger(&mut graph, &mut log);
            // Shift rows starting at row 10, moving down by 2
            editor.shift_rows(0, 10, 2);
        }

        // Verify change log contains the shift operation
        assert_eq!(log.len(), 1);
        match &log.events()[0] {
            ChangeEvent::SetValue { addr, new, .. } => {
                assert_eq!(addr.sheet_id, 0);
                assert_eq!(addr.coord.row(), 10);
                if let LiteralValue::Text(msg) = new {
                    assert!(msg.contains("Row shift"));
                    assert!(msg.contains("start=10"));
                    assert!(msg.contains("delta=2"));
                }
            }
            _ => panic!("Expected SetValue event for row shift"),
        }
    }

    #[test]
    fn test_shift_columns() {
        let mut graph = create_test_graph();
        let mut log = ChangeLog::new();

        {
            let mut editor = VertexEditor::with_logger(&mut graph, &mut log);

            // Create vertices at different columns
            let cell1 = CellRef {
                sheet_id: 0,
                coord: Coord::new(1, 5, true, true),
            };
            let cell2 = CellRef {
                sheet_id: 0,
                coord: Coord::new(1, 10, true, true),
            };

            editor.set_cell_value(cell1, LiteralValue::Number(1.0));
            editor.set_cell_value(cell2, LiteralValue::Number(2.0));
        }

        // Clear change log
        log.clear();

        {
            let mut editor = VertexEditor::with_logger(&mut graph, &mut log);
            // Shift columns starting at col 8, moving right by 3
            editor.shift_columns(0, 8, 3);
        }

        // Verify change log
        assert_eq!(log.len(), 1);
        match &log.events()[0] {
            ChangeEvent::SetValue { addr, new, .. } => {
                assert_eq!(addr.sheet_id, 0);
                assert_eq!(addr.coord.col(), 8);
                if let LiteralValue::Text(msg) = new {
                    assert!(msg.contains("Column shift"));
                    assert!(msg.contains("start=8"));
                    assert!(msg.contains("delta=3"));
                }
            }
            _ => panic!("Expected SetValue event for column shift"),
        }
    }

    #[test]
    fn test_move_vertex() {
        let mut graph = create_test_graph();
        let mut editor = VertexEditor::new(&mut graph);

        let meta = VertexMeta::new(5, 10, 0, VertexKind::Cell);
        let vertex_id = editor.add_vertex(meta);

        // Move vertex returns Result
        assert!(editor.move_vertex(vertex_id, GridAddr::new(8, 12)).is_ok());

        // Moving to same position should work
        assert!(editor.move_vertex(vertex_id, GridAddr::new(8, 12)).is_ok());
    }

    #[test]
    fn test_vertex_meta_builder() {
        let meta = VertexMeta::new(1, 2, 3, VertexKind::FormulaScalar)
            .dirty()
            .volatile()
            .with_flags(0x08);

        assert_eq!(meta.coord.row(), 1);
        assert_eq!(meta.coord.col(), 2);
        assert_eq!(meta.sheet_id, 3);
        assert_eq!(meta.kind, VertexKind::FormulaScalar);
        assert_eq!(meta.flags, 0x08); // Last with_flags call overwrites previous flags
    }

    #[test]
    fn test_change_log_management() {
        let mut graph = create_test_graph();
        let mut log = ChangeLog::new();

        {
            let mut editor = VertexEditor::with_logger(&mut graph, &mut log);
            let cell_ref = CellRef {
                sheet_id: 0,
                coord: Coord::new(0, 0, true, true),
            };
            editor.set_cell_value(cell_ref, LiteralValue::Number(1.0));
            editor.set_cell_value(cell_ref, LiteralValue::Number(2.0));
        }

        assert_eq!(log.len(), 2);

        log.clear();
        assert_eq!(log.len(), 0);
    }

    #[test]
    fn test_editor_drop_commits_batch() {
        let mut graph = create_test_graph();
        {
            let mut editor = VertexEditor::new(&mut graph);
            editor.begin_batch();

            let meta = VertexMeta::new(1, 1, 0, VertexKind::Cell);
            editor.add_vertex(meta);

            // Editor will be dropped here, should commit batch
        }

        // If we reach here without hanging, the batch was properly committed
    }
}