1st release#18
Open
venu123143 wants to merge 24 commits into
Open
Conversation
Refresh tokens implementation
refresh token changes
hetzner new s3 based storage system changes
notifications modified to the new cursor technique to improve performance
Refresh tokens
backend changes for the dashboard
get api changes
bakend get all files api changes
Refresh tokens
docker compose changes
ci-cd modified
Refresh tokens
📝 WalkthroughWalkthroughThis PR implements a comprehensive analytics system for tracking user file operations, introduces a complete favorites feature with CRUD operations, adds S3-to-S3 storage migration tooling, implements distributed locking for cron coordination, and updates deployment infrastructure for multi-architecture builds with enhanced authorization controls. ChangesAnalytics System and Favorites Feature
Storage and Infrastructure
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 16
🧹 Nitpick comments (2)
.github/workflows/ci-cd.yml (1)
35-45: ⚡ Quick winDisabling provenance attestation reduces supply chain security transparency.
Setting
provenance: falsedisables SLSA provenance metadata that documents how the image was built. While sometimes needed for compatibility with older registries or clients, this reduces supply chain security by removing verifiable build provenance.Consider enabling provenance if your registry and consumers support it, or document why it must be disabled.
🤖 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 @.github/workflows/ci-cd.yml around lines 35 - 45, The workflow currently sets provenance: false in the docker/build-push-action@v6 step which disables SLSA provenance; change the provenance setting to true (or make it conditional via an env flag) so provenance attestation is emitted when the target registry supports it, or if you must keep it false add a clear comment and README note explaining why; update the docker/build-push-action@v6 step (the block containing context, push, platforms, provenance, tags, cache-from, cache-to) to either set provenance: true or read an env variable like PROVENANCE_ENABLED to toggle it and document the rationale for any deliberate disablement.src/repository/analytics.repository.ts (1)
144-145: 🏗️ Heavy liftFull file-table scans on each event are a scalability hotspot.
Upload/delete/folder events recompute storage by loading all non-deleted files for the user. This will grow linearly with user file count and can become expensive on high event throughput.
Also applies to: 171-172, 197-197
🤖 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/repository/analytics.repository.ts` around lines 144 - 145, The code is performing full user file-table scans by calling calculateUserStorage(userId) on every upload/delete/folder event which will not scale; instead implement incremental storage bookkeeping: add a persistent user_storage (or storage_bytes) column and update it by applying the delta (e.g., +file.size on upload, -file.size on delete, adjust on move/overwrite) inside the event handlers rather than recomputing via calculateUserStorage each time; keep calculateUserStorage only as a fallback reconciliation method and ensure event handlers (the upload/delete/folder handlers that currently call calculateUserStorage) compute and persist deltas atomically (transactionally) so concurrent events are safe.
🤖 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 @.gitignore:
- Around line 17-18: Remove the overly-broad ignore patterns '*.md' and '*.txt'
from .gitignore and replace them with specific patterns or explicit exclusions:
delete the two lines, add narrow ignores for only generated or local files
(e.g., build/*.md, reports/*.txt) or add explicit allow rules for important docs
(e.g., !README.md, !CONTRIBUTING.md, !CHANGELOG.md) so documentation remains
tracked; if those patterns were intended only for personal temp files, move them
to your local .git/info/exclude instead.
In `@src/controllers/analytics.controller.ts`:
- Around line 82-86: The storageUsedPercentage calculation divides by
user.storage_quota without checks; update the object creation (the
storageUsedPercentage field that uses overview.storage.totalSize /
user.storage_quota) to first verify that user.storage_quota is a positive number
(e.g., Number(user.storage_quota) > 0) and only then compute
((overview.storage.totalSize / user.storage_quota) * 100).toFixed(2); if the
quota is zero/invalid, return a safe fallback (e.g., "0.00" or null) to avoid
NaN/Infinity.
- Around line 42-55: After parsing startDateStr and endDateStr into startDate
and endDate, add a validation that startDate.getTime() <= endDate.getTime(); if
not, return the same res.FailureResponse pattern with a 400 status and a clear
message (e.g., "Invalid date range: startDate must be before or equal to
endDate") before calling analyticsService.getAnalyticsByDateRange(user.id,
startDate, endDate).
In `@src/controllers/file.controller.ts`:
- Around line 36-43: The calls to addToAnalyticsQueue are asynchronous and
currently fire-and-forget, which can produce unhandled promise rejections if
Redis/enqueue fails; modify each call to addToAnalyticsQueue (including the ones
used for folder creation, updates, and deletions) to handle failures by either
awaiting the promise inside an async try/catch or by appending a .catch(...)
that logs the error (using the existing logger/processLogger) so rejections are
swallowed and surfaced; ensure every invocation of addToAnalyticsQueue in
file.controller.ts is updated to handle errors consistently.
In `@src/controllers/upload.controller.ts`:
- Around line 38-52: The fire-and-forget call to addToAnalyticsQueue inside
files.forEach can produce unhandled rejections; protect it by attaching a local
catch handler to the promise returned by addToAnalyticsQueue (or wrap the call
in a try/catch if you convert the iterator to an async loop) so transient queue
failures are swallowed or logged. Specifically, in the upload controller where
files.forEach is used and addToAnalyticsQueue({ userId: user.id, eventType:
AnalyticsEventType.FILE_UPLOADED, ... }) is invoked, ensure you call the
function and then chain .catch(err => /* log or noop */) so any rejection is
handled locally and does not create an unhandled rejection.
In `@src/core/analytics-queue.ts`:
- Around line 50-53: The Queue/Worker/QueueEvents instances (analyticsQueue and
associated Worker/QueueEvents created around lines 60-100) are left open on
import; add and export a shutdownAnalyticsQueue function that gracefully closes/
disconnects analyticsQueue, its Worker instance, and QueueEvents by calling
their close()/disconnect() or waitUntilReady()/close equivalents and awaiting
them, ensure it handles in-flight jobs (drain/stop worker then close) and is
idempotent; then call shutdownAnalyticsQueue from the process shutdown hook
(SIGINT/SIGTERM) in your app entry to ensure Redis handles are released and jobs
drained on redeploy.
In `@src/index.ts`:
- Around line 41-43: The protocol log is derived only from
config.HTTP2.SSL.ENABLED which can be true even if TLS cert/key are missing;
change the logic that sets protocol (the variable named protocol near
displayHost/port) to check both the SSL enabled flag and that the configured
certificate and key are actually present (e.g., config.HTTP2.SSL.CERT and
config.HTTP2.SSL.KEY or whatever TLS cert/key fields your config uses) and
ideally readable, then use "https" only when both enabled and cert+key are
available, otherwise fall back to "http".
In `@src/models/Notification.model.ts`:
- Around line 4-7: The DB enum backing NotificationType needs a migration to add
the FILE_SHARE_REVOKED value so runtime writes from code using
NotificationType.FILE_SHARE_REVOKED (referenced in
src/models/Notification.model.ts and used in src/controllers/file.controller.ts)
do not fail; create a new Sequelize migration that alters the PostgreSQL enum
(or creates a new enum and replaces the column) to include 'file_share_revoked'
(using ALTER TYPE ... ADD VALUE 'file_share_revoked' or the safe create-new-enum
+ ALTER COLUMN ... USING ... pattern for older PG versions), run the migration
to update the DB, and then ensure the codebase uses the same enum literal string
as NotificationType.FILE_SHARE_REVOKED.
In `@src/repository/analytics.repository.ts`:
- Around line 146-148: The updates object currently does a read-modify-write
(analytics.uploads_today + 1) which will undercount under concurrency; change
the logic to perform an atomic increment in the database instead of computing
uploads_today in-memory — e.g., use your ORM/connector's atomic increment/update
operation for the uploads_today column (reference variables/fields: analytics,
uploads_today, updates, and storage.totalFiles) and apply the same change for
the other occurrence at the block referencing lines 163-164 so concurrent
workers increment the counter atomically rather than overwriting each other.
In `@src/scripts/migrate-storage.ts`:
- Around line 110-112: migrateFile is intended to stream but currently downloads
whole objects into buffers and re-uploads them, causing OOM for large files;
change migrateFile to open a readable stream from the source (use the SDK method
that returns a stream/streamable response) and pipe or stream that data directly
into the destination upload (use the SDK uploadStream/multipart upload API or a
Writeable stream) instead of buffering the full content, and apply the same
streaming approach to the other places mentioned (the code around the blocks
referenced at 162-167 and 176-182) so all downloads -> uploads are performed via
streaming/piping rather than Buffer allocations.
- Line 113: The code currently flattens object keys with const fileName =
key.split('/').pop() || key, causing different source paths to collide; instead
preserve the relative path (e.g., compute relativeKey by stripping the source
prefix or base folder from key) and use that relativeKey when constructing the
destination object name so directory structure is retained; replace any usage of
fileName in the migration logic (including the blocks around where fileName is
set and the similar logic at the other affected spots) with this relativeKey (or
properly normalized path) so keys remain unique and overwrite collisions are
avoided.
- Around line 287-292: The current validation in main() only checks secondary S3
env vars and lets missing primary S3 credentials surface later; update the
startup validation to also fail fast for primary S3 by checking
PRIMARY_S3_TOKEN_ID, PRIMARY_S3_SECRET_KEY, PRIMARY_S3_ENDPOINT and
PRIMARY_S3_BUCKET_NAME alongside the secondary vars (or factor into a helper
like validateS3EnvVars used for both primary and secondary) and throw a clear
Error when any required primary variable is missing so the script exits
immediately instead of running partial transfers.
- Line 39: The default parameter for migrateFolder erroneously uses
FolderNameEnum.IMAGES instead of mirroring the provided sourceFolder; change the
signature so targetFolder is optional (remove the FolderNameEnum.IMAGES default)
and inside migrateFolder set targetFolder = targetFolder ?? sourceFolder (or
assign to a local variable like const destination = targetFolder ??
sourceFolder) so when callers omit targetFolder it defaults to the sourceFolder;
update references in the function to use that resolved destination and keep the
FolderNameEnum type for targetFolder.
In `@src/services/analytics.service.ts`:
- Around line 24-31: The check in the file upload branch uses a truthy test on
metadata.fileSize which drops valid zero-byte uploads; change the condition in
the case 'file_uploaded' to explicitly test for presence (e.g. metadata.fileSize
!== undefined && metadata.fileType != null) before calling
analyticsRepository.recordFileUpload(userId, metadata.fileSize,
metadata.fileType) so fileSize === 0 is accepted but missing values are still
rejected.
In `@src/services/lock.service.ts`:
- Around line 20-22: Replace the current simple set/delete lock logic with
ownership tokens and an atomic compare-and-delete: when acquiring a lock (where
client.set(fullKey, Date.now().toString(), 'EX', ttl, 'NX') is used) write a
strong random token (e.g., UUID) as the key value and return that token to the
caller instead of Date.now(); when releasing (the places that currently call
client.del(fullKey) in the unlock/finally blocks referenced around lines 37-43
and 79-80) use an atomic Lua script that checks the key's value equals the token
and only then deletes it (EVAL with: if redis.call("GET", KEYS[1]) == ARGV[1]
then return redis.call("DEL", KEYS[1]) else return 0 end); update the lock API
to accept/propagate the token so unlock can supply it and ensure all unlock
paths use the compare-and-delete script rather than blind DEL.
In `@src/utils/cors-options.ts`:
- Around line 11-13: The CORS allowlist currently hardcodes a private LAN origin
("http://192.168.0.8:5173") which should not be used in production; update the
code that defines the allowedOrigins/ corsOptions array to remove hardcoded
private IPs and instead load dev-only origins from an environment variable
(e.g., process.env.DEV_CORS_ORIGINS split by comma) or only add local/dev
origins when NODE_ENV !== 'production'; ensure credentialed CORS (with
credentials: true) only uses trusted production origins from a secure env var
like PROCESS_CORS_ORIGINS and guard any dev origins behind a runtime check so
private LAN addresses are never included in production allowlists.
---
Nitpick comments:
In @.github/workflows/ci-cd.yml:
- Around line 35-45: The workflow currently sets provenance: false in the
docker/build-push-action@v6 step which disables SLSA provenance; change the
provenance setting to true (or make it conditional via an env flag) so
provenance attestation is emitted when the target registry supports it, or if
you must keep it false add a clear comment and README note explaining why;
update the docker/build-push-action@v6 step (the block containing context, push,
platforms, provenance, tags, cache-from, cache-to) to either set provenance:
true or read an env variable like PROVENANCE_ENABLED to toggle it and document
the rationale for any deliberate disablement.
In `@src/repository/analytics.repository.ts`:
- Around line 144-145: The code is performing full user file-table scans by
calling calculateUserStorage(userId) on every upload/delete/folder event which
will not scale; instead implement incremental storage bookkeeping: add a
persistent user_storage (or storage_bytes) column and update it by applying the
delta (e.g., +file.size on upload, -file.size on delete, adjust on
move/overwrite) inside the event handlers rather than recomputing via
calculateUserStorage each time; keep calculateUserStorage only as a fallback
reconciliation method and ensure event handlers (the upload/delete/folder
handlers that currently call calculateUserStorage) compute and persist deltas
atomically (transactionally) so concurrent events are safe.
🪄 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: b0f5748b-fad4-44b9-9ce4-734a7bcb1220
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
.github/workflows/ci-cd.yml.gitignoreDockerfilesrc/config/config.tssrc/config/s3-secondary.config.tssrc/config/s3.config.tssrc/controllers/analytics.controller.tssrc/controllers/cron.controller.tssrc/controllers/favorite.controller.tssrc/controllers/file.controller.tssrc/controllers/upload.controller.tssrc/core/analytics-queue.tssrc/global/routes.tssrc/index.tssrc/middleware/auth.middleware.tssrc/models/Notification.model.tssrc/repository/analytics.repository.tssrc/repository/favorite.repository.tssrc/repository/file.repository.tssrc/routes/analytics.routes.tssrc/routes/favorite.routes.tssrc/routes/file.routes.tssrc/routes/upload.routes.tssrc/scripts/migrate-storage.tssrc/services/analytics.service.tssrc/services/lock.service.tssrc/services/user.service.tssrc/utils/cors-options.tssrc/validation/favorite.validation.tssrc/validation/upload.validation.ts
💤 Files with no reviewable changes (1)
- src/services/user.service.ts
Comment on lines
+17
to
+18
| *.md | ||
| *.txt |
There was a problem hiding this comment.
Critical: Ignoring all .md and .txt files will block essential documentation.
The patterns *.md and *.txt will prevent version control of README.md, CONTRIBUTING.md, CHANGELOG.md, and other important documentation. This will block documentation commits and violate repository best practices.
If you need to ignore specific markdown/text files (e.g., generated reports or local notes), use more specific patterns instead:
🔧 Suggested fix: use specific patterns
-*.md
-*.txt
+# Ignore only generated/local documentation
+/docs/generated/*.md
+/reports/*.txt
+notes.md
+TODO.txtOr if these are truly temporary local files, add them to your personal .git/info/exclude instead.
🤖 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 @.gitignore around lines 17 - 18, Remove the overly-broad ignore patterns
'*.md' and '*.txt' from .gitignore and replace them with specific patterns or
explicit exclusions: delete the two lines, add narrow ignores for only generated
or local files (e.g., build/*.md, reports/*.txt) or add explicit allow rules for
important docs (e.g., !README.md, !CONTRIBUTING.md, !CHANGELOG.md) so
documentation remains tracked; if those patterns were intended only for personal
temp files, move them to your local .git/info/exclude instead.
Comment on lines
+42
to
+55
| const startDate = new Date(startDateStr); | ||
| const endDate = new Date(endDateStr); | ||
|
|
||
| if (isNaN(startDate.getTime()) || isNaN(endDate.getTime())) { | ||
| return res.FailureResponse(c, 400, { | ||
| message: "Invalid date format. Use ISO 8601 format (YYYY-MM-DD)" | ||
| }); | ||
| } | ||
|
|
||
| const analytics = await analyticsService.getAnalyticsByDateRange( | ||
| user.id, | ||
| startDate, | ||
| endDate | ||
| ); |
There was a problem hiding this comment.
Reject reversed date ranges.
After parsing, add validation for startDate <= endDate; otherwise invalid ranges currently pass through and produce confusing results.
🔧 Proposed fix
const startDate = new Date(startDateStr);
const endDate = new Date(endDateStr);
if (isNaN(startDate.getTime()) || isNaN(endDate.getTime())) {
@@
}
+
+if (startDate > endDate) {
+ return res.FailureResponse(c, 400, {
+ message: "startDate must be earlier than or equal to endDate"
+ });
+}🤖 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/controllers/analytics.controller.ts` around lines 42 - 55, After parsing
startDateStr and endDateStr into startDate and endDate, add a validation that
startDate.getTime() <= endDate.getTime(); if not, return the same
res.FailureResponse pattern with a 400 status and a clear message (e.g.,
"Invalid date range: startDate must be before or equal to endDate") before
calling analyticsService.getAnalyticsByDateRange(user.id, startDate, endDate).
Comment on lines
+82
to
+86
| storageQuota: user.storage_quota, | ||
| storageUsed: overview.storage.totalSize, | ||
| storageRemaining: user.storage_quota - overview.storage.totalSize, | ||
| storageUsedPercentage: ((overview.storage.totalSize / user.storage_quota) * 100).toFixed(2) | ||
| } |
There was a problem hiding this comment.
Guard storage percentage calculation for zero/invalid quota.
storageUsedPercentage currently divides by user.storage_quota without safety checks, which can return invalid values when quota is 0/missing.
🔧 Proposed fix
- storageUsedPercentage: ((overview.storage.totalSize / user.storage_quota) * 100).toFixed(2)
+ storageUsedPercentage: user.storage_quota > 0
+ ? ((overview.storage.totalSize / user.storage_quota) * 100).toFixed(2)
+ : "0.00"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| storageQuota: user.storage_quota, | |
| storageUsed: overview.storage.totalSize, | |
| storageRemaining: user.storage_quota - overview.storage.totalSize, | |
| storageUsedPercentage: ((overview.storage.totalSize / user.storage_quota) * 100).toFixed(2) | |
| } | |
| storageQuota: user.storage_quota, | |
| storageUsed: overview.storage.totalSize, | |
| storageRemaining: user.storage_quota - overview.storage.totalSize, | |
| storageUsedPercentage: user.storage_quota > 0 | |
| ? ((overview.storage.totalSize / user.storage_quota) * 100).toFixed(2) | |
| : "0.00" | |
| } |
🤖 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/controllers/analytics.controller.ts` around lines 82 - 86, The
storageUsedPercentage calculation divides by user.storage_quota without checks;
update the object creation (the storageUsedPercentage field that uses
overview.storage.totalSize / user.storage_quota) to first verify that
user.storage_quota is a positive number (e.g., Number(user.storage_quota) > 0)
and only then compute ((overview.storage.totalSize / user.storage_quota) *
100).toFixed(2); if the quota is zero/invalid, return a safe fallback (e.g.,
"0.00" or null) to avoid NaN/Infinity.
Comment on lines
+36
to
+43
| addToAnalyticsQueue({ | ||
| userId: user.id, | ||
| eventType: AnalyticsEventType.FOLDER_CREATED, | ||
| metadata: { | ||
| folderName: value.name, | ||
| isFolder: true | ||
| } | ||
| }); |
There was a problem hiding this comment.
Handle analytics enqueue failures to avoid unhandled rejections.
These new enqueue calls are async but not awaited/caught. If Redis is unavailable, rejected promises can become unhandled and degrade process stability.
🔧 Proposed fix
+const enqueueAnalytics = (event: Parameters<typeof addToAnalyticsQueue>[0]) => {
+ void addToAnalyticsQueue(event).catch((error) => {
+ console.error("Failed to enqueue analytics event:", error);
+ });
+};- addToAnalyticsQueue({
+ enqueueAnalytics({
userId: user.id,
eventType: AnalyticsEventType.FOLDER_CREATED,
metadata: { ... }
});Also applies to: 134-144, 238-246, 284-291
🤖 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/controllers/file.controller.ts` around lines 36 - 43, The calls to
addToAnalyticsQueue are asynchronous and currently fire-and-forget, which can
produce unhandled promise rejections if Redis/enqueue fails; modify each call to
addToAnalyticsQueue (including the ones used for folder creation, updates, and
deletions) to handle failures by either awaiting the promise inside an async
try/catch or by appending a .catch(...) that logs the error (using the existing
logger/processLogger) so rejections are swallowed and surfaced; ensure every
invocation of addToAnalyticsQueue in file.controller.ts is updated to handle
errors consistently.
Comment on lines
+38
to
+52
| files.forEach((file, index) => { | ||
| const result = results[index]; | ||
| if (result) { | ||
| addToAnalyticsQueue({ | ||
| userId: user.id, | ||
| eventType: AnalyticsEventType.FILE_UPLOADED, | ||
| metadata: { | ||
| fileName: file.name, | ||
| fileSize: result.file_size, | ||
| fileType: result.file_type, | ||
| isFolder: false | ||
| } | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Protect fire-and-forget analytics enqueue with error handling.
Inside the upload loop, failed queue writes are currently unhandled. Add a local catch so transient queue failures don’t create unhandled rejections.
🔧 Proposed fix
files.forEach((file, index) => {
const result = results[index];
if (result) {
- addToAnalyticsQueue({
+ void addToAnalyticsQueue({
userId: user.id,
eventType: AnalyticsEventType.FILE_UPLOADED,
metadata: {
fileName: file.name,
fileSize: result.file_size,
fileType: result.file_type,
isFolder: false
}
- });
+ }).catch((error) => {
+ console.error("Failed to enqueue upload analytics:", error);
+ });
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| files.forEach((file, index) => { | |
| const result = results[index]; | |
| if (result) { | |
| addToAnalyticsQueue({ | |
| userId: user.id, | |
| eventType: AnalyticsEventType.FILE_UPLOADED, | |
| metadata: { | |
| fileName: file.name, | |
| fileSize: result.file_size, | |
| fileType: result.file_type, | |
| isFolder: false | |
| } | |
| }); | |
| } | |
| }); | |
| files.forEach((file, index) => { | |
| const result = results[index]; | |
| if (result) { | |
| void addToAnalyticsQueue({ | |
| userId: user.id, | |
| eventType: AnalyticsEventType.FILE_UPLOADED, | |
| metadata: { | |
| fileName: file.name, | |
| fileSize: result.file_size, | |
| fileType: result.file_type, | |
| isFolder: false | |
| } | |
| }).catch((error) => { | |
| console.error("Failed to enqueue upload analytics:", error); | |
| }); | |
| } | |
| }); |
🤖 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/controllers/upload.controller.ts` around lines 38 - 52, The
fire-and-forget call to addToAnalyticsQueue inside files.forEach can produce
unhandled rejections; protect it by attaching a local catch handler to the
promise returned by addToAnalyticsQueue (or wrap the call in a try/catch if you
convert the iterator to an async loop) so transient queue failures are swallowed
or logged. Specifically, in the upload controller where files.forEach is used
and addToAnalyticsQueue({ userId: user.id, eventType:
AnalyticsEventType.FILE_UPLOADED, ... }) is invoked, ensure you call the
function and then chain .catch(err => /* log or noop */) so any rejection is
handled locally and does not create an unhandled rejection.
| * Migrate a single file using streaming to avoid memory issues | ||
| */ | ||
| private async migrateFile(key: string, targetFolder: FolderNameEnum, fileSize: number): Promise<void> { | ||
| const fileName = key.split('/').pop() || key; |
There was a problem hiding this comment.
Preserve relative object keys to avoid overwrite collisions.
Line 113 drops directory structure with .pop(). Different source keys can collapse to the same destination key and overwrite each other.
Suggested fix
- const fileName = key.split('/').pop() || key;
+ const relativeKey = key.includes('/') ? key.substring(key.indexOf('/') + 1) : key;
...
- const exists = await this.fileExists(fileName, targetFolder);
+ const exists = await this.fileExists(relativeKey, targetFolder);
...
- primaryS3Service.uploadBuffer(
- fileName,
+ primaryS3Service.uploadBuffer(
+ relativeKey,
fileBuffer,
- fileName,
+ relativeKey,
metadata.contentType || 'application/octet-stream',
targetFolder
),Also applies to: 118-119, 176-182
🤖 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/scripts/migrate-storage.ts` at line 113, The code currently flattens
object keys with const fileName = key.split('/').pop() || key, causing different
source paths to collide; instead preserve the relative path (e.g., compute
relativeKey by stripping the source prefix or base folder from key) and use that
relativeKey when constructing the destination object name so directory structure
is retained; replace any usage of fileName in the migration logic (including the
blocks around where fileName is set and the similar logic at the other affected
spots) with this relativeKey (or properly normalized path) so keys remain unique
and overwrite collisions are avoided.
Comment on lines
+287
to
+292
| if (!process.env.SECONDARY_S3_TOKEN_ID || | ||
| !process.env.SECONDARY_S3_SECRET_KEY || | ||
| !process.env.SECONDARY_S3_ENDPOINT || | ||
| !process.env.SECONDARY_S3_BUCKET_NAME) { | ||
| throw new Error('Missing required secondary S3 environment variables'); | ||
| } |
There was a problem hiding this comment.
Fail fast on missing primary S3 env vars too.
main() validates secondary credentials only. Missing primary config is detected only during transfer attempts, causing noisy partial runs.
Suggested fix
if (!process.env.SECONDARY_S3_TOKEN_ID ||
!process.env.SECONDARY_S3_SECRET_KEY ||
!process.env.SECONDARY_S3_ENDPOINT ||
!process.env.SECONDARY_S3_BUCKET_NAME) {
throw new Error('Missing required secondary S3 environment variables');
}
+
+ if (!process.env.S3_TOKEN_ID ||
+ !process.env.S3_SECRET_KEY ||
+ !process.env.S3_ENDPOINT ||
+ !process.env.S3_BUCKET_NAME ||
+ !process.env.S3_REGION) {
+ throw new Error('Missing required primary S3 environment variables');
+ }🤖 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/scripts/migrate-storage.ts` around lines 287 - 292, The current
validation in main() only checks secondary S3 env vars and lets missing primary
S3 credentials surface later; update the startup validation to also fail fast
for primary S3 by checking PRIMARY_S3_TOKEN_ID, PRIMARY_S3_SECRET_KEY,
PRIMARY_S3_ENDPOINT and PRIMARY_S3_BUCKET_NAME alongside the secondary vars (or
factor into a helper like validateS3EnvVars used for both primary and secondary)
and throw a clear Error when any required primary variable is missing so the
script exits immediately instead of running partial transfers.
Comment on lines
+24
to
+31
| case 'file_uploaded': | ||
| if (metadata.fileSize && metadata.fileType) { | ||
| await analyticsRepository.recordFileUpload( | ||
| userId, | ||
| metadata.fileSize, | ||
| metadata.fileType | ||
| ); | ||
| } |
There was a problem hiding this comment.
Zero-byte uploads are silently skipped.
The truthy check on Line 25 drops valid fileSize = 0 events, so upload analytics can be inaccurate.
🔧 Proposed fix
- case 'file_uploaded':
- if (metadata.fileSize && metadata.fileType) {
+ case 'file_uploaded':
+ if (metadata.fileSize !== undefined && metadata.fileType) {
await analyticsRepository.recordFileUpload(
userId,
metadata.fileSize,
metadata.fileType
);
}
break;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| case 'file_uploaded': | |
| if (metadata.fileSize && metadata.fileType) { | |
| await analyticsRepository.recordFileUpload( | |
| userId, | |
| metadata.fileSize, | |
| metadata.fileType | |
| ); | |
| } | |
| case 'file_uploaded': | |
| if (metadata.fileSize !== undefined && metadata.fileType) { | |
| await analyticsRepository.recordFileUpload( | |
| userId, | |
| metadata.fileSize, | |
| metadata.fileType | |
| ); | |
| } |
🤖 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/services/analytics.service.ts` around lines 24 - 31, The check in the
file upload branch uses a truthy test on metadata.fileSize which drops valid
zero-byte uploads; change the condition in the case 'file_uploaded' to
explicitly test for presence (e.g. metadata.fileSize !== undefined &&
metadata.fileType != null) before calling
analyticsRepository.recordFileUpload(userId, metadata.fileSize,
metadata.fileType) so fileSize === 0 is accepted but missing values are still
rejected.
Comment on lines
+20
to
+22
| const result = await client.set(fullKey, Date.now().toString(), 'EX', ttl, 'NX'); | ||
|
|
||
| return result === 'OK'; |
There was a problem hiding this comment.
Use lock ownership tokens and compare-and-delete on unlock.
Current unlock deletes by key only. If a job exceeds TTL and another instance acquires the same lock, the first instance can delete the second instance’s lock in finally, allowing overlapping execution.
Suggested fix
+import { randomUUID } from "crypto";
+
class LockService {
@@
- async acquireLock(lockKey: string, ttl: number = this.DEFAULT_TTL): Promise<boolean> {
+ async acquireLock(lockKey: string, ttl: number = this.DEFAULT_TTL): Promise<string | null> {
try {
const client = RedisConnectionManager.getClient();
const fullKey = `${this.LOCK_PREFIX}${lockKey}`;
+ const lockValue = randomUUID();
- const result = await client.set(fullKey, Date.now().toString(), 'EX', ttl, 'NX');
+ const result = await client.set(fullKey, lockValue, 'EX', ttl, 'NX');
-
- return result === 'OK';
+ return result === 'OK' ? lockValue : null;
@@
- async releaseLock(lockKey: string): Promise<void> {
+ async releaseLock(lockKey: string, lockValue: string): Promise<void> {
try {
const client = RedisConnectionManager.getClient();
const fullKey = `${this.LOCK_PREFIX}${lockKey}`;
-
- await client.del(fullKey);
+ const script = `
+ if redis.call("GET", KEYS[1]) == ARGV[1] then
+ return redis.call("DEL", KEYS[1])
+ else
+ return 0
+ end
+ `;
+ await client.eval(script, 1, fullKey, lockValue);
@@
- const acquired = await this.acquireLock(lockKey, ttl);
- if (!acquired) {
+ const lockValue = await this.acquireLock(lockKey, ttl);
+ if (!lockValue) {
@@
- await this.releaseLock(lockKey);
+ await this.releaseLock(lockKey, lockValue);
}
}
}Also applies to: 37-43, 79-80
🤖 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/services/lock.service.ts` around lines 20 - 22, Replace the current
simple set/delete lock logic with ownership tokens and an atomic
compare-and-delete: when acquiring a lock (where client.set(fullKey,
Date.now().toString(), 'EX', ttl, 'NX') is used) write a strong random token
(e.g., UUID) as the key value and return that token to the caller instead of
Date.now(); when releasing (the places that currently call client.del(fullKey)
in the unlock/finally blocks referenced around lines 37-43 and 79-80) use an
atomic Lua script that checks the key's value equals the token and only then
deletes it (EVAL with: if redis.call("GET", KEYS[1]) == ARGV[1] then return
redis.call("DEL", KEYS[1]) else return 0 end); update the lock API to
accept/propagate the token so unlock can supply it and ensure all unlock paths
use the compare-and-delete script rather than blind DEL.
Comment on lines
+11
to
13
| "https://fileflow.nerchuko.in", | ||
| "http://192.168.0.8:5173", | ||
| ], |
There was a problem hiding this comment.
Avoid hardcoding private LAN origins in credentialed CORS allowlist.
This couples production policy to a dev machine/network and expands trusted origins for cookie-bearing requests.
Suggested fix
export const corsOptions = {
- origin: [
+ origin: [
"http://localhost:3000",
"http://localhost:3001",
"http://localhost:5000",
"http://localhost:5173",
"https://fileflow.nerchuko.in",
- "http://192.168.0.8:5173",
- ],
+ ...(process.env.CORS_EXTRA_ORIGINS
+ ? process.env.CORS_EXTRA_ORIGINS.split(",").map((o) => o.trim()).filter(Boolean)
+ : []),
+ ],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "https://fileflow.nerchuko.in", | |
| "http://192.168.0.8:5173", | |
| ], | |
| export const corsOptions = { | |
| origin: [ | |
| "http://localhost:3000", | |
| "http://localhost:3001", | |
| "http://localhost:5000", | |
| "http://localhost:5173", | |
| "https://fileflow.nerchuko.in", | |
| ...(process.env.CORS_EXTRA_ORIGINS | |
| ? process.env.CORS_EXTRA_ORIGINS.split(",").map((o) => o.trim()).filter(Boolean) | |
| : []), | |
| ], |
🤖 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/utils/cors-options.ts` around lines 11 - 13, The CORS allowlist currently
hardcodes a private LAN origin ("http://192.168.0.8:5173") which should not be
used in production; update the code that defines the allowedOrigins/ corsOptions
array to remove hardcoded private IPs and instead load dev-only origins from an
environment variable (e.g., process.env.DEV_CORS_ORIGINS split by comma) or only
add local/dev origins when NODE_ENV !== 'production'; ensure credentialed CORS
(with credentials: true) only uses trusted production origins from a secure env
var like PROCESS_CORS_ORIGINS and guard any dev origins behind a runtime check
so private LAN addresses are never included in production allowlists.
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.
Summary by CodeRabbit
Release Notes
New Features
Infrastructure