Skip to content

fix(dashboard): terminal-aware uploads#896

Open
law-chain-hot wants to merge 17 commits into
boxlite-ai:mainfrom
law-chain-hot:codex/workspace-upload
Open

fix(dashboard): terminal-aware uploads#896
law-chain-hot wants to merge 17 commits into
boxlite-ai:mainfrom
law-chain-hot:codex/workspace-upload

Conversation

@law-chain-hot

@law-chain-hot law-chain-hot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Summary

  • upload files and folders into the active terminal working directory
  • auto-run ls in the existing terminal session after uploads
  • keep terminal fullscreen as a local expand/collapse without refreshing the session
  • update local create-box image refs and websocket auth token handling used by the flow

Verification

  • git diff --check
  • manually tested in local dashboard against dev/local flows

@coderabbitai

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds dashboard Box upload support with TAR-based upload helpers, API integration, mutation wiring, a new upload control, and test execution plumbing. Also changes runner shell startup to prefer /workspace and updates the CLI copy example path.

Changes

Dashboard File Upload Feature

Layer / File(s) Summary
Box upload helpers
apps/dashboard/src/lib/box-upload.ts, apps/dashboard/src/lib/box-upload.test.ts
Adds upload item types, Box upload path normalization, directory grouping, TAR archive creation, and helper tests for path and archive behavior.
Box API upload integration
apps/dashboard/src/lib/cloudBox.ts, apps/dashboard/src/lib/cloudBox.test.ts
Adds Box API helpers that upload tar archives to the files endpoint and tests the request shape and returned remote paths.
Upload mutation hook
apps/dashboard/src/hooks/mutations/useUploadBoxFilesMutation.ts, apps/dashboard/src/hooks/mutations/useUploadBoxFilesMutation.test.tsx
Adds a mutation hook that uploads items sequentially and invalidates box detail queries afterward.
Box upload control
apps/dashboard/src/components/boxes/BoxFileUploadControl.tsx, apps/dashboard/src/components/boxes/BoxFileUploadControl.test.tsx, apps/dashboard/src/components/ui/icon.tsx
Adds the upload icon, drag-and-drop upload control, and tests for file picker and directory-drop handling.
BoxDetails upload wiring
apps/dashboard/src/components/boxes/BoxDetails.tsx
Wires upload state, disabled reasons, and the upload control into the BoxDetails shell area.
Dashboard test target and runner script
apps/dashboard/project.json, apps/scripts/run-dashboard-tests.mjs
Adds the dashboard test target and a Node wrapper that runs Vitest with argument forwarding.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Runner Shell Workspace Default

Layer / File(s) Summary
Workspace-first shell startup
apps/runner/pkg/shellutil/launcher.go, apps/runner/pkg/shellutil/launcher_test.go, src/cli/README.md
Updates shell startup to create and cd into /workspace before HOME fallback, verifies the launcher string, and changes the boxlite cp example destination path.

Dashboard Formatting Cleanup

Layer / File(s) Summary
Formatting-only dashboard edits
apps/dashboard/src/components/BoxSearchCommands.tsx, apps/dashboard/src/components/BoxTable/index.tsx, apps/dashboard/src/components/ui/card.tsx, apps/dashboard/src/components/ui/empty.tsx, apps/dashboard/src/components/ui/sheet.tsx, apps/dashboard/src/components/ui/sonner.tsx, apps/dashboard/src/hooks/useDocsSearchCommands.tsx, apps/dashboard/src/pages/EmailVerify.tsx
Reformats dashboard query, layout, and status JSX, and adjusts Sonner class strings without changing behavior.

API Formatting Cleanup

Layer / File(s) Summary
Box name generator formatting
apps/api/src/box/utils/box-name-generator.ts
Reflows the adjective and animal constant arrays and the generated-name persistence function signature while keeping the same exported behavior.

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant BoxFileUploadControl
  participant BoxDetails
  participant useUploadBoxFilesMutation
  participant cloudBox
  participant BoxAPI

  User->>BoxFileUploadControl: choose or drop files
  BoxFileUploadControl->>BoxDetails: onUpload(items)
  BoxDetails->>useUploadBoxFilesMutation: mutateAsync({boxId, items, destinationDir})
  loop each item
    useUploadBoxFilesMutation->>cloudBox: uploadBoxItemViaBoxApi(item)
    cloudBox->>cloudBox: buildBoxUploadPath + createBoxUploadTar
    cloudBox->>BoxAPI: PUT /files with tar blob
    BoxAPI-->>cloudBox: remote path
  end
  useUploadBoxFilesMutation->>BoxDetails: invalidate detail queries
