feat(notebook-doc): fractional indexing for cell ordering by rgbkrk · Pull Request #694 · nteract/desktop

rgbkrk · 2026-03-11T04:13:54Z

Cells are now stored in an Automerge Map keyed by cell ID instead of an ordered List. Each cell has a position field — a fractional index hex string that determines display order. Moving a cell updates one field instead of delete + re-insert.

Schema change

# Before (schema_version: 1)
ROOT/cells/              ← List
  [0]/ { id, cell_type, source, ... }
  [1]/ { id, cell_type, source, ... }

# After (schema_version: 2)
ROOT/cells/              ← Map keyed by cell ID
  "cell-abc"/ { id, cell_type, position: "80", source, ... }
  "cell-def"/ { id, cell_type, position: "C0", source, ... }

For notebooks opened from .ipynb, the doc is rebuilt from disk — no migration needed. For persisted untitled notebooks (Automerge cache), load_or_create detects schema_version < 2 and runs migrate_v1_to_v2() which converts the List to a Map with fractional positions, preserving all cell content.

What changed

notebook-doc — Core schema change. cells is ObjType::Map, schema_version bumped to 2. All cell operations use direct map key access (O(1) lookup, no more find_cell_index linear scan). New add_cell_after, move_cell, compute_position methods. get_cells() sorts by position with cell ID tiebreaker. migrate_v1_to_v2() for persisted doc upgrades. 110 tests including migration and fractional indexing tests.

runtimed — NotebookSyncClient converted from List to Map schema. move_cell plumbed through SyncCommand → NotebookSyncClient → NotebookSyncHandle. Bulk load in notebook_sync_server.rs generates positions incrementally (O(n), not O(n²)). All 17 integration tests pass.

runtimed-wasm — add_cell_after(cell_id, cell_type, after_cell_id) and move_cell(cell_id, after_cell_id) exposed on NotebookHandle. JsCell gains position field. WASM rebuilt.

runtimed-py — Cell.position field exposed. move_cell(cell_id, after_cell_id) on both Session and AsyncSession.

Frontend — CellSnapshot gains position field. addCell switched from index-based handle.add_cell(idx) to semantic handle.add_cell_after(cellId, cellType, afterCellId). New moveCell function in useAutomergeNotebook hook. WASM .d.ts declarations updated.

Concurrent move semantics

Two users moving the same cell simultaneously: Automerge LWW on the position scalar means one wins deterministically. Both users converge to the same final position after sync. Cells with identical positions are tiebroken by cell ID for deterministic ordering across peers.

Dependencies

