Skip to content

feat: [ENG-2823] render folder metadata in webui context tree#676

Merged
bao-byterover merged 3 commits into
mainfrom
feat/ENG-2823
May 19, 2026
Merged

feat: [ENG-2823] render folder metadata in webui context tree#676
bao-byterover merged 3 commits into
mainfrom
feat/ENG-2823

Conversation

@ncnthien
Copy link
Copy Markdown
Collaborator

Summary

Fixes the Updated by / Last Modified columns showing - for every folder row on the WebUI context tree dashboard (incl. root view). ENG-2823.

  • Webui (context-detail-panel.tsx): drop the type === 'blob' filter so folder paths are included in the metadata request (blobPathsmetadataPaths).
  • Shared UI (@campfirein/byterover-packages1.0.5): drop the node.type === "blob" guard in FolderDetail so folders render their values.
  • Transport JSDoc (context-tree-events.ts): clarify paths accepts file or folder paths.
  • Server tests (context-tree-handler.test.ts): three new GET_FILE_METADATA cases pin the folder-path contract — paths flow through to gitService.log unchanged, response shape matches files.

No server production-code change needed: the handler is already path-agnostic, and isomorphic-git's log({filepath}) resolves the path to a tree-oid for folders (latest commit touching any descendant) or a blob-oid for files, transparently.

Test plan

  • npm test — 7853 passing, 16 pending
  • npm run typecheck — clean (root + webui)
  • npm run lint — 0 errors on changed files
  • npm run build — clean (tsc + Vite + PWA)
  • Manual: open brv webui, navigate to root context view, confirm Updated by / Last Modified populate for each folder (latest commit touching any descendant)
  • Manual: drill into a folder containing files, confirm file rows still show per-file metadata

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 19, 2026

Claude finished @ncnthien's task in 3m 1s —— View job


Claude Code Review

  • Read PR metadata
  • Read changed files
  • Analyze correctness, security, code quality
  • Post inline comments for specific issues
  • Post summary review with score

Summary

Small, well-scoped fix. The root cause is correctly identified as a client-side over-filter (type === 'blob') — the server handler is already path-agnostic because gitService.log({filepath}) accepts both files and folders via isomorphic-git's tree-oid walk. No server-side production code change was needed, which is the right call.

Correctness:

  • context-detail-panel.tsx: dropping the n.type === 'blob' filter and renaming blobPathsmetadataPaths correctly surfaces folder rows to the metadata query. The blob-detail branch (line 112) is untouched, so file rendering is unaffected.
  • The behaviour depends on isomorphic-git's log({filepath: <folder>}) resolving to a tree-oid and returning the latest commit touching any descendant — that is the documented behaviour, but the unit tests stub gitService.log, so they can't catch a regression there. The unchecked manual checkbox in the PR description is the real backstop for this — recommend completing it before merge.
  • The shared-UI bump (1.0.3 → 1.0.5) drops the symmetric guard in FolderDetail. Both halves of the change are required for the fix to be visible.

Security:
No new attack surface. Paths flow to gitService.log which already accepted arbitrary string paths; no filesystem read/write keyed on these paths. The existing handleUpdateFile path-traversal guard is unchanged and unrelated.

Tests: ✅ (with minor strengthening suggestion inline)

  • Three new cases pin the folder-path contract on the handler side.
  • The "mix of file and folder paths" test could be strengthened to verify per-path metadata mapping survives Promise.all — see inline. Non-blocking.
  • Pre-existing nit (out of scope): describe('setup', ...) still asserts "all four event handlers" and doesn't check GET_FILE_METADATA is registered — fine to leave for a follow-up.

Code quality:

  • metadataPaths rename improves clarity.
  • Transport-events JSDoc was updated correctly, though the "via isomorphic-git tree-oid walk" wording leaks an implementation detail into the contract — see inline. Also flagged: the sibling GET_HISTORY JSDoc still says "File path", now inconsistent with the path-agnostic reality.

Perf: ⚠️ minor watch-item
The change roughly doubles the per-folder fanout of gitService.log calls (folders + files vs. files only). Not a regression introduced by this PR's mechanism, but folder counts cluster at the top of the tree. Worth a quick measurement on a heavy repo before shipping — see inline.


Score: 4.5 / 5

