Skip to content

feat(local-explorer-ui): Add shift key multi-select to R2 list#13429

Merged
petebacondarwin merged 17 commits intomainfrom
NuroDev/local-explorer-r2-multi-select
Apr 15, 2026
Merged

feat(local-explorer-ui): Add shift key multi-select to R2 list#13429
petebacondarwin merged 17 commits intomainfrom
NuroDev/local-explorer-r2-multi-select

Conversation

@NuroDev
Copy link
Copy Markdown
Member

@NuroDev NuroDev commented Apr 10, 2026

Fixes N/A.

Summary

Currently, if you try and shift-select a group of items in the R2 object list it will only select items individually. This change makes it so that now actually works as intended.


  • Tests
    • Tests included/updated
    • Automated tests not possible - manual testing has been completed as follows:
    • Additional testing not necessary because:
  • Public documentation
    • Cloudflare docs PR(s):
    • Documentation not necessary because: Niche additional feature / bug fix that doesn't need to be documented.

A picture of a cute animal (not mandatory, but encouraged)

@NuroDev NuroDev self-assigned this Apr 10, 2026
@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Apr 10, 2026

🦋 Changeset detected

Latest commit: a3348b9

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

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

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new bot commented Apr 10, 2026

create-cloudflare

npm i https://pkg.pr.new/create-cloudflare@13429

@cloudflare/kv-asset-handler

npm i https://pkg.pr.new/@cloudflare/kv-asset-handler@13429

miniflare

npm i https://pkg.pr.new/miniflare@13429

@cloudflare/pages-shared

npm i https://pkg.pr.new/@cloudflare/pages-shared@13429

@cloudflare/unenv-preset

npm i https://pkg.pr.new/@cloudflare/unenv-preset@13429

@cloudflare/vite-plugin

npm i https://pkg.pr.new/@cloudflare/vite-plugin@13429

@cloudflare/vitest-pool-workers

npm i https://pkg.pr.new/@cloudflare/vitest-pool-workers@13429

@cloudflare/workers-editor-shared

npm i https://pkg.pr.new/@cloudflare/workers-editor-shared@13429

wrangler

npm i https://pkg.pr.new/wrangler@13429

commit: a3348b9

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 10, 2026

✅ All changesets look good

@NuroDev NuroDev changed the title feat(local-explorer-ui): Add shift multi-select to R2 list feat(local-explorer-ui): Add shift key multi-select to R2 list Apr 10, 2026
@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk bot commented Apr 10, 2026

@NuroDev Bonk workflow was cancelled.

View workflow run · To retry, trigger Bonk again.

NuroDev and others added 12 commits April 12, 2026 12:27
Co-authored-by: Ben Dixon <ben@nuro.dev>
Co-authored-by: Ben <4991309+NuroDev@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…udflare/src/frameworks (#13438)

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Wrangler automated PR updater <wrangler@cloudflare.com>
Co-authored-by: Pete Bacon Darwin <pbacondarwin@cloudflare.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@NuroDev NuroDev marked this pull request as ready for review April 13, 2026 16:18
@workers-devprod workers-devprod requested review from a team and vicb and removed request for a team April 13, 2026 16:19
@workers-devprod
Copy link
Copy Markdown
Contributor

workers-devprod commented Apr 13, 2026

Codeowners approval required for this PR:

  • ✅ @cloudflare/wrangler
Show detailed file reviewers

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 4 additional findings.

Open in Devin Review

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk bot commented Apr 13, 2026

Let me carefully analyze the implementation now.

The key changes are:

  1. useRef for selectionAnchorRef - tracks the anchor for shift-click range selection
  2. Anchor invalidation - clears anchor when the key leaves the visible list
  3. handleSelectItem updated to accept shiftKey and perform range selection/deselection
  4. Checkbox event handling changed - moved from onCheckedChange on Checkbox to onClick on the parent Table.Cell, with filtering to only handle clicks on the checkbox

Let me think through potential issues:

  1. Mutating a ref during render (lines 214-219): The code mutates selectionAnchorRef.current directly in the render body. This is technically a side effect during render. In React 18 strict mode, the render function may be called twice, but since this is a ref mutation (not state), it won't cause re-renders. However, it's an anti-pattern. That said, it's idempotent (setting null when already invalidated), so it's practically harmless. Not actionable enough to flag.

  2. e.preventDefault() on checkbox click (lines 338, 388): The onClick handler on Table.Cell calls e.preventDefault() when a checkbox is clicked. This prevents the default browser behavior for the checkbox toggle, meaning the checkbox state is now entirely controlled by the React state. This is correct since the component is fully controlled.

  3. Removing onCheckedChange from Checkbox: The Checkbox no longer has any onCheckedChange handler. The click is intercepted at the cell level. This means clicking the checkbox label/area would not trigger selection unless it matches the target.closest check. Let me think about this... The Checkbox has aria-label but no visible label element, and the closest check covers button[role='checkbox'] and input[type='checkbox'], which should cover how Kumo renders checkboxes. This seems fine.

  4. Anchor set on deselect: When a plain click deselects an item (line 268-269), the anchor is still set to that key (line 274). This means if you deselect item A, then shift-click item B, it will range-select from A to B. This matches standard behavior (e.g., Windows Explorer, macOS Finder) — the anchor is the last clicked item regardless of select/deselect.

  5. selectableKeys.includes() for anchor invalidation (line 215): This is O(n) but the list size is bounded by R2 object listing pagination, so it's fine.

  6. The handleSelectAll sets anchor to null: This is correct — after "select all" or "deselect all", there's no meaningful anchor.

  7. Text selection on shift-click: One concern is that shift-clicking can cause text selection in the browser. The code doesn't call e.preventDefault() on the cell-level click for non-checkbox targets, and it doesn't prevent text selection. However, since the handler only triggers on checkbox clicks and calls preventDefault there, this should be fine for the checkbox interaction itself.

Let me look more carefully at the event handling pattern. The onClick on Table.Cell fires, checks if the click target is a checkbox, and if so calls e.preventDefault() + handleSelectItem. But the Checkbox has no onCheckedChange anymore. What happens when someone clicks the checkbox?

  • Browser click event fires
  • It bubbles up to Table.Cell's onClick
  • target.closest("button[role='checkbox']") matches → preventDefault() is called, handleSelectItem runs
  • The Checkbox component's internal handler (if any) would normally toggle the checkbox, but preventDefault() stops that
  • The controlled checked prop ensures the visual state matches selectedKeys

This pattern works, but there's a subtle concern: if the Kumo Checkbox component uses onChange internally (which fires after click), preventDefault on click might not prevent the onChange from firing on some implementations. However, since onCheckedChange is not passed, there's no handler to worry about. The checked prop controls the visual state. This is fine.

Overall, this is a clean implementation. The logic is correct, the tests cover both selection and deselection ranges, and the approach is standard. I don't see any logic bugs, security issues, backward compatibility problems, or incorrect API behavior.

LGTM

github run

@petebacondarwin petebacondarwin removed the request for review from vicb April 15, 2026 08:15
Copy link
Copy Markdown
Contributor

@workers-devprod workers-devprod left a comment

Choose a reason for hiding this comment

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

Codeowners reviews satisfied

@github-project-automation github-project-automation bot moved this from Untriaged to Approved in workers-sdk Apr 15, 2026
@petebacondarwin petebacondarwin merged commit 54ceb95 into main Apr 15, 2026
56 checks passed
@petebacondarwin petebacondarwin deleted the NuroDev/local-explorer-r2-multi-select branch April 15, 2026 08:17
@github-project-automation github-project-automation bot moved this from Approved to Done in workers-sdk Apr 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants