fix(data-editor): don't crash appending a row to an empty row-oriented editor - #10651
fix(data-editor): don't crash appending a row to an empty row-oriented editor#10651winklemad wants to merge 2 commits into
Conversation
…d editor Appending a positional edit to an empty row-oriented data editor raised IndexError because the new row's columns were read from data[0], which does not exist when there are no rows yet. Derive the columns from the schema when available and always include the edited column, mirroring the column-oriented path. This also fixes the remove-all-then-add edit replay.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
All contributors have signed the CLA ✍️ ✅ |
There was a problem hiding this comment.
Pull request overview
Fixes a crash when appending the first row in an empty row-oriented data editor.
Changes:
- Adds fallback logic for empty row-oriented appends.
- Adds regression tests for empty and remove-all-then-add scenarios.
- Critical issue: replay via
_convert_valuecan still drop existing columns because no schema is preserved or passed.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Summary |
|---|---|
tests/_plugins/ui/_impl/test_data_editor.py |
Adds regression coverage for empty-row appends. |
marimo/_plugins/ui/_impl/data_editor.py |
Updates empty-row append handling; requires preserving or passing the original editor schema and testing _convert_value. |
Suppressed comments (3)
marimo/_plugins/ui/_impl/data_editor.py:471
- This still appends only one row for every
rowIdx >= len(data). For example, an empty row-oriented input withrowIdx: 1reachesdata[1]after this append and raisesIndexError; the column-oriented path extends through the requested index. Extend byrowIdx - len(data) + 1rows (or reject non-contiguous indices) so the guarded branch is correct for all out-of-range positional edits.
new_row: dict[str, Any] = {col: None for col in columns}
new_row.setdefault(edit["columnId"], None)
data.append(new_row)
marimo/_plugins/ui/_impl/data_editor.py:471
- This fallback only adds the column from the first edit. If one edit batch contains multiple cells for the first new row, the next column enters the existing-row path and
data[0][columnId]raisesKeyError. The UI's row-add replay emits one positional edit per column, so remove-all-then-add with a multi-column row still crashes; ensure the target row contains the column before reading/updating it and add that regression case.
new_row: dict[str, Any] = {col: None for col in columns}
new_row.setdefault(edit["columnId"], None)
data.append(new_row)
marimo/_plugins/ui/_impl/data_editor.py:466
nw.Schemaexposes its column names throughnames()(as used elsewhere in this codebase), notkeys(). This makes the new schema fallback raiseAttributeErrorbefore appending, so the added_with_schemaregression test fails; useschema.names()here.
columns = list(schema.keys())
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| elif schema is not None: | ||
| columns = list(schema.keys()) |
There was a problem hiding this comment.
@winklemad if the PR is ready; please address this comment, before i start a review
The row-oriented replay only fixed the narrowest empty-append crash and still lost data: after a remove-all, a rebuilt row kept only the edited column; a multi-column rebuild hit KeyError on the second column (it read data[0][col]); and a non-contiguous rowIdx past the end raised IndexError. Capture the columns before edits run (so a remove-all doesn't lose them), thread them into the positional handler, extend the data through the requested index, and read the original value with .get. Also use nw.Schema.names() (the idiomatic narwhals accessor). Fixes the dropped-column / KeyError / IndexError cases and covers them via the helper and end-to-end through _convert_value.
|
Thanks — Copilot's critical concern was right, and the first pass only fixed the narrowest case. Verified against the real replay path (
Fixed in 4cd1e13:
Tests: fixed the assertion that pinned the dropped-column behavior, and added multi-column, non-contiguous, and an end-to-end @kirangadhave this should be ready for your review now. |
|
Thanks @winklemad , closing this in favour of #10662 |
|
Thanks @Light2Dark — no worries at all. #10662's broader approach of making the whole replay path robust to structural changes is the better factoring than patching the row-oriented handler in isolation, so it makes sense to land that instead. Glad the four-failure-mode breakdown (dropped column / KeyError / IndexError) was useful as a checklist for it. Happy to give #10662 a review if that would help. |
This pull request was authored by a coding agent.
📝 Summary
Closes #10650
_apply_positional_edit_row_orientedcrashed withIndexErrorwhen appending a new row to an empty row-oriented editor: it built the new row fromdata[0], which raises on an empty list. This is reachable from the UI by deleting all rows and then adding one.When there is no existing row to read column names from, this falls back to the editor's schema (and always includes the edited column), so appending the first row works. The column-oriented path already handled the empty case.
📋 Pre-Review Checklist
✅ Merge Checklist