Refactor virtual drive sync engine - #405
Open
egalvis27 wants to merge 14 commits into
Open
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR adds lazy virtual-drive hydration with SQLite-backed directory state, refactors temporal uploads into an async queue with staged files, updates FUSE operations to hydrate paths on demand, wraps SQLite batch upserts in transactions, and adjusts remote-sync fetching, startup wiring, and DI registration. ChangesVirtual drive hydration, upload queue, and sync flow
Estimated code review effort: 4 (Complex) | ~75 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/features/virtual-drive/ipc/handlers.ts (1)
14-24: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle the fire-and-forget failure path.
void Promise.all([...])drops rejections from either async call, so a failure inupdateVirtualDriveContainerorDirectoryStateSqliteRepository.clear()becomes an unhandled promise rejection.🤖 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/backend/features/virtual-drive/ipc/handlers.ts` around lines 14 - 24, The fire-and-forget logic in remoteChangesSyncedHandler currently ignores failures from both updateVirtualDriveContainer and DirectoryStateSqliteRepository.clear(), which can surface as unhandled rejections. Update this handler to explicitly catch and log errors from the Promise.all work, using the existing remoteChangesSyncedHandler, updateVirtualDriveContainer, and DirectoryStateSqliteRepository.clear symbols so the failure path is handled safely without dropping rejected promises.
🧹 Nitpick comments (5)
src/backend/features/virtual-drive/services/operations/opendir.service.test.ts (1)
57-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest only covers rejection with an already-well-formed
FuseError.This masks the cast issue in
opendir.service.ts(error: err as FuseError) — add a case wherehydrator.readDirectoryrejects with a plainErrorto verify the service still returns a validFuseErrorwith a correct.code.🤖 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/backend/features/virtual-drive/services/operations/opendir.service.test.ts` around lines 57 - 59, The existing opendir service test only exercises a rejection that is already a FuseError, so it does not catch the unsafe cast in opendir.service.ts. Extend the test in opendir.service.test.ts around hydrator.readDirectory to also reject with a plain Error and assert the service still resolves to a FuseError with the expected code. Use the opendir service path and the FuseError/FuseCodes assertions to verify the fallback behavior when err is not already a FuseError.src/backend/features/virtual-drive/services/operations/get-attributes.service.ts (1)
86-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate file/folder attribute-mapping logic.
The hydrated-entry file/folder attribute blocks (Lines 89-117) closely mirror the earlier local-entry blocks (Lines 54-82). Consider extracting
toFileAttributes(file)/toFolderAttributes(folder)helpers to avoid drift between the two near-identical mappings.🤖 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/backend/features/virtual-drive/services/operations/get-attributes.service.ts` around lines 86 - 118, The file and folder attribute mapping in GetAttributesService is duplicated between the local-entry and hydrated-entry branches, which risks drift over time. Refactor the shared logic into reusable helpers such as toFileAttributes and toFolderAttributes (or equivalent methods near findLocalEntry / ensurePathLoaded) and have both branches call them so the mode, size, timestamps, uid/gid, and nlink mapping stays consistent in one place.src/backend/features/virtual-drive/services/operations/get-attributes.service.test.ts (1)
75-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for hydrated-entry early-return branches.
The new logic in
getAttributes(implementation Lines 86-117) re-resolvesfindLocalEntryafter hydration and returns early if a file/folder is now found — this is the core new behavior for this layer. This test only exercises the path where hydration finds nothing and falls back todocument. Add cases wherefileSearcher.run/folderSearcher.runresolve to a value only afterhydrator.ensurePathLoadedis called, to verify the hydrated early-return paths.🤖 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/backend/features/virtual-drive/services/operations/get-attributes.service.test.ts` around lines 75 - 84, The current getAttributes test only covers the fallback path after hydration, but it misses the new early-return behavior after re-running findLocalEntry. Add test cases in get-attributes.service.test.ts where hydrator.ensurePathLoaded is invoked and then fileSearcher.run or folderSearcher.run resolves to a hydrated file/folder, and assert getAttributes returns those attributes immediately instead of continuing to document fallback. Use the existing getAttributes, hydrator.ensurePathLoaded, fileSearcher.run, and folderSearcher.run symbols to target the new hydrated-entry branches.src/context/storage/TemporalFiles/application/upload/TemporalFileUploadQueue.ts (1)
12-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
PropsandUploadTaskare structurally identical.Consider reusing one type to avoid duplication as the shape evolves.
🤖 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/context/storage/TemporalFiles/application/upload/TemporalFileUploadQueue.ts` around lines 12 - 22, Props and UploadTask are duplicated with the same shape, so reuse a single type definition in TemporalFileUploadQueue to avoid drift as fields change. Update the queue’s type declarations so one of the symbols, Props or UploadTask, becomes the shared source of truth and the other references it directly.src/apps/drive/dependency-injection/virtual-drive/registerFolderServices.ts (1)
28-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
// TODO: can be private?comment on line 28 is now stale — this binding is intentionally made non-private so the lazy hydrator can resolveFolderRepository. Consider removing the TODO to avoid confusion.🤖 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/apps/drive/dependency-injection/virtual-drive/registerFolderServices.ts` around lines 28 - 30, The TODO comment in registerFolderServices is stale because the FolderRepository binding is intentionally public for lazy hydrator resolution. Remove the “can be private?” comment from the builder.register(FolderRepository).use(InMemoryFolderRepository).asSingleton() registration so the intent is clear and there’s no confusion for future readers.
🤖 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 `@src/apps/main/database/data-source.ts`:
- Around line 41-49: `initializeVirtualDriveSqlite()` is currently
all-or-nothing, so a single failing bootstrap statement can stop startup and
prevent the `APP_DATA_SOURCE_INITIALIZED` emit from happening. Update this
helper to make `SQLITE_BOOTSTRAP_STATEMENTS` best-effort by handling errors per
statement inside the loop, logging failures with context via
`AppDataSource.query`, and continuing instead of throwing so the virtual-drive
startup path can proceed.
In
`@src/backend/features/virtual-drive/services/lazy/LazyVirtualDriveHydrator.ts`:
- Around line 51-52: The `readLocalDirectory` call in `LazyVirtualDriveHydrator`
is passing an unused `statusScope` argument, which is misleading and flagged by
lint. Remove the `statusScope` parameter from the `readLocalDirectory` signature
and update the `LazyVirtualDriveHydrator` call site (and any other referenced
overloads around the same code path) so `folder` is the only needed argument,
keeping the behavior unchanged.
- Around line 79-96: The loop in LazyVirtualDriveHydrator is intentionally
sequential, so the no-await-in-loop lint warning should be handled explicitly
rather than left unresolved. Add a targeted lint disable comment on the await
inside the path-segment loop in LazyVirtualDriveHydrator, and include a brief
justification that each segment depends on the previous folder being hydrated
before continuing.
- Around line 127-196: The delete order in fetchAndStoreChildren is unsafe
because folderRepository.deleteMatchingPartial and
fileRepository.deleteMatchingPartial run before the new local children are fully
persisted. Update fetchAndStoreChildren to write the replacement folders/files
first using createOrUpdateFolderByBatch, createOrUpdateFileByBatch, and the
Promise.all add/upsert step, then remove stale rows afterward. Keep the existing
mapping logic and error handling, but make sure the durable insert/update
completes before any delete calls.
In
`@src/context/storage/TemporalFiles/application/upload/TemporalFileUploadQueue.ts`:
- Around line 40-56: The deduplication in TemporalFileUploadQueue.enqueue is
vulnerable to a race because queuedPaths is only updated after the await on
repository.stage, allowing duplicate uploads for the same path. Reserve the path
in queuedPaths before calling repository.stage in enqueue, then proceed to stage
and push the task using the existing enqueue, drain, and queuedPaths symbols so
concurrent calls can no longer pass the has check before the path is marked.
In `@src/context/virtual-drive/files/infrastructure/InMemoryFileRepository.ts`:
- Around line 107-118: deleteMatchingPartial in InMemoryFileRepository should
use the same contentsId normalization behavior as matchingPartial, since strict
equality can miss entries that should match. Update the comparison logic inside
deleteMatchingPartial to special-case contentsId the same way matchingPartial
does, while keeping the rest of the key checks unchanged, so the two methods
stay consistent.
---
Outside diff comments:
In `@src/backend/features/virtual-drive/ipc/handlers.ts`:
- Around line 14-24: The fire-and-forget logic in remoteChangesSyncedHandler
currently ignores failures from both updateVirtualDriveContainer and
DirectoryStateSqliteRepository.clear(), which can surface as unhandled
rejections. Update this handler to explicitly catch and log errors from the
Promise.all work, using the existing remoteChangesSyncedHandler,
updateVirtualDriveContainer, and DirectoryStateSqliteRepository.clear symbols so
the failure path is handled safely without dropping rejected promises.
---
Nitpick comments:
In `@src/apps/drive/dependency-injection/virtual-drive/registerFolderServices.ts`:
- Around line 28-30: The TODO comment in registerFolderServices is stale because
the FolderRepository binding is intentionally public for lazy hydrator
resolution. Remove the “can be private?” comment from the
builder.register(FolderRepository).use(InMemoryFolderRepository).asSingleton()
registration so the intent is clear and there’s no confusion for future readers.
In
`@src/backend/features/virtual-drive/services/operations/get-attributes.service.test.ts`:
- Around line 75-84: The current getAttributes test only covers the fallback
path after hydration, but it misses the new early-return behavior after
re-running findLocalEntry. Add test cases in get-attributes.service.test.ts
where hydrator.ensurePathLoaded is invoked and then fileSearcher.run or
folderSearcher.run resolves to a hydrated file/folder, and assert getAttributes
returns those attributes immediately instead of continuing to document fallback.
Use the existing getAttributes, hydrator.ensurePathLoaded, fileSearcher.run, and
folderSearcher.run symbols to target the new hydrated-entry branches.
In
`@src/backend/features/virtual-drive/services/operations/get-attributes.service.ts`:
- Around line 86-118: The file and folder attribute mapping in
GetAttributesService is duplicated between the local-entry and hydrated-entry
branches, which risks drift over time. Refactor the shared logic into reusable
helpers such as toFileAttributes and toFolderAttributes (or equivalent methods
near findLocalEntry / ensurePathLoaded) and have both branches call them so the
mode, size, timestamps, uid/gid, and nlink mapping stays consistent in one
place.
In
`@src/backend/features/virtual-drive/services/operations/opendir.service.test.ts`:
- Around line 57-59: The existing opendir service test only exercises a
rejection that is already a FuseError, so it does not catch the unsafe cast in
opendir.service.ts. Extend the test in opendir.service.test.ts around
hydrator.readDirectory to also reject with a plain Error and assert the service
still resolves to a FuseError with the expected code. Use the opendir service
path and the FuseError/FuseCodes assertions to verify the fallback behavior when
err is not already a FuseError.
In
`@src/context/storage/TemporalFiles/application/upload/TemporalFileUploadQueue.ts`:
- Around line 12-22: Props and UploadTask are duplicated with the same shape, so
reuse a single type definition in TemporalFileUploadQueue to avoid drift as
fields change. Update the queue’s type declarations so one of the symbols, Props
or UploadTask, becomes the shared source of truth and the other references it
directly.
🪄 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: 522a22bd-87c6-469c-a716-8b578d062dac
📒 Files selected for processing (37)
src/apps/drive/dependency-injection/offline-drive/registerTemporalFilesServices.tssrc/apps/drive/dependency-injection/virtual-drive/registerFilesServices.tssrc/apps/drive/dependency-injection/virtual-drive/registerFolderServices.tssrc/apps/drive/dependency-injection/virtual-drive/registerVirtualDriveSharedServices.tssrc/apps/main/database/data-source.tssrc/apps/main/remote-sync/RemoteSyncManager.test.tssrc/apps/main/remote-sync/RemoteSyncManager.tssrc/apps/main/remote-sync/service.tssrc/backend/features/virtual-drive/ipc/handlers.tssrc/backend/features/virtual-drive/services/drive-folder/virtual-drive.service.test.tssrc/backend/features/virtual-drive/services/drive-folder/virtual-drive.service.tssrc/backend/features/virtual-drive/services/lazy/DirectoryStateSqliteRepository.tssrc/backend/features/virtual-drive/services/lazy/LazyVirtualDriveHydrator.tssrc/backend/features/virtual-drive/services/operations/get-attributes.service.test.tssrc/backend/features/virtual-drive/services/operations/get-attributes.service.tssrc/backend/features/virtual-drive/services/operations/open.service.test.tssrc/backend/features/virtual-drive/services/operations/open.service.tssrc/backend/features/virtual-drive/services/operations/opendir.service.test.tssrc/backend/features/virtual-drive/services/operations/opendir.service.tssrc/backend/features/virtual-drive/services/operations/release.service.test.tssrc/backend/features/virtual-drive/services/operations/release.service.tssrc/context/storage/TemporalFiles/application/upload/TemporalFileUploadQueue.tssrc/context/storage/TemporalFiles/domain/TemporalFileRepository.tssrc/context/storage/TemporalFiles/infrastructure/NodeTemporalFileRepository.tssrc/context/virtual-drive/files/domain/FileRepository.tssrc/context/virtual-drive/files/infrastructure/InMemoryFileRepository.tssrc/context/virtual-drive/folders/__mocks__/FolderRepositoryMock.tssrc/context/virtual-drive/folders/application/create/SimpleFolderCreator.tssrc/context/virtual-drive/folders/domain/FolderRepository.tssrc/context/virtual-drive/folders/infrastructure/InMemoryFolderRepository.tssrc/context/virtual-drive/remoteTree/infrastructure/SQLiteRemoteItemsGenerator.tssrc/core/bootstrap/register-session-event-handlers.tssrc/core/electron/paths.tssrc/infra/local-file-system/safe-readdir.test.tssrc/infra/sqlite/services/file/create-or-update-file-by-batch.tssrc/infra/sqlite/services/folder/create-or-update-folder-by-batch.tsvitest.setup.main.ts
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (3)
src/context/storage/TemporalFiles/application/upload/TemporalFileUploadQueue/preserve-rejected-upload.test.ts (1)
29-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd test for the preservation-error case.
There's no test covering when
preserveRejectedFileSizeTooBigreturns an error. In that case,deleter.runshould not be called and the function should return early.♻️ Suggested test
it('should not delete the file when preservation fails', async () => { const deleter = { run: vi.fn().mockResolvedValue(undefined) }; const task = { temporalFile: { contentFilePath: '/tmp/content.bin', size: { value: 123 }, } as unknown as TemporalFile, path: '/target/file.txt', processName: 'process', }; vi.mocked(preserveRejectedFileSizeTooBig).mockResolvedValue({ error: new Error('copy failed') } as never); await preserveRejectedUpload({ task, deleter } as never); expect(deleter.run).not.toHaveBeenCalled(); });🤖 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/context/storage/TemporalFiles/application/upload/TemporalFileUploadQueue/preserve-rejected-upload.test.ts` around lines 29 - 50, The preserveRejectedUpload test suite is missing coverage for the preservation-error path, so add a new test around preserveRejectedUpload and preserveRejectedFileSizeTooBig that mocks a returned error. In this case, assert that deleter.run is not called and that the function returns early without attempting deletion; use the existing task shape and the preserveRejectedFileSizeTooBig mock to locate the behavior.src/context/virtual-drive/files/infrastructure/InMemoryFileRepository.ts (1)
30-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer strict equality (
===) over loose equality (==).The
matchesPartialhelper uses==for both thecontentsIdnormalization branch and the generic comparison. The past review's proposed fix used===, and strict equality is the TypeScript best practice to avoid subtle type-coercion bugs.♻️ Proposed refactor
return keys.every((key: keyof FileAttributes) => { if (key === 'contentsId') { - return attributes[key].normalize() == (partial[key] as string).normalize(); + return attributes[key].normalize() === (partial[key] as string).normalize(); } - return attributes[key] == partial[key]; + return attributes[key] === partial[key]; });🤖 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/context/virtual-drive/files/infrastructure/InMemoryFileRepository.ts` around lines 30 - 36, The matchesPartial helper in InMemoryFileRepository currently uses loose equality for both the contentsId normalization branch and the generic attribute comparison. Update the comparisons in matchesPartial to use strict equality instead, including the normalized contentsId check, so the behavior is explicit and avoids type coercion; keep the rest of the key iteration logic unchanged.src/context/storage/TemporalFiles/application/upload/TemporalFileUploadQueue/drain-upload-queue.test.ts (1)
47-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for the generic (non-UploadSizeLimitError) failure path.
The current tests cover the happy path and the
UploadSizeLimitErrorpath, but there's no test for whenuploadQueuedTaskthrows a generic error. This is the path with the orphaned-file issue, so a test would help verify the intended behavior and prevent regressions.♻️ Suggested test
it('should log and keep task in queue on generic error', async () => { const state = createTemporalFileUploadQueueState(); const task = { temporalFile: { path: '/staged/file.txt' }, path: '/target/file.txt', processName: 'process', }; state.tasks.push(task as never); state.queuedPaths.add(task.path); const deleter = { run: vi.fn().mockResolvedValue(undefined) }; vi.mocked(uploadQueuedTask).mockRejectedValue(new Error('network failure')); await drainUploadQueue({ uploader: {} as never, deleter, fileSearcher: {} as never, state, } as never); expect(deleter.run).not.toHaveBeenCalled(); // Assert based on the intended behavior after fixing the issue: // - If retrying: expect(state.tasks).toHaveLength(1); // - If cleaning up: expect(deleter.run).toHaveBeenCalledWith(task.path); });🤖 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/context/storage/TemporalFiles/application/upload/TemporalFileUploadQueue/drain-upload-queue.test.ts` around lines 47 - 74, Add a test in drainUploadQueue.test for the generic uploadQueuedTask failure path, not just UploadSizeLimitError. Use the existing drainUploadQueue, uploadQueuedTask, and createTemporalFileUploadQueueState setup to mock a non-UploadSizeLimitError rejection (for example, a generic Error) and assert the intended orphaned-file behavior in this branch. Verify the side effects differ from the oversized-upload case by checking whether deleter.run is called or the task remains queued, and keep the task/state assertions aligned with the actual behavior of drainUploadQueue.
🤖 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
`@src/backend/features/user/file-size-limit/upload-size-limit-blocked-paths.ts`:
- Around line 1-13: This file likely has Prettier formatting issues flagged by
the format check; run the repository formatting command to auto-fix the style in
the upload-size-limit-blocked-paths module. Verify the exported helpers
markUploadSizeLimitBlockedPath, isUploadSizeLimitBlockedPath, and
clearUploadSizeLimitBlockedPath remain unchanged functionally, then commit the
formatted output.
In
`@src/backend/features/virtual-drive/services/lazy/virtual-drive-hydrator/children.ts`:
- Around line 106-108: The isExistingFile helper currently only checks
FileDto.status and can still treat deleted or removed files as existing. Update
isExistingFile in children.ts to mirror isExistingFolder by also excluding
deleted and removed states, and ensure the hydration path that uses this helper
does not include those files in listings or persistence.
In
`@src/backend/features/virtual-drive/services/lazy/virtual-drive-hydrator/create-lazy-virtual-drive-hydrator-service.ts`:
- Around line 1-94: The file is failing Prettier check, so reformat the
`createLazyVirtualDriveHydratorService` module to match the project style
without changing behavior. Run the formatter on this file and ensure the
exported `LazyVirtualDriveHydrator` symbol, `readDirectory`, and
`ensurePathLoaded` remain intact with only formatting updates.
- Around line 69-88: Refresh the parent folder before retrying the file lookup
in ensurePathLoaded, since it currently only calls ensureFolderMaterialized and
may skip repopulating children when the parent is already fresh. Update the
logic in create-lazy-virtual-drive-hydrator-service.ts so ensurePathLoaded also
forces a refresh of the parent directory’s children before open retries the
lookup, using the existing refreshChildrenIfNeeded path along with
folderRepository, directoryStateRepository, inflight, and
getFetchAndStoreChildren() to keep new files visible.
In
`@src/backend/features/virtual-drive/services/lazy/virtual-drive-hydrator/fetch-and-store-children.ts`:
- Around line 43-52: The delete-then-add flow in fetchAndStoreChildren leaves
the in-memory repositories inconsistent if one of the Promise.all writes fails.
Update the logic in fetchAndStoreChildren to avoid clearing existing children
before the new ones are safely present, either by upserting/adding the new
folder and file children first and then removing stale entries, or by adding
rollback/compensation around the delete-and-replace sequence so a partial
failure does not leave the virtual drive missing entries.
- Around line 55-58: The catch block in fetchAndStoreChildren lets
directoryStateRepository.markError replace the original failure, which can
prevent logger.error and the intended FuseError from being raised. Update the
error-handling path in fetchAndStoreChildren so markError is best-effort and
wrapped separately, preserving the original caught error; then always log the
failure and throw the FuseError(EIO) for the folder path even if markError
fails.
In `@src/backend/features/virtual-drive/services/operations/release.service.ts`:
- Around line 73-75: In `release.service.ts`, the error handling after
`enqueueTemporalFile()` is deleting the temporal file for all failures, which
can destroy the only copy on transient enqueue errors. Update the `release` flow
to preserve `path` whenever `enqueueTemporalFile()` rejects for
non-`UploadSizeLimitError` cases, and only remove or clean up the file in the
explicit non-recoverable path; use the existing `enqueueTemporalFile`,
`UploadSizeLimitError`, and `deleteTemporalFile` logic to route failures
correctly.
In
`@src/context/storage/TemporalFiles/application/upload/TemporalFileUploadQueue/drain-upload-queue.ts`:
- Around line 15-34: The retry handling in processTask is dropping transient
failures from the in-memory queue while also leaving the staged file on disk,
which can orphan uploads. Update the UploadSizeLimitError /
non-UploadSizeLimitError flow in drain-upload-queue.ts so state.tasks.shift()
and state.queuedPaths.delete(...) only happen after a successful upload or a
permanent rejection handled by preserveRejectedUpload; for transient errors,
keep the task queued for retry (or explicitly delete the file if you choose not
to retry). Ensure the logger.error path and deleter.run behavior in processTask
reflect the chosen retry/delete policy consistently.
In
`@src/context/storage/TemporalFiles/application/upload/TemporalFileUploadQueue/enqueue-upload.ts`:
- Line 40: The fire-and-forget call to drainUploadQueue in enqueue-upload.ts can
still produce an unhandled rejection if preserveRejectedUpload fails, since the
returned promise is discarded. Update the enqueue-upload flow to attach a
top-level catch to the drainUploadQueue invocation, or handle the
preserveRejectedUpload failure inside processTask, so unexpected errors are
swallowed or logged instead of bubbling unhandled.
In
`@src/context/storage/TemporalFiles/application/upload/TemporalFileUploadQueue/state.ts`:
- Around line 1-11: Prettier formatting is out of sync in
createTemporalFileUploadQueueState, so update the affected file to match the
repo’s formatter output. Re-run formatting on the changed files with the
project’s Prettier command, then verify the formatting around
createTemporalFileUploadQueueState and the QueueState object literal is
consistent with CI expectations.
---
Nitpick comments:
In
`@src/context/storage/TemporalFiles/application/upload/TemporalFileUploadQueue/drain-upload-queue.test.ts`:
- Around line 47-74: Add a test in drainUploadQueue.test for the generic
uploadQueuedTask failure path, not just UploadSizeLimitError. Use the existing
drainUploadQueue, uploadQueuedTask, and createTemporalFileUploadQueueState setup
to mock a non-UploadSizeLimitError rejection (for example, a generic Error) and
assert the intended orphaned-file behavior in this branch. Verify the side
effects differ from the oversized-upload case by checking whether deleter.run is
called or the task remains queued, and keep the task/state assertions aligned
with the actual behavior of drainUploadQueue.
In
`@src/context/storage/TemporalFiles/application/upload/TemporalFileUploadQueue/preserve-rejected-upload.test.ts`:
- Around line 29-50: The preserveRejectedUpload test suite is missing coverage
for the preservation-error path, so add a new test around preserveRejectedUpload
and preserveRejectedFileSizeTooBig that mocks a returned error. In this case,
assert that deleter.run is not called and that the function returns early
without attempting deletion; use the existing task shape and the
preserveRejectedFileSizeTooBig mock to locate the behavior.
In `@src/context/virtual-drive/files/infrastructure/InMemoryFileRepository.ts`:
- Around line 30-36: The matchesPartial helper in InMemoryFileRepository
currently uses loose equality for both the contentsId normalization branch and
the generic attribute comparison. Update the comparisons in matchesPartial to
use strict equality instead, including the normalized contentsId check, so the
behavior is explicit and avoids type coercion; keep the rest of the key
iteration logic unchanged.
🪄 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: f294bc9c-486e-4dcf-a2c3-2d73baf7051e
📒 Files selected for processing (40)
src/apps/drive/dependency-injection/offline-drive/registerTemporalFilesServices.tssrc/apps/drive/dependency-injection/virtual-drive/registerVirtualDriveSharedServices.tssrc/backend/features/user/file-size-limit/add-max-file-size-rejection.tssrc/backend/features/user/file-size-limit/upload-size-limit-blocked-paths.tssrc/backend/features/virtual-drive/controllers/operations/release.controller.test.tssrc/backend/features/virtual-drive/controllers/operations/release.controller.tssrc/backend/features/virtual-drive/services/lazy/virtual-drive-hydrator/children.test.tssrc/backend/features/virtual-drive/services/lazy/virtual-drive-hydrator/children.tssrc/backend/features/virtual-drive/services/lazy/virtual-drive-hydrator/create-lazy-virtual-drive-hydrator-service.tssrc/backend/features/virtual-drive/services/lazy/virtual-drive-hydrator/ensure-folder-materialized.test.tssrc/backend/features/virtual-drive/services/lazy/virtual-drive-hydrator/ensure-folder-materialized.tssrc/backend/features/virtual-drive/services/lazy/virtual-drive-hydrator/fetch-and-store-children.test.tssrc/backend/features/virtual-drive/services/lazy/virtual-drive-hydrator/fetch-and-store-children.tssrc/backend/features/virtual-drive/services/lazy/virtual-drive-hydrator/file-name.tssrc/backend/features/virtual-drive/services/lazy/virtual-drive-hydrator/path.test.tssrc/backend/features/virtual-drive/services/lazy/virtual-drive-hydrator/path.tssrc/backend/features/virtual-drive/services/lazy/virtual-drive-hydrator/refresh-children-if-needed.tssrc/backend/features/virtual-drive/services/lazy/virtual-drive-hydrator/resolve-root-folder.tssrc/backend/features/virtual-drive/services/lazy/virtual-drive-hydrator/types.tssrc/backend/features/virtual-drive/services/operations/get-attributes.service.test.tssrc/backend/features/virtual-drive/services/operations/get-attributes.service.tssrc/backend/features/virtual-drive/services/operations/open.service.test.tssrc/backend/features/virtual-drive/services/operations/open.service.tssrc/backend/features/virtual-drive/services/operations/opendir.service.test.tssrc/backend/features/virtual-drive/services/operations/opendir.service.tssrc/backend/features/virtual-drive/services/operations/release.service.test.tssrc/backend/features/virtual-drive/services/operations/release.service.tssrc/context/storage/TemporalFiles/application/upload/TemporalFileUploadQueue/create-temporal-file-upload-queue-service.tssrc/context/storage/TemporalFiles/application/upload/TemporalFileUploadQueue/drain-upload-queue.test.tssrc/context/storage/TemporalFiles/application/upload/TemporalFileUploadQueue/drain-upload-queue.tssrc/context/storage/TemporalFiles/application/upload/TemporalFileUploadQueue/enqueue-upload.test.tssrc/context/storage/TemporalFiles/application/upload/TemporalFileUploadQueue/enqueue-upload.tssrc/context/storage/TemporalFiles/application/upload/TemporalFileUploadQueue/preserve-rejected-upload.test.tssrc/context/storage/TemporalFiles/application/upload/TemporalFileUploadQueue/preserve-rejected-upload.tssrc/context/storage/TemporalFiles/application/upload/TemporalFileUploadQueue/state.tssrc/context/storage/TemporalFiles/application/upload/TemporalFileUploadQueue/types.tssrc/context/storage/TemporalFiles/application/upload/TemporalFileUploadQueue/upload-queued-task.test.tssrc/context/storage/TemporalFiles/application/upload/TemporalFileUploadQueue/upload-queued-task.tssrc/context/virtual-drive/files/infrastructure/InMemoryFileRepository.test.tssrc/context/virtual-drive/files/infrastructure/InMemoryFileRepository.ts
✅ Files skipped from review due to trivial changes (3)
- src/context/storage/TemporalFiles/application/upload/TemporalFileUploadQueue/upload-queued-task.test.ts
- src/backend/features/virtual-drive/services/lazy/virtual-drive-hydrator/path.ts
- src/context/storage/TemporalFiles/application/upload/TemporalFileUploadQueue/types.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- src/backend/features/virtual-drive/services/operations/open.service.ts
- src/apps/drive/dependency-injection/virtual-drive/registerVirtualDriveSharedServices.ts
- src/backend/features/virtual-drive/services/operations/get-attributes.service.ts
- src/apps/drive/dependency-injection/offline-drive/registerTemporalFilesServices.ts
- src/backend/features/virtual-drive/services/operations/open.service.test.ts
- src/backend/features/virtual-drive/services/operations/opendir.service.ts
- src/backend/features/virtual-drive/services/operations/opendir.service.test.ts
- src/backend/features/virtual-drive/services/operations/get-attributes.service.test.ts
… safe-readdir tests
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
egalvis27
force-pushed
the
refactor/sync-engine-process
branch
from
July 30, 2026 21:51
566ecb7 to
30d3244
Compare
…ren functionality
…olderByBatch functions
…e service and tests
… for upload handling
|
egalvis27
commented
Jul 31, 2026
| 'PRAGMA wal_autocheckpoint=1000;', | ||
| 'PRAGMA mmap_size=268435456;', | ||
| `CREATE TABLE IF NOT EXISTS drive_directory_state ( | ||
| folder_id INTEGER NOT NULL, |
Author
There was a problem hiding this comment.
This block was added so that SQLite always starts up with a consistent configuration (WAL, pragmas, and key indexes) and with the directory status table ready. This prevents discrepancies between the sync status and the visible content and improves performance when listing folders.
egalvis27
commented
Jul 31, 2026
| { | ||
| fetchFilesLimitPerRequest: 1000, | ||
| fetchFoldersLimitPerRequest: 1000, | ||
| fetchFilesLimitPerRequest: 250, |
Author
There was a problem hiding this comment.
The number of files and folders retrieved was reduced because the request was becoming too heavy and there was no real benefit; with smaller blocks, they are retrieved faster, and in the same amount of time it used to take for a request of 1,000, many more are retrieved.
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
1 participant
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.
What is Changed / Added
opendir,getattr,open,release) to work with lazy hydration and the new sync behavior.Why
Summary by CodeRabbit
New Features
Bug Fixes
Infrastructure