Tight, surgical fix with proper TDD coverage, clear PR description, and correctly-identified root cause. Half a point off for: (1) the unit tests can't actually validate the folder-path-to-tree-oid claim that the whole fix rests on (manual verification is the only check), (2) one JSDoc inconsistency between the two sibling endpoints, and (3) the test-strengthening opportunity in the mix-paths case.

Inline comments:

  • test/unit/.../context-tree-handler.test.ts:552-571 — strengthen per-path mapping assertion
  • src/shared/transport/events/context-tree-events.ts:83-86 — soften implementation-detail leak; flag sibling JSDoc inconsistency
  • src/webui/features/context/components/context-detail-panel.tsx:57-65 — praise + perf watch-item
    Branch: feat/ENG-2823

Comment thread test/unit/infra/transport/handlers/context-tree-handler.test.ts
Comment thread src/shared/transport/events/context-tree-events.ts
Comment thread src/webui/features/context/components/context-detail-panel.tsx
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 19, 2026

Claude finished @ncnthien's task in 2m 21s —— View job


Claude Code Review (post-update)

  • Read PR metadata and prior review feedback
  • Inspect the new commit (b0af27d) for the test strengthening + JSDoc tightening
  • Re-verify changed files against author's reply
  • Run lint / typecheck on changed files (skipped — prior CI run on this branch confirmed npm test 7853 passing, typecheck/lint/build clean; the two changes since are docstring text + test-only)
  • Post updated review with score

Review of follow-up commit b0af27d

The two actionable suggestions from the prior review were applied exactly as proposed:

1. Test strengthening (context-tree-handler.test.ts:552–572) — ✅ resolved

  • Stub now returns Carol-${filepath} per call, and the assertion uses a Map keyed by path to pin the response-to-request mapping. This catches the Promise.all-order-vs-mapping bug class.
  • Verified at test/unit/infra/transport/handlers/context-tree-handler.test.ts:552–572.

2. JSDoc tightening (context-tree-events.ts:83–86) — ✅ resolved

  • The implementation-detail clause (via isomorphic-git tree-oid walk) is gone. The contract now reads "latest commit that modified any descendant of the folder" — backend-agnostic.
  • The sibling GET_HISTORY JSDoc inconsistency (line 109 still reads "File path") was explicitly deferred. That's a defensible call: the webui only calls GET_HISTORY with file paths (gated by isFilePath(selectedPath) at context-detail-panel.tsx:42), so the JSDoc reflects today's caller, not the underlying capability. Worth opening a follow-up but not blocking.

3. Perf watch-item (context-detail-panel.tsx:57–65) — defended, accepted

  • The author's defense is sound: context-tree roots cluster at ~10–30 top-level folders in practice, each per-folder gitService.log({depth: 1, filepath}) short-circuits on the tree-oid (cheaper than the per-file blob-diff walk), and the right fix for any pathological case is a batched server-side endpoint — out of scope for this ticket.
  • No regression introduced.

Additional cross-checks

  • Submodule ↔ lockfile consistency: packages/byterover-packages HEAD 72327af1… matches package-lock.json resolved SHA …#72327af1…. Internally consistent.
  • Rename hygiene: zero leftover blobPaths references across src/ and test/. The metadataPaths rename is complete.
  • Untouched correctness paths: the file-detail branch guard selectedNode?.type === 'blob' at context-detail-panel.tsx:112 is preserved — that's a rendering switch, not a metadata-fanout filter, and removing it would be wrong. Correctly left alone.
  • Server handler: handleGetFileMetadata (context-tree-handler.ts:102–126) is path-agnostic and untouched. The fix is entirely consumer-side, which matches the PR description.

Outstanding (non-blocking)

  • The manual checkbox in the PR description (open brv webui, navigate to root context view, confirm Updated by / Last Modified populate for each folder) is still unchecked. As noted previously, the unit tests stub gitService.log so they cannot validate the isomorphic-git tree-oid behaviour that the fix relies on — the manual verification is the only real backstop. Recommend completing before merge.
  • Follow-up: align ContextTreeGetHistoryRequest.path JSDoc with the path-agnostic reality (cosmetic only).

Score: 4.8 / 5

Both actionable inline comments were addressed precisely; the perf concern was given a defensible, well-reasoned response. The remaining 0.2 is the unverified manual smoke test — the same caveat as before, since no unit test can validate the folder-path-to-tree-oid contract while gitService.log is stubbed.
Branch: feat/ENG-2823

@bao-byterover bao-byterover merged commit b1feb03 into main May 19, 2026
6 checks passed
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.

3 participants