diff --git a/frontend/src/utils/__tests__/id-tree.test.ts b/frontend/src/utils/__tests__/id-tree.test.ts index b1b43135122..3fb76467d8f 100644 --- a/frontend/src/utils/__tests__/id-tree.test.ts +++ b/frontend/src/utils/__tests__/id-tree.test.ts @@ -1991,4 +1991,52 @@ describe("CollapsibleTree.fromWithPreviousShape", () => { expect(tree.isCollapsed("one")).toBe(false); expect(tree.topLevelIds).toEqual(["one", "three", "four"]); }); + + it("reproduces GitHub issue #6188: inOrderIds with nested collapses", () => { + // Simplified reproduction case: + // Cell A (markdown): # Section 1 Header + // Cell B (markdown): ## Subsection 1.1 Header + // Cell C (python): print("This cell should be in Subsection 1.1") + // Cell D (python): print("This cell should also be in Subsection 1.1") + // Cell E (markdown): ## Subsection 1.2 Header + // Cell F (python): print("This cell should be in Subsection 1.2") + + const originalOrder = [ + "CellA", // # Section 1 Header + "CellB", // ## Subsection 1.1 Header + "CellC", // print("This cell should be in Subsection 1.1") + "CellD", // print("This cell should also be in Subsection 1.1") + "CellE", // ## Subsection 1.2 Header + "CellF", // print("This cell should be in Subsection 1.2") + ]; + + // Create notebook with one column + let notebook = MultiColumn.from([originalOrder]); + + // Before collapsing, order should be correct + expect(notebook.inOrderIds).toEqual(originalOrder); + + const columnId = notebook.getColumnIds()[0]; + + // Follow the exact reproduction steps from the GitHub issue: + // 1. Collapse B (subsection 1.1) - includes C, D (stops before E) + notebook = notebook.transform(columnId, (tree) => + tree.collapse("CellB", "CellD"), + ); + + // 2. Collapse E (subsection 1.2) - includes F (to end) + notebook = notebook.transform(columnId, (tree) => + tree.collapse("CellE", undefined), + ); + + // 3. Collapse A (section 1) - includes all the rest (B, E and their children) + notebook = notebook.transform(columnId, (tree) => + tree.collapse("CellA", undefined), + ); + + const finalOrder = notebook.inOrderIds; + + // inOrderIds now correctly preserves logical order even with nested collapses + expect(finalOrder).toEqual(originalOrder); + }); }); diff --git a/frontend/src/utils/id-tree.tsx b/frontend/src/utils/id-tree.tsx index a10ef2c8bc3..7d2beccbfe4 100644 --- a/frontend/src/utils/id-tree.tsx +++ b/frontend/src/utils/id-tree.tsx @@ -77,15 +77,17 @@ export class TreeNode { @Memoize() get inOrderIds(): T[] { const result: T[] = []; - const queue = [...this.children]; - while (queue.length > 0) { - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - const node = queue.shift()!; + // Use depth-first traversal to preserve logical document order + const traverse = (node: TreeNode) => { result.push(node.value); + for (const child of node.children) { + traverse(child); + } + }; - // Add children to queue to maintain breadth-first traversal - queue.push(...node.children); + for (const child of this.children) { + traverse(child); } return result; diff --git a/marimo/_ast/load.py b/marimo/_ast/load.py index 87bf033005d..c3e6f77452e 100644 --- a/marimo/_ast/load.py +++ b/marimo/_ast/load.py @@ -187,6 +187,13 @@ def get_notebook_status(filename: str) -> LoadResult: return LoadResult(status="valid", notebook=notebook, contents=contents) +FAILED_LOAD_NOTEBOOK_MESSAGE = ( + "Static loading of notebook failed; falling back to dynamic loading. " + "If you can, please report this issue to the marimo team and include your notebook if possible — " + "https://github.com/marimo-team/marimo/issues/new?template=bug_report.yaml" +) + + def load_app(filename: Optional[str]) -> Optional[App]: """Load and return app from a marimo-generated module. @@ -225,10 +232,5 @@ def load_app(filename: Optional[str]) -> Optional[App]: # Security advantages of static load are lost here, but reasonable # fallback for now. _app = _dynamic_load(filename) - LOGGER.warning( - "Static loading of notebook failed; " - "falling back to dynamic loading. " - "If you can, please report this issue to the marimo team — " - "https://github.com/marimo-team/marimo/issues/new?template=bug_report.yaml" - ) + LOGGER.warning(FAILED_LOAD_NOTEBOOK_MESSAGE) return _app