fix(ui): preserve root dir state when sibling dir is expanded#2393
fix(ui): preserve root dir state when sibling dir is expanded#2393trivikr wants to merge 1 commit intonpmx-dev:mainfrom
Conversation
- Recompute expanded state after toggling or auto-expanding directories - Only auto-expand ancestors for the top-level tree - Add regression coverage for sibling directory expansion
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughThis pull request addresses a bug where expanding one directory in the source tree-view would incorrectly re-expand a previously collapsed sibling directory. The changes modify the auto-expansion logic in Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5fb276bf-51e8-4d8d-a8c7-d5b8cd6fdb78
📒 Files selected for processing (3)
app/components/Code/FileTree.vueapp/composables/useFileTreeState.tstest/nuxt/components/CodeFileTree.spec.ts
| const wrapper = await mountCodeFileTree() | ||
|
|
||
| await vi.waitFor(() => { | ||
| expect(wrapper.text()).toContain('constants.d.ts') | ||
| expect(wrapper.text()).not.toContain('common.d.ts') | ||
| }) | ||
|
|
||
| const coreButton = findDirButton(wrapper, 'core') | ||
| expect(coreButton).toBeDefined() | ||
| await coreButton!.trigger('click') | ||
|
|
||
| await vi.waitFor(() => { | ||
| expect(wrapper.text()).not.toContain('constants.d.ts') | ||
| }) | ||
|
|
||
| const typesButton = findDirButton(wrapper, 'types') | ||
| expect(typesButton).toBeDefined() | ||
| await typesButton!.trigger('click') | ||
|
|
||
| await vi.waitFor(() => { | ||
| expect(wrapper.text()).toContain('common.d.ts') | ||
| expect(wrapper.text()).not.toContain('constants.d.ts') | ||
| }) | ||
|
|
||
| wrapper.unmount() | ||
| }) |
There was a problem hiding this comment.
Ensure cleanup always runs when the test fails.
With attachTo: document.body, cleanup at Line 87 is skipped if an earlier assertion fails. Wrap the test body in try/finally so unmount() is guaranteed.
♻️ Suggested reliability fix
it('keeps a collapsed sibling directory closed when another sibling expands', async () => {
const wrapper = await mountCodeFileTree()
+ try {
- await vi.waitFor(() => {
- expect(wrapper.text()).toContain('constants.d.ts')
- expect(wrapper.text()).not.toContain('common.d.ts')
- })
+ await vi.waitFor(() => {
+ expect(wrapper.text()).toContain('constants.d.ts')
+ expect(wrapper.text()).not.toContain('common.d.ts')
+ })
- const coreButton = findDirButton(wrapper, 'core')
- expect(coreButton).toBeDefined()
- await coreButton!.trigger('click')
+ const coreButton = findDirButton(wrapper, 'core')
+ expect(coreButton).toBeDefined()
+ await coreButton!.trigger('click')
- await vi.waitFor(() => {
- expect(wrapper.text()).not.toContain('constants.d.ts')
- })
+ await vi.waitFor(() => {
+ expect(wrapper.text()).not.toContain('constants.d.ts')
+ })
- const typesButton = findDirButton(wrapper, 'types')
- expect(typesButton).toBeDefined()
- await typesButton!.trigger('click')
+ const typesButton = findDirButton(wrapper, 'types')
+ expect(typesButton).toBeDefined()
+ await typesButton!.trigger('click')
- await vi.waitFor(() => {
- expect(wrapper.text()).toContain('common.d.ts')
- expect(wrapper.text()).not.toContain('constants.d.ts')
- })
+ await vi.waitFor(() => {
+ expect(wrapper.text()).toContain('common.d.ts')
+ expect(wrapper.text()).not.toContain('constants.d.ts')
+ })
- wrapper.unmount()
+ } finally {
+ wrapper.unmount()
+ }
})
| } | ||
|
|
||
| describe('CodeFileTree', () => { | ||
| it('keeps a collapsed sibling directory closed when another sibling expands', async () => { |
There was a problem hiding this comment.
I'm personally a little wary of tests like this, because the space of "this thing should not happen" is incredibly large. There's nothing wrong with the expectation, but maybe it can just be part of a normal collapse/expand test, instead of being a test specifically for this bug. Wdyt?
2 participants
🔗 Linked issue
resolves #2363
🧭 Context
The package code sidebar was re-expanding a collapsed sibling directory when another directory in the same tree was opened. On
ky@1.14.3, collapsingcore/and then expandingtypes/caused both directories to appear expanded again.This change fixes the tree expansion logic so only the intended directory opens, and adds coverage for the regression.
📚 Description
The root cause was that the code tree auto-expanded the current file path from recursive
CodeFileTreeinstances, not just from the root tree. That meant a subtree remount could re-apply ancestor expansion and reopen a directory the user had just collapsed.This PR:
Setafter mutations