Loading

Possibly related PRs

  • boxlite-ai/boxlite#880: Touches apps/dashboard/src/components/BoxTable/index.tsx, which this PR also edits in the same pagination UI area.

Suggested reviewers: DorianZheng

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ⚠️ Warning The description omits the required Changes section and includes details not reflected in the diff, so it doesn't fit the template well. Add a Changes section with the key upload/workspace updates and align Summary/Verification with the actual Box upload and shell changes.
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title is concise and matches the upload/workspace-focused changes, though it doesn't mention the main Box upload support explicitly.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@law-chain-hot law-chain-hot marked this pull request as ready for review July 1, 2026 09:44
@law-chain-hot law-chain-hot requested a review from a team as a code owner July 1, 2026 09:44

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 5

🧹 Nitpick comments (4)
src/cli/src/commands/upload.rs (1)

119-147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add negative-path test coverage for validation helpers.

normalize_destination_dir's error branches (empty, non-absolute) and upload_remote_path's directory-vs-file branch aren't covered by any test here, though they're the primary validation/branching logic for the command.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/cli/src/commands/upload.rs` around lines 119 - 147, Add negative-path
tests for the validation helpers in the upload command tests module. Extend the
existing `tests` block to cover `normalize_destination_dir` rejecting empty and
non-absolute directories, and add a test for `upload_remote_path` that exercises
the directory-vs-file branch and verifies the resulting path behavior. Use the
existing helper names `normalize_destination_dir`, `upload_remote_path`, and
`parse_upload_destination` so the new cases are easy to locate alongside the
current coverage.
apps/dashboard/src/lib/box-upload.test.ts (1)

10-12: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Test name overstates coverage — add a case for traversal via destinationDir.

This test only passes a traversal sequence in the filename ('../secret.txt'); destinationDir itself is always a clean literal ('/workspace/'). Given the gap flagged in box-upload.ts (normalizeDestinationDir), consider adding a case like buildBoxFileUploadPath('/workspace/../../etc', file) to lock in the intended behavior once fixed.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/dashboard/src/lib/box-upload.test.ts` around lines 10 - 12, The current
test in buildBoxFileUploadPath only covers traversal in the file name and does
not exercise destinationDir normalization. Update the box-upload test to add a
case where buildBoxFileUploadPath is called with a traversal-style
destinationDir such as /workspace/../../etc and verify normalizeDestinationDir
keeps the path anchored under /workspace. Keep the existing filename traversal
check if useful, but rename or expand the test so its coverage matches the
behavior enforced by normalizeDestinationDir in box-upload.ts.
apps/dashboard/src/hooks/mutations/useUploadBoxFilesMutation.ts (1)

27-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Error lacks operation/resource context.

'Missing organization' doesn't identify the operation (upload) or the box being targeted, making it harder to diagnose from logs/toasts.

As per coding guidelines, "Use explicit errors ... include operation, resource id, endpoint/status, and input shape."

