fix: hide playground sidebar subtrees when an ancestor folder is collapsed - #6278
Open
prql-bot wants to merge 1 commit into
Open
fix: hide playground sidebar subtrees when an ancestor folder is collapsed#6278prql-bot wants to merge 1 commit into
prql-bot wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The playground sidebar decided whether to draw a row by testing only its immediate parent (
parent == null || openFolders[parent] || depth === 0), so collapsing a folder hid its children but left its grandchildren on screen — they float up to the top of the sidebar, indented but with no visible parent. This walks the ancestor chain instead: a row renders only if every folder above it is open, which is what the disclosure triangle already implies.Found during the nightly survey of
web/playground/src/sidebar/Sidebar.jsx.Reproducing it
Open project in the sidebar, open target.md inside it, then collapse project again. The four snippet rows from
target.mdstay visible.Against the real generated
book.json(210 entries, max depth 4), replaying the old and new predicates over the sameopenFoldersstate —{"project/target.md": true}, i.e.projectcollapsed while the child folder is still flagged open:The orphans are
project/target.md_examples.prql,project/target.md_examples_2.prql,project/target.md_version.prql,project/target.md_version_2.prql.The same replay confirms the states that already worked are unchanged: with nothing open only the 3 depth-0 rows render, and with both
projectandproject/target.mdopen all 4 snippet rows render — including after reopeningproject, so the child folder's open state survives an ancestor collapse rather than being reset.The
childrenVisibleaccumulator relies on parents being emitted before their children. That holds by construction:getAllFilesyields a directory and then recurses into it, andgenerateBook.cjswrites entries in that order.Two smaller things in the same rendering loop, both previously masked:
marginLeft: ${12 * depth}pxproducedNaNpxfor theexamples,tablesandlocal storagesections, whose entries are[editor, content]with no tree metadata. Browsers drop the invalid value, so it looked right; it's nowdepth ?? 0.toggleFoldermutated theopenFoldersstate object in place before spreading it intosetOpenFolders. Replaced with the functional updater, which is the same behavior without the mutation.The wrapping
React.Fragmentwent away with the inline conditional — the row is now skipped withcontinue— which also drops theReactimport this file no longer needs, and lets the key be the filename rather than a positional index.Verification
No regression test: the playground has no test runner.
@testing-library/reactand@testing-library/jest-domare independencies, but there is notestscript inweb/playground/package.jsonand no test files anywhere underweb/playground/, so a component test would mean introducing a runner and wiring it into CI — worth doing, but as its own change rather than folded in here. The behavior was instead verified by replaying both predicates over the realbook.jsonas above.build-webruns on this PR (thewebpath filter intests.yamlmatchesweb/**) and builds the playground, so the Vite build is covered there.