Skip to content

Add file upload API and UI integration - #21

Merged
cyberinferno merged 3 commits into
masterfrom
feat/upload
Jun 23, 2026
Merged

Add file upload API and UI integration#21
cyberinferno merged 3 commits into
masterfrom
feat/upload

Conversation

@cyberinferno

@cyberinferno cyberinferno commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add backend file upload routes and related server wiring
  • Update the OpenAPI spec and shared constants for upload handling
  • Add UI support for browsing and uploading files from the file tree
  • Document the new workflow in the README

Testing

  • Added/updated server tests for file upload routes
  • UI changes were validated against the existing file browser and uploader flow
  • Not run (not requested)

Summary by CodeRabbit

  • New Features
    • Added drag-and-drop file and folder uploads to the file browser, including chunked uploads with retry, heartbeat, and session cancel.
    • Real-time upload progress with per-file status; SHA-256 integrity verification on completion.
    • Automatic conflict-safe renaming when duplicates exist, and temporary upload data no longer appears in browsing.
  • Documentation
    • Updated API docs and the user guide with the new chunked upload workflow, clarified admin upload permissions, and documented upload size limits.

@coderabbitai

coderabbitai Bot commented Jun 23, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This PR implements end-to-end chunked file upload support. The backend adds a Go fileUploadManager with in-memory session tracking, chunk writing to .part files, SHA-256 integrity verification, name reservation with "(copy)" conflict resolution, and a persistent temp-root registry. Five new HTTP endpoints are wired under /api/file-tree/uploads. The frontend adds a useFileBrowserUploader React hook with a sequential upload worker, exponential retry/backoff, incremental client-side SHA-256 hashing, drag-and-drop/folder support, and an UploadTaskRow progress UI. Game-client data upload handlers are refactored to use shared size-validation logic. Configuration defaults, error constants, upload-limit helpers, API client bindings, OpenAPI spec, comprehensive tests, and documentation updates complete the feature.

Changes

Chunked File Upload Feature

Layer / File(s) Summary
Configuration defaults, error codes, and upload limits
internal/config/config.go, internal/config/config_test.go, internal/constants/constants.go, internal/server/upload_limits.go
Adds DefaultMaxFileUploadSizeMb (1024), MaxFileUploadSizeBytes() conversion with fallback logic, parseMaxFileUploadSizeMb validation, error constants (FILE_TOO_LARGE, DISK_FULL, HASH_MISMATCH, UPLOAD_EXPIRED), and upload-limit helpers for message formatting, HTTP 413 responses, byte formatting, and memory capping.
Server upload manager: session, chunk, completion, and conflict resolution
internal/server/file_upload_routes.go
Implements in-memory upload manager with session creation (path validation, temp-root registration, name reservation with "(copy)" conflict resolution), chunk writing to .part files at offsets, file completion with SHA-256 verification and final rename with conflict resolution, heartbeat/cancellation, TTL-based cleanup, persistent temp-root registry, request normalization, target allocation, and error classification.
Server wiring, lifecycle, and file-tree filtering
internal/server/server.go, internal/server/file_system_routes.go
Adds uploadManager field to Server, initializes and starts it in NewServer, registers shutdown hook, wires all five upload routes under /api/file-tree/uploads, and filters hidden temp directory from file-tree traversal in getSystemRoots and getDirectoryNode.
Frontend API client: routes, schemas, and functions
cmd/omnihance-a3-agent/omnihance-a3-agent-ui/src/lib/api.ts
Extends API_ROUTES with upload URL helpers, adds Zod schemas and TypeScript types for the full upload lifecycle, augments StatusResponse with max_file_upload_size_bytes, and exports createFileUpload, uploadFileChunk, completeFileUpload, heartbeatFileUpload, and cancelFileUpload async functions.
Frontend uploader hook: worker, retry, SHA-256, and drag-and-drop
cmd/omnihance-a3-agent/omnihance-a3-agent-ui/src/components/file-browser-uploader.tsx
Implements useFileBrowserUploader hook with task queue/dedup, sequential worker, per-task orchestration, exponential retry/backoff, incremental client-side SHA-256, WebKit directory entry traversal for folder drops, UploadTaskRow UI component, and helper utilities for state updates, cancellation, error mapping, and async coordination.
Frontend file-tree integration: drop handlers, UI, and polling fix
cmd/omnihance-a3-agent/omnihance-a3-agent-ui/src/components/file-tree.tsx
Integrates useFileBrowserUploader with destination path, permission/emptiness gating, and max-upload-size; wraps file-tree UI with drop handlers and drag-over visual feedback; conditionally renders "Upload files" button; renders uploader dialog and progress panel; fixes directory-download polling effects with window.setTimeout(..., 0) deferred updates.
Game client data upload validation and size enforcement
cmd/omnihance-a3-agent/omnihance-a3-agent-ui/src/lib/upload-validation.ts, cmd/omnihance-a3-agent/omnihance-a3-agent-ui/src/components/client-data/upload-validation.ts, cmd/omnihance-a3-agent/omnihance-a3-agent-ui/src/components/client-data/*.tsx, internal/server/game_client_data_routes.go, internal/server/game_client_data_routes_test.go
Adds validateGameClientUploadFile utility and getUploadSizeError function; integrates validation into MonsterFileUpload, MapFileUpload, and ItemFileUpload with optional max-size prop, selectFile validation flow, formatted file size display, and combined error rendering; refactors game-client upload handlers to use shared readGameClientUploadFile helper with http.MaxBytesReader, errors.As(*http.MaxBytesError) detection, and standardized too-large responses.
Status API: upload size limit exposure
internal/server/status_routes.go, internal/server/status_routes_test.go
Extends /api/status response to include max_file_upload_size_bytes field; adds corresponding StatusResponse struct field with JSON tag; verifies field is populated in tests.
Go server tests: upload routes and game-data validation
internal/server/file_upload_routes_test.go, internal/server/game_client_data_routes_test.go
Tests role rejection, input validation, chunk/file size limits, manager restart, name reservation with copy-naming across tabs/directories/sessions, cancellation and TTL cleanup, hash mismatch HTTP 409 conflict with reservation release, final-path conflict resolution, and game-data oversized-file responses (IT1, MON).
OpenAPI spec and README documentation
cmd/omnihance-a3-agent/docs/openapi.yml, README.md
Updates OpenAPI spec with all five /api/file-tree/uploads endpoints and component schemas; updates game-client-data endpoint descriptions with per-file limit enforcement; augments status endpoint with max_file_upload_size_bytes. Updates README RBAC to include uploading, adds "Chunked File Uploads" capability section, documents MAX_FILE_UPLOAD_SIZE_MB as per-file limit, lists API endpoints, documents per-file limit enforcement for game-client endpoints, and updates usage step 6 with drag-and-drop instructions and directory-download resume guidance.

AGENTS.md Workflow Guidelines

Layer / File(s) Summary
Contributor workflow guidelines
AGENTS.md
Adds "Workflow Guidelines" subsection directing contributors to use GitHub CLI and to sync/branch from master/main before starting work.

Sequence Diagram(s)

sequenceDiagram
  participant Browser as Browser (useFileBrowserUploader)
  participant Server as Server (HTTP handlers)
  participant Manager as fileUploadManager
  participant Disk as Filesystem

  Browser->>Server: POST /api/file-tree/uploads (files metadata, destinationPath, chunkSize)
  Server->>Manager: CreateSession(destPath, files)
  Manager->>Disk: mkdir temp root, register in persistent registry
  Manager-->>Server: sessionId, per-file serverFileIds, reserved target paths, total chunk counts
  Server-->>Browser: CreateFileUploadResponse

  loop For each file × chunk
    Browser->>Server: PUT .../chunks/{index} (binary chunk blob)
    Server->>Manager: UploadChunk(sessionId, fileId, chunkIndex, data)
    Manager->>Disk: write chunk to .part file at offset
    Manager-->>Server: received/total chunk counts
    Server-->>Browser: FileUploadChunkResponse
  end

  par Client-side hashing
    Browser->>Browser: IncrementalSha256.update(chunks)
    Browser->>Browser: IncrementalSha256.finalize() → hex digest
  end

  Browser->>Server: POST .../complete (fileId, sha256)
  Server->>Manager: CompleteFile(sessionId, fileId, sha256)
  Manager->>Disk: hash temp file, compare SHA-256 vs provided
  alt SHA-256 matches
    Manager->>Disk: rename temp → final path (or copy-name if collision)
    Manager-->>Server: finalPath, resolvedPath, serverSha256
    Manager-->>Manager: release file reservation, remove session if all complete
  else SHA-256 mismatch
    Manager->>Disk: delete temp .part file
    Manager-->>Server: HTTP 409 Conflict
    Manager-->>Manager: release only this file's reservation (session remains)
  end
  Server-->>Browser: CompleteFileUploadResponse

  opt Keep-alive
    Browser->>Server: POST .../heartbeat
    Server->>Manager: ExtendTTL(sessionId)
    Manager-->>Server: FileUploadHeartbeatResponse (updated expiresAt)
    Server-->>Browser: 200 OK
  end

  opt User cancellation
    Browser->>Server: DELETE /api/file-tree/uploads/{upload_id}
    Server->>Manager: CancelSession(sessionId)
    Manager->>Disk: delete temp root, release all file reservations
    Server-->>Browser: 204 No Content
  end

  opt Background TTL cleanup
    Manager->>Manager: periodic cleanup ticker
    Manager->>Manager: check LastSeenAt > TTL
    Manager->>Manager: CancelSession (same as explicit cancel)
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Poem

🐇 Hop hop, the files now fly,
In chunks they travel, chunk by chunk they try.
A SHA hash confirms each byte in place,
Copy names prevent a collision's disgrace.
The temp dirs hide, the progress glows below—
Upload complete! The rabbit says: "Let's go!" 🎉

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main changes: adding file upload API and UI integration, which encompasses the new upload routes, OpenAPI specs, and frontend components across multiple files.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/upload

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.

@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: 8

🧹 Nitpick comments (3)
AGENTS.md (1)

136-136: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Consider restructuring for conciseness.

The second bullet is longer and more complex than other guidelines in this document. Following the project's principle to "be concise and minimize prose," consider tightening it by separating concerns or using a numbered substeps format (similar to the "Goal-Driven Execution" section above).

Example restructure:

- Before implementing, check your branch:
  1. On `master` or `main`? Pull latest, then create a new branch: `feat/`, `fix/`, or `docs/` with short meaningful name.
  2. Already on a feature branch? No action needed.
🤖 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 `@AGENTS.md` at line 136, The second bullet point under branch setup guidance
is too verbose and violates the document's principle of conciseness. Restructure
the bullet point that starts with "Before starting to implement anything please
check the active branch" by breaking it into numbered substeps (1 and 2) that
separate the concerns of checking branch status and pulling latest changes from
the default branch versus the action to take when already on a feature branch.
Simplify the language to match the brevity of other guidelines in the document
while preserving all the essential information about branch naming conventions
(feat/, fix/, docs/ prefixes) and keeping names short and meaningful.

Source: Learnings

internal/server/file_upload_routes_test.go (1)

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

Reorder imports to match the repo’s Go import grouping rule.

The import block places internal imports before third-party imports. It should be grouped as: stdlib, third-party, internal.

Suggested diff
 import (
 	"bytes"
 	"crypto/sha256"
 	"encoding/hex"
 	"encoding/json"
 	"net/http"
 	"net/http/httptest"
 	"os"
 	"path/filepath"
 	"strconv"
 	"strings"
 	"sync"
 	"testing"
 	"time"
 
+	"github.com/stretchr/testify/require"
+
 	"github.com/omnihance/omnihance-a3-agent/internal/config"
 	"github.com/omnihance/omnihance-a3-agent/internal/constants"
 	"github.com/omnihance/omnihance-a3-agent/internal/services"
 	"github.com/omnihance/omnihance-a3-agent/internal/utils"
-	"github.com/stretchr/testify/require"
 )

As per coding guidelines, "Group imports in Go files: Stdlib, Third-party, Internal (github.com/omnihance/omnihance-a3-agent/...)."

🤖 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 `@internal/server/file_upload_routes_test.go` around lines 18 - 23, The imports
in the file_upload_routes_test.go file are not properly ordered according to Go
conventions. Reorder the imports block so that the third-party import
(github.com/stretchr/testify/require) comes before the internal imports
(github.com/omnihance/omnihance-a3-agent/internal/config, constants, services,
and utils). Group them as: any stdlib imports first, then third-party imports,
then internal imports, with blank lines separating each group.

Source: Coding guidelines

cmd/omnihance-a3-agent/omnihance-a3-agent-ui/src/components/file-browser-uploader.tsx (1)

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

Unify duplicated drop-upload logic to prevent drift.

The async dataTransfer -> sources -> enqueueUpload -> toast flow is duplicated in two places. Extracting one helper keeps behavior aligned.

Based on learnings, "Do NOT write redundant code" and "Write reusable functions and follow the DRY (Don't Repeat Yourself) principle".

Refactor sketch
+  const enqueueDroppedDataTransfer = useCallback(
+    async (dataTransfer: DataTransfer) => {
+      try {
+        const sources = await uploadSourcesFromDataTransfer(dataTransfer);
+        enqueueUpload(sources);
+      } catch (error) {
+        toast.error(
+          error instanceof Error ? error.message : 'Failed to read dropped files',
+        );
+      }
+    },
+    [enqueueUpload],
+  );

   const handleDrop = useCallback(
     async (event: React.DragEvent<HTMLDivElement>) => {
       if (!canUpload) {
         return;
       }

       event.preventDefault();
       event.stopPropagation();
       setIsDragging(false);
-
-      try {
-        const sources = await uploadSourcesFromDataTransfer(event.dataTransfer);
-        enqueueUpload(sources);
-      } catch (error) {
-        toast.error(
-          error instanceof Error
-            ? error.message
-            : 'Failed to read dropped files',
-        );
-      }
+      await enqueueDroppedDataTransfer(event.dataTransfer);
     },
-    [canUpload, enqueueUpload],
+    [canUpload, enqueueDroppedDataTransfer],
   );

Also applies to: 338-353

🤖 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
`@cmd/omnihance-a3-agent/omnihance-a3-agent-ui/src/components/file-browser-uploader.tsx`
around lines 214 - 233, The dataTransfer processing and upload error handling
logic in the handleDrop function is duplicated in another location (also around
line 338-353). Extract the common pattern of calling
uploadSourcesFromDataTransfer, then enqueueUpload, followed by the toast.error
handling into a single reusable helper function. Then call this new helper from
both duplicate locations to eliminate code duplication and ensure consistent
behavior across both places where this upload flow occurs.

Source: Learnings

🤖 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 `@AGENTS.md`:
- Line 135: Update the text on the line containing "Github CLI" to use the
correct product name capitalization "GitHub CLI" instead, ensuring the H is
capitalized in both occurrences of the product name on that line.
- Line 136: The grammar in the sentence uses the possessive form "its" when it
should use the contraction "it's" (meaning "it is"). In the AGENTS.md file,
replace both instances of "If its" with "If it's" - once before "master or main"
and once before "already in non default branch".

In `@cmd/omnihance-a3-agent/docs/openapi.yml`:
- Around line 5273-5278: The sha256 field in the openapi.yml file currently only
validates length (64 characters) but does not enforce hexadecimal format,
allowing non-hex 64-character strings that would fail backend validation. Add a
regex pattern constraint to the sha256 field definition in openapi.yml that
validates the string contains only lowercase hexadecimal characters (0-9 and
a-f). Then apply the same hexadecimal pattern validation to the sha256 field in
the CompleteFileUploadRequestSchema defined in
cmd/omnihance-a3-agent/omnihance-a3-agent-ui/src/lib/api.ts to ensure the
client-side schema matches the backend requirements.

In
`@cmd/omnihance-a3-agent/omnihance-a3-agent-ui/src/components/file-browser-uploader.tsx`:
- Around line 824-830: The deduplication logic in the file upload handler is
using case-insensitive comparison with toLowerCase() on lines 825 and 829, which
incorrectly treats distinct files with different cases (like Readme.txt and
README.txt) as duplicates on case-sensitive filesystems. Remove the
toLowerCase() calls when checking if a path exists in the seen Set and when
adding paths to the seen Set. Instead, store and compare the relativePath
directly in its original case, so that files with different cases are correctly
recognized as separate files during the deduplication process.
- Around line 501-513: The progress bar container lacks accessibility attributes
needed for screen readers to announce upload progress to assistive technology
users. Add the role attribute with value "progressbar" to the inner div element
(the one with dynamic width styling), and include ARIA value attributes
aria-valuenow set to the current progress percentage value, aria-valuemin set to
0, and aria-valuemax set to 100 to expose the upload progress semantics to
assistive technology. Consider also adding an aria-label attribute to describe
what task or file is being uploaded for better context.

In `@internal/server/file_upload_routes.go`:
- Around line 416-417: The `chunk_size` parameter has no upper bound validation,
which allows an attacker to declare arbitrarily large chunks and exhaust memory
when io.ReadAll buffers the entire chunk body. Add an upper bound validation
check on `chunk_size` before it is used to calculate `expectedSize` in the
io.LimitReader call. Implement this validation at all locations where chunk data
is read with io.ReadAll(io.LimitReader(body, expectedSize+1)) pattern to ensure
chunk_size cannot exceed a reasonable maximum threshold.
- Around line 95-107: The Start() method marks m.started as true before cleanup
succeeds, leaving the manager in an inconsistent started state if cleanup fails.
Move the assignment of m.started = true to after the successful completion of
m.cleanupRegisteredTempRoots() so the manager is only marked as started once
cleanup succeeds. Additionally, the Stop() method has a race condition where it
closes and immediately swaps the stopCh channel, which can cause the worker to
miss the stop signal if it re-enters the select statement after the swap.
Implement proper synchronization in Stop() to ensure the worker has fully
received and processed the stop signal before replacing the stopCh channel, such
as waiting for an acknowledgment from the worker or using a context with
cancellation instead of channel swapping.
- Around line 397-458: The UploadChunk method holds the mutex m.mu across
multiple I/O operations including reading the request body with io.ReadAll,
creating directories with m.fileEditor.MkdirAll, and writing chunks with
tempFile.WriteAt. Refactor the method to release the lock after validating the
upload session and file state checks (after the activeFileLocked call and before
io.ReadAll), perform all I/O operations without holding the lock, then
re-acquire the lock only to update the shared state like file.ReceivedChunks and
session timestamps before returning. This prevents blocking other sessions
during slow I/O operations.

---

Nitpick comments:
In `@AGENTS.md`:
- Line 136: The second bullet point under branch setup guidance is too verbose
and violates the document's principle of conciseness. Restructure the bullet
point that starts with "Before starting to implement anything please check the
active branch" by breaking it into numbered substeps (1 and 2) that separate the
concerns of checking branch status and pulling latest changes from the default
branch versus the action to take when already on a feature branch. Simplify the
language to match the brevity of other guidelines in the document while
preserving all the essential information about branch naming conventions (feat/,
fix/, docs/ prefixes) and keeping names short and meaningful.

In
`@cmd/omnihance-a3-agent/omnihance-a3-agent-ui/src/components/file-browser-uploader.tsx`:
- Around line 214-233: The dataTransfer processing and upload error handling
logic in the handleDrop function is duplicated in another location (also around
line 338-353). Extract the common pattern of calling
uploadSourcesFromDataTransfer, then enqueueUpload, followed by the toast.error
handling into a single reusable helper function. Then call this new helper from
both duplicate locations to eliminate code duplication and ensure consistent
behavior across both places where this upload flow occurs.

In `@internal/server/file_upload_routes_test.go`:
- Around line 18-23: The imports in the file_upload_routes_test.go file are not
properly ordered according to Go conventions. Reorder the imports block so that
the third-party import (github.com/stretchr/testify/require) comes before the
internal imports (github.com/omnihance/omnihance-a3-agent/internal/config,
constants, services, and utils). Group them as: any stdlib imports first, then
third-party imports, then internal imports, with blank lines separating each
group.
🪄 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

Run ID: 052c6c1a-c1e3-4042-a25a-5622bbe7d0ef

📥 Commits

Reviewing files that changed from the base of the PR and between 7c86794 and 5418383.

📒 Files selected for processing (11)
  • AGENTS.md
  • README.md
  • cmd/omnihance-a3-agent/docs/openapi.yml
  • cmd/omnihance-a3-agent/omnihance-a3-agent-ui/src/components/file-browser-uploader.tsx
  • cmd/omnihance-a3-agent/omnihance-a3-agent-ui/src/components/file-tree.tsx
  • cmd/omnihance-a3-agent/omnihance-a3-agent-ui/src/lib/api.ts
  • internal/constants/constants.go
  • internal/server/file_system_routes.go
  • internal/server/file_upload_routes.go
  • internal/server/file_upload_routes_test.go
  • internal/server/server.go

Comment thread AGENTS.md Outdated
Comment thread AGENTS.md Outdated
Comment thread cmd/omnihance-a3-agent/docs/openapi.yml
Comment thread internal/server/file_upload_routes.go
Comment thread internal/server/file_upload_routes.go
Comment thread internal/server/file_upload_routes.go
@cyberinferno

Copy link
Copy Markdown
Contributor Author

@CodeRabbit review please

@coderabbitai

coderabbitai Bot commented Jun 23, 2026

Copy link
Copy Markdown

@cyberinferno Sure, I'll review the PR now!

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@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: 2

🧹 Nitpick comments (4)
cmd/omnihance-a3-agent/omnihance-a3-agent-ui/src/components/client-data/item-file-upload.tsx (1)

36-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff

Consider extracting the shared selection/validation logic.

ItemFileUpload, MapFileUpload, and MonsterFileUpload now carry an almost identical file/validationError state block, selectFile flow, and remove handler (differing only by label, icon, and mutationFn). A small shared hook (e.g. useGameClientFileUpload) would remove the triplicated logic.

Based on learnings: "Write reusable functions and follow the DRY principle".

🤖 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
`@cmd/omnihance-a3-agent/omnihance-a3-agent-ui/src/components/client-data/item-file-upload.tsx`
around lines 36 - 111, Extract the duplicated file upload logic from
ItemFileUpload, MapFileUpload, and MonsterFileUpload components into a custom
hook called useGameClientFileUpload. Move the state declarations (file,
isDragging, validationError, fileInputRef), the upload mutation setup, and all
event handlers (handleDragOver, handleDragLeave, handleDrop, handleFileSelect)
and the selectFile validation logic into this new hook. Have the hook accept
parameters like fileLabel, maxFileUploadSizeBytes, and the mutationFn to handle
the differences between the three components, then return the state variables
and handlers as an object. Replace the duplicated code in each of the three
components with a single call to this custom hook, reducing code repetition
while maintaining the same functionality.

Source: Learnings

internal/config/config.go (1)

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

Replace magic numbers with a named constant.

Line 197 uses 1024 * 1024 as a magic number for the MB-to-bytes conversion factor. As per coding guidelines, avoid using magic numbers in Go; use constants or named variables instead.

♻️ Suggested refactor

Define a constant for the conversion factor:

+const bytesPerMegabyte = 1024 * 1024
+
 func (e *EnvVars) MaxFileUploadSizeBytes() int64 {
 	maxFileUploadSizeMb := DefaultMaxFileUploadSizeMb
 	if e != nil && e.MaxFileUploadSizeMb > 0 {
 		maxFileUploadSizeMb = e.MaxFileUploadSizeMb
 	}
 
-	return int64(maxFileUploadSizeMb) * 1024 * 1024
+	return int64(maxFileUploadSizeMb) * bytesPerMegabyte
 }
🤖 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 `@internal/config/config.go` around lines 191 - 198, The
MaxFileUploadSizeBytes() method uses the magic number 1024 * 1024 for converting
megabytes to bytes. Define a named constant at the package level (such as
BytesPerMegabyte or MbToBytes) with the value 1024 * 1024, and replace the
hardcoded multiplication in the return statement of MaxFileUploadSizeBytes()
with a reference to this constant to improve code readability and
maintainability.

Source: Coding guidelines

internal/server/upload_limits.go (2)

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

Replace magic numbers with named constants.

Lines 14-15 use 1024 as a magic number repeatedly. As per coding guidelines, avoid using magic numbers in Go; use constants or named variables instead.

♻️ Suggested refactor
+const (
+	bytesPerKilobyte = 1024
+	bytesPerMegabyte = bytesPerKilobyte * 1024
+)
+
 const (
-	gameClientMultipartMemoryLimit = 32 * 1024 * 1024
-	gameClientMultipartOverhead    = 1024 * 1024
+	gameClientMultipartMemoryLimit = 32 * bytesPerMegabyte
+	gameClientMultipartOverhead    = bytesPerMegabyte
 )
🤖 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 `@internal/server/upload_limits.go` around lines 13 - 16, The constants
gameClientMultipartMemoryLimit and gameClientMultipartOverhead use the magic
number 1024 directly in their calculations. Create a new named constant to
represent 1024 (one kilobyte) and replace all occurrences of the literal 1024
value with this constant reference in both constant definitions to follow Go
coding guidelines against magic numbers.

Source: Coding guidelines


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

Replace magic numbers with named constants.

Lines 40, 42, 47, and 48 use 1024 as a magic number. As per coding guidelines, avoid using magic numbers in Go; use constants or named variables instead. The same bytesPerKilobyte constant suggested for the file constants can be reused here.

♻️ Suggested refactor
 func formatUploadSize(bytes int64) string {
 	if bytes < 0 {
 		bytes = 0
 	}
 
-	if bytes < 1024 {
+	if bytes < bytesPerKilobyte {
 		return fmt.Sprintf("%d Bytes", bytes)
 	}
 
 	units := []string{"KB", "MB", "GB", "TB"}
-	divisor := int64(1024)
+	divisor := int64(bytesPerKilobyte)
 	unitIndex := 0
-	for bytes/divisor >= 1024 && unitIndex < len(units)-1 {
-		divisor *= 1024
+	for bytes/divisor >= bytesPerKilobyte && unitIndex < len(units)-1 {
+		divisor *= bytesPerKilobyte
 		unitIndex++
 	}
 
 	value := float64(bytes) / float64(divisor)
 	formatted := strconv.FormatFloat(value, 'f', 2, 64)
 	formatted = strings.TrimRight(strings.TrimRight(formatted, "0"), ".")
 	return formatted + " " + units[unitIndex]
 }
🤖 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 `@internal/server/upload_limits.go` around lines 35 - 56, Replace the magic
number 1024 used throughout the formatUploadSize function with a named constant.
Define a package-level constant named bytesPerKilobyte with the value 1024, then
replace all four occurrences of the literal 1024 in the formatUploadSize
function (in the initial bytes comparison, the divisor initialization, and the
for loop condition checking bytes/divisor >= 1024 and divisor *= 1024) with
references to this constant to improve code readability and maintainability.

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 `@AGENTS.md`:
- Line 135: In the AGENTS.md file, change "GitHub related actions" to
"GitHub-related actions" by adding a hyphen between "GitHub" and "related". This
applies the correct compound modifier rule where "GitHub-related" acts as a
single descriptor for the noun "actions".

In `@internal/server/upload_limits.go`:
- Line 15: Remove the unused constant `gameClientMultipartOverhead` from the
upload_limits.go file. This constant is not referenced anywhere in the codebase
and should be deleted to eliminate dead code and improve code cleanliness.

---

Nitpick comments:
In
`@cmd/omnihance-a3-agent/omnihance-a3-agent-ui/src/components/client-data/item-file-upload.tsx`:
- Around line 36-111: Extract the duplicated file upload logic from
ItemFileUpload, MapFileUpload, and MonsterFileUpload components into a custom
hook called useGameClientFileUpload. Move the state declarations (file,
isDragging, validationError, fileInputRef), the upload mutation setup, and all
event handlers (handleDragOver, handleDragLeave, handleDrop, handleFileSelect)
and the selectFile validation logic into this new hook. Have the hook accept
parameters like fileLabel, maxFileUploadSizeBytes, and the mutationFn to handle
the differences between the three components, then return the state variables
and handlers as an object. Replace the duplicated code in each of the three
components with a single call to this custom hook, reducing code repetition
while maintaining the same functionality.

In `@internal/config/config.go`:
- Around line 191-198: The MaxFileUploadSizeBytes() method uses the magic number
1024 * 1024 for converting megabytes to bytes. Define a named constant at the
package level (such as BytesPerMegabyte or MbToBytes) with the value 1024 *
1024, and replace the hardcoded multiplication in the return statement of
MaxFileUploadSizeBytes() with a reference to this constant to improve code
readability and maintainability.

In `@internal/server/upload_limits.go`:
- Around line 13-16: The constants gameClientMultipartMemoryLimit and
gameClientMultipartOverhead use the magic number 1024 directly in their
calculations. Create a new named constant to represent 1024 (one kilobyte) and
replace all occurrences of the literal 1024 value with this constant reference
in both constant definitions to follow Go coding guidelines against magic
numbers.
- Around line 35-56: Replace the magic number 1024 used throughout the
formatUploadSize function with a named constant. Define a package-level constant
named bytesPerKilobyte with the value 1024, then replace all four occurrences of
the literal 1024 in the formatUploadSize function (in the initial bytes
comparison, the divisor initialization, and the for loop condition checking
bytes/divisor >= 1024 and divisor *= 1024) with references to this constant to
improve code readability and maintainability.
🪄 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

Run ID: 455a7a20-53c4-4a55-a8f1-e36701d84d0f

📥 Commits

Reviewing files that changed from the base of the PR and between 5418383 and 2a4f330.

📒 Files selected for processing (23)
  • AGENTS.md
  • README.md
  • cmd/omnihance-a3-agent/docs/openapi.yml
  • cmd/omnihance-a3-agent/omnihance-a3-agent-ui/src/components/client-data-page.tsx
  • cmd/omnihance-a3-agent/omnihance-a3-agent-ui/src/components/client-data/item-file-upload.tsx
  • cmd/omnihance-a3-agent/omnihance-a3-agent-ui/src/components/client-data/map-file-upload.tsx
  • cmd/omnihance-a3-agent/omnihance-a3-agent-ui/src/components/client-data/monster-file-upload.tsx
  • cmd/omnihance-a3-agent/omnihance-a3-agent-ui/src/components/client-data/upload-validation.ts
  • cmd/omnihance-a3-agent/omnihance-a3-agent-ui/src/components/file-browser-uploader.tsx
  • cmd/omnihance-a3-agent/omnihance-a3-agent-ui/src/components/file-tree.tsx
  • cmd/omnihance-a3-agent/omnihance-a3-agent-ui/src/lib/api.ts
  • cmd/omnihance-a3-agent/omnihance-a3-agent-ui/src/lib/upload-validation.ts
  • internal/config/config.go
  • internal/config/config_test.go
  • internal/constants/constants.go
  • internal/server/file_upload_routes.go
  • internal/server/file_upload_routes_test.go
  • internal/server/game_client_data_routes.go
  • internal/server/game_client_data_routes_test.go
  • internal/server/server.go
  • internal/server/status_routes.go
  • internal/server/status_routes_test.go
  • internal/server/upload_limits.go
✅ Files skipped from review due to trivial changes (4)
  • cmd/omnihance-a3-agent/omnihance-a3-agent-ui/src/components/client-data/upload-validation.ts
  • internal/server/status_routes_test.go
  • internal/constants/constants.go
  • README.md
🚧 Files skipped from review as they are similar to previous changes (7)
  • internal/server/server.go
  • internal/server/file_upload_routes_test.go
  • cmd/omnihance-a3-agent/omnihance-a3-agent-ui/src/components/file-tree.tsx
  • cmd/omnihance-a3-agent/omnihance-a3-agent-ui/src/components/file-browser-uploader.tsx
  • cmd/omnihance-a3-agent/omnihance-a3-agent-ui/src/lib/api.ts
  • internal/server/file_upload_routes.go
  • cmd/omnihance-a3-agent/docs/openapi.yml

Comment thread AGENTS.md
Comment thread internal/server/upload_limits.go
@cyberinferno
cyberinferno merged commit 2e445f1 into master Jun 23, 2026
2 checks passed
@cyberinferno
cyberinferno deleted the feat/upload branch June 23, 2026 09:42
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.

1 participant