Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Apply the documented default column `type` and `id` to reactive (state-backed) DataEditor columns, fixing a render crash when a column omits them.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The file name uses the issue number. towncrier's issue_format turns 7596 into a link to the issue, not to this PR. Please rename the file to 7597.bugfix.md or +<slug>.bugfix.md.

Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,11 @@ class DataEditorThemeBase:
text_medium: str | None = None


# Instance attribute carrying the hoisted reactive-columns declaration (const
# name and JS expression) from create() to add_hooks().
_HOISTED_COLUMNS_ATTR = "_hoisted_columns"


@dataclasses.dataclass(init=False)
class DataEditorTheme(DataEditorThemeBase):
"""The theme for the DataEditor component."""
Expand Down Expand Up @@ -485,15 +490,28 @@ def add_hooks(self) -> list[str]:
data_callback = f"getData_{editor_id}"
self.get_cell_content = Var(_js_expr=data_callback)

code = [f"function {data_callback}([col, row]){{"]

columns_path = str(self.columns)
data_path = str(self.data)

code.extend([
f" return formatDataEditorCells(col, row, {columns_path}, {data_path});",
" }",
])
hoisted_columns: tuple[str, str] | None = getattr(
self, _HOISTED_COLUMNS_ATTR, None
)
if hoisted_columns is not None:
# Reactive columns: the grid prop points at this same hoisted
# const (see create), so the expression evaluates once per render
# instead of once per cell. Glide calls the callback for every
# visible cell on every draw.
columns_var, columns_expr = hoisted_columns
code = [
f"const {columns_var} = {columns_expr};",
f"function {data_callback}([col, row]){{",
f" return formatDataEditorCells(col, row, {columns_var}, {data_path});",
" }",
]
else:
code = [
f"function {data_callback}([col, row]){{",
f" return formatDataEditorCells(col, row, {self.columns!s}, {data_path});",
" }",
]

return ["\n".join(code)]

Expand Down Expand Up @@ -526,7 +544,24 @@ def create(cls, *children, extended_cell_types: bool = False, **props) -> Compon

props["rows"] = data.length() if isinstance(data, ArrayVar) else len(data)

if not isinstance(columns, Var) and len(columns):
hoisted_columns: tuple[str, str] | None = None
if isinstance(columns, Var):
# Reactive (Var) columns skip the literal formatting in the elif
# below, so apply the documented default column type and id in JS
# when rendering (#7596). An explicit per-column type or id still
# wins via the spread order. The expression is declared once, in
# the component hook (see add_hooks), and both the grid prop and
# the per-cell callback reference that hoisted const, so the
# mapping runs once per render even though Glide calls the
# callback for every visible cell on every draw.
hoist_name = f"columns_{get_unique_variable_name()}"
hoist_expr = (
f'({columns!s}).map((col) => ({{ type: "str", '
f'id: (col.title ?? "").toLowerCase(), ...col }}))'
)
hoisted_columns = (hoist_name, hoist_expr)
props["columns"] = columns._replace(_js_expr=hoist_name)
elif len(columns):
if types.is_dataframe(type(data)) or (
isinstance(data, Var) and types.is_dataframe(data._var_type)
):
Expand Down Expand Up @@ -573,6 +608,8 @@ def create(cls, *children, extended_cell_types: bool = False, **props) -> Compon
)

grid = super().create(*children, **props)
if hoisted_columns is not None:
setattr(grid, _HOISTED_COLUMNS_ATTR, hoisted_columns)
return Div.create(
grid,
width=props.pop("width", "100%"),
Expand Down
2 changes: 1 addition & 1 deletion pyi_hashes.json
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
"packages/reflex-components-core/src/reflex_components_core/el/elements/tables.pyi": "7b4c2eaff78f6a412f804457570014c4",
"packages/reflex-components-core/src/reflex_components_core/el/elements/typography.pyi": "a3d0d5b0ad9d896261091603df6d2c03",
"packages/reflex-components-core/src/reflex_components_core/react_router/dom.pyi": "7617635157b816f70354e2db98b38642",
"packages/reflex-components-dataeditor/src/reflex_components_dataeditor/dataeditor.pyi": "8aedc1629118cd3c0075340a5a0c3356",
"packages/reflex-components-dataeditor/src/reflex_components_dataeditor/dataeditor.pyi": "1fefd7d3b12c9abaaf280083f9ac8c0f",
"packages/reflex-components-gridjs/src/reflex_components_gridjs/datatable.pyi": "2ce1c076ecf5c2fa4945b4abdbf2f91d",
"packages/reflex-components-lucide/src/reflex_components_lucide/icon.pyi": "1e331a3d6420b97e5b1ce7f63ad53de8",
"packages/reflex-components-markdown/src/reflex_components_markdown/markdown.pyi": "6759daf6e242e209d9298045c7b4456a",
Expand Down
104 changes: 103 additions & 1 deletion tests/units/components/datadisplay/test_dataeditor.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
import json
import re
import subprocess
from pathlib import Path
from typing import cast
from typing import Any, cast

import reflex_base
from reflex_base.vars import Var
from reflex_components_dataeditor.dataeditor import DataEditor

from reflex.state import BaseState


def test_dataeditor():
editor_wrapper = DataEditor.create().render()
Expand Down Expand Up @@ -85,3 +89,101 @@ def test_dataeditor_template_formats_image_cells():
"allowOverlay": True,
},
]


