feat(sidebar): feature-flagged file-system tree sidebar#2468
Open
raquelmsmith wants to merge 6 commits into
Open
feat(sidebar): feature-flagged file-system tree sidebar#2468raquelmsmith wants to merge 6 commits into
raquelmsmith wants to merge 6 commits into
Conversation
Behind the `posthog-code-file-system-sidebar` flag (force-enabled in dev),
replace the sidebar's repo/task list with a tree of items from the PostHog
file-system "desktop" surface (PostHog PR #61047). Folders render with a `#`
icon and are expandable like the current repo sections; leaf items render as
inert rows for now.
- posthogClient: getDesktopFileSystem() hits the server-controlled
/api/projects/{id}/desktop_file_system/ route via the raw fetcher with
pagination-following (route isn't in the generated OpenAPI client).
- buildFileSystemTree(): pure util turning the flat list into a nested tree
(derives intermediate folders, attaches explicit folder rows, guards empty
segments, sorts folders-first then alphabetically) + unit tests.
- useDesktopFileSystem(): useAuthenticatedQuery wrapper mirroring useTaskSummaries.
- FileSystemTreeView: recursive renderer reusing SidebarSection (folders) and
SidebarItem (leaves), with empty state and fs:-namespaced collapse keys.
- SidebarSection: additive `depth` prop for nested indentation (depth=0 keeps
the existing TaskListView rendering unchanged).
- SidebarMenu: flag branch between the new tree view and TaskListView.
Generated-By: PostHog Code
Task-Id: 26a4498f-f1b9-405b-a439-56af14877c6d
Make the flagged file-system sidebar a panel the user can switch away from, instead of fully replacing the task list. Adds a two-segment "Files" / "Tasks" toggle shown only when the file-system sidebar flag is on, with the selected panel persisted in the sidebar store. When the flag is off, the task list renders as before. Generated-By: PostHog Code Task-Id: 26a4498f-f1b9-405b-a439-56af14877c6d
Rename the file-system sidebar panel label from "Files" to "Channels" (keeping the persisted "files" panel value), and add the ability to create and delete top-level channels synced to the cloud desktop_file_system surface. - SidebarPanelToggle: label "Channels" with Hash icon - posthogClient: createDesktopFileSystemChannel / deleteDesktopFileSystem - useDesktopFileSystemMutations: create/delete with query invalidation - ChannelsHeader: inline add-channel input - SidebarSection: generic hover delete action - FileSystemTreeView: delete top-level channels behind a confirmation Generated-By: PostHog Code Task-Id: b3c22a1c-8b74-4790-b7a1-673715dee381
Replace the inline channel-name input in the sidebar with a Slack-style "Create a channel" modal: a Name field with a # prefix, character counter, helper text, and a Create action. Generated-By: PostHog Code Task-Id: b3c22a1c-8b74-4790-b7a1-673715dee381
…tem-sidebar # Conflicts: # apps/code/src/renderer/features/sidebar/components/SidebarMenu.tsx
Adds a "File to..." submenu to the task right-click menu listing desktop file system folder channels. Selecting one creates a task-typed entry under that channel that links back to the source task via `ref`, so clicking it in the channels sidebar opens the task in the main pane. Task leaves render indented with a code icon. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
apps/code/src/renderer/features/sidebar/utils/fileSystemTree.ts:75-93
**Duplicate leaf entries when two items share a normalized path**
When a second non-folder item lands on a path that already has a leaf, `existing?.isFolder` is `false`, so the guard doesn't fire and the code falls through: a fresh node is pushed to `parent.children` while `byPath` is overwritten. The result is two siblings with the same `id` string — React warns about duplicate keys, and both entries render in the tree. A concrete trigger today is filing the same task to the same channel twice (or two tasks whose titles are identical under the same channel).
### Issue 2 of 3
apps/code/src/renderer/features/sidebar/components/SidebarMenu.tsx:336-338
Task titles that contain `/` are passed raw into the path, so `buildFileSystemTree` splits on every slash and creates phantom intermediate folders. Filing a task titled "fix/auth" to channel "Work" produces the path `Work/fix/auth`, and the tree renders a spurious `fix` folder node between `Work` and `auth`. Sanitize the title before building the path.
```suggestion
await fileEntry({
path: `${channelPath}/${task.title.replace(/\//g, "-")}`,
type: "task",
```
### Issue 3 of 3
apps/code/src/renderer/features/sidebar/utils/fileSystemTree.test.ts:38-63
**Prefer parameterised tests for data-driven cases**
The sorting test and the empty-segment guard are driven by tabular input and would be cleaner as `it.each` tables — consistent with the project's preference for parameterised tests. There's also no test for the leaf-vs-leaf collision (two non-folder items sharing a normalised path), which is where the duplicate-child bug hides.
Reviews (1): Last reviewed commit: "feat(code): file tasks to channels from ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+75
to
+93
| // Leaf. If a folder already claims this exact path, keep folder semantics | ||
| // and just attach the source row. | ||
| const existing = byPath.get(normalizedPath); | ||
| if (existing?.isFolder) { | ||
| existing.item = existing.item ?? item; | ||
| continue; | ||
| } | ||
|
|
||
| const parent = ensureFolder(segments.slice(0, -1)); | ||
| const leaf: FileSystemTreeNode = { | ||
| id: normalizedPath, | ||
| name, | ||
| path: normalizedPath, | ||
| isFolder: false, | ||
| item, | ||
| children: [], | ||
| }; | ||
| byPath.set(normalizedPath, leaf); | ||
| parent.children.push(leaf); |
Contributor
There was a problem hiding this comment.
When a second non-folder item lands on a path that already has a leaf, existing?.isFolder is false, so the guard doesn't fire and the code falls through: a fresh node is pushed to parent.children while byPath is overwritten. The result is two siblings with the same id string — React warns about duplicate keys, and both entries render in the tree. A concrete trigger today is filing the same task to the same channel twice (or two tasks whose titles are identical under the same channel).
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/renderer/features/sidebar/utils/fileSystemTree.ts
Line: 75-93
Comment:
**Duplicate leaf entries when two items share a normalized path**
When a second non-folder item lands on a path that already has a leaf, `existing?.isFolder` is `false`, so the guard doesn't fire and the code falls through: a fresh node is pushed to `parent.children` while `byPath` is overwritten. The result is two siblings with the same `id` string — React warns about duplicate keys, and both entries render in the tree. A concrete trigger today is filing the same task to the same channel twice (or two tasks whose titles are identical under the same channel).
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+336
to
+338
| await fileEntry({ | ||
| path: `${channelPath}/${task.title}`, | ||
| type: "task", |
Contributor
There was a problem hiding this comment.
/ are passed raw into the path, so buildFileSystemTree splits on every slash and creates phantom intermediate folders. Filing a task titled "fix/auth" to channel "Work" produces the path Work/fix/auth, and the tree renders a spurious fix folder node between Work and auth. Sanitize the title before building the path.
Suggested change
| await fileEntry({ | |
| path: `${channelPath}/${task.title}`, | |
| type: "task", | |
| await fileEntry({ | |
| path: `${channelPath}/${task.title.replace(/\//g, "-")}`, | |
| type: "task", |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/renderer/features/sidebar/components/SidebarMenu.tsx
Line: 336-338
Comment:
Task titles that contain `/` are passed raw into the path, so `buildFileSystemTree` splits on every slash and creates phantom intermediate folders. Filing a task titled "fix/auth" to channel "Work" produces the path `Work/fix/auth`, and the tree renders a spurious `fix` folder node between `Work` and `auth`. Sanitize the title before building the path.
```suggestion
await fileEntry({
path: `${channelPath}/${task.title.replace(/\//g, "-")}`,
type: "task",
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+38
to
+63
| expect(leaf.isFolder).toBe(false); | ||
| expect(leaf.item?.href).toBe("/x"); | ||
| }); | ||
|
|
||
| it("attaches explicit folder rows to their node", () => { | ||
| const tree = buildFileSystemTree([ | ||
| item({ path: "Reports", type: "folder" }), | ||
| item({ path: "Reports/Sales", type: "dashboard", href: "/d" }), | ||
| ]); | ||
|
|
||
| expect(tree).toHaveLength(1); | ||
| const reports = tree[0]; | ||
| expect(reports.isFolder).toBe(true); | ||
| expect(reports.item?.type).toBe("folder"); | ||
| expect(reports.children.map((c) => c.name)).toEqual(["Sales"]); | ||
| }); | ||
|
|
||
| it("sorts folders first, then alphabetically", () => { | ||
| const tree = buildFileSystemTree([ | ||
| item({ path: "zeta", type: "insight", href: "/z" }), | ||
| item({ path: "alpha", type: "insight", href: "/a" }), | ||
| item({ path: "Mango", type: "folder" }), | ||
| item({ path: "Apple", type: "folder" }), | ||
| ]); | ||
|
|
||
| expect(tree.map((n) => n.name)).toEqual([ |
Contributor
There was a problem hiding this comment.
The sorting test and the empty-segment guard are driven by tabular input and would be cleaner as it.each tables — consistent with the project's preference for parameterised tests. There's also no test for the leaf-vs-leaf collision (two non-folder items sharing a normalised path), which is where the duplicate-child bug hides.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/renderer/features/sidebar/utils/fileSystemTree.test.ts
Line: 38-63
Comment:
**Prefer parameterised tests for data-driven cases**
The sorting test and the empty-segment guard are driven by tabular input and would be cleaner as `it.each` tables — consistent with the project's preference for parameterised tests. There's also no test for the leaf-vs-leaf collision (two non-folder items sharing a normalised path), which is where the duplicate-child bug hides.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
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
1 participant
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.
human description
We want a place to keep artifacts organized in PostHog Code. This stubs that out, though there is likely much to do still to make it nice. Requires PostHog/posthog#61507 to file tasks into the folders.
Summary
Behind the
posthog-code-file-system-sidebarfeature flag (force-enabled in dev), this replaces the sidebar's repo/task list with a tree of items from the PostHog file-system desktop surface introduced server-side by PostHog #61047. Folders render with a#icon and are expandable exactly like the current repo sections; leaf items render as inert rows (no-op click) for this first pass.The desktop tree is served from its own server-controlled route
GET /api/projects/{id}/desktop_file_system/(surface is not a query param). That route isn't in the generated OpenAPI client yet, so it's called via the raw fetcher, mirroringgetTaskSummaries.Changes
New (under
apps/code/src/renderer/features/sidebar/):utils/fileSystemTree.ts— purebuildFileSystemTree()turning the flatFileSystem[]into a nested tree: derives intermediate folders from path segments, attaches explicittype: "folder"rows, guards empty segments, sorts folders-first then alphabetically.utils/fileSystemTree.test.ts— 6 unit tests (folder derivation, explicit rows, sorting, empty-segment guarding, folder/leaf collisions).hooks/useDesktopFileSystem.ts—useAuthenticatedQuerywrapper, mirroringuseTaskSummaries.components/FileSystemTreeView.tsx— recursive renderer usingSidebarSection(folders,#icon) andSidebarItem(leaves), with a "Nothing here yet" empty state,fs:-namespaced collapse keys, and a visual indent cap.Modified:
api/posthogClient.ts—getDesktopFileSystem()with pagination-following.shared/constants.ts—FILE_SYSTEM_SIDEBAR_FLAG.components/SidebarSection.tsx— additivedepthprop for nested indentation (depth=0reproduces the priorpl-2, soTaskListViewis visually unchanged).components/SidebarMenu.tsx— flag branch betweenFileSystemTreeViewand the existingTaskListView; top nav andProjectSwitcheruntouched.Notes
Verification
pnpm --filter code typecheckpassespnpm --filter code test fileSystemTree— 6/6 passNot yet verified with a live Electron run.