Save local changes: update auth & upload controller#353
Open
JAbhishek09 wants to merge 2 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAuth service and upload controller now import ChangesProfile Image URL Signing
CI Deploy Region
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/src/module/upload/upload.controller.ts (1)
131-135: ⚡ Quick winDeduplicate user-media signing in upload responses.
Line 131 and Line 167 repeat the same signing sequence. Please move this into a shared helper and reuse it in both handlers.
♻️ Proposed refactor
+async function signUserMediaFields<T extends { profilePic: string | null; coverImage: string | null; resumes: string[] }>(user: T): Promise<T> { + if (user.profilePic) (user as Record<string, unknown>).profilePic = await signUrl(user.profilePic); + if (user.coverImage) (user as Record<string, unknown>).coverImage = await signUrl(user.coverImage); + if (user.resumes.length > 0) (user as Record<string, unknown>).resumes = await signUrls(user.resumes); + return user; +} ... - if (user.profilePic) (user as Record<string, unknown>).profilePic = await signUrl(user.profilePic); - if (user.coverImage) (user as Record<string, unknown>).coverImage = await signUrl(user.coverImage); - if (user.resumes.length > 0) (user as Record<string, unknown>).resumes = await signUrls(user.resumes); + await signUserMediaFields(user); ... - if (user.profilePic) (user as Record<string, unknown>).profilePic = await signUrl(user.profilePic); - if (user.coverImage) (user as Record<string, unknown>).coverImage = await signUrl(user.coverImage); - if (user.resumes.length > 0) (user as Record<string, unknown>).resumes = await signUrls(user.resumes); + await signUserMediaFields(user);As per coding guidelines, "Apply DRY principle: no duplicate helpers, shared animation variants per file".
Also applies to: 167-171
🤖 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 `@server/src/module/upload/upload.controller.ts` around lines 131 - 135, The user media signing logic (calls to signUrl/signUrls for user.profilePic, user.coverImage, user.resumes) is duplicated in two handlers in upload.controller.ts; extract that sequence into a single helper function (e.g., signUserMedia(user): Promise<User>) and replace both occurrences with a call to this helper from the handlers that currently run the inline signing (the blocks around where signUrl/signUrls are invoked). Ensure the helper accepts the user object, performs the same conditional assignments for profilePic, coverImage, and resumes using signUrl/signUrls, and returns the updated user so both handler functions simply await signUserMedia(user) instead of repeating the code.server/src/module/auth/auth.service.ts (1)
358-363: ⚡ Quick winExtract repeated media-signing logic into one helper.
The same signing block is duplicated at Line 358, Line 406, and Line 440. Please centralize this in a private helper to keep behavior consistent and reduce drift risk.
♻️ Proposed refactor
+ private async signProfileMedia<T extends { resumes: string[]; profilePic: string | null; coverImage: string | null }>(entity: T): Promise<T> { + if (entity.resumes.length > 0) (entity as Record<string, unknown>).resumes = await signUrls(entity.resumes); + if (entity.profilePic) (entity as Record<string, unknown>).profilePic = await signUrl(entity.profilePic); + if (entity.coverImage) (entity as Record<string, unknown>).coverImage = await signUrl(entity.coverImage); + return entity; + } ... - if (user.resumes.length > 0) { - (user as Record<string, unknown>).resumes = await signUrls(user.resumes); - } - if (user.profilePic) { - (user as Record<string, unknown>).profilePic = await signUrl(user.profilePic); - } - if (user.coverImage) { - (user as Record<string, unknown>).coverImage = await signUrl(user.coverImage); - } + await this.signProfileMedia(user);As per coding guidelines, "Apply DRY principle: no duplicate helpers, shared animation variants per file".
Also applies to: 406-411, 440-445
🤖 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 `@server/src/module/auth/auth.service.ts` around lines 358 - 363, The media-signing logic for user.profilePic and user.coverImage is duplicated; create a private async helper (e.g., private async signUserMedia(user: any): Promise<void>) that checks user.profilePic and user.coverImage and replaces them with await signUrl(...) (preserving the existing type cast to Record<string, unknown> if needed), then replace each duplicated block (the occurrences around the current if blocks) with a single await this.signUserMedia(user) call to centralize behavior and avoid drift.
🤖 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.
Nitpick comments:
In `@server/src/module/auth/auth.service.ts`:
- Around line 358-363: The media-signing logic for user.profilePic and
user.coverImage is duplicated; create a private async helper (e.g., private
async signUserMedia(user: any): Promise<void>) that checks user.profilePic and
user.coverImage and replaces them with await signUrl(...) (preserving the
existing type cast to Record<string, unknown> if needed), then replace each
duplicated block (the occurrences around the current if blocks) with a single
await this.signUserMedia(user) call to centralize behavior and avoid drift.
In `@server/src/module/upload/upload.controller.ts`:
- Around line 131-135: The user media signing logic (calls to signUrl/signUrls
for user.profilePic, user.coverImage, user.resumes) is duplicated in two
handlers in upload.controller.ts; extract that sequence into a single helper
function (e.g., signUserMedia(user): Promise<User>) and replace both occurrences
with a call to this helper from the handlers that currently run the inline
signing (the blocks around where signUrl/signUrls are invoked). Ensure the
helper accepts the user object, performs the same conditional assignments for
profilePic, coverImage, and resumes using signUrl/signUrls, and returns the
updated user so both handler functions simply await signUserMedia(user) instead
of repeating the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 16cfe180-f972-456f-8869-7702c631ef17
📒 Files selected for processing (2)
server/src/module/auth/auth.service.tsserver/src/module/upload/upload.controller.ts
Sachinchaurasiya360
added
bug
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/workflows/deploy.yml:
- Line 24: The workflow hardcodes aws-region: ap-south-1 but the ECR login step
still uses ${{ secrets.AWS_REGION }}, causing a region mismatch; make the region
source consistent by either (A) replacing the hardcoded aws-region: ap-south-1
with aws-region: ${{ secrets.AWS_REGION }} so the whole job uses the secret, or
(B) change the ECR login command that references ${{ secrets.AWS_REGION }} to
use the same literal ap-south-1; update the aws-region key and the ECR login
command (search for the aws-region setting and the ECR login step referencing
${{ secrets.AWS_REGION }}) so both use the same single canonical region value.
🪄 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: 4868bc04-b328-4ed7-a7d0-a8c6fa4e8550
📒 Files selected for processing (1)
.github/workflows/deploy.yml
| aws-secret-access-key: ${{ secrets.AWS_SECRET_ACCESS_KEY }} | ||
| aws-region: ${{ secrets.AWS_REGION }} | ||
|
|
||
| aws-region: ap-south-1 |
Contributor
There was a problem hiding this comment.
Critical: Inconsistent AWS region configuration will break deployment.
Line 24 now hardcodes the region to ap-south-1, but line 53 still uses ${{ secrets.AWS_REGION }} for the ECR login command on the EC2 instance. If the secret differs from ap-south-1 or is not set, the ECR authentication will fail and deployment will break.
🔧 Proposed fix to align region on line 53
script: |
# Login to ECR
- aws ecr get-login-password --region ${{ secrets.AWS_REGION }} | docker login --username AWS --password-stdin $ECR_REGISTRY
+ aws ecr get-login-password --region ap-south-1 | docker login --username AWS --password-stdin $ECR_REGISTRY
# Pull latest image🤖 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/deploy.yml at line 24, The workflow hardcodes aws-region:
ap-south-1 but the ECR login step still uses ${{ secrets.AWS_REGION }}, causing
a region mismatch; make the region source consistent by either (A) replacing the
hardcoded aws-region: ap-south-1 with aws-region: ${{ secrets.AWS_REGION }} so
the whole job uses the secret, or (B) change the ECR login command that
references ${{ secrets.AWS_REGION }} to use the same literal ap-south-1; update
the aws-region key and the ECR login command (search for the aws-region setting
and the ECR login step referencing ${{ secrets.AWS_REGION }}) so both use the
same single canonical region value.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit