cockpit: Add file chooser component

[mvollmer]

Demo: https://youtu.be/5XSY9Gw7JJM
In action in c-machines: https://youtu.be/Zcinb-Ny8BQ
For directories, includes "Open in file browser": https://youtu.be/A4ls61bGjdk

[cockpituous]

There are more than 10 code coverage comments, see the full report here.

[cockpituous]

There are more than 10 code coverage comments, see the full report here.

[cockpituous]

There are more than 10 code coverage comments, see the full report here.

[cockpituous]

There are more than 10 code coverage comments, see the full report here.

[cockpituous]

There are more than 10 code coverage comments, see the full report here.

[cockpituous]

There are more than 10 code coverage comments, see the full report here.

[cockpituous]

There are more than 10 code coverage comments, see the full report here.

[cockpituous]

There are more than 10 code coverage comments, see the full report here.

[mvollmer]

From Claude:

Code Review — cockpit: Add file chooser component

1. Crash: dangling symlink → null.type TypeError in watchFiles

File: pkg/lib/cockpit/react/FileChooser.tsx:256

In watchFiles, when a symlink's target is absent from both info.entries and
info.targets (e.g. a dangling symlink), the backend sets targets[name] = null.
The expression info.entries[entry.target] || info.targets[entry.target] then
evaluates to null, and the subsequent cockpit.assert(entry.type) throws a
TypeError reading .type of null — crashing the entire change handler and
breaking the file listing for that directory.

---

2. Bug: inverted error-suppression logic in getFileInfos

File: pkg/lib/cockpit/react/FileChooser.tsx:281

The condition is backwards:

if (!(ex && typeof ex == "object" && "problem" in ex && ex.problem != "not-found"))
    console.error("Failed to get file type:", p);

• When problem == "not-found" (expected, should be silent): inner AND is
  false → !false = true → console.error fires.
• When problem == "access-denied" (unexpected, should be logged): inner AND
  is true → !true = false → error is silently swallowed.

Fix: remove the outer !.

---

3. Resource leak: FsInfoClient not closed on unmount

File: pkg/lib/cockpit/react/FileChooser.tsx:448

The useEffect that starts the file watcher via setPath/setCollection
returns no cleanup function. When the dialog closes, fsInfoClientRef.current
is never closed. The client continues delivering change events and its callback
attempts to update state on a dead component until the channel is garbage
collected.

---

4. Bug: duplicate React keys in boldify

File: pkg/lib/cockpit/react/FileChooser.tsx:305

<u key={pos}> uses pos — the match offset within the remaining substring,
not the original string. When filterText appears consecutively in a filename
(e.g. "foofoo" with filter "foo"), the second iteration also finds pos = 0,
producing two siblings with key={0}. React warns and may reconcile
incorrectly. Fix: use a separate incrementing index as the key.

---

5. Dark mode: hardcoded color: black ignores PatternFly theming

File: pkg/lib/cockpit/react/FileChooser.css:63

.pf-v6-c-table tr.file-chooser-selected:where(.pf-v6-c-table__tr) > :where(th, td) {
    background: var(--pf-t--global--color--nonstatus--blue--default);
    color: black;
}

Cockpit supports dark mode via .pf-v6-theme-dark. color: black is always
black regardless of theme, bypassing the PatternFly semantic color tokens (e.g.
--pf-t--global--color--text--on-status-and-brand) that invert correctly per
theme. This breaks contrast guarantees in dark and high-contrast modes.

---

6. Missing await on async_sleep in playground shortcuts callback

File: pkg/playground/dialog.tsx:790

async () => {
    async_sleep(500);   // Promise discarded — no await
    return [{ label: "Test files", path: "/var/lib/cockpittest" }];
}

Every other call site in the file correctly uses await async_sleep(...). The
500 ms delay is skipped entirely, so the demo no longer exercises the
async-shortcut loading path as intended.

[cockpituous]

There are more than 10 code coverage comments, see the full report here.

[mvollmer]

From Claude:

1. Crash: dangling symlink → null.type TypeError in watchFiles

Fixed by skipping dangling symlinks altogether.

2. Bug: inverted error-suppression logic in getFileInfos

if (!(ex && typeof ex == "object" && "problem" in ex && ex.problem != "not-found"))

Changed to

if (!(ex && typeof ex == "object" && "problem" in ex && ex.problem == "not-found"))

We want to log everything expect a "not-found". Claude's fix would log only errors that do contain a ".problem" field. In practice it's probably the same, but...

3. Resource leak: FsInfoClient not closed on unmount

Fixed by returning a cleanup function from useEffect.

4. Bug: duplicate React keys in boldify

Fixed.

5. Dark mode: hardcoded color: black ignores PatternFly theming

Fixed by

.pf-v6-c-table tr.file-chooser-selected:where(.pf-v6-c-table__tr) > :where(th, td) {
    background: var(--pf-t--global--color--nonstatus--blue--default);
    color: var(--pf-t--global--text--color--nonstatus--on-blue--default);
}

6. Missing await on async_sleep in playground shortcuts callback

Fixed.

[mvollmer]

Second Claude round:

Code Review Findings

Bug: JSON.parse in listRecent throws on corrupted localStorage (FileChooser.tsx:388)

If localStorage[recentKey] contains non-JSON (corrupted data or a value written by another tool), JSON.parse throws a SyntaxError. The exception propagates through collection.list() into set_async, which catches and silently discards it — the
files field stays null forever and the listing shows an indefinite spinner with no error message to the user.

Both listRecent and rememberRecent (line 1064) need a try/catch around JSON.parse, returning [] on failure.

---

Bug: JSON.parse in rememberRecent surfaces a raw SyntaxError as the dialog error (FileChooser.tsx:1064)

Same root cause as above, different code path. If localStorage[recentKey] contains non-JSON, JSON.parse throws synchronously inside onAction. run_action catches it and stores it in dlg.error, which DialogErrorMessage renders as a "Failed"
alert showing the raw SyntaxError text (e.g. Unexpected token 'x' is not valid JSON) — a confusing internal error shown to the user when they click Select.

---

Bug: rememberRecent is called before await action(full), so a failed action still adds the path to recents (FileChooser.tsx:589)

async function onAction() {
    const full = selected_path();
    cockpit.assert(full);
    rememberRecent(full, recentKey);  // runs unconditionally
    await action(full);               // may throw — dialog stays open
}

If action throws (e.g. the error shown in the playground test: "Does not start with 'foo'"), the dialog stays open showing the error but the path is already written to localStorage. Repeated failures pollute the Recent list with files the user
never successfully used. Fix: move rememberRecent to after await action(full).

---

Bug: Empty/error states are rendered inside <caption> instead of a <tbody> row (FileChooser.tsx:908)

When a directory is empty, filtered to nothing, or has a permission error, listingBody() returns <Caption><EmptyState …/></Caption>, producing a <table> with only a <caption> and no rows. Screen readers announce the EmptyState content
(including interactive buttons like "Clear filters") as the table's accessible description rather than as main content. The established pattern in cockpit-components-table.tsx wraps empty states in a colSpan <Td> row instead.

---

Efficiency: getFileInfos fires N sequential fsinfo RPCs instead of parallel (FileChooser.tsx:373)

Opening the Recent collection with 20 entries fires 20 sequential bridge round-trips. Total wait is 20× a single RTT instead of 1×. Promise.allSettled would preserve the existing per-entry error handling while parallelizing all calls.

---

Efficiency: stdShortcuts awaits getHomeDir() and getDownloadDir() sequentially (FileChooser.tsx:265)

The two calls are independent — one may await cockpit.init(), the other spawns xdg-user-dir. Dialog open latency is the sum of both instead of the max. One-line fix: const [home, dd] = await Promise.all([getHomeDir(), getDownloadDir()]).

---

Cleanup: Local FileIcon SVG duplicates the PatternFly FileIcon (FileChooser.tsx:275)

