Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR implements a presigned PUT fallback mechanism for S3 uploads. When the SDK's ChangesS3 Presigned PUT Fallback
Sequence DiagramsequenceDiagram
participant Client
participant S3Service as S3StorageService
participant S3SDK as S3 SDK<br/>(PutObjectAsync)
participant MetadataCheck as GetObjectMetadata
participant PresignedHTTP as Presigned PUT<br/>(HttpClient)
Client->>S3Service: UploadAsync(stream, key)
S3Service->>S3SDK: PutObjectAsync
S3SDK-->>S3Service: FormatException
S3Service->>MetadataCheck: GetObjectMetadataAsync
alt Object exists
MetadataCheck-->>S3Service: Success
S3Service-->>Client: Success
else Object missing
MetadataCheck-->>S3Service: NotFound
S3Service->>PresignedHTTP: Presigned PUT<br/>(with retry on transient)
PresignedHTTP-->>S3Service: Success
S3Service-->>Client: Success
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 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. Review rate limit: 0/1 reviews remaining, refill in 50 minutes and 54 seconds.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: 1
🧹 Nitpick comments (1)
Web/Resgrid.Web.Tts/Services/S3StorageService.cs (1)
15-15: ⚡ Quick winUse the configured presigned URL lifetime for the fallback PUT.
Web/Resgrid.Web.Tts/Configuration/S3StorageOptions.csalready exposesPresignedUrlExpiryMinutes, andGetObjectUrlAsynchonors it. Hardcoding 5 minutes here makes the recovery path ignore that deployment setting, which can still break uploads in environments that need a longer validity window for clock skew or slower S3-compatible backends.🔧 Proposed fix
- private const int PresignedPutUrlExpiryMinutes = 5; @@ private string CreatePresignedPutUrl(string objectKey, string contentType) { return _s3Client.GetPreSignedURL(new GetPreSignedUrlRequest { BucketName = _options.Bucket, Key = objectKey, Verb = HttpVerb.PUT, ContentType = contentType, - Expires = DateTime.UtcNow.AddMinutes(PresignedPutUrlExpiryMinutes) + Expires = DateTime.UtcNow.AddMinutes(_options.PresignedUrlExpiryMinutes) }); }Also applies to: 219-228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Tts/Services/S3StorageService.cs` at line 15, The hardcoded PresignedPutUrlExpiryMinutes constant in S3StorageService prevents the fallback PUT presigned URL from using the configured lifetime; replace uses of PresignedPutUrlExpiryMinutes with the configured value (e.g., S3StorageOptions.PresignedUrlExpiryMinutes) accessed from the service's injected options instance (the same one used by GetObjectUrlAsync) inside S3StorageService (remove/stop referencing the PresignedPutUrlExpiryMinutes constant and use the injected _options.PresignedUrlExpiryMinutes or equivalent) so both the normal and fallback PUT URL generation honor the deployment setting; apply this change to all occurrences (including the block around the GetObjectUrlAsync fallback at lines ~219-228).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Web/Resgrid.Web.Tts/Services/S3StorageService.cs`:
- Around line 116-145: WasUploadPersistedAsync currently catches
AmazonServiceException, HttpRequestException, and IOException but not non-user
cancellation timeouts; add a catch for TaskCanceledException in
WasUploadPersistedAsync (the same method that calls ExistsAsync) that logs a
warning similar to the others and returns false so timeouts fall back to
UploadWithPresignedUrlAsync consistently with the existing transient-timeout
handling used by UploadWithPresignedUrlAsync.
---
Nitpick comments:
In `@Web/Resgrid.Web.Tts/Services/S3StorageService.cs`:
- Line 15: The hardcoded PresignedPutUrlExpiryMinutes constant in
S3StorageService prevents the fallback PUT presigned URL from using the
configured lifetime; replace uses of PresignedPutUrlExpiryMinutes with the
configured value (e.g., S3StorageOptions.PresignedUrlExpiryMinutes) accessed
from the service's injected options instance (the same one used by
GetObjectUrlAsync) inside S3StorageService (remove/stop referencing the
PresignedPutUrlExpiryMinutes constant and use the injected
_options.PresignedUrlExpiryMinutes or equivalent) so both the normal and
fallback PUT URL generation honor the deployment setting; apply this change to
all occurrences (including the block around the GetObjectUrlAsync fallback at
lines ~219-228).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6ea2e768-4be4-40c6-9ce1-a5f7082087cc
📒 Files selected for processing (3)
Tests/Resgrid.Tests/Web/Tts/S3StorageServiceTests.csWeb/Resgrid.Web.Tts/Program.csWeb/Resgrid.Web.Tts/Services/S3StorageService.cs
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
1 participant
Summary by CodeRabbit