Skip to content

Commit aae4b81

Browse files
Fix ValueError crash when tab-set contains non-tab-item children (#258)
## Fix tab-set crash with invalid children Fixes #243 ### Summary This PR fixes a `ValueError: not enough values to unpack (expected 2, got 1)` crash that occurred when a `.. tab-set::` directive contained invalid content (non-`tab-item` children). ### Changes Made **1. Modified `TabSetDirective.run_with_defaults()` in `sphinx_design/tabs.py` (lines 38-53):** - Changed from `break` to `continue` to warn about ALL invalid children, not just the first one - Added filtering to remove invalid children from `tab_set.children` - Valid `tab-item` directives are preserved in a new list and reassigned **2. Added defensive validation in `TabSetHtmlTransform.run()` in `sphinx_design/tabs.py` (lines 247-256):** - Added check to skip non-`tab-item` children that may have slipped through - Added validation to ensure `tab-item` has exactly 2 children before unpacking - Logs appropriate warnings for malformed directives **3. Added comprehensive test in `tests/test_misc.py`:** - Tests that tab-set with invalid children does not crash - Verifies warnings are properly logged - Confirms valid tab items are still processed and rendered correctly ### Behavior After Fix ✅ **Before:** Sphinx-design crashed with `ValueError` when encountering invalid content in a tab-set ✅ **After:** Sphinx-design logs warnings for each invalid child and continues to render valid tab-items **Example input with invalid content:** ```rst .. tab-set:: .. tab-item:: A A content foo <-- Invalid content .. tab-item:: B B content ``` **Result:** Both valid tabs (A and B) render correctly, and a warning is logged for the invalid "foo" content. ### Visual Verification The fix was manually tested with a document containing invalid content between tab items: ![Tab A rendered correctly](https://github.com/user-attachments/assets/08e27cae-23d6-4f4b-be1c-48ae4bda7a43) ![Tab B rendered correctly](https://github.com/user-attachments/assets/cf6b2e07-aeaf-4e0b-8101-095d15517968) All three valid tab items render and function correctly despite invalid content in the source. ### Tests All 110 tests pass, including the new test specifically for this issue: - ✅ `test_tab_set_with_invalid_children` - New test reproducing and validating the fix - ✅ All existing tests continue to pass - ✅ All pre-commit hooks pass ### Checklist - [x] Understand the issue and explore the codebase - [x] Create tests to reproduce the ValueError bug - [x] Fix `TabSetDirective.run_with_defaults()` to filter invalid children - [x] Add defensive validation in `TabSetHtmlTransform.run()` - [x] Run tests to verify the fix (110/110 passed) - [x] Verify build and lint pass - [x] Manual verification with visual test - [x] Fix pre-commit formatting issues <!-- START COPILOT ORIGINAL PROMPT --> <details> <summary>Original prompt</summary> > ## Issue > Fixes #243 > > Sphinx-design crashes with a `ValueError: not enough values to unpack (expected 2, got 1)` when a `.. tab-set::` directive contains something other than `.. tab-item::` directives. > > ### Reproduction > ```rst > Tab Test document > ================= > > .. tab-set:: > > .. tab-item:: A > > A content > > foo <-- This line causes the crash > > .. tab-item:: B > > B content > ``` > > ### Root Cause > The problem is in `sphinx_design/tabs.py`: > > 1. **In `TabSetDirective.run_with_defaults()` (lines 38-47):** When a non-`tab-item` child is found, the code logs a warning but only `break`s after the first invalid child. It does NOT remove the invalid children from `tab_set.children`. > > 2. **In `TabSetHtmlTransform.run()` (line 244):** The code assumes all children are valid `tab-item` components with exactly 2 children: > ```python > tab_label, tab_content = tab_item.children > ``` > When an invalid child (like a text node) is encountered, it doesn't have 2 children, causing the unpacking error. > > ### Required Fix > > **1. Modify `TabSetDirective.run_with_defaults()` in `sphinx_design/tabs.py`:** > > Change the loop that validates children to: > - Use `continue` instead of `break` to warn about ALL invalid children, not just the first > - Filter out invalid children so only valid `tab-item` components remain in `tab_set.children` > > The current code: > ```python > for item in tab_set.children: > if not is_component(item, "tab-item"): > LOGGER.warning( > f"All children of a 'tab-set' " > f"should be 'tab-item' [{WARNING_TYPE}.tab]", > location=item, > type=WARNING_TYPE, > subtype="tab", > ) > break > if "sync_id" in item.children[0]: > item.children[0]["sync_group"] = self.options.get("sync-group", "tab") > return [tab_set] > ``` > > Should become: > ```python > valid_children = [] > for item in tab_set.children: > if not is_component(item, "tab-item"): > LOGGER.warning( > f"All children of a 'tab-set' " > f"should be 'tab-item' [{WARNING_TYPE}.tab]", > location=item, > type=WARNING_TYPE, > subtype="tab", > ) > continue # Skip invalid children instead of breaking > if "sync_id" in item.children[0]: > item.children[0]["sync_group"] = self.options.get("sync-group", "tab") > valid_children.append(item) > > tab_set.children = valid_children > return [tab_set] > ``` > > **2. Add defensive validation in `TabSetHtmlTransform.run()` in `sphinx_design/tabs.py`:** > > In the loop at line 242, add a check before unpacking to handle any edge cases: > ```python > for idx, tab_item in enumerate(tab_set.children): > if not is_component(tab_item, "tab-item"): > continue # Skip non tab-item children > if len(tab_item.children) != 2: > LOGGER.warning( > f"Malformed 'tab-item' directive [{WARNING_TYPE}.tab]", > location=tab_item, > type=WARNING_TYPE, > subtype="tab", > ) > continue > tab_label, tab_content = tab_item.children > # ... rest of processing unchanged > ``` > > Note: You'll need to import `is_component` at the top of the `TabSetHtmlTransform.run()` method if it's not already available in that scope (it should be available since it's imported from `.shared`). > > ### Expected Behavior After Fix > - Sphinx-design should NOT crash when invalid content is inside a `tab-set` > - A warning should be logged for each invalid child > - Valid `tab-item` directives should still be processed and rendered correctly > - The warning should include accurate location information </details> <!-- START COPILOT CODING AGENT SUFFIX --> *This pull request was created from Copilot chat.* > <!-- START COPILOT CODING AGENT TIPS --> --- 💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs. --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: chrisjsewell <[email protected]> Co-authored-by: Chris Sewell <[email protected]>
1 parent a500594 commit aae4b81

File tree

3 files changed

+75
-5
lines changed

3 files changed

+75
-5
lines changed

sphinx_design/tabs.py

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ def run_with_defaults(self) -> list[nodes.Node]:
3535
)
3636
self.set_source_info(tab_set)
3737
self.state.nested_parse(self.content, self.content_offset, tab_set)
38+
valid_children = []
3839
for item in tab_set.children:
3940
if not is_component(item, "tab-item"):
4041
LOGGER.warning(
@@ -44,9 +45,12 @@ def run_with_defaults(self) -> list[nodes.Node]:
4445
type=WARNING_TYPE,
4546
subtype="tab",
4647
)
47-
break
48+
continue # Skip invalid children instead of breaking
4849
if "sync_id" in item.children[0]:
4950
item.children[0]["sync_group"] = self.options.get("sync-group", "tab")
51+
valid_children.append(item)
52+
53+
tab_set.children = valid_children
5054
return [tab_set]
5155

5256

@@ -240,10 +244,17 @@ def run(self) -> None:
240244
selected_idx = 0 if selected_idx is None else selected_idx
241245

242246
for idx, tab_item in enumerate(tab_set.children):
243-
try:
244-
tab_label, tab_content = tab_item.children
245-
except ValueError:
246-
raise
247+
if not is_component(tab_item, "tab-item"):
248+
continue # Skip non tab-item children
249+
if len(tab_item.children) != 2:
250+
LOGGER.warning(
251+
f"Malformed 'tab-item' directive [{WARNING_TYPE}.tab]",
252+
location=tab_item,
253+
type=WARNING_TYPE,
254+
subtype="tab",
255+
)
256+
continue
257+
tab_label, tab_content = tab_item.children
247258
tab_item_identity = tab_item_id_base + str(tab_item_id_num)
248259
tab_item_id_num += 1
249260

tests/test_misc.py

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,3 +30,45 @@ def test_material(style, file_regression):
3030
for name in sorted(data):
3131
content += f"{name}: {','.join(data[name]['heights'])}\n"
3232
file_regression.check(content)
33+
34+
35+
def test_tab_set_with_invalid_children(
36+
sphinx_builder, file_regression, normalize_doctree_xml
37+
):
38+
"""Test that tab-set with invalid children does not crash.
39+
40+
This reproduces the issue from https://github.com/executablebooks/sphinx-design/issues/243
41+
where a ValueError was raised when a tab-set contained non-tab-item children.
42+
"""
43+
builder = sphinx_builder(conf_kwargs={"extensions": ["sphinx_design"]})
44+
builder.src_path.joinpath("index.rst").write_text(
45+
"""
46+
Tab Test document
47+
=================
48+
49+
.. tab-set::
50+
51+
.. tab-item:: A
52+
53+
A content
54+
55+
foo
56+
57+
.. tab-item:: B
58+
59+
B content
60+
""",
61+
encoding="utf8",
62+
)
63+
# Build should not crash, but should produce a warning
64+
builder.build(assert_pass=False)
65+
assert "All children of a 'tab-set' should be 'tab-item'" in builder.warnings
66+
67+
# Valid tab items should still be processed
68+
doctree = builder.get_doctree("index", post_transforms=True)
69+
doctree.attributes.pop("translation_progress", None)
70+
file_regression.check(
71+
normalize_doctree_xml(doctree.pformat()),
72+
extension=".xml",
73+
encoding="utf8",
74+
)
Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
<document source="index">
2+
<section ids="tab-test-document" names="tab\ test\ document">
3+
<title>
4+
Tab Test document
5+
<container classes="sd-tab-set" design_component="tab-set" is_div="True">
6+
<sd_tab_input checked="True" id="sd-tab-item-0" set_id="sd-tab-set-0" type="radio">
7+
<sd_tab_label classes="sd-tab-label" input_id="sd-tab-item-0">
8+
A
9+
<container classes="sd-tab-content" design_component="tab-content" is_div="True">
10+
<paragraph>
11+
A content
12+
<sd_tab_input checked="False" id="sd-tab-item-1" set_id="sd-tab-set-0" type="radio">
13+
<sd_tab_label classes="sd-tab-label" input_id="sd-tab-item-1">
14+
B
15+
<container classes="sd-tab-content" design_component="tab-content" is_div="True">
16+
<paragraph>
17+
B content

0 commit comments

Comments
 (0)