mirror of
https://github.com/OpenHands/OpenHands.git
synced 2026-09-29 17:19:17 +08:00
* Add Files tab with file viewer and diff view modes Replaces the previous Changes and App (served-host) tabs with a unified Files tab that supports two top-level modes: - Diff View (default when working inside a git repo): renders the existing Changes UI in-place. - File Viewer: shows a quick-access row of the most important files (index.html, package.json, README.md, etc.) with an overflow dropdown, plus an on-demand expandable file tree. Selecting a file shows its content with a Rich/Plain toggle. Rich mode renders HTML, markdown, and images in a sandboxed iframe; Plain mode shows plaintext with a binary-fallback message. Includes a new useIsGitRepo hook and supporting workspace-files hooks plus utilities for sorting files by priority and building a tree from flat paths. Co-authored-by: openhands <openhands@all-hands.dev> * fix(files-tab): only default to diff view when a repo was explicitly attached The previous detection relied on whether 'git status' succeeded against the workspace, but the agent-server initialises every workspace as an internal git worktree for change tracking. As a result, a brand-new conversation with no user-attached repo was incorrectly treated as a git repo and the Files tab opened in diff view. Drop the filesystem probe and use the conversation's 'selected_repository' as the sole signal — that's the field populated by the repo picker for 'an existing git repo' from the user's point of view. Co-authored-by: openhands <openhands@all-hands.dev> * feat(files-tab): refresh button covers file list, auto-refresh on edits Two related fixes to the Files tab data lifecycle: 1. The toolbar refresh button used to only refetch git changes (the diff view). It now also invalidates the workspace file list and any cached file contents, so clicking it works as expected in both modes. 2. Add a useAutoRefreshFilesOnEdit hook mounted by FilesTab that watches the conversation event store and invalidates the workspace-files, workspace-file-content and file_changes queries whenever the agent produces a mutating file-editor observation (create / str_replace / insert / undo_edit). Read-only 'view' commands and non-file observations are ignored. The hook is array-position based so it processes each event exactly once. Tests: 4 new for the hook, all existing files-tab and conversation-tabs tests still pass. Co-authored-by: openhands <openhands@all-hands.dev> * feat(files-tab): tree toggle, full paths, real markdown rendering UI changes to the workspace file viewer: - Replace the trailing 'more files' overflow dropdown with a single caret button on the LEFT of the quick-access row that toggles the left-hand file tree. Tree is shown by default; users who want more horizontal space for the content pane can collapse it. There is no longer a dropdown listing extra files — anything that doesn't fit in the pills row is reachable by opening the tree. - Pills in the quick-access row now display the full relative file path (e.g. 'src/main.ts') instead of just the basename, so users can distinguish between same-named files in different folders at a glance. The full path also serves as the tooltip. - Markdown files are now rendered via the existing MarkdownRenderer (react-markdown + remark-gfm + remark-breaks) inside a styled prose container. The old approach piped raw text into a sandboxed <iframe> wrapped in <pre>, which displayed unrendered markdown source. The iframe path is removed for .md / .markdown / .mdx files. Tests: 3 new cases in files-tab.test.tsx (full-path pills, tree-toggle round-trip, markdown rendering with h1 + bold + no iframe). All 28 existing files-tab / conversation-tabs / auto-refresh tests still pass, plus markdown component tests (27/27). AGENTS.md now records the worktree policy: don't auto-switch the main workspace away from the worktree's branch unless the user explicitly asks. Co-authored-by: openhands <openhands@all-hands.dev> * feat(files-tab): static fileserver + tab/toolbar layout pass Switches the Files tab to the agent server's static workspace fileserver (software-agent-sdk PR #3192) and shuffles the layout in response to real-world usage feedback. == Static workspace fileserver == The agent server now exposes each conversation's workspace at GET /api/conversations/{conversation_id}/workspace/{file_path:path}. A new utility, buildWorkspaceFileUrl, composes that URL from the conversation_url (host + optional path-prefix for proxy deployments) and conversation id. useWorkspaceFileContent is refactored to: * always expose a 'staticUrl' field, so consumers can point an iframe or <img> at the same-origin fileserver and benefit from relative asset resolution. * skip fetching the bytes for image/PDF kinds entirely (the consumer renders staticUrl directly). * for text-classified files, fetch via the static URL with X-Session-API-Key auth instead of going through the typescript- client RemoteWorkspace.downloadAsBlob (which we no longer need for this view). * drop blobUrl and absolutePath from WorkspaceFileContent — they were only used by the iframe renderer, which now uses staticUrl. FileContentViewer in rich mode: * HTML/SVG and PDFs render with <iframe src={staticUrl}> (no sandbox) so relative asset references load against the same origin. * Images render with <img src={staticUrl}> (no blob URL plumbing). * Markdown still uses MarkdownRenderer; plain mode unchanged. Caveat (intentional, matches user spec): on agent servers with a configured session_api_keys list, iframe src cannot send the X-Session-API-Key header, so rich HTML/PDF previews won't load there. Unauthenticated (auto_error=False, empty key list) servers — the default for local dev — work. == Tab bar / toolbar layout == * Files tab moved to the leftmost slot in ConversationTabs (was second, after Planner). Task-list insertion adjusted from unshift() to splice(1, 0, ...) so Files stays leftmost even when present. * Refresh button removed from the top tab bar and re-homed inside the FilesTab toolbar; it now sits next to the diff/files and rich/plain toggles on the right edge of that row. * Rich/Plain toggle moved from the right-hand side of the toolbar to sit immediately next to the Diff/Files toggle on the left (justify-between → flex-start gap-3, with the refresh button using ml-auto). * Left-hand file tree collapsed by default (was expanded). The quick-row caret on the pill row is the toggle, as before. * 'Diff view' label shortened to just 'Diff' across all 15 locales in translation.json (key FILES retained to avoid a noisy rename in the generated declaration.ts). == Tests == * New: __tests__/utils/workspace-file-url.test.ts (7 tests) covers null guards, encoding of path segments, leading-slash stripping, omitted relativePath, and proxy-deployment path prefixes. * New: __tests__/i18n/files-diff-label.test.ts locks the renamed English label in translation.json (the test environment's i18next mock returns keys, so we assert against the source-of-truth file). * files-tab.test.tsx: - mock content shape updated to use staticUrl (no more blobUrl / absolutePath). - default-state expectations updated for the collapsed tree. - tree-toggle test inverted: hidden → expand → hide. - new test asserts HTML files render as <iframe src={staticUrl}> (no sandbox attribute). - new test asserts the refresh button lives in the files-tab toolbar and triggers refetchGitChanges. * conversation-tabs.test.tsx: - old 'refresh button in the top tab bar' test replaced with the inverse assertion (no standalone refresh <button> there now). - two new tests pin Files as the leftmost tab in both has-tasklist and no-tasklist cases. All 40 focused tests pass; typecheck clean; src/ lint + prettier clean. Co-authored-by: openhands <openhands@all-hands.dev> * feat(files-tab): GFM+safe HTML markdown; suppress diff default on empty repos == Markdown rendering == MarkdownRenderer was only running remark-gfm + remark-breaks. Raw inline HTML in markdown source (e.g. <details>, <kbd>, <mark>, GitHub-style badges, or anchor target=_blank tricks) was silently dropped by react-markdown's default behaviour. This commit: * Adds rehype-raw to parse raw HTML embedded in markdown into the rehype tree. * Adds rehype-sanitize to strip anything dangerous before render -- scripts, event handlers, javascript:/data: URLs in href, and any tags not in the allow-list. * Extends the default sanitize schema with markdown-friendly extras (className/id/style on all elements; target+rel on anchors; img with safe src schemes only; <details>/<summary>, <figure>/ <figcaption>, <mark>, <kbd>, <sub>, <sup>). * Restricts URL protocols on href to http/https/mailto/tel and on src to http/https/data (so data:image/... still works for inline base64 images, but data:text/html -- XSS vector -- does not). The new behaviour is opt-out via a new MarkdownRenderer prop, 'allowHtml', which defaults to true. The sanitize schema makes raw HTML safe by construction, so on-by-default is the right call -- and it's consistent with how GitHub and most markdown renderers behave. 11 new markdown-renderer tests cover GFM tables/strikethrough/task lists, inline HTML rendering (<mark>, <kbd>, <details>/<summary>), sanitisation of <script>/onclick/javascript:/<iframe>, safe URL schemes (https/mailto) passing through, and the allowHtml=false opt-out path. All 217 tests in the chat / diff-viewer / planner / conversation-panel suites still pass -- confirming no regression from making allowHtml default-on for the existing call sites. == Diff-view default for empty repos == The Files tab was defaulting to diff view whenever the conversation had a selected_repository, even on attached repos with zero commits (unborn HEAD -- e.g. a freshly-created empty GitHub repo). In that state the diff view has nothing to diff against and looks broken; the file viewer is a much better landing experience. * Adds src/hooks/query/use-has-git-commits.ts -- a thin useQuery- backed hook that shells out via the conversation's RemoteWorkspace to run 'git rev-parse --verify HEAD' in the working dir. Exit 0 -> hasCommits: true; non-zero -> false. The enabled flag is plumbed through so we don't probe when there's no attached repo to check (saves a workspace round trip on every plain conversation). * FilesTab now derives diffViewDefault as isGitRepo && hasCommits !== false -- i.e. only enables diff by default when both conditions hold, treating the in-flight 'null' state as optimistically true so we don't get a files->diff flash on the common path. 3 new files-tab tests cover empty-repo behaviour, the enabled-gating of the probe when no repo is attached, and the optimistic in-flight default. == Tests / Quality gates == * typecheck: clean (react-router typegen + tsc). * eslint + prettier on changed src files: clean. * Vitest focused run: 52/52 (markdown + files-tab). * Vitest chat + diff-viewer + planner + conversation-panel: 217/217 (no regressions from default-on allowHtml). Co-authored-by: openhands <openhands@all-hands.dev> * feat(files-tab): mint workspace cookie via startWorkspaceSession for iframe/img auth Bumps @openhands/typescript-client to feat/workspace-static-session (f062287d) which adds POST /api/auth/workspace-session. Calling it exchanges the X-Session-API-Key for an HttpOnly cookie scoped to /api/conversations -- which is the only auth mechanism the browser attaches to top-level <iframe src> / <img src> requests. == New plumbing == * src/api/typescript-client.ts: factory createRemoteConversation(id) that wraps RemoteConversation with a placeholder Agent (required by the constructor but unused for startWorkspaceSession). RemoteConv and Agent come from the package root -- they're not exposed under the package's subpath exports. * src/hooks/query/use-workspace-session.ts: useWorkspaceSession hook that fires startWorkspaceSession once per conversation via react- query, caches the result with staleTime: Infinity (the cookie sticks in the browser jar; re-issuing the POST is wasted work), and exposes { baseUrl }. retry: false because a 401 here is a fixed config issue, not transient. * Same file: joinWorkspaceUrl(baseUrl, relativePath) -- URL-encodes each path segment but preserves "/" separators. Replaces the standalone src/utils/workspace-file-url.ts which is removed. == Refactor == * use-workspace-file-content.ts now derives staticUrl from useWorkspaceSession's baseUrl via joinWorkspaceUrl, gates the query on !!baseUrl (so we don't fire fetches against an unauthenticated URL), and switches fetch() to credentials: "include" instead of the X-Session-API-Key header. This makes our JS fetch ride the same auth path as the iframe/img -- one behavior, one CORS story, no preflight for a custom header. == Tests == * __tests__/hooks/query/use-workspace-session.test.tsx -- 9 tests: happy path (POST fires, baseUrl flows through, createRemote Conversation gets the right args); runtime-not-ready and no- conversation-id both gate the POST; error surfaces as isError + error.message; joinWorkspaceUrl covers empty / single-segment / nested / leading-slash / unicode + space encoding. * Deleted __tests__/utils/workspace-file-url.test.ts -- the helper it pinned is gone, joinWorkspaceUrl is covered by the new tests. == Quality gates == * typecheck clean (react-router typegen + tsc). * Targeted vitest run: 251/251 across markdown / files-tab / chat / diff-viewer / planner / conversation-panel / workspace-session. * Lint has known issues in the new test file (display-name and function-component-definition warnings on the QueryClient wrapper factory; no-promise-executor-return on the `await new Promise(r => setTimeout(r, 10))` polling -- both patterns already in use in other __tests__/hooks/query/*.test.tsx files); will sweep separately. Co-authored-by: openhands <openhands@all-hands.dev> * chore(files-tab): clear remaining lint errors on the branch * __tests__/hooks/query/use-workspace-session.test.tsx: name the QueryClientProvider wrapper component (silences react/display-name + react/function-component-definition) and lift the setTimeout("yield to scheduler") trick into a flushScheduler() helper (silences no-promise-executor-return -- ESLint forbids returning a value from a Promise executor). * src/utils/conversation-local-storage.ts: replace the destructure- with-throwaway-name pattern (which tripped naming-convention on `_drop`) with a plain spread + delete -- one statement clearer, no rename gymnastics. Also re-wrap the signature so prettier is happy. * src/components/features/conversation/conversation-tabs/conversation- tab-content/conversation-tab-content.tsx: prettier reformat (one long line broken). Verified: full `npm run lint` passes (10 pre-existing warnings in files outside this branch's scope remain). Co-authored-by: openhands <openhands@all-hands.dev> * feat(files-tab): use RemoteWorkspace.startWorkspaceSession directly typescript-client PR #155 was reshaped to land startWorkspaceSession on RemoteWorkspace (taking conversationId as an argument) rather than on RemoteConversation. The new shape is strictly simpler for our use case: minting a workspace cookie no longer requires constructing a placeholder Agent + RemoteConversation just to call one method. Changes: * package.json / package-lock.json: bump @openhands/typescript-client pin to github:OpenHands/typescript-client#6b5a65c5 (head of feat/workspace-static-session, sole commit on PR #155). * src/api/typescript-client.ts: drop the createRemoteConversation factory along with the Agent + RemoteConversation imports it needed -- callers can now go through createRemoteWorkspace (which already existed for git-service) for everything we use. * src/hooks/query/use-workspace-session.ts: call createRemoteWorkspace({ conversationUrl, sessionApiKey }) and then workspace.startWorkspaceSession(conversationId). Same return value (baseUrl string), same caching semantics. * __tests__/hooks/query/use-workspace-session.test.tsx: rename the mocked factory + assertions to match. The hook's surface (data, isLoading, isError, error) is unchanged so the rest of the suite is untouched. Verified: typecheck clean, workspace-session + files-tab tests (23/23) pass, full `npm run lint` reports 0 errors (10 pre-existing warnings in unrelated files remain). Co-authored-by: openhands <openhands@all-hands.dev> * feat(files-tab): persist toggles + cache-bust iframe + depth-first sort + external-open link Five behavioural improvements to the Files tab, plus the corresponding typescript-client pin bump now that PR #155 has merged to main. 1. Persist diff-view + rich/plain choice per-conversation. Both Files tab toggles now live in conversation localStorage rather than transient component state, so switching to another conversation and back restores whatever view the user last selected. Implemented by adding `filesTabDiffView` (nullable -- null means 'fall back to repo-aware default') and `filesTabContentViewMode` to `ConversationState`, exposing matching setters from `useConversationLocalStorageState`, and wiring the toolbar `SegmentedToggle`s to them. When the hook is called with an empty or task-placeholder id it now mirrors updates into local React state instead of dropping them on the floor -- keeps the UI reactive in unit tests / pre-route renders. 2. Fix initial files→diff flash inside a real repo. While `useIsGitRepo` is still loading we now stay optimistic (`isGitRepo || isGitRepoLoading`), matching the existing optimism around `hasCommits`. The user-visible bug was: a brand new conversation attached to a git repo would show files-view for one frame before flipping to diff-view, and any persisted choice we made during that frame stuck. 3. Cache-bust iframes / images after every file-editor observation. New `useWorkspaceMutationCounter` zustand store: a monotonic counter `useAutoRefreshFilesOnEdit` bumps every time it sees a mutating `FileEditorObservation` / `StrReplaceEditorObservation` / `PlanningFileEditorObservation`. `FileContentViewer` and the toolbar 'open in new window' link append it to the workspace static URL as `?v=<n>`, so the browser refetches HTML/CSS/images the agent just rewrote on disk instead of showing the stale cached response. Read-only `view` observations and unrelated kinds (e.g. `ExecuteBashObservation`) don't bump. 4. Depth-first file ranking. `sortFilesByPriority` now sorts by path depth first (shallower wins unconditionally), then by high/secondary basename importance, then alphabetically. Concretely: top-level `README.md` outranks `foo/bar/index.html`, but `index.html` still beats `README.md` at the same depth. Updated docs + added tests for the new contract. 5. 'Open in new window' affordance. New external-link button in the Files tab toolbar, rendered next to the refresh button while in file-viewer mode whenever a file is selected and we've resolved its workspace static URL. Hidden in diff-view (no single meaningful URL to point at). Cache-bust query string applied so the popped-out tab also sees the latest bytes. Also bumps `@openhands/typescript-client` to `ef62e82fc3dfb03991a1c8025429caf354427263` -- the merge commit of PR #155 on main. No API change vs the previous `6b5a65c5...` pin (that was the only commit on the merged branch); this just gets us off the soon-to-be-deleted feature branch ref. Tests: * Full suite: 1805 passed / 12 skipped / 9 todo across 283 files. * `__tests__/utils/file-priority.test.ts`: added two new cases pinning the depth-first + same-depth rules. * `__tests__/hooks/use-auto-refresh-files-on-edit.test.tsx`: added two cases pinning the mutation-counter bump (yes on mutations, no on `view` / non-file observations). * `__tests__/routes/files-tab.test.tsx`: existing iframe-src test switched from strict equality to a `^staticUrl\?v=\d+$` regex to allow the cache-buster suffix. * `__tests__/hooks/use-draft-persistence.test.tsx` and `__tests__/hooks/use-handle-plan-click.test.tsx`: fixtures updated to include the new ConversationState fields and matching setter mocks. Lint clean (0 errors; 10 pre-existing warnings remain). Typecheck clean. New i18n key `FILES$OPEN_IN_NEW_WINDOW` translated across all 15 locales. Co-authored-by: openhands <openhands@all-hands.dev> * fix(files-tab): address PR #284 review comments - Markdown sanitizer: drop `style` attribute from allowlist (CSS-injection / data-exfiltration channel) and remove `data:` from `src` protocol allowlist (data:text/html bypass). - HTML / PDF preview iframes: add `sandbox="allow-same-origin"` so scripts and inline event handlers in previewed files cannot execute in the canvas context while relative asset refs still resolve. - Auto-refresh hook: track processed event ids in a Set instead of slicing the tail by length, so out-of-order events inserted into the (sorted) event store still trigger invalidation + cache-bust. - File tree builder: replace O(n) `children.find` with an O(1) side-table Map per parent; promote a leaf node to a directory if a deeper path arrives later. - Conversation localStorage: filter removed tab names ("editor", "served", "changes", "app") out of `selectedTab` and `unpinnedTabs` on read so ghost entries don't linger. - File-content viewer: distinguish load-error from binary-fallback with a new `FILES$LOAD_ERROR` i18n key. - package.json: exact-pin `rehype-raw` and `rehype-sanitize` (no caret). - Tests: add security regressions (style attr, data:text/html, inline event handlers), tree promotion + wide-dir smoke test, out-of-order mutation event test, unpinnedTabs migration test; update HTML-preview iframe sandbox assertion. Co-authored-by: openhands <openhands@all-hands.dev> * Update src/components/features/markdown/markdown-renderer.tsx Co-authored-by: OpenHands Bot <contact@all-hands.dev> * feat(files-tab): drop "read-only" from terminal title, theme markdown preview, prism-highlight source/plain views Three small UI changes that go together: 1. Terminal tab title — strip the "(read-only)" qualifier from COMMON$TERMINAL in every locale. The fact that the embedded xterm doesn't echo stdin is internal plumbing; users just want to see "Terminal" in the tab strip. 2. Rich-mode markdown preview — paint the wrapper in the right-pane chrome color (\#25272D) and force every text node to white. The old `bg-white text-[#222]` made markdown files look like a stark card floating on the dark canvas. Switched to `prose prose-invert` and layered arbitrary CSS-variable utilities (`[--tw-prose-body:#fff]` et al.) on top, because the typography plugin's prose-invert default is off-white (#e5e5e5) rather than pure white. Existing custom heading components already use text-white and continue to win. 3. Source-code views (rich AND plain) and plain views of markdown/HTML — feed everything through the existing PrismLight pipeline used by chat code-block rendering. New `HighlightedSourceView` component wraps SyntaxHighlighter with vscDarkPlus + a transparent background so the highlighter blends with the right-pane chrome instead of painting its own slab. New `getPrismLanguageForFile` util resolves extensions (.ts, .py, .yaml, ...) and well-known no-extension filenames (Dockerfile, Makefile, .bashrc, ...) to Prism grammars, falling back to a raw `<pre>` when nothing matches. Behavior matrix in the files tab now reads: Rich mode: HTML/SVG -> sandboxed iframe preview Markdown -> rendered (dark bg, white text) Image -> <img> PDF -> sandboxed iframe Source -> highlighted source (no other "rich" form for code) Plain mode: Source -> highlighted source Markdown -> highlighted markdown source (see the markup) HTML -> highlighted markup source Other -> raw <pre> fallback (rare) Binary -> binary fallback message Tests: - New unit tests for getPrismLanguageForFile (extensions, no-ext filenames, case-insensitive, mime-type fallback, null on unknown). - files-tab integration tests now assert the markdown wrapper paints bg-[#25272D]/text-white, and that toggling .md to plain renders highlighted markdown source rather than rich markup. Lint and full test suite intentionally not re-run on this commit; follow-up commits address upstream issues and PR review feedback. Co-authored-by: openhands <openhands@all-hands.dev> * fix(markdown): repair botched suggestion in fc208bc — collapse rel-attribute schema fc208bc applied PR review feedback via the GitHub web-UI "commit suggestion" button, but the suggested replacement ended up *inside* the existing array literal instead of replacing it: a: [ ...(defaultSchema.attributes?.a ?? []), "target", a: ["href", "title", "target", "rel"], // ← syntax error ], That's a labeled-statement-like token inside an array literal — it fails both `tsc` and `eslint` parsing, breaking the branch's build and typecheck for everyone pulling this PR. Fix-forward (preserves rbren's authorship of fc208bc in history) by applying the reviewer's actual intent: collapse the `a` allow-list to `["href", "title", "target", "rel"]`. This addresses the security concern the reviewer raised in thread 3215928329 — the old `["rel", "noopener", "noreferrer", "nofollow"]` form is rehype-sanitize's exact-value variant, which strips the standard space-separated `rel="noopener noreferrer"` and reintroduces a reverse-tabnabbing vector on raw HTML anchors with `target="_blank"`. Added a comment in the schema explaining the reasoning. A regression test for this is added in the next commit (PR review feedback round 2) alongside the rest of the round-2 fixes. Co-authored-by: openhands <openhands@all-hands.dev> * fix(files-tab): address PR #284 round-2 review comments Round 2 of review feedback on PR #284. The five threads addressed here all came in together at 01:39 UTC; the related rel-attribute schema fix is in the previous commit (7c099bf). Hook (use-auto-refresh-files-on-edit): * Guard against undefined event.id (3215928337). The event store accepts events with no id (`getEventId` returns `string | number | undefined`). The previous version of the hook blindly called `has(event.id)` / `add(event.id)`, which put the literal `undefined` into the Set on the first id-less arrival and then silently swallowed every subsequent id-less event because the Set already contained that key. Now we only consult/touch the set when the id is defined; id-less events are always treated as new. * Widen processedIdsRef from Set<string> to Set<string | number> (3215928342). The formal EventID type is string, but the event store itself uses Set<string | number> defensively and getEventId returns string | number | undefined — the hook mirrors that tolerance so a stray numeric id never sneaks past dedup. Tests added for the above (3215928347): * `does NOT deduplicate id-less events (every id-less arrival is a new event)` — three distinct id-less FileEditorObservations land in sequence and we expect three counter bumps; with the old implementation only the first would land. * `dedupes numeric event ids the same way as string ids` — same numeric-id event added twice produces exactly one counter bump. Markdown sanitizer (3215928336): * Export MARKDOWN_SANITIZE_SCHEMA so tests can target the schema directly. Wrote a docstring explaining why the through-component test the reviewer suggested wouldn't catch the bug: our custom `anchor` component hard-codes `target="_blank" rel="noopener noreferrer"`, so the final DOM is safe regardless of what the schema does to HAST. We have to test the schema in isolation. * Two new `describe("MARKDOWN_SANITIZE_SCHEMA")` tests that run hast-util-sanitize directly on hand-built HAST trees: - rel="noopener noreferrer" survives sanitization (regression for the fc208bc bug — the old exact-match schema would have stripped it) - rel="nofollow ugc" also survives (locks in the property that *any* rel-token combination is safe, since rel doesn't execute code or navigate) Conversation local storage (3215928349, 3215928351): * Existing unpinnedTabs filter test extended from `["editor", "changes", "served"]` to all four removed tabs (`+"app"`). The previous version of the test missed "app" and that gap is exactly what let the original whitelist-vs-denylist bug slip through. * New `describe("filesTabDiffView persistence")` block with four tests: default-null, round-trip true, round-trip false, isolation between conversations. The boolean is per-conversation and its default-null is load-bearing — the higher layer relies on null to apply the git-repo-aware default. * New `describe("filesTabContentViewMode persistence")` block with four tests: default-'rich', round-trip 'plain', round-trip explicit 'rich', isolation between conversations. The 'rich' default is locked in so a careless field-initializer rename doesn't quietly flip every user from rich to plain after deploy. Lint and full test suite intentionally not re-run on this commit per the user's instruction; will run on the next pass once any further review feedback is in. Co-authored-by: openhands <openhands@all-hands.dev> * fix(files-tab): make CI green (lint + correct id-less event dedup) CI was red on `4e9271b` for two reasons: 1. **Lint errors** (prettier + no-continue) that I'd skipped on the user's instruction during the round-2 commit: * `src/hooks/use-auto-refresh-files-on-edit.ts:75` — `no-continue` violation from the new id-guard. Rewrote to use an inverted `alreadyProcessed` predicate instead of `continue` so the control flow reads the same way without breaking the lint rule. * `src/utils/file-language.ts` — three prettier diffs (string-quote style + Math.max arg wrapping). Auto-fixed via `eslint --fix`. * `src/components/features/files-tab/file-content-viewer.tsx` — two prettier wraps on long className + JSX expression that showed up after the round-1 round of edits. Auto-fixed. 2. **A real bug** the lint-fix exposed in the round-2 test: the suggested test (and mine, which followed it) asserted `counter === 3` after adding three id-less events inside a single `act()` block. That assertion is wrong because the hook bumps the counter exactly once per effect flush (the bump is outside the event loop), and act() batches all three addEvent calls into one flush. But while investigating, I found the underlying bug the reviewer was probing toward: id-less events were not durably deduped. The events array is rebuilt on every store mutation but its element references are stable, so a single id-less event would re-trigger the bump on every subsequent render — spamming cache invalidations forever. Fix: added a second tracking store, `processedEventsRef`, a `WeakSet<OHEvent>` keyed by object reference. Id-less events dedup by reference; id-bearing events still dedup by id. The WeakSet doesn't pin the events in memory after the store clears them, so there's no leak. Long explanatory comment in the hook covers both halves of the contract (why we can't put `undefined` in the id Set, and why id-less events still need *some* form of dedup). Tests restructured accordingly: * `processes each id-less event distinctly` — three SEPARATE `act()` calls (one per event) so each gets its own effect flush; counter ends at 3. Comment explains why the single-act() version of this assertion is meaningless. * `does NOT re-bump on subsequent renders for the same id-less event` — NEW test that catches the spurious-rebump bug directly: add one id-less event, then `rerender()` three extra times, assert counter stays at 1. This would fail against the previous version of the fix (no WeakSet path). Verified locally: `npm run typecheck` clean, `npm run lint` 0 errors (10 pre-existing warnings in files I didn't touch), `npm test` all 1862 tests pass (286 files, 12 skipped + 9 todo — all pre-existing). Co-authored-by: openhands <openhands@all-hands.dev> --------- Co-authored-by: openhands <openhands@all-hands.dev> Co-authored-by: OpenHands Bot <contact@all-hands.dev>
75 lines
2.7 KiB
TypeScript
75 lines
2.7 KiB
TypeScript
import { describe, it, expect } from "vitest";
|
|
|
|
import { buildFileTree } from "#/utils/file-tree";
|
|
|
|
describe("buildFileTree", () => {
|
|
it("builds a nested tree from flat paths", () => {
|
|
const root = buildFileTree([
|
|
"src/a.ts",
|
|
"src/sub/b.ts",
|
|
"README.md",
|
|
]);
|
|
|
|
expect(root.children.map((c) => c.name)).toEqual(["src", "README.md"]);
|
|
|
|
const srcDir = root.children.find((c) => c.name === "src");
|
|
expect(srcDir?.isDirectory).toBe(true);
|
|
expect(srcDir?.children.map((c) => c.name)).toEqual(["sub", "a.ts"]);
|
|
|
|
const readme = root.children.find((c) => c.name === "README.md");
|
|
expect(readme?.isDirectory).toBe(false);
|
|
expect(readme?.path).toBe("README.md");
|
|
});
|
|
|
|
it("sorts directories before files at every level", () => {
|
|
const root = buildFileTree([
|
|
"z-file.ts",
|
|
"dir/inner.ts",
|
|
"a-file.ts",
|
|
]);
|
|
const names = root.children.map((c) => c.name);
|
|
expect(names).toEqual(["dir", "a-file.ts", "z-file.ts"]);
|
|
});
|
|
|
|
it("does not duplicate directory nodes when many files share a directory", () => {
|
|
const root = buildFileTree([
|
|
"src/a.ts",
|
|
"src/b.ts",
|
|
"src/c.ts",
|
|
]);
|
|
expect(root.children).toHaveLength(1);
|
|
expect(root.children[0].children).toHaveLength(3);
|
|
});
|
|
|
|
it("returns an empty tree when given no paths", () => {
|
|
const root = buildFileTree([]);
|
|
expect(root.children).toEqual([]);
|
|
});
|
|
|
|
it("promotes a previously-leaf node to a directory when a deeper path needs it", () => {
|
|
// Regression test: feeding the builder a flat list that contains both
|
|
// `src` (treated as a file by virtue of having no further segments)
|
|
// and `src/index.ts` used to silently drop `index.ts` because the
|
|
// `src` node had `isDirectory: false` and we never descended into
|
|
// it. The builder now promotes the leaf to a directory.
|
|
const root = buildFileTree(["src", "src/index.ts"]);
|
|
|
|
const srcNode = root.children.find((c) => c.name === "src");
|
|
expect(srcNode).toBeDefined();
|
|
expect(srcNode?.isDirectory).toBe(true);
|
|
expect(srcNode?.children.map((c) => c.name)).toEqual(["index.ts"]);
|
|
});
|
|
|
|
it("handles very wide directories efficiently (regression: O(n) lookup)", () => {
|
|
// Just a smoke test — with the old O(n²) `find` lookup, building a
|
|
// tree of 5000 siblings took noticeably long. We don't time the
|
|
// call (flaky in CI); we just exercise the path to make sure the
|
|
// builder doesn't blow up and produces the right shape.
|
|
const paths = Array.from({ length: 5000 }, (_, i) => `pkg/file_${i}.ts`);
|
|
const root = buildFileTree(paths);
|
|
expect(root.children).toHaveLength(1);
|
|
expect(root.children[0].name).toBe("pkg");
|
|
expect(root.children[0].children).toHaveLength(5000);
|
|
});
|
|
});
|