The inline SVG uses Font Awesome 4.x geometry (viewBox="0 0 1536 1792"), inconsistent with the PF5 icon family already used by FolderIcon, FolderOpenIcon, etc. in the same file. @patternfly/react-icons already exports FileIcon — adding it to
the existing named import and deleting the local definition (lines 275–287) would make the icon visually consistent with no call-site changes.

[mvollmer]

Second Claude round:

Bug: JSON.parse in listRecent throws on corrupted localStorage (FileChooser.tsx:388)
Bug: JSON.parse in rememberRecent surfaces a raw SyntaxError as the dialog error (FileChooser.tsx:1064)

Fixed with a common "readRecent" function.

Bug: rememberRecent is called before await action(full), so a failed action still adds the path to recents (FileChooser.tsx:589)

Hmm, no, the user might want to try again by closing the dialog, fixing something, and then open it again. The file from the previous attempt should be listed as recent.

Bug: Empty/error states are rendered inside <caption> instead of a <tbody> row (FileChooser.tsx:908)

Ok. (I think I copied the <Caption> thing from somewhere... the PF docs also use the full <Tr<><Td><Bullseye> scaffolding, so let's do that.)

Efficiency: getFileInfos fires N sequential fsinfo RPCs instead of parallel (FileChooser.tsx:373)

Yes, hmm, maybe.

Efficiency: stdShortcuts awaits getHomeDir() and getDownloadDir() sequentially (FileChooser.tsx:265)

This is negligible since getHomeDir will be synch from the second call on. We could also cache the download dirs.

Cleanup: Local FileIcon SVG duplicates the PatternFly FileIcon (FileChooser.tsx:275)

Our FileIcon is different: it is hollow and thus easily distinguishable from directories, which are filled. The icon is copied from cockpit-files. Maybe we should use a different name.

[cockpituous]

There are more than 10 code coverage comments, see the full report here.

[mvollmer]

Third round:

Bug: Prepared filter receives full path instead of basename in collection mode

File: pkg/lib/cockpit/react/FileChooser.tsx, line 762

In collection mode (path == ""), f.name is a full absolute path (e.g. /home/user/foo.txt), but the prepared filter is called with it directly:

const preFiltered = withoutHidden.filter(
    f => (!onlyDirectories && f.type == "dir") || dlg.values.filter.filter(f.name, f.type)
);

The docs promise the filter receives "the base name of a file", so a filter like n => n.startsWith('f') would silently reject every file in Recent/collections because all paths start with '/'. Fix: use basename(f.name) when dlg.values.path == "".

---

Bug: Text filter matched against full path but highlight shown on basename

File: pkg/lib/cockpit/react/FileChooser.tsx, line 768

  const filtered = preFiltered.filter(f => f.name.includes(dlg.values.textFilter));

In collection mode f.name is a full path, but the row displays basename(f.name) and boldify highlights within that basename. If the user types "foo", the file /home/foo/bar.txt passes the filter (the directory component matches) but renders as bar.txt
with no underline — a match with no visible reason. Fix: filter on basename(f.name) when in collection mode.

---

Bug: Sort key puts block/char devices before directories

File: pkg/lib/cockpit/react/FileChooser.tsx, line 262

  result.sort((a, b) => (a.type + a.name).localeCompare(b.type + b.name));

The intent is directories first, then files. But 'blk' < 'dir' < 'reg' in ASCII order, so block devices and character devices sort above directories. This is visible when navigating to /dev. Fix: sort explicitly — bucket 'dir' as 0 and everything else
as 1, then fall back to name comparison.

---

Efficiency: getFileInfos issues sequential fsinfo calls for collections

File: pkg/lib/cockpit/react/FileChooser.tsx, line 372

  for (const p of paths) {
      const info = await fsinfo(p, ["type"], ...);
      ...
  }

Each call is a separate round-trip over the Cockpit transport. With up to 20 recent files this is 20 serial requests; over SSH the latency adds up visibly. The calls are independent — replace with Promise.all(paths.map(p => fsinfo(...).catch(...))).

---

Efficiency: stdShortcuts awaits home dir and downloads dir sequentially

File: pkg/lib/cockpit/react/FileChooser.tsx, line 265

  const home = await getHomeDir();
  const dd = await getDownloadDir();

getHomeDir may call cockpit.init() and getDownloadDir spawns xdg-user-dir — neither depends on the other. On a cold start this serialises an IPC call and a process spawn unnecessarily. Fix:

const [home, dd] = await Promise.all([getHomeDir(), 
  getDownloadDir()]).

[mvollmer]

Bug: Prepared filter receives full path instead of basename in collection mode
Bug: Text filter matched against full path but highlight shown on basename

Yep, fixed.

Bug: Sort key puts block/char devices before directories

Ouch, indeed. Fixed.

Efficiency: getFileInfos issues sequential fsinfo calls for collections
Efficiency: stdShortcuts awaits home dir and downloads dir sequentially

Still ignoring these.

[mvollmer]

Forth round: (CLaude starts to screw up markdown syntax now... hmm.)

Bug: preFiltered silently hides directories when onlyDirectories=true + custom filter active

pkg/lib/cockpit/react/FileChooser.tsx line 869

When onlyDirectories=true, watchFiles already restricts entries to type == "dir". But in listingBody, the short-circuit in preFiltered is:

const preFiltered = withoutHidden.filter(
    f => (!onlyDirectories && f.type == "dir") || dlg.values.filter.filter(basename(f.name), f.type)
);

When onlyDirectories=true, the left side is always false, so every directory must pass the custom prepared filter. If a caller passes a filters prop (e.g. a "No dots" filter that rejects names containing "."), any directory whose name contains a dot is
removed from the listing entirely — making it impossible to navigate into it.

The fix is to always pass directories through, regardless of the prepared filter:

  f => f.type == "dir" || dlg.values.filter.filter(basename(f.name), f.type)

---

Bug: textFilter cleared on double-click of any file, not just directories

pkg/lib/cockpit/react/FileChooser.tsx line 904

The onDoubleClick handler clears textFilter unconditionally:

  onDoubleClick={event => {
      event.preventDefault();
      if (f.type == "dir")
          setPath(dlg, full_path(dlg.values.path, f.name));
      dlg.field("textFilter").set("");  // runs even for regular files
      focusFilter();
  }}

Double-clicking a regular file does nothing else (no navigation, no confirm), so the user sees their typed filter text disappear with no explanation. The clear should be inside the if (f.type == "dir") block.

---

Bug: isAriaDisabled does not block clicks; assert error surfaces when Select is triggered with no selection

pkg/lib/cockpit/react/FileChooser.tsx line 970

  <DialogActionButton
      dialog={dlg}
      isAriaDisabled={selected_path() === null}
      ...
  >

PatternFly v6 isAriaDisabled only sets the aria-disabled attribute — it does not suppress onClick. A keyboard user pressing Enter/Space when no file is selected will fire onAction(), which calls cockpit.assert(full) with full = null. run_action catches
the throw and renders it as a dialog error alert, which is confusing rather than simply keeping the button inert.

Use isDisabled here instead of isAriaDisabled.

---

Efficiency: getFileInfos serializes N fsinfo() round trips

pkg/lib/cockpit/react/FileChooser.tsx line 380

  for (const p of paths) {
      const info = await fsinfo(p, ["type"], ...);
      ...
  }

Each await blocks before the next call starts. For a Recent collection with 20 entries over a remote connection, this is 20 sequential channel round trips. Promise.all would issue them concurrently:

  const results = await Promise.all(paths.map(async p => {
      try {
          const info = await fsinfo(p, ["type"], ...);
          if (info.type && (!onlyDirectories || info.type == "dir"))
              return { name: p, type: info.type };
      } catch (ex) {
          if (!(ex && typeof ex == "object" && "problem" in ex && ex.problem == "not-found"))
              console.error("Failed to get file type:", p);
      }
      return null;
  }));
  return results.filter(r => r !== null);

---

Efficiency: getHomeDir() and getDownloadDir() awaited sequentially in stdShortcuts

pkg/lib/cockpit/react/FileChooser.tsx line 264

  const home = await getHomeDir();
  const dd = await getDownloadDir();  // spawns xdg-user-dir — always a process

These are independent. stdShortcuts is on the critical path of init(), blocking the dialog from rendering. Use Promise.all:

  const [home, dd] = await Promise.all([getHomeDir(), getDownloadDir()]);

---

Cleanup: OutlineFileIcon duplicates @patternfly/react-icons' existing FileIcon

pkg/lib/cockpit/react/FileChooser.tsx line 275

@patternfly/react-icons (already imported on the same line as FolderIcon, FolderOpenIcon, etc.) exports FileIcon, which renders the same folded-corner document shape. The inline SVG uses a different coordinate system (FA4 1536×1792) and won't follow PF
design-system updates.

  import { FolderIcon, FolderOpenIcon, OutlinedHddIcon, SearchIcon, FileIcon } from '@patternfly/react-icons';

Then replace with and remove the inline component.

[mvollmer]

Bug: preFiltered silently hides directories when onlyDirectories=true + custom filter active

This current behavior is logical, imo. However, filters don't make a lot of sense together with onlyDirectories and I don't expect people to use them.

Bug: textFilter cleared on double-click of any file, not just directories

Yes, fixed.

Bug: isAriaDisabled does not block clicks; assert error surfaces when Select is triggered with no selection

This seems to be false. isAriaDisabled prevents any interaction that would run the onClick handler. A recent change in the dialog kit allows us to use isDisabled, however, so let's do that.

Efficiency: getFileInfos serializes N fsinfo() round trips
Efficiency: getHomeDir() and getDownloadDir() awaited sequentially in stdShortcuts
Cleanup: OutlineFileIcon duplicates @patternfly/react-icons' existing FileIcon

Yada yada. :-) It's good to see Claude being consistent, however.

[mvollmer]

Fifth round... Now we get into actual coding style and maintainability issues...

  [
    {
      "file": "pkg/lib/cockpit/react/FileChooser.tsx",
      "line": 354,
      "summary": "Inline symlink resolution lacks the '.' special-case that FsInfoClient.target() handles, silently dropping any entry whose symlink target is '.'",
      "failure_scenario": "A directory contains 'self' (a symlink with target='.'). entry.target is truthy so the if-block runs: info.entries['.'] is undefined (entries are keyed by filename, never by '.'), info.targets['.'] is also absent (fsinfo.ts:118
  shows FsInfoClient.target() explicitly handles '.' to return the parent info, implying the server does not put '.' in the targets dict). entry becomes undefined; the if (entry && entry.type) guard drops it. The 'self' entry never appears in the 
  listing."
    },
    {
      "file": "pkg/lib/cockpit/react/FileChooser.tsx",
      "line": 384,
      "summary": "getFileInfos serializes one cockpit bridge round-trip per path instead of running them in parallel",
      "failure_scenario": "The Recent collection stores up to 20 paths (line 1090: recent.slice(0, 20)). Each fsinfo() is a separate bridge message. At 50 ms RTT on a remote host, 20 serial calls add ~1 s of latency before the Recent view renders any 
  entries. Promise.all(paths.map(p => fsinfo(p,...))) would cap latency at the single slowest call."
    },
    {
      "file": "pkg/lib/cockpit/react/FileChooser.tsx",
      "line": 269,
      "summary": "stdShortcuts awaits getHomeDir() and getDownloadDir() sequentially even though they are independent",
      "failure_scenario": "getDownloadDir() spawns 'xdg-user-dir DOWNLOAD' as a subprocess. Because it is only started after getHomeDir() resolves, every FileChooser open pays the sum of both RTTs instead of the maximum. On a slow or remote host this 
  doubles the visible spinner duration before the sidebar shortcuts appear."
    },
    {
      "file": "pkg/lib/cockpit/react/FileChooser.tsx",
      "line": 743,
      "summary": "The 'Filesystem' shortcut literal is hardcoded independently in both header() (for narrow viewports) and sidebar() (for wide viewports), requiring two-site edits for any change",
      "failure_scenario": "A maintainer adds an icon or a permission check to the Filesystem entry in sidebar() but misses the identical literal in header()'s KebabDropdown. Wide-viewport users see the updated entry; narrow/mobile users see the stale 
  one."
    },
    {
      "file": "pkg/lib/cockpit/react/FileChooser.tsx",
      "line": 642,
      "summary": "The 'if (crumbs.length > 0)' guard is dead code: dirs always starts with ['/'] so crumbs is never empty in this branch",
      "failure_scenario": "breadcrumbs() returns null early when path==='', so the else-branch only runs when path is non-empty. dirs is always ['/'].concat(...) with at least one element; the forEach always pushes at least one BreadcrumbItem. The length
    },
    {
      "file": "pkg/lib/cockpit/react/FileChooser.tsx",
      "line": 410,
      "summary": "listRecent is an unnecessary async wrapper: it does nothing except return the synchronous result of readRecent()",
      "failure_scenario": "A reader following the call chain from recent_collection.list traces through listRecent before reaching readRecent, expecting to find I/O or error handling that isn't there. Inline 'list: async () => readRecent(recentKey)' at 
  the call site (line 503) would eliminate the indirection."
    },
    {
      "file": "pkg/lib/cockpit/react/FileChooser.tsx",
      "line": 812,
      "summary": "emptyState() uses magic numbers 0/1/2/3 to encode three orthogonal behaviors: whether to show a button, what label to use, and what action to take",
      "failure_scenario": "A new caller adding a fourth 'clear' variant must decode the numeric convention from scratch without compiler help, and risks picking the wrong number. An optional 'action?: { label: string; onClick: () => void }' parameter 
  would make each call site self-documenting."
    },
    {
      "file": "pkg/lib/cockpit/react/FileChooser.tsx",
      "line": 873,
      "summary": "The preFiltered condition '(!onlyDirectories && f.type==\"dir\") || filter.filter(...)' always passes directories when onlyDirectories=false, but this intent is not commented, creating a maintenance trap",
      "failure_scenario": "A maintainer 'simplifies' the condition to 'filter.filter(basename(f.name), f.type)' to make it uniform. File-type filters (e.g. '*.txt') then also filter out directories, making the listing unnavigable when any prepared filter
  is active. A one-line comment stating 'always show dirs for navigation' would prevent this."
    },
    {
      "file": "pkg/lib/cockpit/react/FileChooser.tsx",
      "line": 341,
      "summary": "The guard treats falsy info.targets as 'Permission denied', but an incremental fsinfo update that omits the targets key would trigger this error path for an accessible directory",
      "failure_scenario": "FsInfoClient merges incremental JSON patches; if a change event updates info.type and info.entries but omits info.targets (e.g., as a future server size optimization when no symlinks changed), the accumulated state has 
  info.targets===undefined. The check !(info.type && info.entries && undefined) is true, so callback fires with FileError('Permission denied') and the directory listing is cleared despite the directory being fully accessible."
    },
    {
      "file": "pkg/lib/cockpit/react/FileChooser.tsx",
      "line": 999,
      "summary": "The FileChooserButton onClick is marked async but contains no await, unnecessarily wrapping the call in a Promise on every click",
      "failure_scenario": "Every icon click creates a new Promise even though Dialogs.show() is synchronous and nothing is awaited. The async wrapper also silently converts any synchronous exception thrown inside Dialogs.show() into an unhandled Promise 
  rejection instead of a synchronous throw, making errors harder to observe."
    }
  ]

[cockpituous]

There are more than 10 code coverage comments, see the full report here.

[cockpituous]

These 3 added lines are not executed by any test. Details

[cockpituous]

This added line is not executed by any test. Details

[cockpituous]

This added line is not executed by any test. Details

[cockpituous]

These 2 added lines are not executed by any test. Details

[cockpituous]

These 3 added lines are not executed by any test. Details

[cockpituous]

These 3 added lines are not executed by any test. Details

[cockpituous]

This added line is not executed by any test. Details

[cockpituous]

These 3 added lines are not executed by any test. Details

[cockpituous]

These 2 added lines are not executed by any test. Details

[cockpituous]

This added line is not executed by any test. Details