Skip to content

Add @uppy/s3 provider plugin + Companion S3 provider (prototype) - #6506

Open
kvz wants to merge 50 commits into
mainfrom
s3-provider-prototype
Open

Add @uppy/s3 provider plugin + Companion S3 provider (prototype)#6506
kvz wants to merge 50 commits into
mainfrom
s3-provider-prototype

Conversation

@kvz

@kvz kvz commented Aug 28, 2026

Copy link
Copy Markdown
Member

What

Browse and manage S3-compatible object storage (AWS S3, Cloudflare R2, MinIO, Transloadit Storage) from the Dashboard, on the same footing as Box/Dropbox/WebDAV — the same widget that powers the Console File Library (content#5810) and that customers can embed.

  1. @uppy/s3 + Companion S3 provider — simple-auth ({ bucket, prefix } session), ListObjectsV2 + pagination, download; reuses Companion's s3 options so R2/MinIO is configuration only. Safe by default: browsing needs s3.browsableBuckets / COMPANION_AWS_BROWSABLE_BUCKETS, mutations additionally s3.mutableBuckets / COMPANION_AWS_MUTABLE_BUCKETS.
  2. Mutations + actions slot — Companion Provider gains deleteItem/moveItem/createFolder (+ POST /:provider/mutate/*): moves never overwrite, folders cannot move into themselves, non-empty folders cannot be deleted, folder moves walk the tree with delimiter listings and copy before deleting (≤ 1000 entries). ProviderViews gains per-item "⋯" actions (keyboard-navigable native popover, one open at a time), toolbarActions, refreshCurrentFolder(), and native <dialog> prompt()/confirm() — no window.prompt. @uppy/s3 ships Rename/move…, Delete, New folder with toasts.
  3. @uppy/transloadit-storageTransloaditStorage extends S3: workspace slug as bucket (dev) or a server-issued grant; Copy Smart CDN URL via a getSmartCdnUrl(key) callback (sign on your server — the plugin holds no credentials); storeUploads (an installed @uppy/transloadit stores uploads into the open folder; you sign via signAssembly; buildStoreAssemblyParams/createStoreAssemblyOptions exported); Download; reopenAfterUpload; "Storing…" instead of "Encoding…".
  4. Manager mode (mode: 'manager', the standalone default): clicking a file opens a styled detail modal (preview via getPreviewUrl, size/type/date, the same actions as the "⋯" menu — defined once); checkboxes hide behind a Select multiple toggle with a bulk Move…/Delete footer; S3.openFolderPath(key) gives embedders typed deep-link folder restores (serialized with the panel's own first load); onUploadRequest lets the host open its own upload UI (the Console opens a Dashboard modal with remote sources) and S3.refreshListing() re-lists afterwards. Picker mode is byte-identical to before: every new capability is behind an option with a no-op default.
  5. Trust model: server-issued storage grants. POST /s3/simple-auth accepts { form: { grant } }, an HS256 JWT your backend mints (s3.grantSecret / COMPANION_AWS_GRANT_SECRET). Companion re-checks bucket/prefix/scopes/expiry on every operation (expired → 401 → @uppy/s3 re-grants via getGrant() and retries once); read-only grants hide mutations; once a secret is set, client-supplied buckets are refused unless s3.allowBucketAuth (dev). The grant wire contract (claims, decode, HS256 mint/verify) lives in @transloadit/utils@4.8.0 (decodeStorageGrant in the browser, verifyStorageGrant in Companion) — one codec shared with api2's POST /storage/grants and the Console.
  6. Theming: --uppy-color-primary(-hover), --uppy-color-secondary, --uppy-color-danger, --uppy-font-family CSS custom properties, falling back to the current palette.

Tested

Companion unit tests 135/135 (S3 provider incl. grants, mutation gating, collisions, folder moves), @uppy/s3 browser tests 10/10 with msw (auto-connect, stale session, menu, dialogs, rename/move/delete → exact Companion requests), @uppy/transloadit-storage known-answer signing tests, core/dashboard suites green, typecheck + Biome clean, changesets added. End to end: the Console File Library e2e suite (7/7) and Opus user-test PASS on all four DAM use-case tutorials — after every round on this branch (regression recipe: content repodocs/dam-use-case-validation.md).

Notes for reviewers

  • The native <dialog>/popover primitives feature-detect down to the iOS ≥ 13.4 end of browserslist; menu positioning stays JS (CSS anchor positioning is not in Firefox stable).
  • mockCompanion (@uppy/s3/mockCompanion) is one in-memory S3 provider used by this package's browser tests and the Console's Playwright suite — no separate fakes.
  • Fixed here because this branch surfaced it: generic build:css in turbo.json missed dependsOn: ["^build:css"], so dashboard css could serve a stale cache when only core's provider-views scss changed.

Known follow-ups (not in this PR)

Docs / uppy bundle inclusion; image thumbnails in rows (natural fit: builtin/storage-preview signed URLs); a supported initialPanel option (embedders call showPanel() and get a one-frame source-chooser flash); duplicate Informers on app-level errors; #939393 secondary text fails WCAG AA (measured 3.07:1 / 2.55:1 — #666 fixes it); 44px mobile touch targets; a rare post-upload /s3/list 401 in rapid fresh contexts (regrant-retry may race reopenAfterUpload); Companion providers still carrying private withErrorHandling copies (parked: fold into a provider base-class PR only if all eight migrate together).

History

Built in reviewed iterations (provider → mutations → trust model → manager mode → DRY): details in the commit messages and the converged LoC/DRY plan ~/code/dam-loc-round4-proposal.md. Every round ended with the full e2e + use-case regression suite green.

🤖 Generated with Claude Code

Browse and import files from S3-compatible object storage (AWS S3,
Cloudflare R2, MinIO, ...) from the Dashboard, on the same footing as
Box/Dropbox/WebDAV.

Companion:
- New `s3` provider (simple auth, no OAuth). Session is `{ bucket, prefix }`;
  listing/downloading is confined to that prefix so an integrator can scope
  a user to e.g. `assets/customer-123/`.
- ListObjectsV2 with delimiter, continuation-token pagination, MIME lookup.
- Reuses the existing `s3` options (key/secret/region/endpoint) so pointing
  it at R2/MinIO is configuration only.
- Safe by default: browsing is disabled unless `s3.browsableBuckets`
  (`COMPANION_AWS_BROWSABLE_BUCKETS`) allowlists buckets, or `*`.

Client:
- `@uppy/s3`: thin plugin over ProviderViews (list/grid, breadcrumbs,
  filter, pagination come for free). `bucket` option pre-fills the form.
- Auth form uses the plugin's own i18n (AuthView only sees core i18n);
  strings also added to @uppy/locales en_US.

Dev harness wires the plugin in; `VITE_S3_BROWSE_BUCKET` pre-fills the bucket.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@changeset-bot

changeset-bot Bot commented Aug 28, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 984eb72

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@uppy/companion Minor
@uppy/core Minor
@uppy/s3 Minor
@uppy/transloadit-storage Minor
@uppy/locales Minor
uppy Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

kvz and others added 12 commits August 28, 2026 13:07
Companion:
- Provider base gains optional deleteItem/moveItem/createFolder and a
  static supportsMutations flag; new routes
  POST /:provider/mutate/{delete,move,create-folder} (token-protected).
- S3 provider implements them with DeleteObject, CopyObject+DeleteObject
  and a trailing-slash PutObject, confined to the session prefix. 4xx S3
  errors surface their message to the user.

Core (@uppy/core/provider-views):
- ProviderViews accepts `actions` (per-item, shown in a "⋯" menu on list
  and grid items) and `toolbarActions` (folder-level, shown in the header),
  runs them with a small context and refreshes the current folder
  afterwards (refreshCurrentFolder drops the cached subtree and re-lists).
- companion-client Provider gains deleteItem/moveItem/createFolder.

@uppy/s3 ships built-in actions (Rename / move…, Delete, New folder) when
`enableActions` is not false, plus `actions`/`toolbarActions` passthrough.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
TransloaditStorage extends the S3 provider plugin: the workspace slug is
the bucket, an optional prefix confines browsing, and a 'Copy Smart CDN
URL' item action signs URLs in the browser (WebCrypto HMAC-SHA256 with a
pure-JS fallback for insecure dev origins), byte-compatible with
@transloadit/utils and api2's Signature.getSmartCDNUrl.

Dev harness: VITE_TRANSLOADIT_STORAGE_WORKSPACE switches the Dashboard to
the Storage tab; with VITE_UPLOADER=transloadit, uploads go through
/transloadit/store into the folder currently open in that tab and the
view refreshes on completion.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Skips the auth form on first render when the integrator already told the
plugin which bucket (and prefix) to open, so a management page can show the
storage immediately. Opt out with autoConnect: false.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adding files makes the Dashboard close the picker panel, which reset the
provider view to the root before uploads into 'the current folder' could
read it. keepStateOnClose (default on for TransloaditStorage) keeps the
browsing state across panel closes; the dev harness uses Transloadit's
${file.name} interpolation for the stored path.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Drop the hand-rolled SHA-256/HMAC fallback. On insecure origins (plain
http, non-localhost) crypto.subtle is unavailable and signing now fails
with a clear error instead of silently using a homegrown digest.

Also removes an unused Preact import and an unused destructured var
flagged by Biome.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- s3.mutableBuckets / COMPANION_AWS_MUTABLE_BUCKETS gates delete, move
  and create-folder separately from browsing (read-only by default)
- moves refuse to overwrite an existing key, folders cannot be moved
  into themselves, and non-empty folders cannot be deleted
- folder moves walk the tree with delimiter listings (so empty
  sub-folders survive), create destinations and copy everything before
  deleting anything, capped at 1000 entries
- prefix violations are user errors instead of auth errors, so the
  Dashboard shows the message rather than re-prompting for a bucket

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…selection on refresh

- ProviderView.prompt() / confirm() render an inline ProviderDialog
  (autofocus, Enter/Escape, backdrop click, focus restore) backed by
  plugin state, so plugins no longer need window.prompt/confirm
- a single item-actions popover owned by Browser, positioned from the
  trigger's rect inside the browser body (no clipping by the scrolling
  list), closes on Escape/outside click/scroll/action, arrow-key
  navigation, aria-expanded on the trigger
- refreshCurrentFolder() re-applies checked items that still exist

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…toasts, stale-session check, tests

- Rename / move… works on folders too; a bare name renames inside the
  current folder, a value with '/' is a full key
- success toasts via uppy.info(); delete/new-folder use the inline
  confirm/prompt dialogs
- autoConnect logs out a persisted session for another bucket first
- Vitest browser test with msw for the whole flow; known-answer test
  for Smart CDN URL signing
- dev harness passes cdn=required when pointed at a local api2

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The s3 and webdav providers carried the same try/log/rethrow wrapper;
the base now owns it and providers only override mapProviderError().

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
delete/move/create-folder shared the provider check, body validation,
try/catch and error mapping; keep the routes, messages and status codes
identical.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
kvz and others added 5 commits August 28, 2026 16:33
createMockS3Companion() serves /s3/simple-auth, list, mutate/* and
logout from an in-memory folder tree with the provider's real semantics
(rename in place, folder moves, collisions, non-empty folder deletes),
exported as @uppy/s3/mockCompanion so integrators' Playwright suites can
use the same fixture; handleFetchRequest()/toMswHandlers() adapt it to
fetch/msw. The browser test now runs on it (6 tests).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ProviderDialogController owns the prompt/confirm promise + plugin state;
useItemMenu owns which item menu is open. ProviderView and Browser only
wire them. No behaviour or public API change.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Destroying an Uppy with a provider panel open rendered the Dashboard
once more after the provider plugin was removed: PickerPanelContent
threw on the missing plugin and useSearchForm's cleanup threw when its
form was already gone. Both surfaced as unhandled errors (and a red
vitest run for @uppy/s3); render nothing / use form.remove() instead.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…transloadit/utils

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@transloadit/utils 4.6.0 ships an isomorphic (WebCrypto)
getSignedSmartCdnUrl; use it instead of our own copy so the string to
sign can never drift from the Node SDK and api2. What remains here is
the endpoint override for local api2s. The Copy Smart CDN URL action is
only offered when the plugin has credentials to sign with.

.yarnrc.yml preapproves @transloadit/utils for the 1-week minimal-age
gate, the same way the Console repo does for Transloadit's own packages.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@socket-security

socket-security Bot commented Aug 28, 2026

Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Addednpm/​@​transloadit/​utils@​4.8.09810010096100

View full report

kvz and others added 6 commits August 29, 2026 10:31
…item menu

ProviderDialog is a <dialog> opened with showModal(): the focus trap,
Escape (cancel event), ::backdrop and focus restore are the browser's,
so the overlay element, its click handler and the manual focus
bookkeeping go. The item-actions menu is a popover="auto" element in
the top layer: light-dismiss, Escape and focus return are native, the
outside-click and resize listeners go, and no ancestor can clip it.
Engines without showModal/showPopover (iOS < 15.4 / < 17) fall back to
the same markup inline. The popover is keyed per item so switching menus
unmounts the old one instead of letting its queued toggle event close
the new one.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… moves

The five operations repeated the same preamble (authenticated session,
browsable bucket, mutable bucket, keys inside the scoped prefix, client);
#session() does it once and returns { bucket, prefix, client }.
#moveFolder walks with paginateListObjectsV2 instead of a hand-rolled
continuation loop. The test's client stub now carries the S3Client
prototype because the SDK paginator checks instanceof.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Each action's run() now only asks, calls Companion and returns the
success toast; withToast() shows it. The stored-session check and
auto-connect share one #warn() for their log lines. Requests and toasts
are unchanged (pinned by the browser tests).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…xpiry

simple-auth accepts { form: { grant } }: an HS256 JWT minted by the
integrator's server (s3.grantSecret / COMPANION_AWS_GRANT_SECRET) with
claims { v: 1, bucket, prefix, scopes: ['read'|'write'], exp }. The
session carries prefix, scopes and exp; every operation re-checks them —
expired → auth error (client fetches a new grant), missing scope → user
error — on top of the bucket allowlists and prefix confinement. Once a
grant secret is configured, client-supplied bucket names are refused
unless s3.allowBucketAuth is set for development.

Folder moves use Companion's existing p-map instead of a local batching
helper.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
kvz and others added 2 commits August 30, 2026 12:36
New getGrant() option: the plugin auto-connects with the grant your
backend mints, shows only a Connect button instead of the bucket form,
never reuses a persisted session, hides the mutation actions when the
grant is read-only, and fetches a fresh grant once when Companion reports
an expired session mid-way. decodeGrant() reads the unverified claims.
The mock Companion understands grants too (mockGrant() for tests).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…oads, reopenAfterUpload

The plugin no longer signs anything: getSmartCdnUrl(key) returns the
Smart CDN URL (sign it on your server) and the Copy action is only
offered when it is set; authKey/authSecret/template/urlParams/cdnEndpoint
and the @transloadit/utils dependency are gone. With getGrant the grant
decides the bucket; workspace stays the development fallback.

storeUploads: { signAssembly, conflictStrategy } wires an installed
@uppy/transloadit plugin to store uploads in the folder that is open
(createStoreAssemblyOptions() for doing it by hand); reopenAfterUpload
clears, reopens the panel and refreshes once an upload completed without
failures — on the next macrotask, no fixed timer.

The dev harness provides both callbacks (it signs with @transloadit/utils
and the dev secret; a real app signs server-side).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
kvz and others added 16 commits August 30, 2026 16:37
With standalone: true the plugin is the whole page, so the Dashboard omits
the picker panel's title bar entirely (the page owns the heading) and the
provider header drops its user/logout row (the app owns the session).
Toolbar actions and the filter stay.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…he session root

createStoreAssemblyOptions built a bucket-root path when no folder was open,
so a prefix-confined session (grant) produced paths outside the grant and the
signing server rightly refused them. At the root, fall back to the plugin's
prefix (normalized to a trailing slash); open folders already carry it in
their full-key ids. Found by the use-case-3 tutorial validation loop.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Provider views gain an opt-in mode: 'manager' for browsing-first UIs
(Transloadit Storage standalone defaults to it):

- Clicking a file opens a detail modal (preview via getPreviewUrl,
  metadata, and the same actions the '…' menu shows — defined once).
- Checkboxes hide behind an explicit 'Select multiple' toggle; while
  selecting, a bulk-actions footer (move/delete) replaces the picker
  footer.
- The '…' menu now repositions after showPopover() and follows page
  scroll/resize, so it opens at its trigger instead of low on the page.
- Transloadit Storage adds an 'Upload files' toolbar action, a
  'Download' item action, and overrides the 'Encoding…' progress
  label with 'Storing…' (files are stored verbatim).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The generic build:css task had no dependsOn, so a package whose css
`@use`s another package's scss (e.g. @uppy/dashboard bundling
@uppy/core's provider-views styles) kept serving a stale cached
artifact when only the upstream scss changed. Add ^build:css, matching
the uppy#build:css entry.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Manager-mode grid files render a real <button> (like list rows) instead
of a label with a bare onClick, and the popover's reposition callback is
memoized so it can be an honest hook dependency.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- The item detail modal gets real structure: a header with the name and
  a close button, a padded body, a bordered preview panel, a key/value
  facts grid, and a footer with bordered secondary action buttons
  (danger actions in red) next to a primary Close — instead of unpadded
  content in a bare dialog.
- Bulk-action buttons keep the primary button metrics (the danger
  variant previously lost its padding), the selection count reads as a
  proper label, and destructive entries in the '…' menu render red.
- ProviderAction gains the danger flag (S3's Delete now sets it).
- TransloaditStorage.onUploadRequest lets the host take over the
  toolbar's Upload files action (e.g. to open a Dashboard modal with
  remote sources); S3.refreshListing() re-lists the open folder after
  such out-of-band uploads.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
An app that takes over uploads via onUploadRequest (e.g. a Dashboard
modal with remote sources) needs the same /transloadit/store params the
widget builds; export the builder instead of having hosts duplicate the
steps blob.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The footer's primary Close duplicated the header X's accessible name
(strict-mode ambiguity for tests, clutter for people). The header X,
Escape, and the backdrop close the dialog; the footer is actions-only.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two visible Cancel buttons (toolbar toggle + footer link) is confusing;
the footer keeps the count and the actions only.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
openFolder() returns cached folders without refetching, so the previous
implementation was a no-op exactly when it mattered (after out-of-band
uploads into the open folder).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Embedders can now set --uppy-color-primary(-hover), --uppy-color-danger,
--uppy-color-secondary, and --uppy-font-family on any ancestor (all
fall back to the current palette, so the default rendering is
byte-identical). Covers the primary buttons, danger surfaces (confirm,
menu, detail actions), secondary detail actions, and the root font.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…olderPath

- One storage-bucket SVG (color-configurable) instead of two copies.
- One prefix normalizer shared by the plugin and storeAssemblyOptions.
- The detail dialog's hand-rolled byte formatter becomes
  @transloadit/prettier-bytes (visible change accepted: 1.0 KB → 1 KB,
  two decimals for non-integral GB+).
- S3.openFolderPath(key): typed deep-folder navigation that walks from
  the root and stops at the deepest surviving ancestor — so embedders
  (the Console's ?folder= restore) stop reaching into private view
  internals and polling.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Console restores ?folder= right as the panel auto-opens the root;
walking concurrently let the root load win. The walk now waits for the
current listing to settle and reuses a cached root instead of racing a
second root load.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Dashboard's first render auto-opens the root; when that fires after the
walk, openFolder's cached-return still resets currentFolderId to the
root. Settle and reassert (twice, cheap cached opens) so deep links win
regardless of who finishes first.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The first-render auto-open fires only after auth + the root listing —
seconds after a deep-link restore starts on a cold load. Waiting for
the root to be cached (with a fallback self-load for headless use)
serializes the two instead of racing, so the reassert is a belt, not
the mechanism.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@kvz
kvz marked this pull request as ready for review August 31, 2026 17:05
@kvz
kvz requested review from mifi, qxprakash and remcohaszing August 31, 2026 17:05
kvz and others added 3 commits August 31, 2026 19:33
verifyStorageGrant/normalizeStorageGrantPrefix replace Companion's
jsonwebtoken+parseGrantClaims pair (expired grants still map to
ProviderAuthError so clients re-grant), and the browser's decodeGrant
delegates to decodeStorageGrant — one wire contract, published in
utils 4.8.0.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The regex swap left the old decodeGrant beneath the new one, the local
S3GrantClaims keeps its optional exp for API compatibility, and
Companion's bucket sessions take their full-access scopes from the
codec's STORAGE_GRANT_SCOPES.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
yarn dedupe --check (Lint yarn.lock CI) flagged the leftover ^4.6.0
resolution after @uppy/s3 and @uppy/companion moved to ^4.8.0.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment on lines +14 to +24
const readString = (body: unknown, key: string): string | null => {
if (!isRecord(body)) return null
const value = body[key]
return typeof value === 'string' && value.length > 0 ? value : null
}

const readNullableString = (body: unknown, key: string): string | null => {
if (!isRecord(body)) return null
const value = body[key]
return typeof value === 'string' ? value : null
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like these functions should be merged, the function should be named readString, and the implementation should be the one from readNullableString.

Whether or not an empty string is accepted, can be based on a booliness check.

const value = readString(object, 'key')

if (!value) {
  // value is null or an empty string
}

if (value == null) {
  // value is null
}

I also prefer to avoid null, but this hasn’t been applied to Uppy so far.

type Query,
} from '../Provider.js'

export type S3GrantScope = StorageGrantScope

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

export type A = B creates a new type A, that is equivalent of B with default generic options.

Typically, a better approach is something like:

export type { B as A}

But should we even have the type S3GrantScope at all?

Comment on lines +59 to +62
await run(
{ provider, providerUserSession, companion: req.companion },
parsed.input,
),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could be wrong, but it looks like this yields a status 200 response with a body of {"ok": false, "message": "…"}. Is this correct? If so, I think we should return a proper status code instead.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite — parsed.ok === false goes through res.status(400).json(...) two lines up; the res.json(await run(...)) is the success path. The ParsedInput union is only the parser's return shape, not the HTTP body.

const status = (err['$metadata'] as { httpStatusCode?: number } | undefined)
?.httpStatusCode
return (
err['name'] === 'NotFound' || err['name'] === 'NoSuchKey' || status === 404

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can’t depend on prototype names (such as error names). They may be different if minified. This is also prone to bugs due to internal refactors.

It’s better to use an instanceof check.

Comment on lines +94 to +106
/**
* Buckets that the S3 *provider* (browsing/importing files from S3 in the
* Dashboard) is allowed to list and download from. Use `['*']` to allow any
* bucket the credentials can access (e.g. when an upstream proxy already
* enforces authorization). Unset/empty disables S3 browsing entirely.
*/
browsableBuckets?: string[] | undefined
/**
* Buckets the S3 provider may *change* (delete, rename/move, create
* folders) from the Dashboard. Separate from `browsableBuckets` so a
* read-only browser is the default; `['*']` allows every browsable bucket.
*/
mutableBuckets?: string[] | undefined

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed below that you default these values to an empty array. There’s a nice formal JSDoc notation to communicate this.

Suggested change
/**
* Buckets that the S3 *provider* (browsing/importing files from S3 in the
* Dashboard) is allowed to list and download from. Use `['*']` to allow any
* bucket the credentials can access (e.g. when an upstream proxy already
* enforces authorization). Unset/empty disables S3 browsing entirely.
*/
browsableBuckets?: string[] | undefined
/**
* Buckets the S3 provider may *change* (delete, rename/move, create
* folders) from the Dashboard. Separate from `browsableBuckets` so a
* read-only browser is the default; `['*']` allows every browsable bucket.
*/
mutableBuckets?: string[] | undefined
/**
* Buckets that the S3 *provider* (browsing/importing files from S3 in the
* Dashboard) is allowed to list and download from. Use `['*']` to allow any
* bucket the credentials can access (e.g. when an upstream proxy already
* enforces authorization). Unset/empty disables S3 browsing entirely.
*
* @default []
*/
browsableBuckets?: string[] | undefined
/**
* Buckets the S3 provider may *change* (delete, rename/move, create
* folders) from the Dashboard. Separate from `browsableBuckets` so a
* read-only browser is the default; `['*']` allows every browsable bucket.
*
* @default []
*/
mutableBuckets?: string[] | undefined

Comment thread packages/@uppy/s3/vitest.config.ts Outdated

export default defineConfig({
test: {
include: ['src/**/*.test.{ts,tsx}'],

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let’s stick to the default include patterns to avoid needsless complexity in the configuration.

Suggested change
include: ['src/**/*.test.{ts,tsx}'],

}),
opts: { prefix: state.prefix },
}),
} as never

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this cast to never? A return type of never means the function always throws.

link.href = blobUrl
link.download = item.data.name ?? 'download'
link.click()
URL.revokeObjectURL(blobUrl)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you revoke object URLs while they’re still in use? If so, TIL.

| { showPanel?: (id: string) => void }
| undefined
dashboard?.showPanel?.(this.id)
void this.view.refreshCurrentFolder()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO the void keyword doesn’t really add anything useful (ever).

Suggested change
void this.view.refreshCurrentFolder()
this.view.refreshCurrentFolder()

"description": "Browse and manage files in Transloadit Storage from Uppy, upload into the open folder, and copy Smart CDN URLs.",
"version": "0.1.0",
"license": "MIT",
"types": "types/index.d.ts",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"types": "types/index.d.ts",

kvz and others added 2 commits September 2, 2026 22:30
JSDoc @default on the S3 provider options, toReversed(), PromiseLike for
the preview callback, ternaries instead of && in the detail dialog,
form.remove(), the redundant types fields and vitest include, no void,
and the simpleAuth assertions as plain awaits.

Co-authored-by: Remco Haszing <remcohaszing@gmail.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019iuG1VuT9gbLTXaqqqpH8g
- companion mutate: one readString (nullable), emptiness decided by the
  callers; S3 provider: drop the S3GrantScope/S3Grant aliases, isNotFound
  via the SDK error classes (NotFound, NoSuchKey, S3ServiceException 404),
  for-of over the growing folder queue, reject backslashes in folder names;
  companion targets es2023 (Node >= 22) so toReversed() type-checks
- companion tests: NotFound instances instead of duck-typed errors, the
  remaining .resolves assertions as plain awaits, specific rejects
- provider-views: some() for the applies-to check, AbortSignal for the
  popover listeners, derived (not effect-synced) open item menu, no memo
  in BulkActions, UIPluginOptions instead of {} in PickerPanelContent,
  Promise.resolve() around the PromiseLike preview callback
- @uppy/s3 browser test: inline the two-site helper
- transloadit-storage: honest Uppy cast in the test; defer revoking the
  download blob URL so the download can start

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019iuG1VuT9gbLTXaqqqpH8g
@kvz

kvz commented Sep 2, 2026

Copy link
Copy Markdown
Member Author

Thanks for the thorough pass, @remcohaszing. Addressed in two commits: 012983f applies your suggestion blocks as-is (co-authored)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants