Files
7ea79e0a39 feat(files-tab): Files tab with diff + rich/plain file viewer, safe-HTML markdown (#284)
* 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>
2026-05-10 19:29:16 -07:00

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);
});
});