Uses loro_fractional_index = "=1.6.0" from crates.io (Loro project's fractional indexing)
Depends on PR feat!: bump all artifacts to v2.0.0, clean up release flow #692 for schema_version field in the Automerge doc root

PR submitted by @rgbkrk's agent Quill, via Zed

BREAKING CHANGE: Schema version bumped to 2. Cells are now stored in a Map keyed by cell ID instead of a List. Each cell has a 'position' field (fractional index hex string) that determines ordering. Key changes: - CellSnapshot now includes position field - NotebookDoc::new() creates Map-based cells structure - add_cell_after() for semantic insertion (after specified cell) - move_cell() updates position field only (no delete/re-insert) - delete_cell() is now O(1) direct map delete - get_cells() sorts by position before returning Uses loro_fractional_index crate (pinned to =1.6.0) for position generation. Positions are generated incrementally during bulk loads to avoid O(n²). Concurrent move semantics: LWW on position scalar - both users see same final position after sync (coordination problem, not data integrity issue).

Updates all cell manipulation methods in NotebookSyncClient from the old List schema (cells_list_id → find_cell_index → cell_at_index) to Map-based lookups (cells_map_id → cell_obj_id). Cell add/add_with_source now compute fractional positions from the index parameter. Delete is now O(1) by key instead of requiring a linear scan. Also wires up the position field in runtimed-py Cell::from_snapshot.

…t hooks - Add position field to CellSnapshot interface in materialize-cells.ts - Declare add_cell_after and move_cell on NotebookHandle in .d.ts - Add position field to JsCell in .d.ts - Switch addCell from index-based handle.add_cell to handle.add_cell_after (eliminates the index→afterCellId round-trip) - Add moveCell function to useAutomergeNotebook hook - Remove unused getNotebookCellsSnapshot import

Adds MoveCell to SyncCommand, NotebookSyncClient, and NotebookSyncHandle so the full chain works: Python Session.move_cell() → NotebookSyncHandle.move_cell() → SyncCommand::MoveCell → NotebookSyncClient.move_cell() → Automerge position update + sync to daemon Both Session (sync) and AsyncSession (async) get the method. Same implementation as WASM — one source of truth in notebook-doc.

Copilot

Pull request overview

Implements schema v2 cell ordering via fractional indexing: cells are stored in an Automerge Map keyed by cell ID, with a per-cell position hex string determining display order. This reduces move operations to a single-field update and avoids list delete/reinsert churn across sync layers.

Changes:

notebook-doc: switch cells from List → Map, add position, implement add_cell_after/move_cell, and sort get_cells() by position.
runtimed: adapt sync server/client to the map schema and generate positions incrementally during notebook loads; add a MoveCell sync command.
Bindings/UI: expose position and new semantic APIs in WASM + Python, update TS declarations and frontend hook to use add_cell_after/move_cell.

Reviewed changes

Copilot reviewed 12 out of 14 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
crates/runtimed/src/notebook_sync_server.rs	Generates fractional positions during ipynb parsing/streaming load; passes positions into `add_cell_full`.
crates/runtimed/src/notebook_sync_client.rs	Migrates client-side mutations from list indices to map keys + positions; adds `MoveCell` command/handler.
crates/runtimed/Cargo.toml	Adds workspace dependency on `loro_fractional_index`.
crates/runtimed-wasm/src/lib.rs	Exposes `position` on `JsCell`; adds `add_cell_after` and `move_cell` APIs.
crates/runtimed-py/src/session.rs	Adds synchronous `move_cell` API returning new position string.
crates/runtimed-py/src/async_session.rs	Adds async `move_cell` coroutine API returning new position string.
crates/runtimed-py/src/output.rs	Exposes `Cell.position` in Python object model.
crates/notebook-doc/src/lib.rs	Core schema v2 change: map-based cells + fractional position ordering; new add/move helpers and updated tests.
crates/notebook-doc/Cargo.toml	Adds `loro_fractional_index` dependency.
apps/notebook/src/wasm/runtimed-wasm/runtimed_wasm.d.ts	Updates TS declarations for `position`, `add_cell_after`, and `move_cell`.
apps/notebook/src/lib/materialize-cells.ts	Extends `CellSnapshot` type with `position`.
apps/notebook/src/hooks/useAutomergeNotebook.ts	Switches insertion to `add_cell_after`; adds `moveCell` hook API with optimistic reorder.
Cargo.toml	Pins workspace dependency `loro_fractional_index = "=1.6.0"`.
Cargo.lock	Adds lock entries for `loro_fractional_index`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Copilot · 2026-03-11T04:21:35Z

crates/notebook-doc/src/lib.rs

+        // Convert index to after_cell_id
+        let cells = self.get_cells();
+        let after_cell_id = if index == 0 {
+            None
+        } else {
+            cells.get(index.saturating_sub(1)).map(|c| c.id.as_str())


NotebookDoc::add_cell converts an index to after_cell_id using cells.get(index - 1), but it never clamps index to cells.len(). If index is greater than the current cell count (common for the legacy index-based API), cells.get(..) returns None and the code inserts at the beginning instead of the end. Clamp the index to cells.len() (or explicitly treat index >= len as "insert at end" by using the last cell as after_cell_id).

Suggested change

// Convert index to after_cell_id

let cells = self.get_cells();

let after_cell_id = if index == 0 {

None

} else {

cells.get(index.saturating_sub(1)).map(|c| c.id.as_str())

// Convert index to after_cell_id. Indices greater than the current cell

// count are treated as "insert at end" by clamping to cells.len().

let cells = self.get_cells();

let clamped = index.min(cells.len());

let after_cell_id = if clamped == 0 {

None

} else {

cells.get(clamped - 1).map(|c| c.id.as_str())

Copilot · 2026-03-11T04:21:35Z

crates/notebook-doc/src/lib.rs

+                            &cells[i].position,
+                        ))
+                    }
+                    None => FractionalIndex::default(),


compute_position (used by both add_cell_after and move_cell) silently falls back to FractionalIndex::default() when after_cell_id is provided but not found. That makes typos / stale IDs look like a successful insert/move while actually picking an arbitrary position (and can create duplicate positions). Consider returning an error when after_cell_id is Some(..) but missing, or explicitly defining a deterministic fallback (e.g. treat as end) and documenting it.

Suggested change

None => FractionalIndex::default(),

None => {

// `after_cell_id` not found: insert at end (after the last cell)

cells

.last()

.map(|c| {

FractionalIndex::new_after(

&FractionalIndex::from_hex_string(&c.position),

)

})

.unwrap_or_default()

}

Adds migrate_v1_to_v2() to NotebookDoc: reads cells from the old List schema, recreates them in a Map with fractional positions, preserving source, outputs, execution_count, and metadata. load_or_create detects stale schema versions and migrates in-place, so persisted untitled notebooks survive the schema change. Rebuilds WASM artifacts with the new fractional indexing code. Adds getrandom 0.2 js feature for WASM compatibility with loro_fractional_index's rand dependency. 5 migration tests: empty doc, cells with content, execution count and outputs preservation, v2 no-op, and post-migration operations.

Two concurrent inserts at the same position (e.g., both get FractionalIndex::default() in an empty notebook) produce identical position strings. Without a tiebreaker, display order depends on Map iteration order which can differ between peers. Cell ID is deterministic everywhere.

…after_cell_id Addresses Copilot review feedback: - add_cell now clamps index to cells.len() so out-of-bounds indices insert at the end instead of the beginning - compute_position falls back to inserting at the end (after last cell) when after_cell_id is provided but not found, instead of silently using the default midpoint position

Copilot

Pull request overview

Copilot reviewed 15 out of 19 changed files in this pull request and generated 10 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Copilot · 2026-03-11T04:47:25Z

crates/notebook-doc/src/lib.rs

+        // Delete old cells key
+        let _ = self.doc.delete(automerge::ROOT, "cells");
+
+        // Create new Map
+        let cells_map = self
+            .doc
+            .put_object(automerge::ROOT, "cells", ObjType::Map)?;


migrate_v1_to_v2 deletes and recreates ROOT.cells unconditionally, even when the existing cells value is not a v1 List. If this method is accidentally called on an already-v2 doc, it will drop all existing cells. Consider making this migration idempotent by returning early when old_cells_id is None (or when schema_version >= SCHEMA_VERSION), and only deleting/replacing cells when a v1 List is actually present.

Copilot · 2026-03-11T04:47:26Z

crates/runtimed/src/notebook_sync_server.rs

+            // Generate position incrementally (O(1) per cell, not O(n²))
+            let position = match &prev_position {
+                None => FractionalIndex::default(),
+                Some(prev) => FractionalIndex::new_after(prev),
+            };
+            let position_str = position.to_string();
+            prev_position = Some(position);


parse_cells_from_ipynb now generates fractional position values, but downstream code paths (e.g. apply_ipynb_changes / load_notebook_from_disk) still rebuild docs via doc.add_cell(index, ...) rather than using the precomputed cell.position with add_cell_full. That means the extra position computation here doesn’t avoid the O(n²) insertion cost during bulk rebuilds. Consider switching those call sites to add_cell_full(..., &cell.position, ...) (or stop generating positions here if they’re unused).

Suggested change

// Generate position incrementally (O(1) per cell, not O(n²))

let position = match &prev_position {

None => FractionalIndex::default(),

Some(prev) => FractionalIndex::new_after(prev),

};

let position_str = position.to_string();

prev_position = Some(position);

// Use the linear index as the position; fractional positions are

// not currently consumed by downstream bulk rebuild paths.

let position_str = index.to_string();

Copilot · 2026-03-11T04:47:26Z

crates/runtimed/src/notebook_sync_client.rs

+            let prev_pos = FractionalIndex::from_hex_string(&sorted_cells[clamped - 1].position);
+            if clamped < sorted_cells.len() {
+                let next_pos = FractionalIndex::from_hex_string(&sorted_cells[clamped].position);
+                FractionalIndex::new(Some(&prev_pos), Some(&next_pos)).unwrap_or_default()


If FractionalIndex::new(Some(prev), Some(next)) fails, unwrap_or_default() will silently fall back to default() (often "80"), which can create duplicate positions and reorder the inserted cell unexpectedly. Prefer a deterministic fallback like new_after(prev_pos) (or match NotebookDoc::compute_position’s new_between(...).unwrap_or_else(|| new_after(...)) pattern).

Suggested change

FractionalIndex::new(Some(&prev_pos), Some(&next_pos)).unwrap_or_default()

FractionalIndex::new(Some(&prev_pos), Some(&next_pos))

.unwrap_or_else(|| FractionalIndex::new_after(&prev_pos))

Copilot · 2026-03-11T04:47:26Z

crates/runtimed/src/notebook_sync_client.rs

+            let prev_pos = FractionalIndex::from_hex_string(&sorted_cells[clamped - 1].position);
+            if clamped < sorted_cells.len() {
+                let next_pos = FractionalIndex::from_hex_string(&sorted_cells[clamped].position);
+                FractionalIndex::new(Some(&prev_pos), Some(&next_pos)).unwrap_or_default()


Same issue as above: unwrap_or_default() on FractionalIndex::new(Some(prev), Some(next)) can produce a non-monotonic/duplicate position when the between-generation fails. Use a deterministic fallback (e.g. new_after(prev_pos) or new_before(next_pos)) instead of defaulting.

Suggested change

FractionalIndex::new(Some(&prev_pos), Some(&next_pos)).unwrap_or_default()

FractionalIndex::new(Some(&prev_pos), Some(&next_pos))

.unwrap_or_else(|| FractionalIndex::new_after(&prev_pos))

Copilot · 2026-03-11T04:47:26Z

crates/runtimed/src/notebook_sync_client.rs

+                sorted_cells
+                    .first()
+                    .map(|c| {
+                        FractionalIndex::new_before(&FractionalIndex::from_hex_string(&c.position))
+                    })
+                    .unwrap_or_default()
+            }
+            Some(after_id) => {
+                let idx = sorted_cells.iter().position(|c| c.id == after_id);
+                match idx {
+                    Some(i) if i + 1 < sorted_cells.len() => {
+                        let prev = FractionalIndex::from_hex_string(&sorted_cells[i].position);
+                        let next = FractionalIndex::from_hex_string(&sorted_cells[i + 1].position);
+                        FractionalIndex::new(Some(&prev), Some(&next)).unwrap_or_default()
+                    }
+                    Some(i) => FractionalIndex::new_after(&FractionalIndex::from_hex_string(
+                        &sorted_cells[i].position,
+                    )),
+                    None => FractionalIndex::default(),


In move_cell, falling back to unwrap_or_default() when generating a between-position can move the cell to default() (often "80"), potentially jumping it away from the requested area and colliding with existing positions. Prefer the same fallback strategy used in notebook-doc (try-between, else new_after(prev)), and consider handling after_cell_id not found as a deterministic start/end fallback rather than default().

Suggested change

sorted_cells

.first()

.map(|c| {

FractionalIndex::new_before(&FractionalIndex::from_hex_string(&c.position))

})

.unwrap_or_default()

}

Some(after_id) => {

let idx = sorted_cells.iter().position(|c| c.id == after_id);

match idx {

Some(i) if i + 1 < sorted_cells.len() => {

let prev = FractionalIndex::from_hex_string(&sorted_cells[i].position);

let next = FractionalIndex::from_hex_string(&sorted_cells[i + 1].position);

FractionalIndex::new(Some(&prev), Some(&next)).unwrap_or_default()

}

Some(i) => FractionalIndex::new_after(&FractionalIndex::from_hex_string(

&sorted_cells[i].position,

)),

None => FractionalIndex::default(),

if sorted_cells.is_empty() {

// No existing cells: use the default initial position

FractionalIndex::default()

} else {

let first_pos =

FractionalIndex::from_hex_string(&sorted_cells[0].position);

// Try to create a position before the first cell; if that fails,

// fall back to appending after the last cell to avoid jumping to

// a far-away default position.

FractionalIndex::new_before(&first_pos).unwrap_or_else(|| {

let last_pos = FractionalIndex::from_hex_string(

&sorted_cells[sorted_cells.len() - 1].position,

);

FractionalIndex::new_after(&last_pos)

})

}

}

Some(after_id) => {

let idx = sorted_cells.iter().position(|c| c.id == after_id);

match idx {

Some(i) if i + 1 < sorted_cells.len() => {

let prev = FractionalIndex::from_hex_string(&sorted_cells[i].position);

let next =

FractionalIndex::from_hex_string(&sorted_cells[i + 1].position);

// Prefer a between-position; if that fails, fall back to a

// position after `prev`, matching the strategy in notebook-doc.

FractionalIndex::new(Some(&prev), Some(&next))

.unwrap_or_else(|| FractionalIndex::new_after(&prev))

}

Some(i) => FractionalIndex::new_after(&FractionalIndex::from_hex_string(

&sorted_cells[i].position,

)),

None => {

// If the requested after_cell_id is not found, fall back

// deterministically to appending at the end (or default if

// there are no cells), rather than using a fixed default

// position that may collide with existing ones.

if let Some(last) = sorted_cells.last() {

let last_pos =

FractionalIndex::from_hex_string(&last.position);

FractionalIndex::new_after(&last_pos)

} else {

FractionalIndex::default()

}

}

Copilot · 2026-03-11T04:47:27Z

apps/notebook/src/wasm/runtimed-wasm/runtimed_wasm.js

+     * - `after_cell_id = None` → insert at the beginning
+     * - `after_cell_id = Some(id)` → insert after that cell


The JSDoc here uses Rust Option terminology (None / Some(id)), but the actual JS API uses null (or omitted) vs string (see the @param {string | null} type). Consider updating the bullets to after_cell_id = null / after_cell_id = "id" to avoid confusing JS/TS consumers.

Suggested change

* - `after_cell_id = None` → insert at the beginning

* - `after_cell_id = Some(id)` → insert after that cell

* - `after_cell_id = null` → insert at the beginning

* - `after_cell_id = "id"` → insert after that cell

Copilot · 2026-03-11T04:47:27Z

crates/notebook-doc/src/lib.rs

+                        // after_cell_id not found: insert at end (after the last cell)
+                        cells
+                            .last()
+                            .map(|c| {
+                                FractionalIndex::new_after(&FractionalIndex::from_hex_string(


When after_cell_id is provided but not found, compute_position falls back to FractionalIndex::default(). default() (often "80") isn’t guaranteed to be before the first cell or after the last, so a stale/invalid after_cell_id can move/insert a cell to an unexpected spot. Consider either returning an error for unknown after_cell_id, or using a deterministic fallback (e.g., treat as insert-at-start like None, or insert-at-end).

Suggested change

// after_cell_id not found: insert at end (after the last cell)

cells

.last()

.map(|c| {

FractionalIndex::new_after(&FractionalIndex::from_hex_string(

// after_cell_id not found: treat as insert at start (same as `None`)

cells

.first()

.map(|c| {

FractionalIndex::new_before(&FractionalIndex::from_hex_string(

Copilot · 2026-03-11T04:47:27Z

crates/notebook-doc/src/lib.rs

+        // Convert index to after_cell_id. Indices greater than the current cell
+        // count are treated as "insert at end" by clamping to cells.len().
+        let cells = self.get_cells();
+        let clamped = index.min(cells.len());
+        let after_cell_id = if clamped == 0 {
+            None
+        } else {


add_cell’s index→after_cell_id conversion doesn’t clamp indexes beyond the current cell count. If index > cells.len(), cells.get(index.saturating_sub(1)) returns None, which makes after_cell_id None and inserts at the beginning—different from the previous v1 behavior where out-of-range indexes were clamped to the end. Consider clamping index to cells.len() first, then deriving after_cell_id from the clamped index.

Copilot · 2026-03-11T04:47:27Z

apps/notebook/src/wasm/runtimed-wasm/runtimed_wasm.js

+     * - `after_cell_id = None` → move to the beginning
+     * - `after_cell_id = Some(id)` → move after that cell


Same doc issue: this JSDoc describes after_cell_id as None / Some(id), but callers pass null/string in JS. Updating the wording will keep the JS-facing docs accurate.

Suggested change

* - `after_cell_id = None` → move to the beginning

* - `after_cell_id = Some(id)` → move after that cell

* - `after_cell_id === null` → move to the beginning

* - `after_cell_id` is a cell id string → move after that cell

Copilot · 2026-03-11T04:47:28Z

crates/runtimed-py/src/session.rs

+    ///     after_cell_id: Place the cell after this cell ID. None means move to the start.
+    ///
+    /// Returns:
+    ///     The new fractional position string.


Session.move_cell takes after_cell_id: Option<&str> but doesn’t declare a #[pyo3(signature = (cell_id, after_cell_id=None))] like AsyncSession::move_cell. Without an explicit default, Python callers will likely have to pass after_cell_id positionally every time (even when they want the default “move to start”), which is inconsistent with the async API and the docstring. Consider adding the pyo3 signature defaulting after_cell_id to None.

Suggested change

/// The new fractional position string.

/// The new fractional position string.

#[pyo3(signature = (cell_id, after_cell_id=None))]

- Make migrate_v1_to_v2 idempotent (early return if schema >= current) - Replace unwrap_or_default() with new_after(prev) fallback in NotebookSyncClient position computation (3 call sites) - Fix move_cell fallbacks in NotebookSyncClient: missing after_cell_id falls back to end, empty notebook uses default - Add #[pyo3(signature)] to Session.move_cell for consistent Python API - Fix WASM doc comments: use JS null/string instead of Rust None/Some - Rebuild WASM artifacts

Copilot

Pull request overview

Copilot reviewed 15 out of 19 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Copilot · 2026-03-11T04:55:35Z

crates/notebook-doc/src/lib.rs

+        };
+
+        // Delete old cells key
+        let _ = self.doc.delete(automerge::ROOT, "cells");


migrate_v1_to_v2 unconditionally deletes the ROOT/cells key before creating a new Map. If schema_version < 2 but cells is already a Map (or any non-List object), old_cells_id will be None, old_cells will be empty, and this delete+recreate will wipe all existing cells. Consider only deleting/recreating when old_cells_id.is_some(), otherwise leave the existing cells object intact (or just bump schema_version).

Copilot · 2026-03-11T04:55:36Z

crates/notebook-doc/src/lib.rs

+                        let version = loaded.schema_version().unwrap_or(1);
+                        if version < SCHEMA_VERSION {
+                            info!(
+                                "[notebook-doc] Migrating schema v{} → v{} at {:?} for {}",
+                                version, SCHEMA_VERSION, path, notebook_id
+                            );
+                            if let Err(e) = loaded.migrate_v1_to_v2() {
+                                warn!(
+                                    "[notebook-doc] Migration failed for {}: {}. Creating fresh doc.",
+                                    notebook_id, e
+                                );
+                            } else {
+                                info!("[notebook-doc] Migration complete for {}", notebook_id);
+                                return loaded;


PR description says "No migration needed", but load_or_create now performs an on-load schema migration for persisted Automerge docs. Either update the PR description to reflect this behavior, or remove/disable migration if the intended contract is truly "rebuild from .ipynb only".

Copilot · 2026-03-11T04:55:36Z

crates/notebook-doc/src/lib.rs

+        // Calling migrate on a v2 doc should succeed but not lose data.
+        // The List lookup returns None, so it creates an empty Map and
+        // overwrites — but we guard against this at the call site.
+        // This test verifies the call site guard is needed.
+        let result = doc.migrate_v1_to_v2();
+        // The migration finds no List, creates empty Map, loses cells.
+        // This is why load_or_create only calls migrate when version < SCHEMA_VERSION.
+        assert!(result.is_ok());
+
+        // Verify the call-site version check is essential:
+        // A v2 doc should never reach migrate_v1_to_v2 in production.
+        let _ = cells_before; // acknowledged


This test's comments claim calling migrate_v1_to_v2() on a v2 doc will wipe cells, but the migration function currently returns early when schema_version >= SCHEMA_VERSION, so it should be a true no-op. To avoid misleading future readers, either assert that cells_before == doc.get_cells() after the call, or adjust the test/comments to match the actual behavior (and/or make migration safe even when invoked on non-List cells).

Suggested change

// Calling migrate on a v2 doc should succeed but not lose data.

// The List lookup returns None, so it creates an empty Map and

// overwrites — but we guard against this at the call site.

// This test verifies the call site guard is needed.

let result = doc.migrate_v1_to_v2();

// The migration finds no List, creates empty Map, loses cells.

// This is why load_or_create only calls migrate when version < SCHEMA_VERSION.

assert!(result.is_ok());

// Verify the call-site version check is essential:

// A v2 doc should never reach migrate_v1_to_v2 in production.

let _ = cells_before; // acknowledged

// Calling migrate on a v2 doc should succeed and be a true no-op.

// The migration function returns early when schema_version >= SCHEMA_VERSION,

// so it must not modify existing cells on already-v2 documents.

let result = doc.migrate_v1_to_v2();

assert!(result.is_ok());

// Verify that a v2 doc is unchanged by migrate_v1_to_v2.

assert_eq!(cells_before, doc.get_cells());

Only deletes/recreates cells if a v1 List is actually found. If cells is missing or already a Map, just bumps schema_version. Fixes the v2 no-op test to assert cells are preserved.

rgbkrk added 5 commits March 10, 2026 20:46

wip

be1b110

rgbkrk requested a review from Copilot March 11, 2026 04:14

Copilot started reviewing on behalf of rgbkrk March 11, 2026 04:14 View session

rgbkrk marked this pull request as draft March 11, 2026 04:15

Copilot AI reviewed Mar 11, 2026

View reviewed changes

rgbkrk added 3 commits March 10, 2026 21:21

fix(test): add position field to CellSnapshot test fixtures

fde77d7

rgbkrk marked this pull request as ready for review March 11, 2026 04:38

rgbkrk requested a review from Copilot March 11, 2026 04:38

Copilot started reviewing on behalf of rgbkrk March 11, 2026 04:38 View session

Copilot AI reviewed Mar 11, 2026

View reviewed changes

rgbkrk requested a review from Copilot March 11, 2026 04:50

Copilot started reviewing on behalf of rgbkrk March 11, 2026 04:50 View session

Copilot AI reviewed Mar 11, 2026

View reviewed changes

fix(notebook-doc): make migrate_v1_to_v2 safe when no List exists

023b57d

Only deletes/recreates cells if a v1 List is actually found. If cells is missing or already a Map, just bumps schema_version. Fixes the v2 no-op test to assert cells are preserved.

rgbkrk enabled auto-merge (squash) March 11, 2026 05:07

rgbkrk merged commit c4816a3 into main Mar 11, 2026
14 checks passed

rgbkrk deleted the quill/fractional-indexing branch March 11, 2026 05:34

rgbkrk mentioned this pull request Mar 11, 2026

Release plan: nteract 2.0.0 #704

Open

48 tasks

	FractionalIndex::new(Some(&prev_pos), Some(&next_pos)).unwrap_or_default()
	FractionalIndex::new(Some(&prev_pos), Some(&next_pos))
	.unwrap_or_else(\|\| FractionalIndex::new_after(&prev_pos))

		* - `after_cell_id = None` → insert at the beginning
		* - `after_cell_id = Some(id)` → insert after that cell

		* - `after_cell_id = None` → move to the beginning
		* - `after_cell_id = Some(id)` → move after that cell

	/// The new fractional position string.
	/// The new fractional position string.
	#[pyo3(signature = (cell_id, after_cell_id=None))]

Conversation

rgbkrk commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Schema change

What changed

Concurrent move semantics

Dependencies

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rgbkrk commented Mar 11, 2026 •

edited

Loading