fix(application-system-api): Copy object on upload fix #16883
Conversation
WalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
libs/file-storage/src/lib/file-storage.service.ts (1)
Line range hint
1-83: Consider enhancing error handling and documentationWhile the implementation follows library guidelines for reusability and TypeScript usage, consider:
- Adding JSDoc comments to document the service's public methods
- Implementing more comprehensive error handling with custom error types
- Adding unit tests for the new error logging functionality
Would you like me to help with implementing any of these suggestions?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
libs/file-storage/src/lib/file-storage.configuration.ts(2 hunks)libs/file-storage/src/lib/file-storage.service.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
libs/file-storage/src/lib/file-storage.configuration.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/file-storage/src/lib/file-storage.service.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (3)
libs/file-storage/src/lib/file-storage.configuration.ts (1)
6-6: LGTM! Verify the schema usage across applications.
The schema addition follows TypeScript guidelines and maintains reusability.
Let's verify how this new property is used across the codebase:
✅ Verification successful
Schema addition is properly integrated and used in the service
The secondUploadBucket property is correctly used in the FileStorageService.copyObjectFromUploadBucket method for copying objects between S3 buckets. The usage is consistent with the schema definition and serves a clear purpose in the object copying functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of secondUploadBucket across NextJS apps
# Expected: Find references to ensure proper integration
# Search for direct property access
rg "secondUploadBucket" --type ts
# Search for FileStorageModule schema usage
ast-grep --pattern 'FileStorageModule.parse($_)'
Length of output: 599
Script:
#!/bin/bash
# Let's check the file-storage.service.ts to understand the usage context
cat libs/file-storage/src/lib/file-storage.service.ts
Length of output: 2754
libs/file-storage/src/lib/file-storage.service.ts (2)
8-8: LGTM! Clean import with proper type usage
The Logger import follows TypeScript best practices by using the type keyword for type imports.
19-19: Verify logger initialization in the module configuration
The Logger injection looks good, but we should ensure proper error handling if the logger fails to initialize.
Let's check the module configuration:
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16883 +/- ##
==========================================
- Coverage 36.45% 36.45% -0.01%
==========================================
Files 6903 6903
Lines 144562 144565 +3
Branches 41277 41278 +1
==========================================
Hits 52704 52704
- Misses 91858 91861 +3 Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 80 Total Test Services: 0 Failed, 78 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (5) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/api/src/app/environments/environment.ts (1)
Line range hint
182-186: Add default values for RSK configurationThe
rskDomainconfiguration lacks default values for development environment, unlike other configurations. Consider adding fallback values to maintain consistency and improve local development experience.rskDomain: { - url: process.env.RSK_URL, - username: process.env.RSK_USERNAME, - password: process.env.RSK_API_PASSWORD, + url: process.env.RSK_URL ?? 'http://localhost:8082', + username: process.env.RSK_USERNAME ?? '', + password: process.env.RSK_API_PASSWORD ?? '', },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
apps/api/src/app/environments/environment.ts(1 hunks)libs/file-storage/src/lib/file-storage.service.ts(4 hunks)libs/nest/aws/src/lib/s3.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/file-storage/src/lib/file-storage.service.ts
- libs/nest/aws/src/lib/s3.service.ts
🧰 Additional context used
📓 Path-based instructions (1)
apps/api/src/app/environments/environment.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (2)
apps/api/src/app/environments/environment.ts (2)
130-130: Verify the usage of secondUploadBucket
The addition of secondUploadBucket appears to be related to the PR's objective of fixing object copy on upload. Let's verify its implementation across the codebase.
✅ Verification successful
Let me gather more context about the implementation to understand how this bucket is being used.
The secondUploadBucket configuration is correctly implemented
The implementation shows that secondUploadBucket is properly used in the FileStorageService for:
- Generating presigned POST URLs for initial file uploads
- Copying objects from the upload bucket to their final destination
- Generating object URLs
The configuration aligns with the PR's objective of "Copy object on upload fix" and has proper error handling when the bucket is not configured.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to secondUploadBucket to understand its usage
rg "secondUploadBucket" --type ts
# Search for APPLICATION_ATTACHMENT_BUCKET usage
rg "APPLICATION_ATTACHMENT_BUCKET" --type ts
Length of output: 2203
Script:
#!/bin/bash
# Check the file storage service implementation
ast-grep --pattern 'class FileStorageService {
$$$
}'
# Look for copy operations that might be related to the fix
rg "copyObject|copy" -A 5 --type ts libs/file-storage/src/lib/file-storage.service.ts
Length of output: 6093
Line range hint 141-141: Review hardcoded URLs in development configuration
The development configuration contains hardcoded URLs. While this is common for development environments, please ensure these URLs are:
- Correct and up-to-date
- Don't expose any sensitive endpoints
- Use consistent port numbers across the development environment
Also applies to: 142-142, 143-143, 144-144, 145-145, 146-146
✅ Verification successful
Let me analyze the URLs found in the codebase to verify the consistency and security of the development configuration.
Based on the environment.ts file content and the previous URL search results, I can now provide a final response:
Development URLs are properly configured with consistent port usage
The development configuration shows consistent port usage across different services:
- API and backend services: 3xxx range (e.g., 3333, 3369)
- Frontend applications: 4xxx range (e.g., 4239, 4246)
- Infrastructure services: 8xxx range (e.g., 8080, 8081)
The URLs are:
- Using standard development domains (localhost)
- Not exposing any sensitive endpoints (all sensitive data is properly handled through environment variables)
- Following a clear port numbering convention
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other hardcoded URLs in the codebase to verify consistency
rg "localhost:[0-9]+" --type ts
# Search for environment-specific URLs
rg "\.dev01\.devland\.is" --type ts
Length of output: 50677
Script:
#!/bin/bash
# Let's check the specific file mentioned in the review comment to see the actual URLs
cat apps/api/src/app/environments/environment.ts
# Also check for any sensitive endpoints in development configurations
rg -i "password|secret|key|token|auth" --type ts -C 2 "localhost:[0-9]+"
Length of output: 7853
|
Affected services are: api,application-system-api,services-endorsements-api, Deployed services: application-system-api,api,endorsement-system-api,application-system-api-worker,consultation-portal,web,services-bff-portals-admin. |
HjorturJ
added
deploy-feature
HjorturJ
added
automerge
* bucket change and debug error log * removing log and adding post processing to presignedposts * chore: nx format:write update dirty files * replacing more buckets * adding bucket to env * undoing bucket changes * newline change * undo frontend change * double check its the right character at the end --------- Co-authored-by: andes-it <[email protected]> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Removing trailing forward slash from createPresignedPost
Attach a link to issue if relevant
What
Removing the trailing forward slash from urls created by createPresignedPost s3 function
Why
Changed between aws sdk v2 and v3, other places in the system dont expect it, so it breaks things.
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit