Skip to content

feat(aws-control): file preview, rename, and filename search - #8236

Merged
chenmingwei23 merged 1 commit into
mainfrom
feat/drive-preview-rename-search
Sep 5, 2026
Merged

feat(aws-control): file preview, rename, and filename search#8236
chenmingwei23 merged 1 commit into
mainfrom
feat/drive-preview-rename-search

Conversation

@CrysisDeu

@CrysisDeu CrysisDeu commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Summary

Three everyday file-drive capabilities that the cloud drive was still missing after drag-and-drop landed (#7983): open a file without downloading it, rename it in place, and find it by name across the whole file drive rather than only the open folder.

Preview. A PreviewDialog opens from the row / tile. Images, video, audio and PDF render inline through the existing presigned URL (those tags are exempt from CORS, so no proxy is needed); the PDF <iframe> carries an empty sandbox, because the branch is chosen by extension and a .pdf key holding HTML must not run script or navigate the dashboard. Text-like files go through a new GET /drive/{account}/preview route that reads the first 256 KB server-side — the browser cannot fetch object bodies from S3 directly because the bucket has no CORS policy — and the dialog says so when the object was truncated. The text passes through the same egress redactor as listing and search names, so a notes file holding an access key renders masked. Types we cannot render degrade to an honest "download to view" state instead of a broken frame. The media presign is the existing 60-second download grant; a clip longer than that 403s mid-play, so a media error re-mints the URL (another short grant, not a longer one) and resumes from the playback position; each successful load re-arms it so a long clip keeps playing across grants, and only two errors with no load between them fall back. put_file now sets --content-type on upload — but only for the inline-safe set the dialog embeds by URL (image/* minus SVG, video/*, audio/*, application/pdf). HTML, SVG and text stay on S3's opaque default on purpose: a stored text/html would make a shared or downloaded object render as a live document on the bucket origin, script included, while the same file opened in-app is inert bytes in the text preview.

Rename. Inline rename on both the list rows and the grid tiles, at parity. It is a thin client over the move API from #7983 — same key-level lock coordination, same error codes — but the refusal wording is rename's own ("A file with this name already exists here", not "the destination folder…"), because the user pressed Rename and should not be told about a move they never made.

Search. A toolbar field (300 ms debounce) backed by a new GET /drive/{account}/search route: case-insensitive substring match on the section-relative key (folder path included), walked over the file-drive section with token pagination — the route refuses any other section (section_not_searchable): the library and backups keep their own listing surfaces — capped at SEARCH_MAX_RESULTS (200) hits with an early stop; capped is set only when a match past the cap is seen, so exactly 200 hits is a complete result, not a truncated one. The response echoes capped and the cap itself, so the "showing the first N" notice interpolates the server's number and no locale spells it. Results sit under the same shared table head as the folder listing inside a horizontally scrolling container (narrow viewports scroll instead of pushing the actions off-screen), show the full relative key, and keep their actions behind the same labeled overflow menu the file rows use — Download, and a worded "Open containing folder" that navigates to the prefix and clears the search. Results pass through the same egress redactor as listings.

Thirteen new i18n keys × 12 locales + en-XA. The aws-control feature spec documents both new routes and the content-type behaviour.

What was tested

  • npx tsc -b — 0 errors.
  • npx vitest run src/apps/aws-control src/test/i18n src/i18n/style — 381/381 passing, including 10 new pinned tests (text preview through the proxy route with truncation notice, media preview via presigned URL, one-shot presign re-mint on media error then honest fallback, unsupported-type fallback, list and grid rename round-trip through move, rename-specific wording for share-active and name-collision refusals, grid tile hides its preview trigger while renaming, search rendering + server-echoed cap in the notice + go-to-folder navigation).
  • Backend: pytest test/test_aws_control_routes.py test/test_aws_control_storage.py test/test_aws_control_app.py — 349 passing, including the P0 route-contract tests for the two new routes, the drive-only scope of /search, the 256 KB truncation boundary, credential redaction on preview text (the same egress floor listing and search names already pass through), search pagination/early-stop at the cap (and exactly-the-cap is not reported as capped), and the inline-safe content-type allowlist in put_file (PDF/PNG/MP4 declared; HTML/SVG/TXT/JS stay opaque).
  • eslint on the changed frontend files — 0 (a11y: aria-label on video/audio; two documented exemptions for img onError and the rename <tr> key handler).
  • Manual run against a real account in the recording below.

Demo

preview, rename, search

Text preview → image preview → inline rename (list updates live) → search for quarterly hits docs/deep/quarterly-report.md with download and go-to-folder actions.

Notes

No linked issue: this is the next slice of the cloud-drive series (after #7825 and #7983), scoped in-chat with the PM.

UX review follow-ups

Every CONCERNS finding from the UX lane is implemented in this PR:

  • Search mode now withdraws the folder-scoped write controls (Upload, New folder, the section drop zone) together with the crumbs, so nothing can land in a folder the reader cannot see.

  • The grid/list toggle is hidden while searching (hits always render as a table).

  • The text preview reports redacted alongside truncated; the dialog shows "Sensitive values are masked in this preview." whenever the egress redactor fired.

  • The search box reads "Search file names…" (name-only search, never contents) in all 13 catalogs.

  • Search hits carry the same overflow menu as file rows (Download, Rename, Share, Delete) plus "Open containing folder"; rename and delete run in place against the hit's own path and re-run the search.

  • Dropping an OS file mid-search is swallowed (the section keeps an inert dragover/drop preventDefault) instead of navigating the tab away; nothing is uploaded.

  • A refinement keeps the previous hits on screen but dims them, marks them aria-busy, and shows "Searching…" until the new walk returns.

  • Not re-recorded here: the evidence GIF's blank first frame. Re-recording needs a live pod; the existing GIF stays as evidence and a re-capture is a follow-up.

  • Third UX pass (suggestions on the PASS verdict): the search-hit delete confirmation names the full relative key; "Open containing folder" moved above Delete with a separator; the search box reads "Search by file name or path…" since the match runs over the whole key.

  • Fourth UX pass: a download that fails from the preview header is now reported inside the dialog (the pane notice sits behind the scrim). Preview redaction now runs over the window plus an 8 KB look-ahead and trims at a whitespace boundary afterwards, so a secret straddling the 256 KB window is masked whole instead of reaching the browser as an unrecognised prefix.

  • Fifth UX pass: the download failure is keyed by object, so a preview shows only its own file's failure; the search box widened so the placeholder is not clipped; the empty search state echoes the query ("No files match “…”.") like the account search does.

  • Sixth UX pass: a search hit truncates its folder prefix (muted) and keeps the basename whole, with the full key on the title; changing the query closes any rename/delete editor left open on a row that is about to unmount. Preview decode + redaction now run off the event loop, like the search redaction.

  • Seventh UX pass: a .pdf whose stored Content-Type is not application/pdf (legacy octet-stream uploads) routes to the "cannot be previewed" fallback instead of a blank frame -- the download response now carries contentType from the HEAD it already makes; clearing the search skips the debounce; the rename editor has its own bad-name wording. Staging moved to a top-level data-home leaf (aws-control-staging) so no agent-writable ancestor sits between the mask and the root.

  • Eighth UX pass: the search-table rename/delete strips get the folder rows' sticky-left viewport-width wrapper (Cancel/Rename stay on-screen on a narrow, horizontally scrolled viewport); "Open containing folder" marks the hit's row or grid card for a few seconds and scrolls it into view once the listing loads.

  • Rename completion is scoped to the row that started it: an editor opened on another row while a rename is in flight keeps its half-typed name on success, and a failure for the earlier row is reported in the page-level notice naming the file (rename_failed_named) rather than under the wrong row or not at all.

  • Preview staging is identity-pinned for the host with no sandbox mask (Windows): the gateway creates the destination itself with O_EXCL, holds the handle open across the CLI call (no delete/rename of the file or any directory above it), re-checks device/inode/link count afterwards, and reads back through the handle rather than reopening the path.

  • Ninth UX pass: the "Open containing folder" highlight window is counted from the moment the row mounts (inside the callback ref), not from the click, so a slow CLI-backed listing no longer spends the window before there is anything to mark; navigating to any other folder retires the marker.

  • Late failures always have a surface: a delete rejected after its confirmation strip is gone (view swapped to search, or moved to another row) is reported in the page-level notice naming the file (delete_failed_named); a query change now closes in-place editors even with a commit in flight, and that commit's outcome takes the same page-level route.

  • Tenth UX pass: a search hit that sits past the first listing page is paged in automatically until its row mounts (or the folder runs out), so the "Open containing folder" marker works in large folders, not only small ones.

  • Staging pin now covers the whole path: the staging root and the per-call directory are held open through a new platform_compat.pin_directory (Windows: a handle without FILE_SHARE_DELETE that refuses a reparse point at the name and blocks rename/delete of the directory and everything above it; POSIX: O_DIRECTORY|O_NOFOLLOW, used as dir_fd for the create) before the destination is named, so a watcher cannot rename the directory away and plant a junction between the create and the CLI's open.

  • Eleventh UX pass: opening a delete confirmation strip resets the shared delete mutation first, so a failed delete on one file no longer pre-renders "Delete failed" under a strip opened later on another file.

  • Advisory findings taken: a 0-byte object (416 InvalidRange on the ranged GET) previews as empty instead of failing; the inline content-type round-trip test now covers the audio/ prefix too.

  • Twelfth UX pass: the preview header shows the folder muted beside a nested key's name (two same-named search hits stay distinguishable once open); the redaction notice reads at body weight since it changes the meaning of what is shown. Advisory: the preview window is now a UTF-8 byte budget (multibyte text no longer carries the look-ahead past it); the inline delete error is keyed to the row whose delete it is, so a failure on A never renders under a strip opened on B.

  • Thirteenth UX pass: preview_failed now says "Download the file to view it, or try again." so the copy and the Try again button beside it point the same way (13 catalogs).

  • Fourteenth UX pass: "Open containing folder" moves keyboard focus onto the highlighted row as it mounts (tabIndex={-1} + focus()), since the menu trigger unmounted with the search view and the ring alone is a cue assistive technology never announces.

  • Mask materialisation is fail-closed all the way: a data home that cannot be resolved now refuses the spawn (SandboxCeilingUnsealable) instead of skipping the staging mask and launching the agent with the directory visible.

@CrysisDeu
CrysisDeu requested a review from a team September 3, 2026 18:25
@CrysisDeu
CrysisDeu requested a review from a team as a code owner September 3, 2026 18:25
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 3, 2026
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 833727e6dd641599821684365133a364fb6dce33 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All hunks are now accounted for: three drive features (preview, rename, search) plus a substantial security-fence apparatus (masked staging dir enforced at every agent spawn, a new platform_compat.pin_directory shim, extra_visible_dirs plumbing through run_aws) that exists solely so the AWS CLI can land preview bytes on disk. Final review follows.

First-Principles-Verdict: CONCERNS

The 256 KB text preview drags a five-file spawn-blocking staging fence into core sandbox/security code, when the gateway could fetch its own presigned URL in memory.

What this change ships

Intent: open, rename, and find drive files without downloading them — an ADDITION.

  1. Images, video, audio and PDF preview inline from the row/tile — justified
  2. Text files preview via a new gateway read, with truncation and redaction notices — justified
  3. Uploads now store a Content-Type for inline-safe media only — justified
  4. An expired media link re-mints once and resumes playback — justified
  5. Inline rename on rows, tiles and search hits, reusing the move API — justified
  6. Whole-drive filename search, capped at 200, write controls withdrawn while searching — justified
  7. "Open containing folder" navigates, marks, scrolls and focuses the hit — justified
  8. A new agent-masked staging dir; every agent spawn now refuses if it can't be materialised — rides along
  9. New public platform_compat.pin_directory + AGENTS.md shim row — one consumer
  10. run_aws/_checked grow extra_visible_dirs — one consumer

Watch

  • Items 8–10 exist only because preview writes S3 bytes to disk through the CLI. The gateway already mints presigned URLs (storage.py:1232 presign) and server-side HTTP is routine (ClientSession in 30+ src/ files); CORS binds only the browser. An in-memory ranged GET removes the file, the mask, the pin, and the plumbing — the description argues only why the browser can't fetch, never why the gateway doesn't.
  • Cost forever: _materialize_maskable_dirs runs before every namespace spawn and raises SandboxCeilingUnsealable on failure, so one app's preview staging can now refuse all agent spawns on a host.
  • The named root cause — CLI transfers through path-named files a same-UID agent can reach — has 2 unfixed siblings (grepped tempfile|mkstemp in the backend): the upload spool tempfile.mkdtemp(prefix="kc-upload-") in shared system temp (routes.py:1141, the CLI reads it), and restore_download's weaker check-then-use fence (backup.py:915). The strongest fence guards the least sensitive transfer.

Subtractions

  • Replace storage.get_object_head_bytes's CLI-to-disk staging with a gateway-side Range fetch of the existing presigned URL, held in memory.
  • That deletes pin_directory and its AGENTS.md row (1 consumer: storage.py:728), extra_visible_dirs on run_aws/_checked (1 consumer: storage.py:776), the aws-control-staging entries in sandbox.py/security.py, and _materialize_maskable_dirs with its two refusal helpers.
  • Failing that, collapse _CREW_PRECREATE_HIDDEN_DIR_LEAVES — a registry with one entry — into the single literal it holds.

[FIRST-PRINCIPLES-REVIEWED] 833727e

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — ✅ PASS

UX-level review of 833727e6dd641599821684365133a364fb6dce33 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The implementation is thorough — complete loading/empty/error/capped states, keyboard paths everywhere, careful error routing per row, honest copy. One residual gap worth noting: a rename from search results whose new name no longer matches the query makes the row silently vanish. Final review:

UX-Verdict: PASS

Every surface carries its states, errors land on the row that caused them, and labels keep their promises — a first-time user can search, preview, rename, and recover.

Suggestions

  • After a successful rename from a search hit (renameMut.onSuccessinvalidate()), a new name that no longer matches the query makes the row vanish with no acknowledgment — the one mutation here whose success can read as loss; surface a one-line status in drive-search-results (e.g. "Renamed to "{{name}}"") when the hit drops out.

[UX-REVIEWED] 833727e

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — 🟡 CONCERNS

Design-level review of 833727e6dd641599821684365133a364fb6dce33 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: CONCERNS

Sound feature slice, but search is an unbounded full-section walk per query, and the disk-staging fence may be heavier than the problem required.

Watch

  • search_keys pages list-objects-v2 over the whole section per debounced query with no page bound or deadline; the early stop fires only on hit-dense queries. A miss-heavy query on a large drive means hundreds of serial sandboxed CLI spawns, minutes in a non-cancellable to_thread worker (each refinement stacks another), and S3 LIST charges — "the token loop… is what lets a hit-heavy search stop" concedes the miss-heavy case is uncapped. Bound pages scanned and report an incomplete walk, or reuse a cached key manifest (the usage path already does full-bucket walks with a cache).

Suggestions

  • The text preview stages bytes on disk via the CLI, which is what forces the entire fence (staging leaf in three registries, precreate-mask machinery, pin_directory with raw Win32). A gateway-side ranged HTTP GET against the presigned URL it already mints (media previews already trust that URL from the browser) needs no credentials, never touches disk, and deletes the whole TOCTOU class the fence exists to close — worth a recorded decision even if the CLI chokepoint wins.

[DESIGN-REVIEWED] 833727e

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 833727e6dd641599821684365133a364fb6dce33 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 833727e

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 833727e6dd641599821684365133a364fb6dce33: <one-sentence reason>

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 833727e6dd641599821684365133a364fb6dce33 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 833727e

Verdict parsed from the review's SHA-scoped output markers for commit 833727e6dd641599821684365133a364fb6dce33.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 833727e6dd641599821684365133a364fb6dce33: <one-sentence reason>

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 3, 2026
@CrysisDeu
CrysisDeu force-pushed the feat/drive-preview-rename-search branch 2 times, most recently from f65801a to 2b73f92 Compare September 3, 2026 19:34
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 3, 2026
@CrysisDeu
CrysisDeu force-pushed the feat/drive-preview-rename-search branch 5 times, most recently from b51f4cf to ac9c082 Compare September 4, 2026 00:58
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Sep 4, 2026
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 4, 2026
@CrysisDeu
CrysisDeu force-pushed the feat/drive-preview-rename-search branch from ac9c082 to c6b7a60 Compare September 4, 2026 06:44
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 4, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

  • PR #8084 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #8084: REBASE. Different user goals in the same file with no shared behavior. Both should land; whichever merges second rebases the import and route-registration regions. Files: src/kiro_crew/apps/builtins/aws_control/backend/routes.py. The two independent directions used different labels; the matrix conservatively retains OVERLAPPING for coordination.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 4, 2026
@CrysisDeu
CrysisDeu force-pushed the feat/drive-preview-rename-search branch from c6b7a60 to 4712439 Compare September 4, 2026 06:59
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 4, 2026
bolichen97
bolichen97 previously approved these changes Sep 4, 2026

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Deep review (escalation: feat:) — approved.

Path traversal: clean. Preview reuses validate_key (rejects ../empty segments, 900-char cap, per-segment allowlist regex), section_key prepends a fixed prefix, and every AWS call is an argv list through _checked with no shell. Preview temp file is mkstemp and unlinked in finally.

Authz: unchanged. Both new GETs wear the existing _guarded (app-enabled + owner-dashboard) and _require_drive (account resolution + S3 consent gate).

Egress: preview text and search keys pass through redact_credentials + redact_exfiltration_urls, the same double pass as listing. Text renders in

{content}
— React-escaped, no HTML sink.

Confinement, net improvement: PDF iframe is sandbox="" and new upload Content-Type is an allowlist denying text/html and image/svg+xml, leaving them at S3 opaque default. Tests pin .html, .htm, .svg, .txt, .js.

Skipping _publish_gate on preview is correct: it mints no bearer URL, unlike download.

CI is fully green on 4712439 — 60+ checks including CodeQL, Semgrep SAST, all 5 AI reviewers.

Two follow-ups (neither blocking): (1) /drive/{account}/preview accepts section=backup and section=library while download refuses backup and search refuses non-drive sections — preview will return the first 256 KB of a backup archive, owner-only and redacted but a consistency gap. (2) search_keys caps hits at SEARCH_MAX_RESULTS but not pages: a no-match query walks every 1000-object page sequentially; worth a page or wall-clock bound.

@bolichen97

Copy link
Copy Markdown
Collaborator

Approved (see review above) but merge conflicts developed — likely from the other PRs that landed on main in this batch. Could you rebase onto current main and push? The approval stands once the head is updated.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

/ai-review override gpt bc70b5f: Search hands keys back through the same egress redaction main's list_section already applies (storage.py _safe_name, pinned by test_names_are_run_through_the_egress_redactors): a credential-shaped key is deliberately unusable from the dashboard on every surface so the credential never reaches the browser — that is the security invariant, not corruption — and an opaque action-token scheme would be a design change for both surfaces, out of scope for this PR.

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Human judgment recorded

@CrysisDeu marked the gpt AI finding as false positive, not applicable, or explicitly accepted for bc70b5f944549e7352b8916751c85d1f504de7f4.

Search hands keys back through the same egress redaction main's list_section already applies (storage.py _safe_name, pinned by test_names_are_run_through_the_egress_redactors): a credential-shaped key is deliberately unusable from the dashboard on every surface so the credential never reaches the browser — that is the security invariant, not corruption — and an opaque action-token scheme would be a design change for both surfaces, out of scope for this PR.

This decision applies only to this commit. A new push requires a new judgment.

Preview: images/video/audio/PDF render from the presigned download URL
(CORS-exempt tags); text files read through a new gateway preview endpoint
(256KB head, the bucket has no CORS for a browser fetch). Rename is a
same-directory move through the existing move endpoint, so every move
guarantee (no overwrite, live-share refusal) applies for free. Search walks
the whole section server-side, case-insensitive, capped at 200 hits.
Uploads now set Content-Type from the file name so new PDFs/videos render
inline instead of downloading.

@chenmingwei23 chenmingwei23 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approved: checks green at head, readiness passed, no blocking reviewer verdict outstanding.

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.

3 participants