feat: enhance file download and caching mechanisms - #404
Conversation
| const ALLOWED_BLOCK_SIZE_MB = new Set([1, 2, 4, 8]); | ||
|
|
||
| function getConfiguredBlockSizeInMb() { | ||
| const configuredValue = process.env.INTERNXT_DRIVE_DOWNLOAD_BLOCK_SIZE_MB; |
There was a problem hiding this comment.
Why this needs to be a enviroment variable
There was a problem hiding this comment.
My idea is that these values should be easy to change. Right now, the optimal settings are 4 MB per download and 3 prefetch blocks. These values can be increased if changes are made to the backend to reduce the latency of download requests, which would greatly improve performance.
Looking back, we can adjust these values to achieve better performance if the conditions are right; that’s why I set them as environment variables so they can be easily modified.
| blockLength, | ||
| }: Props): Promise<Result<void, Error>> { | ||
| if (isAborted(state)) return { data: undefined }; | ||
| if (blockLength <= 0 || blockStart >= virtualFile.size) return { data: undefined }; |
There was a problem hiding this comment.
Is this something that can happen? meaning: Fuse can ask for a block that is negative?
There was a problem hiding this comment.
It's not just to validate negative ranges; it's also to prevent readings in the 0 range—which fuse does sometimes—and to ensure that invalid ranges aren't attempted, which is rare but does happen.
| return { blockStart: Math.max(0, Math.min(range.position, fileSize)), blockLength: 0 }; | ||
| } | ||
|
|
||
| const clampedStart = Math.max(0, range.position); |
There was a problem hiding this comment.
Why? i assume that range will always be 0 or more
There was a problem hiding this comment.
We might assume that's the case, but there are very rare instances where it might not be; this change is a simple safeguard that definitively prevents that from happening.
| import { type HandleReadDeps, type ReadRange } from './types'; | ||
| import { isThumbnailProcess } from './thumbnail-processes'; | ||
|
|
||
| const PREFETCH_DEFAULT_BLOCKS_AHEAD = 5; |
| function getThumbnailPrefetchBlocksAhead() { | ||
| return getPrefetchBlocksAhead({ | ||
| configuredValue: process.env.INTERNXT_DRIVE_THUMBNAIL_PREFETCH_BLOCKS_AHEAD, | ||
| }); | ||
| } | ||
|
|
||
| function getReadPrefetchBlocksAhead() { | ||
| return getPrefetchBlocksAhead({ | ||
| configuredValue: process.env.INTERNXT_DRIVE_READ_PREFETCH_BLOCKS_AHEAD, | ||
| }); | ||
| } |
There was a problem hiding this comment.
As I mentioned earlier, the idea is to be able to easily change the values.
| return Math.max(0, Math.min(parsed, PREFETCH_MAX_BLOCKS_AHEAD)); | ||
| } | ||
|
|
||
| function getThumbnailPrefetchBlocksAhead() { |
There was a problem hiding this comment.
Why, for a thumbnail, we need to do a prefetch? would not this slow the initial load of the file explorer?
There was a problem hiding this comment.
I've been running tests focused on thumbnails, and you're right—the improvement is very small, and it slows down the network for other processes.
| const downloads = missingBlocks.map((block) => { | ||
| const start = block * BLOCK_SIZE; | ||
| const end = Math.min(start + BLOCK_SIZE, virtualFile.size); | ||
| const boundedBlockLength = end - start; |
There was a problem hiding this comment.
Same, i feel like there has to be a way to prevent this whole "negative block request" instead of just adding everywhere the check, this just adds unnecessary complexity dont you think
There was a problem hiding this comment.
You're right—the extra validation was too conservative; the first validation at the beginning of the function is enough.
| if ( | ||
| !shouldEmitProgress({ | ||
| now: Date.now(), | ||
| bytesDownloaded, | ||
| fileSize, | ||
| state: progressReporterState, | ||
| }) | ||
| ) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
This is just to avoid spamming progress updates.
Downloads can report progress very frequently, and without this check we would send too many UI updates for very small changes.
So this acts like a small throttle: it only emits from time to time, but still allows the final update through when the download reaches 100%.
| const PROGRESS_UPDATE_INTERVAL_MS = 250; | ||
|
|
||
| type ProgressReporterState = { | ||
| lastUpdateAt: number; | ||
| }; | ||
|
|
||
| function shouldEmitProgress({ | ||
| now, | ||
| bytesDownloaded, | ||
| fileSize, | ||
| state, | ||
| }: { | ||
| now: number; | ||
| bytesDownloaded: number; | ||
| fileSize: number; | ||
| state: ProgressReporterState; | ||
| }) { | ||
| const reachedEnd = bytesDownloaded >= fileSize; | ||
| const elapsedSinceLastUpdate = now - state.lastUpdateAt; | ||
|
|
||
| if (!reachedEnd && elapsedSinceLastUpdate < PROGRESS_UPDATE_INTERVAL_MS) { | ||
| return false; | ||
| } | ||
|
|
||
| state.lastUpdateAt = now; | ||
| return true; | ||
| } | ||
|
|
There was a problem hiding this comment.
IF this is necessary this should be in its own file, where we should test it separatedly
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change adds download-cache boundary validation and block prefetching, throttles download progress updates, scopes rate-limit delays by request key, and validates ranged downloads before network access. ChangesDownload cache reads
Download progress throttling
Rate-limit request scoping
Ranged download handling
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
| const ALLOWED_BLOCK_SIZE_MB = new Set([1, 2, 4, 8]); | ||
|
|
||
| function getConfiguredBlockSizeInMb() { | ||
| const configuredValue = process.env.INTERNXT_DRIVE_DOWNLOAD_BLOCK_SIZE_MB; |
| // Thumbnail reads should not spam progress updates in UI. | ||
| onDownloadProgress: () => undefined, | ||
| // Thumbnail reads should not register files as offline available. | ||
| saveToRepository: async () => undefined, |
There was a problem hiding this comment.
Why not? imagine a file gets fully hydrated because a thumbnail call, why not save locally? same goes for progress
There was a problem hiding this comment.
We could do that, but the goal of this path is to keep the thumbnails' behavior as lightweight as possible. If a thumbnail hydrates the file, it could indeed be saved locally, but that would also trigger additional side effects, such as progress tracking and offline availability logging. To avoid clutter and keep the flow as isolated as possible, for now I prefer that this scenario not alter the cache state or the UI.
There was a problem hiding this comment.
but then a file would be downloaded 2 times no?
| virtualFile, | ||
| filePath, | ||
| range, | ||
| prefetchBlocksAhead: 0, |
There was a problem hiding this comment.
this is not necessary because you are already adding default value to 0
| @@ -0,0 +1,24 @@ | |||
| const PROGRESS_UPDATE_INTERVAL_MS = 250; | |||
There was a problem hiding this comment.
This should go into the constants.ts file
There was a problem hiding this comment.
Since we're using this function as the central point, and that value isn't needed anywhere else but here, I don't think it's worth creating a new file just to store this value.
| import { File } from '../../../../../context/virtual-drive/files/domain/File'; | ||
| import { StorageFile } from '../../domain/StorageFile'; | ||
|
|
||
| const PROGRESS_UPDATE_INTERVAL_MS = 250; |
There was a problem hiding this comment.
you have a duplicated constant that should go into the constants.ts
There was a problem hiding this comment.
The dependency on this constant has been removed; now the same function from the previous comment is used.
| const method = config.method?.toUpperCase() ?? 'GET'; | ||
| const url = config.url ?? ''; | ||
|
|
||
| return `${method}:${url}`; |
There was a problem hiding this comment.
Should we realy return a key if no method and url? also, what happens if there are multiple pending requests?
There was a problem hiding this comment.
Yes, that makes sense. If there's no method or URL, the fallback is still valid, but the key isn't as precise. The important thing is that the block only applies to requests with the same key; if several arrive at once, they are chained to the same pending wait—they aren't all serialized globally.
| function getRequestKey({ method, url }: { method?: string; url?: string }) { | ||
| return `${method?.toUpperCase() ?? 'GET'}:${url ?? ''}`; | ||
| } |
There was a problem hiding this comment.
This method is dupliacted, why not unify them?
| import { AxiosResponse } from 'axios'; | ||
| import { RateLimitState } from './rate-limiter.types'; | ||
|
|
||
| const MAX_REASONABLE_RESET_SECONDS = 120; |
|
|
||
| # Download and prefetch settings | ||
| INTERNXT_DRIVE_DOWNLOAD_BLOCK_SIZE_MB= | ||
| INTERNXT_DRIVE_READ_PREFETCH_BLOCKS_AHEAD= |
There was a problem hiding this comment.
I would just add them as a constant, way simpler and it does not do any harm to have it "hardcoded" just as we do with any other value
![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/fuse/on-read/read-or-hydrate.test.ts (1)
1-30: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winKeep mock calls isolated between tests.
beforeEachresets values, but the module-level mocks keep call history across tests. Reset the relevant mock at the start of each test before asserting exact call counts, or add a Vitest mock-clearing global setting.🤖 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/fuse/on-read/read-or-hydrate.test.ts` around lines 1 - 30, Isolate module-level mock call history between tests in the readOrHydrate test suite. Clear the relevant mocks created by partialSpyOn—especially readChunkFromDiskMock, fileExistsOnDiskMock, allocateFileMock, and downloadAndCacheBlockMock—before tests assert exact call counts, or enable the project’s Vitest mock-clearing configuration.
🧹 Nitpick comments (3)
src/backend/features/fuse/on-read/read-or-hydrate.ts (1)
147-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract duplicated block-range arithmetic.
schedulePrefetchBlocksAhead(Lines 148-150) andensureRangeDownloaded'smissingBlocks.map(Lines 257-259) both compute a block's byte start, clamped end, and length fromblock * BLOCK_SIZEandvirtualFile.size. Extract a shared helper to keep this arithmetic in one place and reduce the risk of the two call sites drifting apart.♻️ Proposed refactor
+function getBlockByteRange(block: number, fileSize: number): { start: number; end: number; length: number } { + const start = block * BLOCK_SIZE; + const end = Math.min(start + BLOCK_SIZE, fileSize); + return { start, end, length: end - start }; +} + function schedulePrefetchBlocksAhead({ ... for (const block of prefetchedBlocks) { - const start = block * BLOCK_SIZE; - const end = Math.min(start + BLOCK_SIZE, virtualFile.size); - const blockLength = end - start; + const { start, blockLength } = getBlockByteRange(block, virtualFile.size); ...Apply the equivalent change inside
ensureRangeDownloaded'smissingBlocks.map.Also applies to: 256-259
🤖 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/fuse/on-read/read-or-hydrate.ts` around lines 147 - 150, Extract the duplicated block-range calculation into a shared helper near the existing read/hydration utilities, returning the block’s start, clamped end, and length from block, BLOCK_SIZE, and virtualFile.size. Update both schedulePrefetchBlocksAhead and ensureRangeDownloaded’s missingBlocks.map to use this helper while preserving their existing behavior.src/infra/environment/download-file/download-file.test.ts (1)
71-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover negative lengths as well.
The test name claims coverage for non-positive lengths, but it only exercises
length: 0. Add alength: -1case to protect the other branch of the<= 0condition.🤖 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/infra/environment/download-file/download-file.test.ts` around lines 71 - 84, Update the test around downloadFileRange to also cover a negative range length, such as length: -1, while preserving the assertions that it returns an empty buffer and skips both sdkDownloadFileMock and axiosGetMock. Keep the existing zero-length coverage or parameterize both non-positive cases.src/infra/environment/download-file/download-file.ts (1)
108-124: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRelease unused buffer capacity after growth.
Buffer.subarrayshares the original backing storage. When the response exceeds the requested length, the returned view keeps the larger buffer alive. Copy only when spare capacity exists.[details]
Proposed fix
- response.data.on('end', () => resolve(buffer.subarray(0, bytesRead))); + response.data.on('end', () => { + const result = buffer.subarray(0, bytesRead); + resolve(bytesRead === buffer.length ? result : Buffer.from(result)); + });[/details]
🤖 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/infra/environment/download-file/download-file.ts` around lines 108 - 124, Update the response.data end handler to avoid returning a subarray that retains an oversized buffer: when bytesRead is less than buffer.length, copy the valid bytes into an exact-sized Buffer before resolving; otherwise preserve the existing zero-copy result. Keep the growth logic in the data handler unchanged.
🤖 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/fuse/on-read/download-cache/constants.ts`:
- Around line 6-9: Update PREFETCH_BLOCKS_AHEAD in
src/backend/features/fuse/on-read/download-cache/constants.ts:6-9 from 3 to 5.
Do not modify
src/backend/features/fuse/on-read/download-cache/constants.test.ts:21-27 or
src/backend/features/fuse/on-read/handle-read-callback.test.ts:133-153; both are
corrected by the constant change.
In `@src/context/shared/domain/progress-constants.test.ts`:
- Line 1: Resolve the missing progress module referenced by
progress-constants.test.ts: either add ./progress-constants exporting
PROGRESS_UPDATE_INTERVAL_MS and shouldEmitProgress, or move the tests to
should-emit-progress.ts and update them to use its existing helper API. Ensure
npm run test:main can resolve the import and the tests exercise the actual
progress implementation.
In `@src/infra/drive-server/client/interceptors/rate-limiter/constants.ts`:
- Line 1: Run the configured Prettier formatter on the file containing
MAX_REASONABLE_RESET_SECONDS and apply its generated formatting changes, without
manually altering the constant’s behavior.
In
`@src/infra/drive-server/client/interceptors/rate-limiter/create-response-interceptor.ts`:
- Around line 53-55: Update the response interceptor’s delay tracking to store
pending state per request key instead of sharing delayState.requestKey and
delayState.pending across requests. Ensure overlapping 429 handlers for
different keys do not overwrite each other, and clear only the completed key
after waitForDelay finishes. Add an integration test covering overlapping 429
responses for two distinct request keys and verifying each key’s required delay
is preserved.
In
`@src/infra/drive-server/client/interceptors/rate-limiter/update-state-from-headers.ts`:
- Around line 5-15: Update parseNumberHeader to reject partially numeric header
strings such as “10seconds” rather than returning the truncated integer;
validate that the entire trimmed value is a valid integer before parsing. Add
regression coverage confirming malformed partial values return null and do not
update state.reset.
In `@src/infra/environment/download-file/download-file.ts`:
- Around line 18-19: Update downloadFileRange and fetchEncryptedRange to
validate range.position and range.length before constructing requests or
allocating buffers: require finite, non-negative safe integers, and reject any
range whose computed end offset is not a safe integer. Preserve the existing
empty-buffer behavior only for valid zero-length ranges, and ensure invalid
values are rejected before sending the HTTP Range header.
---
Outside diff comments:
In `@src/backend/features/fuse/on-read/read-or-hydrate.test.ts`:
- Around line 1-30: Isolate module-level mock call history between tests in the
readOrHydrate test suite. Clear the relevant mocks created by
partialSpyOn—especially readChunkFromDiskMock, fileExistsOnDiskMock,
allocateFileMock, and downloadAndCacheBlockMock—before tests assert exact call
counts, or enable the project’s Vitest mock-clearing configuration.
---
Nitpick comments:
In `@src/backend/features/fuse/on-read/read-or-hydrate.ts`:
- Around line 147-150: Extract the duplicated block-range calculation into a
shared helper near the existing read/hydration utilities, returning the block’s
start, clamped end, and length from block, BLOCK_SIZE, and virtualFile.size.
Update both schedulePrefetchBlocksAhead and ensureRangeDownloaded’s
missingBlocks.map to use this helper while preserving their existing behavior.
In `@src/infra/environment/download-file/download-file.test.ts`:
- Around line 71-84: Update the test around downloadFileRange to also cover a
negative range length, such as length: -1, while preserving the assertions that
it returns an empty buffer and skips both sdkDownloadFileMock and axiosGetMock.
Keep the existing zero-length coverage or parameterize both non-positive cases.
In `@src/infra/environment/download-file/download-file.ts`:
- Around line 108-124: Update the response.data end handler to avoid returning a
subarray that retains an oversized buffer: when bytesRead is less than
buffer.length, copy the valid bytes into an exact-sized Buffer before resolving;
otherwise preserve the existing zero-copy result. Keep the growth logic in the
data handler 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 Plus
Run ID: 92278d37-0702-4994-b676-d7df8abdbd77
📒 Files selected for processing (31)
.env.examplesrc/backend/features/fuse/on-read/download-cache/constants.test.tssrc/backend/features/fuse/on-read/download-cache/constants.tssrc/backend/features/fuse/on-read/download-cache/download-and-save-block.tssrc/backend/features/fuse/on-read/download-cache/expand-to-block-boundaries.test.tssrc/backend/features/fuse/on-read/download-cache/expand-to-block-boundaries.tssrc/backend/features/fuse/on-read/download-cache/hydration-state.test.tssrc/backend/features/fuse/on-read/download-cache/hydration-state.tssrc/backend/features/fuse/on-read/handle-read-callback.test.tssrc/backend/features/fuse/on-read/handle-read-callback.tssrc/backend/features/fuse/on-read/read-or-hydrate.test.tssrc/backend/features/fuse/on-read/read-or-hydrate.tssrc/backend/features/virtual-drive/services/operations/read.service.tssrc/backend/features/virtual-drive/services/operations/should-emit-progress.test.tssrc/backend/features/virtual-drive/services/operations/should-emit-progress.tssrc/context/shared/domain/progress-constants.test.tssrc/context/storage/StorageFiles/application/download/download-with-progress-tracking.test.tssrc/context/storage/StorageFiles/application/download/download-with-progress-tracking.tssrc/infra/drive-server/client/interceptors/rate-limiter/attach-rate-limiter-interceptors.test.tssrc/infra/drive-server/client/interceptors/rate-limiter/attach-rate-limiter-interceptors.tssrc/infra/drive-server/client/interceptors/rate-limiter/constants.tssrc/infra/drive-server/client/interceptors/rate-limiter/create-request-interceptor.test.tssrc/infra/drive-server/client/interceptors/rate-limiter/create-request-interceptor.tssrc/infra/drive-server/client/interceptors/rate-limiter/create-response-interceptor.test.tssrc/infra/drive-server/client/interceptors/rate-limiter/create-response-interceptor.tssrc/infra/drive-server/client/interceptors/rate-limiter/get-request-key.tssrc/infra/drive-server/client/interceptors/rate-limiter/rate-limiter.types.tssrc/infra/drive-server/client/interceptors/rate-limiter/update-state-from-headers.test.tssrc/infra/drive-server/client/interceptors/rate-limiter/update-state-from-headers.tssrc/infra/environment/download-file/download-file.test.tssrc/infra/environment/download-file/download-file.ts
| delayState.requestKey = getRequestKey(config); | ||
| await waitForDelay(delayState, waitMs); | ||
| delayState.requestKey = null; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Track pending delays by request key.
If two 429 responses overlap, the second handler replaces the first handler's requestKey while delayState.pending still belongs to the first request. A new request with the first key then bypasses its required delay.
Replace the single pending and requestKey fields with per-key delay state. Clear only the completed key. Add an integration test with overlapping 429 responses for two different keys.
🤖 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/infra/drive-server/client/interceptors/rate-limiter/create-response-interceptor.ts`
around lines 53 - 55, Update the response interceptor’s delay tracking to store
pending state per request key instead of sharing delayState.requestKey and
delayState.pending across requests. Ensure overlapping 429 handlers for
different keys do not overwrite each other, and clear only the completed key
after waitForDelay finishes. Add an integration test covering overlapping 429
responses for two distinct request keys and verifying each key’s required delay
is preserved.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/infra/drive-server/client/interceptors/rate-limiter/attach-rate-limiter-interceptors.test.ts`:
- Line 63: Update the test around the delayState setup to capture both
interceptor factory call arguments, then assert that the request and response
factories receive the identical DelayState object by reference rather than
merely equivalent object contents.
In
`@src/infra/drive-server/client/interceptors/rate-limiter/wait-for-delay.test.ts`:
- Line 16: Strengthen the assertions in the rate-limiter test around
pendingByKey so the stored promise is explicitly verified with toBeDefined()
before any identity comparison. In the same-key caller scenario, also assert the
fake-timer count shows only one delay was scheduled, covering the shared-delay
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8c6b38d4-91e3-4840-b74b-c1716741b29d
📒 Files selected for processing (16)
src/backend/features/fuse/on-read/download-cache/constants.test.tssrc/backend/features/fuse/on-read/handle-read-callback.test.tssrc/infra/drive-server/client/interceptors/rate-limiter/attach-rate-limiter-interceptors.test.tssrc/infra/drive-server/client/interceptors/rate-limiter/attach-rate-limiter-interceptors.tssrc/infra/drive-server/client/interceptors/rate-limiter/constants.tssrc/infra/drive-server/client/interceptors/rate-limiter/create-request-interceptor.test.tssrc/infra/drive-server/client/interceptors/rate-limiter/create-request-interceptor.tssrc/infra/drive-server/client/interceptors/rate-limiter/create-response-interceptor.test.tssrc/infra/drive-server/client/interceptors/rate-limiter/create-response-interceptor.tssrc/infra/drive-server/client/interceptors/rate-limiter/rate-limiter.types.tssrc/infra/drive-server/client/interceptors/rate-limiter/update-state-from-headers.test.tssrc/infra/drive-server/client/interceptors/rate-limiter/update-state-from-headers.tssrc/infra/drive-server/client/interceptors/rate-limiter/wait-for-delay.test.tssrc/infra/drive-server/client/interceptors/rate-limiter/wait-for-delay.tssrc/infra/environment/download-file/download-file.test.tssrc/infra/environment/download-file/download-file.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- src/infra/drive-server/client/interceptors/rate-limiter/constants.ts
- src/infra/drive-server/client/interceptors/rate-limiter/update-state-from-headers.test.ts
- src/infra/environment/download-file/download-file.test.ts
- src/infra/drive-server/client/interceptors/rate-limiter/create-request-interceptor.ts
- src/infra/drive-server/client/interceptors/rate-limiter/update-state-from-headers.ts
- src/infra/drive-server/client/interceptors/rate-limiter/create-request-interceptor.test.ts
- src/infra/drive-server/client/interceptors/rate-limiter/create-response-interceptor.ts
- src/backend/features/fuse/on-read/handle-read-callback.test.ts
- src/infra/environment/download-file/download-file.ts
|
|
||
| it('should share the same delay state between request and response interceptors', () => { | ||
| const delayState = { pending: null }; | ||
| const delayState = { pendingByKey: {} }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Verify that both interceptors receive the same DelayState object.
The current structural assertions pass when each factory receives a separate { pendingByKey: {} } object. Capture the factory call arguments and assert reference equality between the request and response delay-state arguments.
🤖 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/infra/drive-server/client/interceptors/rate-limiter/attach-rate-limiter-interceptors.test.ts`
at line 63, Update the test around the delayState setup to capture both
interceptor factory call arguments, then assert that the request and response
factories receive the identical DelayState object by reference rather than
merely equivalent object contents.
| const promise = waitForDelay(state, 100); | ||
| expect(state.pending).not.toBeNull(); | ||
| const promise = waitForDelay(state, 'GET:/test', 100); | ||
| expect(state.pendingByKey['GET:/test']).not.toBeNull(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that pendingByKey contains a promise before comparing it.
undefined passes not.toBeNull() on Line 16. It also makes the identity assertion on Line 33 pass when no entry exists. Assert toBeDefined() for the stored promise. Also verify that same-key callers create one delay, for example by checking the fake-timer count.
Also applies to: 28-33
🤖 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/infra/drive-server/client/interceptors/rate-limiter/wait-for-delay.test.ts`
at line 16, Strengthen the assertions in the rate-limiter test around
pendingByKey so the stored promise is explicitly verified with toBeDefined()
before any identity comparison. In the same-key caller scenario, also assert the
fake-timer count shows only one delay was scheduled, covering the shared-delay
behavior.
…ests in rate limiter
|
2 participants
What is Changed / Added
Why
Summary by CodeRabbit
New Features
Bug Fixes