Skip to content

Commit dad1613

Browse files
authored
fix: switch to depth-first traversal for inOrderIds method (#6548)
- Updated the inOrderIds method to use depth-first traversal instead of breadth-first, preserving logical document order. Fixes #6188
1 parent 80dfe60 commit dad1613

File tree

3 files changed

+64
-12
lines changed

3 files changed

+64
-12
lines changed

frontend/src/utils/__tests__/id-tree.test.ts

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1991,4 +1991,52 @@ describe("CollapsibleTree.fromWithPreviousShape", () => {
19911991
expect(tree.isCollapsed("one")).toBe(false);
19921992
expect(tree.topLevelIds).toEqual(["one", "three", "four"]);
19931993
});
1994+
1995+
it("reproduces GitHub issue #6188: inOrderIds with nested collapses", () => {
1996+
// Simplified reproduction case:
1997+
// Cell A (markdown): # Section 1 Header
1998+
// Cell B (markdown): ## Subsection 1.1 Header
1999+
// Cell C (python): print("This cell should be in Subsection 1.1")
2000+
// Cell D (python): print("This cell should also be in Subsection 1.1")
2001+
// Cell E (markdown): ## Subsection 1.2 Header
2002+
// Cell F (python): print("This cell should be in Subsection 1.2")
2003+
2004+
const originalOrder = [
2005+
"CellA", // # Section 1 Header
2006+
"CellB", // ## Subsection 1.1 Header
2007+
"CellC", // print("This cell should be in Subsection 1.1")
2008+
"CellD", // print("This cell should also be in Subsection 1.1")
2009+
"CellE", // ## Subsection 1.2 Header
2010+
"CellF", // print("This cell should be in Subsection 1.2")
2011+
];
2012+
2013+
// Create notebook with one column
2014+
let notebook = MultiColumn.from([originalOrder]);
2015+
2016+
// Before collapsing, order should be correct
2017+
expect(notebook.inOrderIds).toEqual(originalOrder);
2018+
2019+
const columnId = notebook.getColumnIds()[0];
2020+
2021+
// Follow the exact reproduction steps from the GitHub issue:
2022+
// 1. Collapse B (subsection 1.1) - includes C, D (stops before E)
2023+
notebook = notebook.transform(columnId, (tree) =>
2024+
tree.collapse("CellB", "CellD"),
2025+
);
2026+
2027+
// 2. Collapse E (subsection 1.2) - includes F (to end)
2028+
notebook = notebook.transform(columnId, (tree) =>
2029+
tree.collapse("CellE", undefined),
2030+
);
2031+
2032+
// 3. Collapse A (section 1) - includes all the rest (B, E and their children)
2033+
notebook = notebook.transform(columnId, (tree) =>
2034+
tree.collapse("CellA", undefined),
2035+
);
2036+
2037+
const finalOrder = notebook.inOrderIds;
2038+
2039+
// inOrderIds now correctly preserves logical order even with nested collapses
2040+
expect(finalOrder).toEqual(originalOrder);
2041+
});
19942042
});

frontend/src/utils/id-tree.tsx

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -77,15 +77,17 @@ export class TreeNode<T> {
7777
@Memoize()
7878
get inOrderIds(): T[] {
7979
const result: T[] = [];
80-
const queue = [...this.children];
8180

82-
while (queue.length > 0) {
83-
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
84-
const node = queue.shift()!;
81+
// Use depth-first traversal to preserve logical document order
82+
const traverse = (node: TreeNode<T>) => {
8583
result.push(node.value);
84+
for (const child of node.children) {
85+
traverse(child);
86+
}
87+
};
8688

87-
// Add children to queue to maintain breadth-first traversal
88-
queue.push(...node.children);
89+
for (const child of this.children) {
90+
traverse(child);
8991
}
9092

9193
return result;

marimo/_ast/load.py

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,13 @@ def get_notebook_status(filename: str) -> LoadResult:
187187
return LoadResult(status="valid", notebook=notebook, contents=contents)
188188

189189

190+
FAILED_LOAD_NOTEBOOK_MESSAGE = (
191+
"Static loading of notebook failed; falling back to dynamic loading. "
192+
"If you can, please report this issue to the marimo team and include your notebook if possible — "
193+
"https://github.com/marimo-team/marimo/issues/new?template=bug_report.yaml"
194+
)
195+
196+
190197
def load_app(filename: Optional[str]) -> Optional[App]:
191198
"""Load and return app from a marimo-generated module.
192199
@@ -225,10 +232,5 @@ def load_app(filename: Optional[str]) -> Optional[App]:
225232
# Security advantages of static load are lost here, but reasonable
226233
# fallback for now.
227234
_app = _dynamic_load(filename)
228-
LOGGER.warning(
229-
"Static loading of notebook failed; "
230-
"falling back to dynamic loading. "
231-
"If you can, please report this issue to the marimo team — "
232-
"https://github.com/marimo-team/marimo/issues/new?template=bug_report.yaml"
233-
)
235+
LOGGER.warning(FAILED_LOAD_NOTEBOOK_MESSAGE)
234236
return _app

0 commit comments

Comments
 (0)