feat: uploads sync#30
Open
MoustaphaCamara wants to merge 57 commits into
Open
Conversation
allow scrolling in general when overflowing y axes
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesUpload Storage, Sync, and Frontend Integration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
Comment |
Contributor
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
uploads.go (2)
116-125: 💤 Low valueCreatedAt and ModifiedAt timestamps not populated in returned struct.
Number One notes that while the database auto-generates
created_atandmodified_atvia the schema defaults, the returnedStoredUploadstruct does not include these values. If the frontend relies on these fields immediately after import, they will be empty strings. Consider querying the inserted row to retrieve the actual timestamps, or generating them in Go before insertion.🤖 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 `@uploads.go` around lines 116 - 125, The returned StoredUpload struct is missing CreatedAt and ModifiedAt values; update the insertion flow that constructs and returns &StoredUpload{...} to populate CreatedAt and ModifiedAt (either by generating timestamps in Go before inserting or by querying the inserted row from the DB to read the auto-generated created_at/modified_at); specifically set the CreatedAt and ModifiedAt fields on the returned StoredUpload (alongside ID, RealName, LongName, StorageName, Hash, HashAlgorithm, Filesize, State) using the chosen approach so the frontend receives real timestamps immediately after import.
54-57: 💤 Low valueCaptain, this approach reads the entire file into memory before encryption.
Number One reporting a potential memory concern, Captain. For files of significant magnitude, loading the complete contents via
os.ReadFilecould strain our ship's resources considerably. I recommend we consider a streaming approach for production use—processing the file in chunks would allow us to compute the hash incrementally while encrypting without holding the entire payload in memory.For the current mission parameters, if file sizes are expected to remain modest (a few megabytes), this approach is acceptable. However, I advise logging this in the ship's manifest for future enhancement.
🤖 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 `@uploads.go` around lines 54 - 57, The current code reads the entire file into memory via os.ReadFile(sourcePath) (the block creating content, err), which can OOM for large files; replace it with a streaming approach: open the file with os.Open(sourcePath), defer Close, create a hash.Hash (e.g., sha256.New()) and an encrypting writer/stream (your existing encryption routine or a cipher.StreamWriter), then use io.Copy or io.TeeReader to copy from the file into the encrypting writer while updating the hash incrementally so you never buffer the whole file in memory; update any uses of the in-memory content variable to work with the streamed output (e.g., writing encrypted bytes to disk or returning a reader).sync_push.go (1)
432-441: ⚡ Quick winRecommend consolidating entity type mapping logic.
Number One observes that
elabftwUploadEntityTypeduplicates entity type mapping logic already present inelabftwEntityPath(lines 32-41). Both functions map "experiment" → "experiments" and "resource" → "items", differing only in the leading slash.Consider refactoring to eliminate duplication and ensure consistency. One approach:
♻️ Proposed consolidation
+func elabftwEntityName(entityType string) (string, error) { + switch entityType { + case "experiment": + return "experiments", nil + case "resource": + return "items", nil + default: + return "", fmt.Errorf("Invalid eLabFTW entity type") + } +} + func elabftwEntityPath(entityType string) (string, error) { - switch entityType { - case "experiment": - return "/experiments", nil - case "resource": - return "/items", nil - default: - return "", fmt.Errorf("Invalid eLabFTW entity type") + name, err := elabftwEntityName(entityType) + if err != nil { + return "", err } + return "/" + name, nil } -// handle uploads func elabftwUploadEntityType(entityType string) (string, error) { - switch entityType { - case "experiment": - return "experiments", nil - case "resource": - return "items", nil - default: - return "", fmt.Errorf("Invalid eLabFTW entity type") - } + return elabftwEntityName(entityType) }🤖 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 `@sync_push.go` around lines 432 - 441, elabftwUploadEntityType duplicates the "experiment"→"experiments" and "resource"→"items" mapping already implemented in elabftwEntityPath; consolidate by centralizing the mapping in a single helper used by both functions (for example, create/reuse a single map or helper function like elabftwEntityBase that returns "experiments" or "items") and update elabftwEntityPath to prefix the slash as needed and elabftwUploadEntityType to call that helper (preserve same error behavior and error message when unknown types are passed).sync_links.go (1)
78-81: 💤 Low valueRecommendation: Wrap the URL generation error with context.
Number One observes that when
elabftwOnlineURLfails during iteration, the error lacks context about which remote link caused the failure. This could complicate debugging when multiple remote links exist for an entry. Consider enriching the error message with the failing link's identifiers.🔧 Proposed enhancement
link.URL, err = elabftwOnlineURL(link.SiteURL, link.Type, link.RemoteID) if err != nil { - return nil, err + return nil, fmt.Errorf("generate URL for remote link (type=%s, remoteID=%d): %w", link.Type, link.RemoteID, err) }🤖 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 `@sync_links.go` around lines 78 - 81, The call to elabftwOnlineURL that assigns link.URL lacks contextual info on error; modify the error handling around elabftwOnlineURL(link.SiteURL, link.Type, link.RemoteID) so that when it returns an error you wrap or annotate it with the link identifiers (e.g., link.SiteURL, link.Type, link.RemoteID or link.RemoteID) and a short message indicating which remote link failed—use fmt.Errorf or errors.Wrap to return a new error that includes the original error plus those identifiers where link.URL is being set.sync_uploads.go (1)
115-119: ⚖️ Poor tradeoffOperational consideration: Partial upload push can leave inconsistent state.
Number One notes that if pushing multiple uploads and one fails mid-iteration, previously pushed uploads remain on the remote server while the function returns an error. This creates a partially-synchronized state that may confuse users or require manual cleanup.
Consider one of the following approaches:
- Document this behavior clearly for callers
- Track which uploads were successfully pushed in a local2remote-like join table
- Implement idempotency checks before pushing each upload
This is primarily an operational concern for the calling code to handle, but warrants explicit documentation or enhanced state tracking.
🤖 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 `@sync_uploads.go` around lines 115 - 119, The loop over uploads calling a.pushUploadToRemoteEntity can leave a partially-synced state if one push fails; modify the code so each successful push is recorded in a local-to-remote join (or tracking) table and make pushUploadToRemoteEntity idempotent: before pushing, check the join table for an existing mapping for upload.ID and remoteEntityID to skip duplicates, after a successful push insert the mapping (e.g., record upload.ID -> remoteUploadID), and ensure failures return without mutating the join table; alternatively, if you prefer not to persist state here, add clear documentation on this function's partial-sync behavior and expected caller compensation.dialogs.go (1)
10-10: 💤 Low valueCaptain's log: Documentation inconsistency detected.
The comment states "Handle uploads interactions," but this file's sole responsibility is file selection via dialog. The actual upload handling occurs in
uploads.go. Recommend updating to: "Handle file selection dialog for uploads."🤖 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 `@dialogs.go` at line 10, Update the top-of-file comment in dialogs.go to accurately describe its responsibility: replace "Handle uploads interactions" with "Handle file selection dialog for uploads" so it doesn't imply upload processing (which happens in uploads.go); ensure any nearby comments or package-level docstrings that reference uploads interactions are similarly corrected to mention file selection/dialog only.frontend/src/components/MainApp.svelte (1)
345-351: 💤 Low valueOrders: Remove debug artifacts before mission completion.
The comment markers
<!-- FILES TEST -->(line 345) and<!-- EOFT-->(line 351) appear to be temporary development annotations. These should be removed or replaced with descriptive section comments before the pull request exits draft status.🧹 Proposed cleanup
- <!-- FILES TEST --> <UploadsPanel {profileUuid} entryId={currentEntryId} onAlert={(nextAlert) => alert = nextAlert} /> - <!-- EOFT-->🤖 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 `@frontend/src/components/MainApp.svelte` around lines 345 - 351, Remove the temporary debug HTML comments around the UploadsPanel component: delete the markers "<!-- FILES TEST -->" and "<!-- EOFT-->" (or replace them with a clear descriptive comment) so the JSX/Svelte markup is clean; ensure the UploadsPanel usage with props profileUuid, entryId={currentEntryId}, and onAlert={(nextAlert) => alert = nextAlert} remains unchanged and only the debug annotations are removed or replaced with a concise, descriptive comment.frontend/src/components/Uploads/UploadsPanel.svelte (2)
72-76: 💤 Low valueNote: File size formatting has GB/TB ceiling.
The
formatFileSizehelper handles bytes, KB, and MB ranges but will display all files ≥1GB as MB with potentially large decimal values (e.g., "1024.0 MB" instead of "1.0 GB"). For encrypted research data or media files, this edge case may surface.🔧 Proposed enhancement
function formatFileSize(bytes: number): string { if (bytes < 1024) return `${bytes} B`; if (bytes < 1024 * 1024) return `${(bytes / 1024).toFixed(1)} KB`; + if (bytes < 1024 * 1024 * 1024) return `${(bytes / 1024 / 1024).toFixed(1)} MB`; - return `${(bytes / 1024 / 1024).toFixed(1)} MB`; + return `${(bytes / 1024 / 1024 / 1024).toFixed(1)} GB`; }🤖 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 `@frontend/src/components/Uploads/UploadsPanel.svelte` around lines 72 - 76, The formatFileSize function currently only formats bytes, KB, and MB and will show >=1GB values as large MB numbers; update formatFileSize in UploadsPanel.svelte to handle GB and TB thresholds as well (e.g., bytes < 1024 -> B, < 1024^2 -> KB, < 1024^3 -> MB, < 1024^4 -> GB, else TB) and format each branch with a suitable toFixed precision (e.g., one decimal) so values like 1024^3 bytes display as "1.0 GB" instead of "1024.0 MB".
41-59: ⚖️ Poor tradeoffRecommendation: Consider transaction-like error handling in the import workflow.
The current implementation calls
ImportUpload(which encrypts and stores the file) followed byAttachUploadToEntry(which creates the entry association). If the attach operation fails after a successful import, the encrypted file remains in storage but orphaned from the entry. While this doesn't corrupt data or prevent functionality, it leaves an unreferenced encrypted file on disk.For improved consistency, consider either:
- Catching the attach failure and displaying guidance to retry attachment, or
- Documenting this behavior as acceptable (orphaned uploads can be cleaned up by a future maintenance operation)
The current behavior is functional and the user experience remains intact through error alerts.
🤖 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 `@frontend/src/components/Uploads/UploadsPanel.svelte` around lines 41 - 59, The import workflow in importUpload calls ImportUpload then AttachUploadToEntry which can leave an orphaned encrypted file if the attach step fails; update importUpload to perform transaction-like cleanup: after a successful ImportUpload, wrap the AttachUploadToEntry call in its own try/catch and on attach failure call the upload-delete/cleanup API (e.g., DeleteUpload or RemoveUpload with profileUuid and upload.id) or at minimum present a clear retry/cleanup alert and surface the upload id so the user or a background job can remove it; ensure you still call refreshUploads and preserve existing onAlert error/success messages (referencing importUpload, ImportUpload, AttachUploadToEntry, DeleteUpload/RemoveUpload, refreshUploads, onAlert).
🤖 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 `@elabftw_client.go`:
- Line 216: elabftwRequestWithContentType builds the request URL by
concatenating elabftwAPIBaseURL(cfg.SiteURL) and path without normalizing, which
is inconsistent with elabftwRequest (which forces apiPath = "/" +
strings.TrimLeft(apiPath, "/")); fix by normalizing the path at the start of
elabftwRequestWithContentType (e.g., set path = "/" + strings.TrimLeft(path,
"/") or reuse elabftwRequest normalization logic) before calling http.NewRequest
so all callers produce well-formed URLs; update the code around the
http.NewRequest(...) invocation in elabftwRequestWithContentType accordingly.
In `@sync_push.go`:
- Around line 209-211: The current flows in patchExistingRemoteEntry and
postNewRemoteEntry call pushEntryUploadsToRemoteEntity after committing the
local DB changes (e.g., updating local2remote.modified_at or inserting
local2remote), which leaves the DB claiming success when uploads fail; fix by
moving upload synchronization earlier or decoupling it: either (A) call
pushEntryUploadsToRemoteEntity before you update/insert local2remote and only
commit the modified_at / insert if the push succeeds (wrap both push + DB update
in the same transaction/rollback path), or (B) add a separate tracking table
(e.g., entry_uploads_synced) and make
postNewRemoteEntry/patchExistingRemoteEntry insert/update the entry row first
but mark uploads as pending, then call pushEntryUploadsToRemoteEntity and
atomically update the entry_uploads_synced row on success; ensure
pushEntryUploadsToRemoteEntity is idempotent and that error returns cause no DB
state that falsely indicates uploads were synced.
In `@sync_uploads.go`:
- Around line 43-51: The code reads the entire encrypted file into memory via
os.ReadFile(encryptedPath) then calls decryptRawBytes(a.activeKey,
encryptedContent) without validating size; add a defensive size check before
reading by stat-ing encryptedPath (os.Stat) and rejecting files over a safe
threshold (e.g., configurable maxUploadDecryptSize) with a clear error, or
switch to streaming (open file, io.Reader) and feed a bounded reader to the
decryption routine; update the code paths that use encryptedPath, os.ReadFile
and decryptRawBytes to enforce this limit (or to accept an io.Reader) so large
or malicious files are rejected or processed in a streaming manner.
In `@uploads.go`:
- Around line 95-109: The INSERT into the uploads table (the Exec call inserting
realName, longName, storageName, hash, hashAlgorithm, filesize with state
"local") will violate the UNIQUE(hash, hash_algorithm) constraint for duplicate
files; change the logic to perform a conflict-safe insert (e.g., "INSERT OR
IGNORE" or "INSERT ... ON CONFLICT DO NOTHING") and then SELECT the existing row
when the insert was ignored so the function returns the existing upload record
instead of erroring; update the block that executes the INSERT (the Exec call
using realName, longName, storageName, hash, hashAlgorithm, filesize) to handle
conflict and fetch the existing row by hash/hash_algorithm when insert did not
create a new row.
- Around line 215-220: DetachUploadFromEntry is missing validation that entryID
and uploadID are > 0; add the same checks used in AttachUploadToEntry so the
function rejects non-positive IDs. Specifically, at the start of
DetachUploadFromEntry (before proceeding with requireUnlockedProfile or early
returned after it) validate entryID > 0 and uploadID > 0 and return the same
validation error/response used by AttachUploadToEntry when those checks fail to
keep behavior consistent.
---
Nitpick comments:
In `@dialogs.go`:
- Line 10: Update the top-of-file comment in dialogs.go to accurately describe
its responsibility: replace "Handle uploads interactions" with "Handle file
selection dialog for uploads" so it doesn't imply upload processing (which
happens in uploads.go); ensure any nearby comments or package-level docstrings
that reference uploads interactions are similarly corrected to mention file
selection/dialog only.
In `@frontend/src/components/MainApp.svelte`:
- Around line 345-351: Remove the temporary debug HTML comments around the
UploadsPanel component: delete the markers "<!-- FILES TEST -->" and "<!--
EOFT-->" (or replace them with a clear descriptive comment) so the JSX/Svelte
markup is clean; ensure the UploadsPanel usage with props profileUuid,
entryId={currentEntryId}, and onAlert={(nextAlert) => alert = nextAlert} remains
unchanged and only the debug annotations are removed or replaced with a concise,
descriptive comment.
In `@frontend/src/components/Uploads/UploadsPanel.svelte`:
- Around line 72-76: The formatFileSize function currently only formats bytes,
KB, and MB and will show >=1GB values as large MB numbers; update formatFileSize
in UploadsPanel.svelte to handle GB and TB thresholds as well (e.g., bytes <
1024 -> B, < 1024^2 -> KB, < 1024^3 -> MB, < 1024^4 -> GB, else TB) and format
each branch with a suitable toFixed precision (e.g., one decimal) so values like
1024^3 bytes display as "1.0 GB" instead of "1024.0 MB".
- Around line 41-59: The import workflow in importUpload calls ImportUpload then
AttachUploadToEntry which can leave an orphaned encrypted file if the attach
step fails; update importUpload to perform transaction-like cleanup: after a
successful ImportUpload, wrap the AttachUploadToEntry call in its own try/catch
and on attach failure call the upload-delete/cleanup API (e.g., DeleteUpload or
RemoveUpload with profileUuid and upload.id) or at minimum present a clear
retry/cleanup alert and surface the upload id so the user or a background job
can remove it; ensure you still call refreshUploads and preserve existing
onAlert error/success messages (referencing importUpload, ImportUpload,
AttachUploadToEntry, DeleteUpload/RemoveUpload, refreshUploads, onAlert).
In `@sync_links.go`:
- Around line 78-81: The call to elabftwOnlineURL that assigns link.URL lacks
contextual info on error; modify the error handling around
elabftwOnlineURL(link.SiteURL, link.Type, link.RemoteID) so that when it returns
an error you wrap or annotate it with the link identifiers (e.g., link.SiteURL,
link.Type, link.RemoteID or link.RemoteID) and a short message indicating which
remote link failed—use fmt.Errorf or errors.Wrap to return a new error that
includes the original error plus those identifiers where link.URL is being set.
In `@sync_push.go`:
- Around line 432-441: elabftwUploadEntityType duplicates the
"experiment"→"experiments" and "resource"→"items" mapping already implemented in
elabftwEntityPath; consolidate by centralizing the mapping in a single helper
used by both functions (for example, create/reuse a single map or helper
function like elabftwEntityBase that returns "experiments" or "items") and
update elabftwEntityPath to prefix the slash as needed and
elabftwUploadEntityType to call that helper (preserve same error behavior and
error message when unknown types are passed).
In `@sync_uploads.go`:
- Around line 115-119: The loop over uploads calling a.pushUploadToRemoteEntity
can leave a partially-synced state if one push fails; modify the code so each
successful push is recorded in a local-to-remote join (or tracking) table and
make pushUploadToRemoteEntity idempotent: before pushing, check the join table
for an existing mapping for upload.ID and remoteEntityID to skip duplicates,
after a successful push insert the mapping (e.g., record upload.ID ->
remoteUploadID), and ensure failures return without mutating the join table;
alternatively, if you prefer not to persist state here, add clear documentation
on this function's partial-sync behavior and expected caller compensation.
In `@uploads.go`:
- Around line 116-125: The returned StoredUpload struct is missing CreatedAt and
ModifiedAt values; update the insertion flow that constructs and returns
&StoredUpload{...} to populate CreatedAt and ModifiedAt (either by generating
timestamps in Go before inserting or by querying the inserted row from the DB to
read the auto-generated created_at/modified_at); specifically set the CreatedAt
and ModifiedAt fields on the returned StoredUpload (alongside ID, RealName,
LongName, StorageName, Hash, HashAlgorithm, Filesize, State) using the chosen
approach so the frontend receives real timestamps immediately after import.
- Around line 54-57: The current code reads the entire file into memory via
os.ReadFile(sourcePath) (the block creating content, err), which can OOM for
large files; replace it with a streaming approach: open the file with
os.Open(sourcePath), defer Close, create a hash.Hash (e.g., sha256.New()) and an
encrypting writer/stream (your existing encryption routine or a
cipher.StreamWriter), then use io.Copy or io.TeeReader to copy from the file
into the encrypting writer while updating the hash incrementally so you never
buffer the whole file in memory; update any uses of the in-memory content
variable to work with the streamed output (e.g., writing encrypted bytes to disk
or returning a reader).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 93bb6c4f-cf63-46d5-9f8f-9550cddc14d8
📒 Files selected for processing (14)
crypto.godialogs.goelabftw_client.gofrontend/src/components/MainApp.sveltefrontend/src/components/Uploads/UploadsPanel.sveltefrontend/wailsjs/go/main/App.d.tsfrontend/wailsjs/go/main/App.jsfrontend/wailsjs/go/models.tssqlite.gostorage.gosync_links.gosync_push.gosync_uploads.gouploads.go
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
MoustaphaCamara
marked this pull request as ready for review
July 17, 2026 14:41
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
uploads.go (2)
87-115: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRoll back metadata when encrypted-file persistence fails, Captain.
The row is committed before path creation and
os.WriteFile. Either failure leaves a listed upload whose file does not exist. Use a transaction with rollback and remove the file if commit subsequently fails.Suggested lifecycle
- res, err := db.Exec(` + tx, err := db.Begin() + if err != nil { + return nil, fmt.Errorf("begin upload import: %w", err) + } + defer tx.Rollback() + + res, err := tx.Exec(` INSERT INTO uploads ( entry_id, real_name, long_name, hash, hash_algorithm, filesize, state ) VALUES (?, ?, ?, ?, ?, ?, ?) `, entryID, realName, longName, hash, hashAlgorithm, filesize, "local") @@ if err := os.WriteFile(destPath, encryptedContent, 0o600); err != nil { return nil, fmt.Errorf("write encrypted file: %w", err) } + + if err := tx.Commit(); err != nil { + _ = os.Remove(destPath) + return nil, fmt.Errorf("commit upload import: %w", err) + }🤖 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 `@uploads.go` around lines 87 - 115, The upload metadata flow around the INSERT and encrypted-file persistence must use a transaction: begin before inserting, execute the INSERT through the transaction, and roll back on path creation or os.WriteFile failure. Commit only after the file is successfully written, and remove the created file if the final commit fails, preserving the existing error context.
60-74: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSet the upload cap before
os.ReadFileinuploads.go:60-71, Captain. A large import can still consume excessive memory, and the plaintext size needs headroom for encryption overhead; otherwise files that fit locally can become unsyncable once stored and hitmaxUploadSizeon push.🤖 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 `@uploads.go` around lines 60 - 74, Update the upload flow before os.ReadFile in the source-file handling path to validate the file size against maxUploadSize with sufficient allowance for encryption overhead, rejecting oversized inputs before reading them into memory. Preserve the existing hashing, metadata, and encryption behavior for files within the adjusted limit.
🤖 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 `@sqlite.go`:
- Line 198: Update the error message returned by the schema migration block to
report version 4 instead of version 5, keeping the existing error wrapping and
migration behavior unchanged.
- Around line 35-39: Update the profile database lifecycle around the SQLite
handle initialization so all operations for the same profile reuse one
profile-scoped *sql.DB or otherwise serialize writes across handles, rather than
relying on SetMaxOpenConns(1) and SetMaxIdleConns(1) per handle. Preserve the
existing single-connection settings and add a bounded SQLite busy timeout only
if needed to handle remaining lock contention.
In `@sync_uploads.go`:
- Around line 65-85: Update the upload construction flow around decryptRawBytes
and multipart.NewWriter to avoid retaining encrypted content, plaintext, and a
complete multipart body simultaneously. Stream the multipart form through an
io.Pipe (or equivalent) while reading and decrypting incrementally, preserving
the existing upload field name and upload.RealName, and pass the streaming
reader to the HTTP request.
- Around line 188-205: Update the upload flow around rememberRemoteUpload so a
successful remote creation and its local mapping are persisted or reconciled
atomically enough for retries; when mapping persistence fails after the POST,
retain the remote upload identity and recover it on the next sync instead of
issuing another remote creation. Preserve the existing warning behavior only
after reconciliation cannot establish the mapping.
- Around line 51-63: Update ImportUpload in uploads.go to enforce the shared
plaintext user-file limit before reading or encrypting the source, while
retaining a separate encrypted-storage bound in sync_uploads.go:51-63 that
allows for AEAD overhead. Update sync_uploads.go:51-63 accordingly without
applying the plaintext limit to encrypted output. In
frontend/src/components/Uploads/UploadsPanel.svelte:76, advertise the shared
limit only after ImportUpload enforces it.
---
Outside diff comments:
In `@uploads.go`:
- Around line 87-115: The upload metadata flow around the INSERT and
encrypted-file persistence must use a transaction: begin before inserting,
execute the INSERT through the transaction, and roll back on path creation or
os.WriteFile failure. Commit only after the file is successfully written, and
remove the created file if the final commit fails, preserving the existing error
context.
- Around line 60-74: Update the upload flow before os.ReadFile in the
source-file handling path to validate the file size against maxUploadSize with
sufficient allowance for encryption overhead, rejecting oversized inputs before
reading them into memory. Preserve the existing hashing, metadata, and
encryption behavior for files within the adjusted limit.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ce7e117e-b3b3-46bb-9432-02335ee40b89
📒 Files selected for processing (17)
crypto.goelabftw_client.gofrontend/src/components/Instances/InstancesView.sveltefrontend/src/components/MainApp.sveltefrontend/src/components/Uploads/UploadsPanel.sveltefrontend/src/css/buttons.cssfrontend/src/css/main.cssfrontend/src/css/uploads.cssfrontend/wailsjs/go/main/App.d.tsfrontend/wailsjs/go/main/App.jsfrontend/wailsjs/go/models.tssqlite.gostorage.gosync_links.gosync_push.gosync_uploads.gouploads.go
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/components/MainApp.svelte
- sync_links.go
previously, upload sync built the entire multipart/form-data request in a bytes.Buffer before sending it to eLabFTW. For large uploads this meant keeping three copies of the data in memory at the same time - encrypted upload read from disk; - decrypted plaintext; - and the complete multipart request body. Replace the in-memory multipart buffer with an io.Pipe so the multipart body is generated and sent as the HTTP request is being transmitted. This removes the additional multipart copy and significantly reduces peak memory usage during upload synchronization. The encrypted upload is still read and decrypted as a single payload, since the current encrypted file format stores each upload as one authenticated XChaCha20-Poly1305 message. True end-to-end streaming would require a different on-disk encryption format and is outside the scope of this change. Also release the encrypted buffer after decryption and clear the plaintext before returning from the function.
… having to check for the entry id everytime
…for passwordInputs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix #27
Add uploads
Handle them inside desktop app
Push to eLabFTW
patch entries
move back button
fix modal
ALSO:
have a button to "see in eLabFTW" if the entry already has a correspondance
e.g.,
See experiment #79 in eLabFTWso we know it's an exp to updateSummary by CodeRabbit