-      if (!selectedOrganization?.id) throw new Error('Missing organization')
+      if (!selectedOrganization?.id) {
+        throw new Error(`Cannot upload files to box ${boxId}: no organization selected`)
+      }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/dashboard/src/hooks/mutations/useUploadBoxFilesMutation.ts` at line 27,
The thrown error in useUploadBoxFilesMutation is too generic and should include
explicit operation/resource context. Update the missing-organization error in
the upload flow to mention the upload operation and the targeted box/resource
(and any relevant input shape or IDs available in the mutation), so logs/toasts
clearly show what failed. Keep the change localized to the
selectedOrganization?.id guard in useUploadBoxFilesMutation and make the message
follow the project’s explicit error guidance.

Source: Coding guidelines

apps/dashboard/src/components/boxes/BoxDetails.tsx (1)

189-195: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Derive the disabled flag from uploadDisabledReason instead of duplicating the conditions, and flatten the nested ternary.

Lines 189-195 encode writePermitted / actionsDisabled / isStoppable(box) as a nested ternary, and line 493 re-derives the exact same three conditions independently for the disabled prop. If either branch is edited later without touching the other, the button's enabled/disabled state and its tooltip reason will silently diverge. The nested ternary also runs against the guideline to prefer guard clauses over deeply nested control flow.

As per coding guidelines, "Prefer guard clauses and early returns over deeply nested control flow."

♻️ Proposed refactor
-  const uploadDisabledReason = !writePermitted
-    ? 'You need write access to upload files'
-    : actionsDisabled
-      ? 'Wait for the current box action to finish'
-      : !box || !isStoppable(box)
-        ? 'Start the box before uploading files'
-        : undefined
+  const getUploadDisabledReason = (): string | undefined => {
+    if (!writePermitted) return 'You need write access to upload files'
+    if (actionsDisabled) return 'Wait for the current box action to finish'
+    if (!box || !isStoppable(box)) return 'Start the box before uploading files'
+    return undefined
+  }
+  const uploadDisabledReason = getUploadDisabledReason()
-                <BoxFileUploadControl
-                  disabled={actionsDisabled || !writePermitted || !isStoppable(box)}
+                <BoxFileUploadControl
+                  disabled={uploadDisabledReason !== undefined}

Also applies to: 493-493

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/dashboard/src/components/boxes/BoxDetails.tsx` around lines 189 - 195,
The upload button state is computed twice and the tooltip reason can drift from
the actual disabled flag. Refactor the BoxDetails component so the
`uploadDisabledReason` logic is flattened into guard-style checks, and then
derive the button’s `disabled` prop directly from `uploadDisabledReason`
(instead of rechecking `writePermitted`, `actionsDisabled`, and
`isStoppable(box)` separately). Use the existing `uploadDisabledReason` and the
upload button render path in `BoxDetails` to keep the enabled/disabled state and
tooltip message in sync.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@apps/dashboard/src/hooks/mutations/useUploadBoxFilesMutation.ts`:
- Around line 29-34: The upload mutation currently only invalidates the
box-detail cache in the success path, so a mid-loop failure in
useUploadBoxFilesMutation leaves already-uploaded files out of sync. Update the
mutation around uploadFileToBoxViaBoxApi and the mutation callbacks so cache
invalidation happens even when the upload throws, preferably in onSettled or
immediately after each successful upload, and ensure the caller can still
identify any failed files for retry.

In `@apps/dashboard/src/lib/box-upload.ts`:
- Around line 55-64: The createTarHeader/writeOctal path can silently overflow
tar header fields when size or other octal values exceed the field capacity. Add
explicit bounds checking inside writeOctal, similar to writeText, so oversized
values throw immediately instead of overwriting the next header field. Use the
writeOctal helper and its callers in createTarHeader to enforce fail-fast
validation for size and mtime inputs.

In `@apps/scripts/run-dashboard-tests.mjs`:
- Around line 27-33: In run-dashboard-tests.mjs, the spawnSync call can fail
with result.error while result.status stays null, causing a silent exit with
code 1. Update the script around the spawnSync result handling to check
result.error first and surface that failure explicitly before exiting. Use the
existing spawnSync invocation and process.exit flow to report the underlying
error details instead of swallowing them.
- Around line 27-31: The spawnSync call in run-dashboard-tests.mjs hardcodes
SHELL to /bin/zsh, which is non-portable and hides missing dependency failures.
Update the child process environment setup to avoid forcing a specific shell
unless there is a documented, necessary reason in the test runner path. If a
shell is required, detect it explicitly and fail fast with a clear error when it
is unavailable, and reference the spawnSync/vitestArgs setup so the intent is
obvious.

In `@src/cli/src/commands/upload.rs`:
- Around line 51-58: The upload flow in handle.copy_into and handle.stop
currently lets a stop failure overwrite the original copy_into error when
!was_running. Restructure the logic so the result from copy_into is preserved
first, then attempt handle.stop() separately and do not use ? in a way that can
replace the upload failure; if both fail, keep the original copy_into error as
the primary one and treat stop errors as secondary. Focus the fix around the
upload command path in upload.rs and the result.map_err(anyhow::Error::from)
return handling.

---

Nitpick comments:
In `@apps/dashboard/src/components/boxes/BoxDetails.tsx`:
- Around line 189-195: The upload button state is computed twice and the tooltip
reason can drift from the actual disabled flag. Refactor the BoxDetails
component so the `uploadDisabledReason` logic is flattened into guard-style
checks, and then derive the button’s `disabled` prop directly from
`uploadDisabledReason` (instead of rechecking `writePermitted`,
`actionsDisabled`, and `isStoppable(box)` separately). Use the existing
`uploadDisabledReason` and the upload button render path in `BoxDetails` to keep
the enabled/disabled state and tooltip message in sync.

In `@apps/dashboard/src/hooks/mutations/useUploadBoxFilesMutation.ts`:
- Line 27: The thrown error in useUploadBoxFilesMutation is too generic and
should include explicit operation/resource context. Update the
missing-organization error in the upload flow to mention the upload operation
and the targeted box/resource (and any relevant input shape or IDs available in
the mutation), so logs/toasts clearly show what failed. Keep the change
localized to the selectedOrganization?.id guard in useUploadBoxFilesMutation and
make the message follow the project’s explicit error guidance.

In `@apps/dashboard/src/lib/box-upload.test.ts`:
- Around line 10-12: The current test in buildBoxFileUploadPath only covers
traversal in the file name and does not exercise destinationDir normalization.
Update the box-upload test to add a case where buildBoxFileUploadPath is called
with a traversal-style destinationDir such as /workspace/../../etc and verify
normalizeDestinationDir keeps the path anchored under /workspace. Keep the
existing filename traversal check if useful, but rename or expand the test so
its coverage matches the behavior enforced by normalizeDestinationDir in
box-upload.ts.

In `@src/cli/src/commands/upload.rs`:
- Around line 119-147: Add negative-path tests for the validation helpers in the
upload command tests module. Extend the existing `tests` block to cover
`normalize_destination_dir` rejecting empty and non-absolute directories, and
add a test for `upload_remote_path` that exercises the directory-vs-file branch
and verifies the resulting path behavior. Use the existing helper names
`normalize_destination_dir`, `upload_remote_path`, and
`parse_upload_destination` so the new cases are easy to locate alongside the
current coverage.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: eff56110-8541-4e27-bfc0-8d3a5f3c1ecd

📥 Commits

Reviewing files that changed from the base of the PR and between 8803834 and 2da489c.

📒 Files selected for processing (19)
  • apps/dashboard/project.json
  • apps/dashboard/src/components/boxes/BoxDetails.tsx
  • apps/dashboard/src/components/boxes/BoxFileUploadControl.test.tsx
  • apps/dashboard/src/components/boxes/BoxFileUploadControl.tsx
  • apps/dashboard/src/components/ui/icon.tsx
  • apps/dashboard/src/hooks/mutations/useUploadBoxFilesMutation.ts
  • apps/dashboard/src/lib/box-upload.test.ts
  • apps/dashboard/src/lib/box-upload.ts
  • apps/dashboard/src/lib/cloudBox.test.ts
  • apps/dashboard/src/lib/cloudBox.ts
  • apps/runner/pkg/shellutil/launcher.go
  • apps/runner/pkg/shellutil/launcher_test.go
  • apps/scripts/run-dashboard-tests.mjs
  • src/cli/README.md
  • src/cli/src/cli.rs
  • src/cli/src/commands/mod.rs
  • src/cli/src/commands/upload.rs
  • src/cli/src/main.rs
  • src/cli/tests/upload.rs

Comment thread apps/dashboard/src/hooks/mutations/useUploadBoxFilesMutation.ts
Comment thread apps/dashboard/src/lib/box-upload.ts
Comment thread apps/scripts/run-dashboard-tests.mjs Outdated
Comment thread apps/scripts/run-dashboard-tests.mjs Outdated
Comment thread src/cli/src/commands/upload.rs Outdated
@law-chain-hot law-chain-hot force-pushed the codex/workspace-upload branch from 2da489c to e015028 Compare July 1, 2026 13:03
@law-chain-hot law-chain-hot changed the title feat(upload): upload files to box workspace feat(files): upload files to box workspace Jul 1, 2026
@law-chain-hot law-chain-hot force-pushed the codex/workspace-upload branch from e015028 to 6beca56 Compare July 1, 2026 13:32

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Nitpick comments (1)
apps/dashboard/src/lib/cloudBox.ts (1)

118-124: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Add upload context to PUT failures.

When this request fails, the caller only gets the raw client exception, which omits which box/path failed. Wrap it with organizationId, boxId, and remotePath while preserving the original cause.

Suggested fix
   const remotePath = buildBoxUploadPath(destinationDir, item)
   const archive = await createBoxUploadTar(item)

-  await api.axiosInstance.put(`${boxesBasePath(organizationId)}/${boxId}/files`, archive, {
-    headers: { 'Content-Type': 'application/x-tar' },
-    params: { path: remotePath },
-  })
+  try {
+    await api.axiosInstance.put(`${boxesBasePath(organizationId)}/${boxId}/files`, archive, {
+      headers: { 'Content-Type': 'application/x-tar' },
+      params: { path: remotePath },
+    })
+  } catch (error) {
+    throw new Error(`Failed to upload Box item to ${remotePath} (org=${organizationId}, box=${boxId})`, {
+      cause: error,
+    })
+  }

As per coding guidelines, "Use explicit errors: fail fast on missing config or invalid inputs; include operation, resource id, endpoint/status, and input shape; preserve the original cause when wrapping; never swallow errors silently."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/dashboard/src/lib/cloudBox.ts` around lines 118 - 124, The PUT in
cloudBox upload currently throws a raw axios error without box/path context.
Update the upload flow around the api.axiosInstance.put call in cloudBox so
failures are caught and rethrown with a clear message that includes
organizationId, boxId, and remotePath, while preserving the original error as
the cause. Keep the existing remotePath/buildBoxUploadPath and
createBoxUploadTar flow, but wrap the request failure in an explicit error in
the same upload function so callers can identify which resource and path failed.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@apps/dashboard/src/components/boxes/BoxFileUploadControl.tsx`:
- Around line 73-78: The drop flow in BoxFileUploadControl’s onDrop currently
awaits buildDroppedUploadItems without handling rejections, so browser read
failures from readFileEntry/readAllDirectoryEntries can fail silently. Wrap the
await in explicit error handling here and in the related folder-read path (the
same upload flow around buildDroppedUploadItems) so rejected dropped-folder
reads are routed to an error callback or toast instead of bypassing onUpload.

In `@apps/dashboard/src/hooks/mutations/useUploadBoxFilesMutation.ts`:
- Around line 25-49: The upload mutation is re-reading selectedOrganization?.id
during invalidation, which can mismatch the organization used by mutationFn if
the user switches orgs mid-upload. In useUploadBoxFilesMutation, snapshot the
starting organization id once inside the mutation flow and reuse that same value
for both uploadBoxItemViaBoxApi calls and
invalidateBoxDetail/queryKeys.boxes.detail so onSettled invalidates the exact
organization and box detail that was updated.

In `@apps/dashboard/src/lib/box-upload.ts`:
- Around line 34-59: Reject duplicate top-level destination names in
buildBoxUploadItems by validating the final BoxUploadItem list before returning
it; if two root selections normalize to the same item.name, throw an error
instead of allowing both uploads. Add a small uniqueness check helper near
buildBoxUploadItems that scans the created items (including
createBoxUploadFileItem and createBoxUploadDirectoryItem outputs) and compares
their name values so collisions are caught early.

---

Nitpick comments:
In `@apps/dashboard/src/lib/cloudBox.ts`:
- Around line 118-124: The PUT in cloudBox upload currently throws a raw axios
error without box/path context. Update the upload flow around the
api.axiosInstance.put call in cloudBox so failures are caught and rethrown with
a clear message that includes organizationId, boxId, and remotePath, while
preserving the original error as the cause. Keep the existing
remotePath/buildBoxUploadPath and createBoxUploadTar flow, but wrap the request
failure in an explicit error in the same upload function so callers can identify
which resource and path failed.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: fb5034da-11fd-4bf7-95c2-3ec283793d54

📥 Commits

Reviewing files that changed from the base of the PR and between e015028 and 6beca56.

📒 Files selected for processing (14)
  • apps/dashboard/project.json
  • apps/dashboard/src/components/boxes/BoxDetails.tsx
  • apps/dashboard/src/components/boxes/BoxFileUploadControl.test.tsx
  • apps/dashboard/src/components/boxes/BoxFileUploadControl.tsx
  • apps/dashboard/src/components/ui/icon.tsx
  • apps/dashboard/src/hooks/mutations/useUploadBoxFilesMutation.ts
  • apps/dashboard/src/lib/box-upload.test.ts
  • apps/dashboard/src/lib/box-upload.ts
  • apps/dashboard/src/lib/cloudBox.test.ts
  • apps/dashboard/src/lib/cloudBox.ts
  • apps/runner/pkg/shellutil/launcher.go
  • apps/runner/pkg/shellutil/launcher_test.go
  • apps/scripts/run-dashboard-tests.mjs
  • src/cli/README.md
✅ Files skipped from review due to trivial changes (1)
  • src/cli/README.md
🚧 Files skipped from review as they are similar to previous changes (6)
  • apps/dashboard/project.json
  • apps/dashboard/src/components/ui/icon.tsx
  • apps/runner/pkg/shellutil/launcher.go
  • apps/scripts/run-dashboard-tests.mjs
  • apps/runner/pkg/shellutil/launcher_test.go
  • apps/dashboard/src/components/boxes/BoxDetails.tsx

Comment thread apps/dashboard/src/components/boxes/BoxFileUploadControl.tsx Outdated
Comment thread apps/dashboard/src/hooks/mutations/useUploadBoxFilesMutation.ts Outdated
Comment thread apps/dashboard/src/lib/box-upload.ts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
apps/dashboard/src/components/boxes/BoxFileUploadControl.test.tsx (1)

61-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Fixed microtask-tick count is fragile; prefer polling for the actual signal.

flushAsyncDrop assumes exactly two Promise.resolve() hops cover the async drop chain (handleDropbuildDroppedUploadItemsreadEntryAsUploadItem/readDirectoryFilesreadAllDirectoryEntries, each adding its own await). This works today but ties the test to internal timing rather than to completion. Any additional await added to the traversal chain, or deeper folder nesting in future fixtures, can silently under-flush and produce assertions that run before onUpload/onError fire — used downstream at lines 149, 168, and 201.

Prefer vi.waitFor(() => expect(onUpload).toHaveBeenCalled()) (or similarly polling for onError) instead of a magic tick count, so the wait scales with actual async completion rather than a hardcoded depth assumption.

♻️ Example refactor
-  async function flushAsyncDrop() {
-    await Promise.resolve()
-    await Promise.resolve()
-  }
+  async function flushAsyncDrop(assertion: () => void) {
+    await vi.waitFor(assertion)
+  }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/dashboard/src/components/boxes/BoxFileUploadControl.test.tsx` around
lines 61 - 64, The test helper flushAsyncDrop is relying on a fixed number of
Promise.resolve() ticks, which makes the drop flow assertions brittle. Update
the BoxFileUploadControl test to wait for the real completion signal instead of
hardcoded microtask flushing, using vi.waitFor around the relevant onUpload or
onError expectation in the async drop scenarios so the test stays stable as
handleDrop, buildDroppedUploadItems, and the readEntry/readDirectory traversal
change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@apps/dashboard/src/components/boxes/BoxFileUploadControl.test.tsx`:
- Around line 61-64: The test helper flushAsyncDrop is relying on a fixed number
of Promise.resolve() ticks, which makes the drop flow assertions brittle. Update
the BoxFileUploadControl test to wait for the real completion signal instead of
hardcoded microtask flushing, using vi.waitFor around the relevant onUpload or
onError expectation in the async drop scenarios so the test stays stable as
handleDrop, buildDroppedUploadItems, and the readEntry/readDirectory traversal
change.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 12f00bb1-70f2-4b3c-89ca-f649129d05f3

📥 Commits

Reviewing files that changed from the base of the PR and between b98755b and 0e841c5.

📒 Files selected for processing (9)
  • apps/dashboard/src/components/boxes/BoxDetails.tsx
  • apps/dashboard/src/components/boxes/BoxFileUploadControl.test.tsx
  • apps/dashboard/src/components/boxes/BoxFileUploadControl.tsx
  • apps/dashboard/src/hooks/mutations/useUploadBoxFilesMutation.test.tsx
  • apps/dashboard/src/hooks/mutations/useUploadBoxFilesMutation.ts
  • apps/dashboard/src/lib/box-upload.test.ts
  • apps/dashboard/src/lib/box-upload.ts
  • apps/dashboard/src/lib/cloudBox.test.ts
  • apps/dashboard/src/lib/cloudBox.ts
🚧 Files skipped from review as they are similar to previous changes (7)
  • apps/dashboard/src/lib/cloudBox.test.ts
  • apps/dashboard/src/hooks/mutations/useUploadBoxFilesMutation.ts
  • apps/dashboard/src/lib/box-upload.test.ts
  • apps/dashboard/src/lib/cloudBox.ts
  • apps/dashboard/src/components/boxes/BoxFileUploadControl.tsx
  • apps/dashboard/src/components/boxes/BoxDetails.tsx
  • apps/dashboard/src/lib/box-upload.ts

Comment thread src/cli/README.md Outdated
@law-chain-hot law-chain-hot changed the title feat(files): upload files to box workspace fix(dashboard): terminal-aware uploads Jul 2, 2026
@law-chain-hot law-chain-hot marked this pull request as draft July 2, 2026 13:43
@law-chain-hot law-chain-hot force-pushed the codex/workspace-upload branch from 98b49dc to 98f4b29 Compare July 2, 2026 13:52
@law-chain-hot law-chain-hot marked this pull request as ready for review July 2, 2026 14:15

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 883c9beedd

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread apps/dashboard/src/lib/cloudBox.ts Outdated
Comment thread apps/dashboard/src/components/boxes/BoxDetails.tsx
Comment thread apps/runner/pkg/api/controllers/proxy.go Outdated
@law-chain-hot law-chain-hot enabled auto-merge July 3, 2026 11:43
@law-chain-hot law-chain-hot force-pushed the codex/workspace-upload branch from 8161b38 to 1bf1134 Compare July 7, 2026 13:25
docker/kubectl exec parity: the guest now remembers the image WORKDIR at
container creation and defaults every exec's cwd to it, so shells and
commands land where the image author intended. The interactive-shell
launcher stops inventing /workspace (custom images may not have it and we
must not create it for them); it only rescues a broken cwd and kicks a
bare-/ landing (image with no WORKDIR) to $HOME for an ssh-like prompt.

Standard BoxLite images are unaffected — they declare WORKDIR /workspace
in their own Dockerfile. Dashboard uploads still start at /workspace and
self-correct to the terminal cwd via OSC 7. CLI cp examples use the
/path/to/your/project placeholder and note the standard-image default.
biome/prettier reflowed 8 files that carry no logic change for this PR
(box-name-generator word lists, several dashboard UI components). Restore
them to main so the diff shows only the workspace-upload / WORKDIR work.
The OCI runtime chdir()s to the exec cwd before our script runs — a
missing WORKDIR fails the spawn itself (same as docker exec), so
[ -d "$PWD" ] could never fire. Keep only the bare-/ → HOME kick, the
one deliberate UX half-step beyond plain docker exec parity.
@law-chain-hot law-chain-hot force-pushed the codex/workspace-upload branch from 35f5bcf to fae5305 Compare July 8, 2026 12:38
@boxlite-agent

boxlite-agent Bot commented Jul 8, 2026

Copy link
Copy Markdown

📦 BoxLite review — couldn't complete

claude exited 1

stdout:
{"type":"result","subtype":"success","is_error":true,"api_error_status":401,"duration_ms":413,"duration_api_ms":0,"num_turns":1,"result":"Invalid API key · Fix external API key","stop_reason":"stop_sequence","session_id":"35fb5525-f1fe-48e3-9e47-2869ad1bf9dc","total_cost_usd":0,"usage":{"input_tokens":0,"cache_creation_input_tokens":0,"cache_read_input_tokens":0,"output_tokens":0,"server_tool_use":{"web_search_requests":0,"web_fetch_requests":0},"service_tier":"standard","cache_creation":{"ephemeral_1h_input_tokens":0,"ephemeral_5m_input_tokens":0},"inference_geo":"","iterations":[],"speed":"standard"},"modelUsage":{},"permission_denials":[],"terminal_reason":"completed","fast_mode_state":"off","uuid":"f4591d48-57a5-4278-a4a3-341537f981a6"}

stderr:
<empty>

powered by BoxLite

@boxlite-agent boxlite-agent Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📦 BoxLite review — 2 issues

Comment thread apps/runner/pkg/api/controllers/proxy.go
Comment thread apps/runner/pkg/api/controllers/proxy.go
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