def _columns_prop(editor_render: dict) -> str:
props = editor_render["children"][0]["props"]
return next(p for p in props if p.startswith("columns:"))


_REACTIVE_EXPR = (
"(state.columns).map((col) => "
'({ type: "str", id: (col.title ?? "").toLowerCase(), ...col }))'
)


def test_dataeditor_literal_columns_default_type():
editor = DataEditor.create(
columns=[{"title": "Name"}, {"title": "Age", "type": "int"}]
).render()
prop = _columns_prop(editor)
assert '["type"] : "str"' in prop, prop
assert '["id"] : "name"' in prop, prop
# An explicit type is kept.
assert '["type"] : "int"' in prop, prop


def test_dataeditor_reactive_columns_default_type():
# Reactive (Var) columns skip literal formatting, so the documented
# defaults are applied in JS at render time. See issue #7596.
wrapper = DataEditor.create(columns=Var(_js_expr="state.columns"))
editor = cast(DataEditor, wrapper.children[0])
# The grid prop references the hoisted const declared in the hook.
prop = _columns_prop(wrapper.render())
assert re.fullmatch(r"columns:columns_\w+", prop), prop
name = prop.removeprefix("columns:")
code = "\n".join(editor.add_hooks())
assert f"const {name} = {_REACTIVE_EXPR};" in code, code


def test_dataeditor_reactive_columns_preserve_var_data():
class EditorState(BaseState):
columns: list[dict[str, str]] = [{"title": "Name"}]

wrapper = DataEditor.create(columns=cast(Any, EditorState.columns))
editor = cast(DataEditor, wrapper.children[0])
# The wrapped expression must keep the original var's data (state dep).
columns_var = cast(Var, cast(Any, editor).columns)
assert columns_var._var_data == cast(Var, EditorState.columns)._var_data
# The grid prop references the hoisted const declared in the hook.
prop = _columns_prop(wrapper.render())
assert re.fullmatch(r"columns:columns_\w+", prop), prop


def test_dataeditor_add_hooks_evaluates_columns_once_per_render():
# Glide calls the getData callback per visible cell per draw, so the
# columns expression (the .map() wrapper for reactive columns) is
# declared once in the hook and reused by both the grid prop and the
# per-cell callback (#7597).
wrapper = DataEditor.create(columns=Var(_js_expr="state.columns"))
editor = cast(DataEditor, wrapper.children[0])
code = "\n".join(editor.add_hooks())
assert code.count(".map(") == 1, code
match = re.search(r"const (columns_\w+) = ", code)
assert match is not None, code
assert f"formatDataEditorCells(col, row, {match.group(1)}," in code, code
assert _columns_prop(wrapper.render()) == f"columns:{match.group(1)}"


def test_dataeditor_reactive_columns_generated_expr_js_semantics():
# Evaluate the expression the component actually generates, not a copy:
# default type, default id, an explicit override, and the dict-row lookup
# the id default exists for (#7596).
editor = cast(
DataEditor, DataEditor.create(columns=Var(_js_expr="state.columns")).children[0]
)
code = "\n".join(editor.add_hooks())
match = re.search(r"const columns_\w+ = (\(.*\));", code)
assert match is not None, code
generated_expr = match.group(1)
assert generated_expr == _REACTIVE_EXPR

test_script = f"""
const state = {{ columns: [{{ title: "Name" }}, {{ title: "Age", type: "int" }}] }};
const cols = {generated_expr};
const row = {{ name: "Alice" }};
console.log(JSON.stringify([cols, row[cols[0].id]]));
"""
result = subprocess.run(
["node", "--input-type=module", "--eval", test_script],
check=True,
capture_output=True,
text=True,
)
assert json.loads(result.stdout) == [
[
{"type": "str", "id": "name", "title": "Name"},
{"type": "int", "id": "age", "title": "Age"},
],
"Alice",
]
Loading