Skip to content

File browser: lazy directory tree in Files tab#503

Closed
srid wants to merge 24 commits intomasterfrom
feat/fs-list-dir
Closed

File browser: lazy directory tree in Files tab#503
srid wants to merge 24 commits intomasterfrom
feat/fs-list-dir

Conversation

@srid
Copy link
Copy Markdown
Member

@srid srid commented Apr 13, 2026

The Files tab is no longer a placeholder — it's a browsable, lazy-loaded directory tree rooted at the active terminal's repo root (or CWD). Phase A of #479, the smallest vertical slice that delivers a real file browser.

The server gets a new fs.listDir oRPC endpoint that reads a directory and returns entries sorted directories-first, with a path traversal guard preventing escape above the repo root. The core logic lives in a new kolu-fs integration package with 18 unit tests covering sorting, traversal attacks (including sibling-prefix and nested ../.. escapes), error propagation, symlink behavior, and FIFO filtering.

On the client, the FilesTab uses Ark UI's TreeView with loadChildren for lazy async expansion — directories signal childrenCount so Ark knows to fetch on expand without preloading the whole tree. A createEffect(on(...)) watches terminal ID and root path so the tree reloads automatically when you switch terminals, and resets cleanly when no terminal is selected. A refresh button in the tab header does what you'd expect.

terminalId is now threaded through TabPropsRightPanelFilesTab so the RPC call can resolve the correct terminal's filesystem root server-side. The hickey evaluation flagged this (plus a stale-closure risk in loadChildren) before implementation started.

Closes Phase A of #479

Try it locally

nix run github:juspay/kolu/feat/fs-list-dir

srid added 4 commits April 13, 2026 16:08
Phase A of #479: the smallest vertical slice delivering a real file browser.

Server: new `fs.listDir` oRPC endpoint resolving terminal CWD/repo root,
with path traversal guard. Core logic extracted to `kolu-fs` integration
package with unit tests.

Client: Ark UI TreeView in FilesTab with lazy async `loadChildren`,
directory-first sorting, expand/collapse, loading indicators, and manual
refresh. Reactive to terminal switches via `createEffect(on(...))`.

Closes Phase A of #479
Cover: empty dirs, deeply nested paths, sub/../.. traversal, sibling
prefix attack, dotfiles, symlink exclusion, ENOENT/ENOTDIR errors,
FIFO filtering, logger invocation, and no-logger path.
@srid
Copy link
Copy Markdown
Member Author

srid commented Apr 13, 2026

/do results

Step Status Duration Verification
sync 8s git fetch succeeded, forge=github
research 3m 1s Understood Phase A scope fully
hickey 57s 2 findings, both fixed in PR
branch 11s feat/fs-list-dir from origin/master
implement 4m 31s fs.listDir + Ark UI TreeView + e2e tests
check 3m 2s typecheck passed (8 workspace projects)
docs No docs configured
police 2m 0s All 3 passes clean (fixed queueMicrotask bug)
fmt 16s 5 files formatted
commit 28s c1d8f2a + merged origin/master
test 3m 17s 188 scenarios, 1349 steps, 18 unit tests
ci 3m 24s nix-build, e2e, typecheck, unit, apm-sync
update-pr 30s Draft PR #503
Total ~24m

Slowest step: implement (4m 31s, 19% of total) — expected for the largest step.

Optimization suggestions

  • ci (3m 24s): the Nix build dominates due to fetchPnpmDeps hash update; subsequent runs on this branch will cache
  • check (3m 2s): first run installed missing node_modules; warm runs are ~15s
  • research (3m 1s): 3 parallel Explore subagents ran concurrently — efficient for the scope
  • test included a Nix rebuild for the pnpmDeps hash fix; pure test time was ~40s

Workflow completed at 2026-04-13T20:16:16Z.

srid added a commit to srid/agency that referenced this pull request Apr 13, 2026
Without this guard, the ci step can be recorded as passed against a
stale commit if new commits land mid-workflow (e.g., user-requested
changes, expanded tests). The verification clause now requires
comparing the CI commit SHA against `git rev-parse HEAD` and re-running
if they diverge.

Observed in juspay/kolu#503: CI passed on cd5b8c5 but a followup
commit 2f86f10 was pushed after — the ci step was already recorded
as passed without covering the latest code.
srid added a commit to srid/agency that referenced this pull request Apr 13, 2026
**CI can silently pass on a stale commit** if new commits land
mid-workflow — the ci step records success against the commit it ran on,
even if HEAD has moved.

Observed in [juspay/kolu#503](juspay/kolu#503):
CI ran against `cd5b8c5`, then a followup commit `2f86f10` (expanded
unit tests) was pushed after the user requested more coverage. The ci
step was already recorded as passed without ever covering the latest
code.

The fix adds a **HEAD-freshness guard** to the ci step's verification
clause: before recording the step as passed, the agent must compare the
commit SHA that CI ran against with `git rev-parse HEAD`. If they
differ, CI must re-run. *This closes the gap regardless of why HEAD
moved* — fix retries, user-requested changes, or any other source of new
commits.
Comment thread packages/common/src/index.ts
Comment thread packages/common/src/contract.ts
Comment thread packages/server/src/router.ts
srid added 18 commits April 13, 2026 16:40
…djs/testing-library

Addresses PR review: Zod schemas (FsListDirInput/Output, FsDirEntry) now
live in kolu-fs and are re-exported by kolu-common — same pattern as
kolu-claude-code owning its own schemas.

Adopts @solidjs/testing-library as a third testing vector for
signal-heavy components. 10 FilesTab component tests covering: initial
load, loading/error/empty states, directory-before-file sorting, refresh
button, terminal switching reactivity, header text, and state cleanup.
Two new tests verify behavior after refresh: consecutive refreshes
fully replace the tree (no ghost entries from previous loads), and
directory-before-file sorting holds after refresh.
Extract reusable assertion helpers (assertTreeRenders, assertDirsBeforeFiles,
assertExpandShowsChildren) and add an "after refresh" describe block that
mounts → refreshes → re-runs the same checks. Catches bugs where refresh
leaves Ark UI TreeView or the collection signal in a broken state that only
surfaces on subsequent interactions.

Also adds: expand test, error-then-refresh recovery test.
Bug: after expanding a directory then refreshing, the expanded directory
could not be re-expanded — Ark UI's internal load tracking persisted
across collection replacements, thinking children were already loaded.

Fix: bump a treeKey counter on every loadRoot, use it to key the
TreeView.Root via <For each={[treeKey()]}> so SolidJS destroys and
recreates the component, clearing all internal Ark UI state.

Found by the new "expand → refresh → expand" component test.
…type

Eliminates parallel type definitions for the same domain concept.
listDir now returns FsDirEntry[] directly from the Zod schema.
Master PR #507 introduced platform-specific pnpmDeps hashes. This
branch added @ark-ui/solid and @solidjs/testing-library, which
invalidate master's Darwin hash. Linux hash verified locally; Darwin
hash must be captured from CI output.
Captured from CI: the new pure-JS deps (@ark-ui/solid,
@solidjs/testing-library) produce the same pnpm store hash on both
Darwin and Linux, so both branches get the same sha256.
Previous commit wrongly set darwin hash equal to linux. Per #509,
these MUST differ because pnpm-lock.yaml contains platform-gated
optionalDependencies.
Verified empirically: on this branch, the darwin fetchPnpmDeps produces
the same content hash as linux. Build log at a82646d shows:
  - ran genuinely on darwin (/private/var/folders/ paths)
  - fetchPnpmDeps uses 'pnpm --force' which downloads ALL platform
    variants regardless of stdenv (sharp-libvips-{linux,darwin,linuxmusl}-*
    all fetched)
  - got: sha256-xcT+qRNmGjF7FmvPv4X25RlntZN8SAiVdWqH4lreS5o=

The #509 comment ('hashes differ by platform') was accurate for master,
but this branch's deps changes collapsed the distinction. The if-split
is kept to preserve master's architecture.
After the nix build, run the derivation's binary to catch
cross-platform cache hits. fetchPnpmDeps content hashes can match
across platforms, letting Nix serve the wrong-platform build from
cache — 'nix build' alone won't notice, but invoking the binary will
fail immediately on the wrong OS/arch.

Added to both _linux-fanout (parallel with home-manager, e2e) and
_darwin (serial before e2e — smoke is faster, so fail fast).
Restore default.nix and ci/mod.just to match origin/master. Master
already has the equivalent binary-smoke CI step (renamed 'run') which
supersedes this branch's 'smoke' iteration.
@srid
Copy link
Copy Markdown
Member Author

srid commented Apr 14, 2026

Closing in favor of #514 — reframed as a code-review surface driven by git state, not a file tree. The listDir / tree-view approach in this PR solved the wrong problem; see the issue for the 4-phase plan.

@srid srid closed this Apr 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant