Conversation
Add webhook verify/HMAC, inbox mailboxes/messages/threads, and v2 docs. Bump System.Text.Json to 8.0.5. Co-authored-by: Cursor <cursoragent@cursor.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThis PR rewrites the Reloop .NET SDK as version 2.0.0 with typed models, validator-driven services, revised exception handling, webhook HMAC verification, inbox APIs, a centralized HTTP client, updated documentation, and expanded xUnit coverage. ChangesReloop SDK 2.0.0 rewrite
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ReloopClient
participant Service
participant API
Caller->>Service: Invoke typed async operation
Service->>Service: Validate parameters and build request
Service->>ReloopClient: FetchAsync(method, path, body, query)
ReloopClient->>API: Send authenticated HTTP request
API-->>ReloopClient: Return response
ReloopClient-->>Caller: Deserialize result or throw typed exception
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 12
🧹 Nitpick comments (8)
tests/Reloop.Tests/ApiKeyServiceTests.cs (1)
27-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the complete name-only payload shape.
The substring checks permit extra serialized/request fields. Parse the JSON and assert that it has exactly one
nameproperty in both tests.Proposed test tightening
- Assert.Contains("\"name\":\"Production Key\"", json); - Assert.DoesNotContain("enabled", json); + using var document = JsonDocument.Parse(json); + Assert.Single(document.RootElement.EnumerateObject()); + Assert.Equal("Production Key", document.RootElement.GetProperty("name").GetString()); - Assert.Contains("\"name\":\"prod\"", await handler.LastRequest!.Content!.ReadAsStringAsync()); + using var body = JsonDocument.Parse( + await handler.LastRequest!.Content!.ReadAsStringAsync()); + Assert.Single(body.RootElement.EnumerateObject()); + Assert.Equal("prod", body.RootElement.GetProperty("name").GetString());🤖 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 `@tests/Reloop.Tests/ApiKeyServiceTests.cs` around lines 27 - 50, The tests CreateApiKeyParams_SerializesNameOnly and CreateAsync_HappyPath currently allow unexpected request properties. Parse the serialized JSON and request content, then assert each object contains exactly one property named “name” and no additional fields, while preserving the existing name-value assertions.ReloopClient.cs (1)
70-138: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider adding
CancellationTokensupport toFetchAsync.Neither overload accepts a
CancellationToken, so consumers can't cancel/timeout individual requests independently of theHttpClient's own timeout.public Task<T?> FetchAsync<T>(HttpMethod method, string path, object? body, Dictionary<string, string?>? query, CancellationToken cancellationToken = default)🤖 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 `@ReloopClient.cs` around lines 70 - 138, Add optional CancellationToken parameters to both FetchAsync overloads, propagate the token from the convenience overload to the full overload, and pass it to _httpClient.SendAsync. Preserve existing call compatibility and response handling.Models/InboxModels.cs (2)
133-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
MessageAttachment/MessageAttachmentItemfields.
MessageAttachmentItemrepeats all 8 fields ofMessageAttachmentand only addsInboundEmailId. Same DRY opportunity as the response classes above.♻️ Proposed refactor
- public class MessageAttachmentItem - { - [JsonPropertyName("id")] - public string? Id { get; set; } - - [JsonPropertyName("inboundEmailId")] - public string? InboundEmailId { get; set; } - - [JsonPropertyName("filename")] - public string? Filename { get; set; } - - [JsonPropertyName("contentType")] - public string? ContentType { get; set; } - - [JsonPropertyName("size")] - public int Size { get; set; } - - [JsonPropertyName("storagePath")] - public string? StoragePath { get; set; } - - [JsonPropertyName("contentDisposition")] - public string? ContentDisposition { get; set; } - - [JsonPropertyName("contentId")] - public string? ContentId { get; set; } - - [JsonPropertyName("createdAt")] - public string? CreatedAt { get; set; } - } + public class MessageAttachmentItem : MessageAttachment + { + [JsonPropertyName("inboundEmailId")] + public string? InboundEmailId { get; set; } + }Also applies to: 250-278
🤖 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 `@Models/InboxModels.cs` around lines 133 - 158, Refactor MessageAttachmentItem to reuse MessageAttachment’s eight existing properties instead of redeclaring them, adding only its InboundEmailId-specific property. Preserve the current JSON names and serialization behavior while eliminating the duplicated field definitions.
79-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
SendEmailResponse/SendEmailOrPendingResponsefields.
SendEmailOrPendingResponserepeats all 5 fields ofSendEmailResponseverbatim and only addsPending/SendAt. Inheritance would remove the duplication.♻️ Proposed refactor
- public class SendEmailOrPendingResponse - { - [JsonPropertyName("success")] - public bool Success { get; set; } - - [JsonPropertyName("messageId")] - public string? MessageId { get; set; } - - [JsonPropertyName("status")] - public string? Status { get; set; } - - [JsonPropertyName("timestamp")] - public string? Timestamp { get; set; } - - [JsonPropertyName("id")] - public string? Id { get; set; } - + public class SendEmailOrPendingResponse : SendEmailResponse + { [JsonPropertyName("pending")] public bool? Pending { get; set; } [JsonPropertyName("sendAt")] public string? SendAt { get; set; } }🤖 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 `@Models/InboxModels.cs` around lines 79 - 119, Refactor SendEmailOrPendingResponse to inherit from SendEmailResponse, removing its duplicated Success, MessageId, Status, Timestamp, and Id properties while retaining Pending and SendAt with their existing JSON mappings and types.Services/InboxMessagesService.cs (1)
225-257: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
BuildComposeBodyreturns an untyped dictionary, contrary to the PR's typed-model direction.Every other request body in this rewrite is a typed model, but Reply/ReplyAll/Forward bodies are built as
Dictionary<string, object?>. Consider introducing a small typedComposeMessageBodyrequest DTO to keep the surface consistent and get compile-time field checking.🤖 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 `@Services/InboxMessagesService.cs` around lines 225 - 257, The BuildComposeBody method should return a typed ComposeMessageBody DTO instead of Dictionary<string, object?>. Define the DTO with nullable Text, Html, Cc, Bcc, and Attachments properties, then populate and return it from BuildComposeBody while preserving the existing validation and optional-field behavior.tests/Reloop.Tests/InboxServiceTests.cs (1)
1-189: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThin coverage for the new Inbox surface; missing tests would have caught the SetRead/SetStar default bug.
Only a handful of methods per service are exercised (Create/Get validation for Mailboxes; Send/validation for Messages; List/Batch validation for Threads). Notably absent: a test calling
SetReadAsync(id)/SetStarAsync(id)with no explicit parameters, which would surface the default-falsebehavior flagged inInboxMessagesService.cs/InboxThreadsService.cs.🤖 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 `@tests/Reloop.Tests/InboxServiceTests.cs` around lines 1 - 189, Expand InboxMessagesServiceTests and InboxThreadsServiceTests with tests that call SetReadAsync and SetStarAsync using only the required id, without explicit parameters. Assert the requests preserve the intended default behavior rather than defaulting to false, and verify the expected HTTP method and endpoint through MockHttpMessageHandler. Use the existing CreateClient test pattern.tests/Reloop.Tests/ContactsServiceRouteTests.cs (1)
17-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRoute coverage for Contacts CRUD is now limited to Create only.
The rewritten suite only asserts routes for
Contacts.CreateAsync,Contacts.Groups.ListContactsAsync,Contacts.Channels.AddContactAsync, andContacts.Properties.CreateAsync. There's no route assertion left forContacts.GetAsync,Contacts.ListAsync,Contacts.UpdateAsync, orContacts.DeleteAsyncagainst the new typed API surface.[Fact] public async Task UpdateAsync_UsesContactPatchRoute() { var (client, handler) = CreateClient(); await client.Contacts.UpdateAsync("con_1", new UpdateContactParams { FirstName = "Ada" }); Assert.Equal(new HttpMethod("PATCH"), handler.LastRequest?.Method); Assert.Equal("/api/contacts/con_1", handler.LastRequest?.RequestUri?.PathAndQuery); }🤖 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 `@tests/Reloop.Tests/ContactsServiceRouteTests.cs` around lines 17 - 51, Add route-focused tests to ContactsServiceRouteTests covering Contacts.GetAsync, ListAsync, UpdateAsync, and DeleteAsync on the typed Contacts API, asserting each request’s HTTP method and expected path/query; include the PATCH update case for contact ID con_1 and preserve the existing route tests.Services/ContactsService.cs (1)
62-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNear-identical ID-validation and membership-validation helpers are duplicated across all four contact sub-services. Each service reimplements the same "wrap
Validators.RequireNonEmptyStringand rethrow with an entity-specific message" pattern, and the group/channel membership validators duplicate the same "require contact_id or email" logic with only minor signature differences. Centralizing these inValidation/Validators.cswould remove ~40 lines of duplicated logic and keep future message/behavior changes in one place.
Services/ContactsService.cs#L62-L73: replaceRequireContactIdwith a sharedValidators.RequireEntityId(id, "Contact", field)helper.Services/ContactPropertiesService.cs#L48-L59: replaceRequirePropertyIdwith the same shared helper ("Property").Services/ContactGroupsService.cs#L92-L103: replaceRequireGroupIdwith the same shared helper ("Group").Services/ContactChannelsService.cs#L77-L88: replaceRequireChannelIdwith the same shared helper ("Channel").Services/ContactGroupsService.cs#L165-L200: extract the contact_id/email "one of required" logic into a sharedValidators.RequireContactIdentifier(contactId, email)used here.Services/ContactChannelsService.cs#L157-L192: reuse the same sharedValidators.RequireContactIdentifierinstead of a near-duplicate local implementation.🤖 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 `@Services/ContactsService.cs` around lines 62 - 73, Centralize the duplicated validation helpers in Validation/Validators.cs: add RequireEntityId to wrap RequireNonEmptyString with the entity-specific error message, and add RequireContactIdentifier for the contact_id/email requirement. Replace RequireContactId, RequirePropertyId, RequireGroupId, and RequireChannelId in Services/ContactsService.cs#L62-73, Services/ContactPropertiesService.cs#L48-59, Services/ContactGroupsService.cs#L92-103, and Services/ContactChannelsService.cs#L77-88 with RequireEntityId using Contact, Property, Group, and Channel respectively; update the membership validation blocks in Services/ContactGroupsService.cs#L165-200 and Services/ContactChannelsService.cs#L157-192 to call RequireContactIdentifier.
🤖 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 `@CONTRIBUTING.md`:
- Line 27: Add the text language identifier to the fenced tree block in
CONTRIBUTING.md by changing the opening fence to ```text, while preserving the
existing tree content and closing fence.
In `@Models/WebhookModels.cs`:
- Around line 126-130: Change the MaxRequestsPerMinute and MaxRetries properties
in the webhook model from nullable double to nullable int, preserving their
existing JSON property names and accessors so fractional limits cannot be
accepted or serialized.
In `@ReloopClient.cs`:
- Around line 38-50: Stop mutating shared HttpClient state in the ReloopClient
constructor: remove the x-api-key and Accept DefaultRequestHeaders updates, and
do not conditionally assign BaseAddress for externally supplied clients. In
FetchAsync, apply the current instance’s _apiKey and application/json Accept
header to each HttpRequestMessage before sending, while preserving the client’s
configured BaseAddress and ensuring request URI construction remains consistent
with BaseUrl.
- Around line 122-129: Update the success-response deserialization in the method
containing responseText and JsonSerializer.Deserialize<T> to catch malformed
JSON JsonException and surface it as a ReloopApiException, matching the existing
API-error and network-error paths while preserving the empty-response default
behavior.
In `@Services/ContactChannelsService.cs`:
- Around line 227-259: The update request bodies must preserve explicit
null-clears instead of serializing them away. In
Services/ContactChannelsService.cs, update ValidateUpdateParams to use the
serializer/configuration that emits null-valued dictionary entries so
DescriptionClear reaches the API. In Services/ContactPropertiesService.cs lines
117-123, update ValidateUpdateParams and its parameter model to track
fallbackValue presence/clear intent, emit null when explicitly cleared, and
reject empty updates when no field was provided.
In `@Services/ContactsService.cs`:
- Around line 14-15: Update UpdatePropertyKeyPattern to use the same
lowercase-letter, digit, and underscore pattern as CreatePropertyKeyPattern, so
keys such as utm2 validate consistently during create and update.
In `@Services/DomainService.cs`:
- Around line 68-95: Update GetAsync, UpdateAsync, DeleteAsync, and VerifyAsync
to pass domainId through a shared path-segment validator/encoder after
validation. Ensure the helper rejects "." and ".." and percent-encodes all
remaining path-segment values before interpolating them into request URLs.
In `@Services/InboxMessagesService.cs`:
- Around line 84-102: The no-argument overloads currently default both state
flags to false; update SetReadAsync and SetStarAsync in
Services/InboxMessagesService.cs (lines 84-102) and SetReadAsync and
SetStarAsync in Services/InboxThreadsService.cs (lines 79-97) so their fallback
SetMessageReadParams, SetMessageStarParams, SetThreadReadParams, and
SetThreadStarParams instances initialize the respective flags to true, while
preserving explicitly supplied parameter values.
- Around line 110-144: Update SendAsync to validate the message body by calling
Validators.RequireComposeBody with parameters.Text and parameters.Html before
constructing the request body, matching the validation used by BuildComposeBody.
Preserve the existing text and HTML values when building SendMessageParams.
In `@Services/WebhookService.cs`:
- Around line 33-40: Update CreateAsync to validate CreateWebhookParams before
calling FetchAsync: require a non-empty callback URL and a non-empty event list,
while preserving the existing null-parameter validation and
ReloopValidationException pattern. Add a regression test confirming invalid
empty parameters fail locally without issuing an HTTP request.
- Around line 291-295: Update the status filter condition in
BuildListWebhooksQuery to check parameters.Status != null rather than using
string.IsNullOrEmpty. This must pass empty strings to RequireDeliveryStatus so
they are rejected, while preserving the existing validation and query assignment
behavior.
In `@Services/WebhookVerify.cs`:
- Around line 28-37: Validate the effective tolerance in the verification flow
after assigning tolerance from parameters.Tolerance or DefaultToleranceSeconds
and before ParseSignatureHeader or VerifyTimestamp; reject negative values with
the established WebhookSignatureException mechanism, while preserving valid
non-negative tolerance behavior.
---
Nitpick comments:
In `@Models/InboxModels.cs`:
- Around line 133-158: Refactor MessageAttachmentItem to reuse
MessageAttachment’s eight existing properties instead of redeclaring them,
adding only its InboundEmailId-specific property. Preserve the current JSON
names and serialization behavior while eliminating the duplicated field
definitions.
- Around line 79-119: Refactor SendEmailOrPendingResponse to inherit from
SendEmailResponse, removing its duplicated Success, MessageId, Status,
Timestamp, and Id properties while retaining Pending and SendAt with their
existing JSON mappings and types.
In `@ReloopClient.cs`:
- Around line 70-138: Add optional CancellationToken parameters to both
FetchAsync overloads, propagate the token from the convenience overload to the
full overload, and pass it to _httpClient.SendAsync. Preserve existing call
compatibility and response handling.
In `@Services/ContactsService.cs`:
- Around line 62-73: Centralize the duplicated validation helpers in
Validation/Validators.cs: add RequireEntityId to wrap RequireNonEmptyString with
the entity-specific error message, and add RequireContactIdentifier for the
contact_id/email requirement. Replace RequireContactId, RequirePropertyId,
RequireGroupId, and RequireChannelId in Services/ContactsService.cs#L62-73,
Services/ContactPropertiesService.cs#L48-59,
Services/ContactGroupsService.cs#L92-103, and
Services/ContactChannelsService.cs#L77-88 with RequireEntityId using Contact,
Property, Group, and Channel respectively; update the membership validation
blocks in Services/ContactGroupsService.cs#L165-200 and
Services/ContactChannelsService.cs#L157-192 to call RequireContactIdentifier.
In `@Services/InboxMessagesService.cs`:
- Around line 225-257: The BuildComposeBody method should return a typed
ComposeMessageBody DTO instead of Dictionary<string, object?>. Define the DTO
with nullable Text, Html, Cc, Bcc, and Attachments properties, then populate and
return it from BuildComposeBody while preserving the existing validation and
optional-field behavior.
In `@tests/Reloop.Tests/ApiKeyServiceTests.cs`:
- Around line 27-50: The tests CreateApiKeyParams_SerializesNameOnly and
CreateAsync_HappyPath currently allow unexpected request properties. Parse the
serialized JSON and request content, then assert each object contains exactly
one property named “name” and no additional fields, while preserving the
existing name-value assertions.
In `@tests/Reloop.Tests/ContactsServiceRouteTests.cs`:
- Around line 17-51: Add route-focused tests to ContactsServiceRouteTests
covering Contacts.GetAsync, ListAsync, UpdateAsync, and DeleteAsync on the typed
Contacts API, asserting each request’s HTTP method and expected path/query;
include the PATCH update case for contact ID con_1 and preserve the existing
route tests.
In `@tests/Reloop.Tests/InboxServiceTests.cs`:
- Around line 1-189: Expand InboxMessagesServiceTests and
InboxThreadsServiceTests with tests that call SetReadAsync and SetStarAsync
using only the required id, without explicit parameters. Assert the requests
preserve the intended default behavior rather than defaulting to false, and
verify the expected HTTP method and endpoint through MockHttpMessageHandler. Use
the existing CreateClient test pattern.
🪄 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: 8458e1c7-1031-4a8e-b82e-1722b5bbfc6c
📒 Files selected for processing (44)
CHANGELOG.mdCONTRIBUTING.mdExceptions/ApiErrorBody.csExceptions/ReloopApiException.csExceptions/ReloopValidationException.csExceptions/WebhookSignatureException.csModels/ApiKeyModels.csModels/ContactModels.csModels/DomainModels.csModels/InboxModels.csModels/MailModels.csModels/Models.csModels/WebhookModels.csREADME.mdReloop.csprojReloopClient.csRequestParameters.csServices/ApiKeyService.csServices/ContactChannelsService.csServices/ContactGroupsService.csServices/ContactPropertiesService.csServices/ContactsService.csServices/DomainService.csServices/InboxMailboxesService.csServices/InboxMessagesService.csServices/InboxService.csServices/InboxThreadsService.csServices/MailService.csServices/WebhookService.csServices/WebhookVerify.csValidation/Validators.csVersion.cstests/Reloop.Tests/ApiKeyServiceRouteTests.cstests/Reloop.Tests/ApiKeyServiceTests.cstests/Reloop.Tests/ContactsServiceRouteTests.cstests/Reloop.Tests/ContactsServiceTests.cstests/Reloop.Tests/DomainServiceRouteTests.cstests/Reloop.Tests/DomainServiceTests.cstests/Reloop.Tests/InboxServiceTests.cstests/Reloop.Tests/MailServiceRouteTests.cstests/Reloop.Tests/MailServiceTests.cstests/Reloop.Tests/MockHttpMessageHandler.cstests/Reloop.Tests/ReloopClientTests.cstests/Reloop.Tests/WebhookServiceTests.cs
💤 Files with no reviewable changes (2)
- RequestParameters.cs
- Models/Models.cs
Comment on lines
+227
to
+259
| private static Dictionary<string, object?> ValidateUpdateParams(UpdateChannelParams? parameters) | ||
| { | ||
| if (parameters == null) | ||
| { | ||
| throw new ReloopValidationException( | ||
| "update requires at least one of name, description, or visibility.", "params"); | ||
| } | ||
|
|
||
| var body = new Dictionary<string, object?>(); | ||
| if (parameters.Name != null) | ||
| { | ||
| body["name"] = RequireChannelName(parameters.Name, "name"); | ||
| } | ||
|
|
||
| if (parameters.DescriptionPresent) | ||
| { | ||
| body["description"] = parameters.DescriptionClear ? null : parameters.Description; | ||
| } | ||
|
|
||
| if (parameters.Visibility != null) | ||
| { | ||
| RequireOptionalVisibility(parameters.Visibility, "visibility"); | ||
| body["visibility"] = parameters.Visibility; | ||
| } | ||
|
|
||
| if (body.Count == 0) | ||
| { | ||
| throw new ReloopValidationException( | ||
| "update requires at least one of name, description, or visibility.", "params"); | ||
| } | ||
|
|
||
| return body; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Relevant files ==\n'
git ls-files | rg '^(Services/ContactChannelsService\.cs|Services/ContactPropertiesService\.cs|ReloopClient\.cs|.*Params\.cs|.*Options\.cs)$'
printf '\n== Outline: ReloopClient.cs ==\n'
ast-grep outline ReloopClient.cs --view expanded || true
printf '\n== Outline: Services/ContactChannelsService.cs ==\n'
ast-grep outline Services/ContactChannelsService.cs --view expanded || true
printf '\n== Outline: Services/ContactPropertiesService.cs ==\n'
ast-grep outline Services/ContactPropertiesService.cs --view expanded || trueRepository: reloop-labs/reloop-dotnet
Length of output: 4945
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Search for serializer options and update params ==\n'
rg -n "DefaultIgnoreCondition|JsonIgnoreCondition|DescriptionPresent|DescriptionClear|FallbackValue|ValidateUpdateParams|UpdateChannelParams|UpdatePropertyParams" \
Services ReloopClient.cs . \
-g '!**/bin/**' -g '!**/obj/**' || true
printf '\n== Read ReloopClient.cs (selected) ==\n'
sed -n '1,220p' ReloopClient.cs 2>/dev/null || true
printf '\n== Read Services/ContactChannelsService.cs (selected) ==\n'
sed -n '200,290p' Services/ContactChannelsService.cs 2>/dev/null || true
printf '\n== Read Services/ContactPropertiesService.cs (selected) ==\n'
sed -n '90,150p' Services/ContactPropertiesService.cs 2>/dev/null || trueRepository: reloop-labs/reloop-dotnet
Length of output: 14810
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== ReloopClient.cs: serializer options and request serialization ==\n'
if [ -f ReloopClient.cs ]; then
rg -n -A4 -B4 "JsonSerializerOptions|DefaultIgnoreCondition|WhenWritingNull|Serialize\(" ReloopClient.cs
fi
printf '\n== Update parameter types ==\n'
rg -n -A4 -B4 "class UpdateChannelParams|class UpdatePropertyParams|DescriptionPresent|DescriptionClear|FallbackValue" Services . \
-g '!**/bin/**' -g '!**/obj/**' || true
printf '\n== Exact method bodies ==\n'
sed -n '1,220p' Services/ContactChannelsService.cs
printf '\n---\n'
sed -n '1,180p' Services/ContactPropertiesService.csRepository: reloop-labs/reloop-dotnet
Length of output: 20651
Null-clears are dropped from both update bodies
Services/ContactChannelsService.cs:body["description"] = nullis serialized away, soDescriptionClearcannot clear the channel description.Services/ContactPropertiesService.cs:ValidateUpdateParamsalways emitsfallbackValue, and with no presence/clear flagUpdateAsync(id)can send{}and cannot explicitly clear the fallback value. Add a presence/clear flag or reject empty updates here too.
📍 Affects 2 files
Services/ContactChannelsService.cs#L227-L259(this comment)Services/ContactPropertiesService.cs#L117-L123
🤖 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 `@Services/ContactChannelsService.cs` around lines 227 - 259, The update
request bodies must preserve explicit null-clears instead of serializing them
away. In Services/ContactChannelsService.cs, update ValidateUpdateParams to use
the serializer/configuration that emits null-valued dictionary entries so
DescriptionClear reaches the API. In Services/ContactPropertiesService.cs lines
117-123, update ValidateUpdateParams and its parameter model to track
fallbackValue presence/clear intent, emit null when explicitly cleared, and
reject empty updates when no field was provided.
Comment on lines
+84
to
+102
| public Task<InboxSuccessResponse?> SetReadAsync(string id, SetMessageReadParams? parameters = null) | ||
| { | ||
| var messageId = Validators.RequireMessageId(id, "id"); | ||
| parameters ??= new SetMessageReadParams(); | ||
| return _client.FetchAsync<InboxSuccessResponse>( | ||
| PatchMethod, | ||
| MessagesV1 + "/" + messageId + "/read", | ||
| new SetMessageReadParams(parameters.IsRead)); | ||
| } | ||
|
|
||
| public Task<InboxSuccessResponse?> SetStarAsync(string id, SetMessageStarParams? parameters = null) | ||
| { | ||
| var messageId = Validators.RequireMessageId(id, "id"); | ||
| parameters ??= new SetMessageStarParams(); | ||
| return _client.FetchAsync<InboxSuccessResponse>( | ||
| PatchMethod, | ||
| MessagesV1 + "/" + messageId + "/star", | ||
| new SetMessageStarParams(parameters.IsStarred)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
No-arg SetReadAsync()/SetStarAsync() silently mark unread/unstarred instead of read/starred.
In both files, the fallback parameters ??= new SetXxxParams(); produces a bool default of false. A caller invoking SetReadAsync(id) or SetStarAsync(id) without an explicit params object — the natural convenience usage implied by the method name — ends up sending false, i.e. marking the item unread/unstarred rather than read/starred.
Services/InboxMessagesService.cs#L84-L102: default the fallbackSetMessageReadParams/SetMessageStarParamsinstances totrue.Services/InboxThreadsService.cs#L79-L97: default the fallbackSetThreadReadParams/SetThreadStarParamsinstances totrue.
🔧 Proposed fix pattern (apply to all four methods)
- parameters ??= new SetMessageReadParams();
+ parameters ??= new SetMessageReadParams(true);- parameters ??= new SetMessageStarParams();
+ parameters ??= new SetMessageStarParams(true);- parameters ??= new SetThreadReadParams();
+ parameters ??= new SetThreadReadParams(true);- parameters ??= new SetThreadStarParams();
+ parameters ??= new SetThreadStarParams(true);📝 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
| public Task<InboxSuccessResponse?> SetReadAsync(string id, SetMessageReadParams? parameters = null) | |
| { | |
| var messageId = Validators.RequireMessageId(id, "id"); | |
| parameters ??= new SetMessageReadParams(); | |
| return _client.FetchAsync<InboxSuccessResponse>( | |
| PatchMethod, | |
| MessagesV1 + "/" + messageId + "/read", | |
| new SetMessageReadParams(parameters.IsRead)); | |
| } | |
| public Task<InboxSuccessResponse?> SetStarAsync(string id, SetMessageStarParams? parameters = null) | |
| { | |
| var messageId = Validators.RequireMessageId(id, "id"); | |
| parameters ??= new SetMessageStarParams(); | |
| return _client.FetchAsync<InboxSuccessResponse>( | |
| PatchMethod, | |
| MessagesV1 + "/" + messageId + "/star", | |
| new SetMessageStarParams(parameters.IsStarred)); | |
| } | |
| public Task<InboxSuccessResponse?> SetReadAsync(string id, SetMessageReadParams? parameters = null) | |
| { | |
| var messageId = Validators.RequireMessageId(id, "id"); | |
| parameters ??= new SetMessageReadParams(true); | |
| return _client.FetchAsync<InboxSuccessResponse>( | |
| PatchMethod, | |
| MessagesV1 + "/" + messageId + "/read", | |
| new SetMessageReadParams(parameters.IsRead)); | |
| } | |
| public Task<InboxSuccessResponse?> SetStarAsync(string id, SetMessageStarParams? parameters = null) | |
| { | |
| var messageId = Validators.RequireMessageId(id, "id"); | |
| parameters ??= new SetMessageStarParams(true); | |
| return _client.FetchAsync<InboxSuccessResponse>( | |
| PatchMethod, | |
| MessagesV1 + "/" + messageId + "/star", | |
| new SetMessageStarParams(parameters.IsStarred)); | |
| } |
📍 Affects 2 files
Services/InboxMessagesService.cs#L84-L102(this comment)Services/InboxThreadsService.cs#L79-L97
🤖 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 `@Services/InboxMessagesService.cs` around lines 84 - 102, The no-argument
overloads currently default both state flags to false; update SetReadAsync and
SetStarAsync in Services/InboxMessagesService.cs (lines 84-102) and SetReadAsync
and SetStarAsync in Services/InboxThreadsService.cs (lines 79-97) so their
fallback SetMessageReadParams, SetMessageStarParams, SetThreadReadParams, and
SetThreadStarParams instances initialize the respective flags to true, while
preserving explicitly supplied parameter values.
Comment on lines
+33
to
+40
| public Task<Webhook?> CreateAsync(CreateWebhookParams parameters) | ||
| { | ||
| if (parameters == null) | ||
| { | ||
| throw new ReloopValidationException("create params are required and must be an object.", "params"); | ||
| } | ||
|
|
||
| return _client.FetchAsync<Webhook>(HttpMethod.Post, WebhookV1 + "/", parameters); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate required create fields before the HTTP call.
new CreateWebhookParams() is non-null and is sent as an empty request body. Require a non-empty callback URL and a non-empty event list locally, and add a no-HTTP regression test.
🤖 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 `@Services/WebhookService.cs` around lines 33 - 40, Update CreateAsync to
validate CreateWebhookParams before calling FetchAsync: require a non-empty
callback URL and a non-empty event list, while preserving the existing
null-parameter validation and ReloopValidationException pattern. Add a
regression test confirming invalid empty parameters fail locally without issuing
an HTTP request.
Scope auth headers per request, preserve null clears in update bodies, encode domain path IDs, tighten webhook/inbox validation, and align property-key patterns. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ReloopClient.cs (1)
79-90: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDispose the request and response messages.
HttpRequestMessageandHttpResponseMessageare bothIDisposable; disposing them here releases request/content resources promptly instead of waiting for GC.ReloopClient.cs:79-90🤖 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 `@ReloopClient.cs` around lines 79 - 90, Update the HTTP request flow around HttpRequestMessage creation and _httpClient.SendAsync to dispose both the request and returned HttpResponseMessage deterministically, while preserving the existing serialization and response handling behavior.
🤖 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 `@ReloopClient.cs`:
- Around line 156-163: Update BuildRequestUri to reject caller-supplied absolute
HTTP(S) URLs unless their origin matches the configured base URI, or require
relative paths before FetchAsync attaches the API key. Preserve valid
same-origin request handling and prevent credentials from being sent to
attacker-controlled hosts.
---
Outside diff comments:
In `@ReloopClient.cs`:
- Around line 79-90: Update the HTTP request flow around HttpRequestMessage
creation and _httpClient.SendAsync to dispose both the request and returned
HttpResponseMessage deterministically, while preserving the existing
serialization and response handling 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: 749d5372-bf94-408b-8e98-0f18936556eb
📒 Files selected for processing (15)
CONTRIBUTING.mdModels/InboxModels.csModels/WebhookModels.csReloopClient.csServices/ContactsService.csServices/DomainService.csServices/InboxMessagesService.csServices/WebhookService.csServices/WebhookVerify.csValidation/Validators.cstests/Reloop.Tests/ContactsServiceTests.cstests/Reloop.Tests/DomainServiceTests.cstests/Reloop.Tests/InboxServiceTests.cstests/Reloop.Tests/ReloopClientTests.cstests/Reloop.Tests/WebhookServiceTests.cs
🚧 Files skipped from review as they are similar to previous changes (12)
- tests/Reloop.Tests/ReloopClientTests.cs
- tests/Reloop.Tests/InboxServiceTests.cs
- Validation/Validators.cs
- Models/InboxModels.cs
- tests/Reloop.Tests/WebhookServiceTests.cs
- Models/WebhookModels.cs
- tests/Reloop.Tests/DomainServiceTests.cs
- Services/WebhookService.cs
- Services/WebhookVerify.cs
- Services/DomainService.cs
- Services/InboxMessagesService.cs
- Services/ContactsService.cs
Prevent API-key exfiltration via caller-supplied absolute paths, and dispose request/response promptly. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ReloopClient.cs (1)
145-153: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject relative or malformed base URLs.
NormalizeBaseUrlstill accepts values like"v2"and query/fragment-bearing strings;BuildRequestUriconcatenates_baseUrl + path, so an injectedHttpClient.BaseAddresscan resolve the request to an unintended host and send thex-api-keythere. Require an absolute HTTP(S) URI with a host and no query or fragment before constructing authenticated requests, and add a regression test for a relative base URL plus a differentBaseAddress.🤖 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 `@ReloopClient.cs` around lines 145 - 153, Update NormalizeBaseUrl to validate the normalized value as an absolute URI with a nonempty host, an HTTP or HTTPS scheme, and no query or fragment; reject invalid or relative inputs before BuildRequestUri constructs authenticated requests, preserving DefaultBaseUrl behavior for null or whitespace values as appropriate. Add a regression test covering a relative base URL combined with a different HttpClient.BaseAddress and verify the API key is not sent to the unintended host.
🤖 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.
Outside diff comments:
In `@ReloopClient.cs`:
- Around line 145-153: Update NormalizeBaseUrl to validate the normalized value
as an absolute URI with a nonempty host, an HTTP or HTTPS scheme, and no query
or fragment; reject invalid or relative inputs before BuildRequestUri constructs
authenticated requests, preserving DefaultBaseUrl behavior for null or
whitespace values as appropriate. Add a regression test covering a relative base
URL combined with a different HttpClient.BaseAddress and verify the API key is
not sent to the unintended host.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a847975-61e4-4891-a09e-57a31071577d
📒 Files selected for processing (2)
ReloopClient.cstests/Reloop.Tests/ReloopClientTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Reloop.Tests/ReloopClientTests.cs
Capture LastRequestBody in the test handler so assertions still work when FetchAsync disposes the request message. Co-authored-by: Cursor <cursoragent@cursor.com>
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