Conversation
|
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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThe PR extends the Resgrid Audio Relay system to support hosted multi-department scenarios with system API key authentication, dispatch code resolution via lookups with optional Redis caching, and new dispatch code types (GroupMessage, DistributionList). Configuration migrates from Changes
Sequence Diagram(s)sequenceDiagram
participant Relay as SMTP Relay Engine
participant Parser as SmtpDispatchAddressParser
participant Cache as CachedLookupsService
participant Redis as Redis Cache
participant API as LookupsApi
participant Resgrid as Resgrid API
Relay->>Parser: ParseRecipients(addresses)
Parser->>Parser: Extract DepartmentId from domains
Parser->>Parser: Detect DispatchCode types
Parser-->>Relay: DispatchParseResult
alt ResolveDispatchCodes enabled
Relay->>Cache: LookupGroupByDispatchCodeAsync(code)
Cache->>Redis: Get dispatch code mapping
alt Cache Hit
Redis-->>Cache: Cached result
else Cache Miss
Cache->>API: LookupGroupByDispatchCodeAsync
API->>Resgrid: GET /api/v4/lookups/groups/dispatch-code
Resgrid-->>API: Group numeric ID
API-->>Cache: Result
Cache->>Redis: Store result with TTL
Redis-->>Cache: Stored
end
Cache-->>Relay: GroupLookupResult
Relay->>Relay: Update DispatchCode.ResolvedId
end
Relay->>Relay: Build dispatch tokens using ResolvedId
Relay->>Resgrid: POST /api/v4/calls (with DepartmentId)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 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 9 minutes and 50 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Providers/Resgrid.Providers.ApiClient/V4/ResgridApiClientOptions.cs (1)
74-100:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't require OAuth client credentials in
SystemApiKeymode.
ResgridV4ApiClientskips token acquisition entirely forSystemApiKeyand only sendsX-Resgrid-SystemApiKey, so the unconditionalClientId/ClientSecretchecks reject a configuration the runtime can otherwise use.Program.ValidateOptionsmirrors the same assumption, so both validators need to be relaxed together.Suggested fix
- if (String.IsNullOrWhiteSpace(ClientId)) - throw new InvalidOperationException("A Resgrid API client id is required."); - - if (String.IsNullOrWhiteSpace(ClientSecret)) - throw new InvalidOperationException("A Resgrid API client secret is required."); - switch (GrantType) { case ResgridAuthGrantType.RefreshToken: + if (String.IsNullOrWhiteSpace(ClientId)) + throw new InvalidOperationException("A Resgrid API client id is required."); + if (String.IsNullOrWhiteSpace(ClientSecret)) + throw new InvalidOperationException("A Resgrid API client secret is required."); + if (String.IsNullOrWhiteSpace(RefreshToken) && String.IsNullOrWhiteSpace(TokenCachePath)) throw new InvalidOperationException( "A refresh token or token cache path is required when using the refresh_token grant type."); break; case ResgridAuthGrantType.ClientCredentials: - // Only ClientId and ClientSecret are required — already validated above. + if (String.IsNullOrWhiteSpace(ClientId)) + throw new InvalidOperationException("A Resgrid API client id is required."); + if (String.IsNullOrWhiteSpace(ClientSecret)) + throw new InvalidOperationException("A Resgrid API client secret is required."); break; case ResgridAuthGrantType.SystemApiKey:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Providers/Resgrid.Providers.ApiClient/V4/ResgridApiClientOptions.cs` around lines 74 - 100, The current validation in ResgridApiClientOptions always enforces ClientId and ClientSecret even when GrantType == ResgridAuthGrantType.SystemApiKey; update the validator so that the checks for ClientId and ClientSecret are skipped when GrantType is SystemApiKey (aligning with ResgridV4ApiClient behavior), and ensure Program.ValidateOptions is changed similarly so both validators allow configurations that use SystemApiKey without requiring OAuth client credentials.
🧹 Nitpick comments (1)
Providers/Resgrid.Providers.ApiClient/V4/DispatchListBuilder.cs (1)
18-39: ⚡ Quick winMake the explicit enum values match the documented pipeline IDs.
The XML docs say DistributionList = 2, Group = 3, and GroupMessage = 4, but the assigned values are
Group = 2,GroupMessage = 3,DistributionList = 4. If these ever cross a boundary as integers, they will be misclassified; even if they do not, the docs are now lying. Please either realign the numeric assignments or remove the type-number claims from the comments.One way to realign the enum
- Group = 2, + DistributionList = 2, /// <summary> - /// Group message email (Type 4 in the Postmark pipeline). + /// Group dispatch email (Type 3 in the Postmark pipeline). + /// The local-part is the dispatch email code stored in + /// DepartmentGroups.DispatchEmail. It resolves to a single numeric + /// group ID via the lookup API. + /// </summary> + Group = 3, + + /// <summary> + /// Group message email (Type 4 in the Postmark pipeline). /// The local-part is the message email code stored in /// DepartmentGroups.MessageEmail. Results in a group message, /// not a call. /// </summary> - GroupMessage = 3, - - /// <summary> - /// Distribution list email (Type 2 in the Postmark pipeline). - /// The local-part is the list address stored in - /// DistributionLists.EmailAddress. Results in list forwarding. - /// </summary> DistributionList = 4🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Providers/Resgrid.Providers.ApiClient/V4/DispatchListBuilder.cs` around lines 18 - 39, The enum explicit values for DistributionList, Group, and GroupMessage in DispatchListBuilder.cs do not match their XML docs (docs claim DistributionList=2, Group=3, GroupMessage=4) which causes misclassification; update the enum numeric assignments so DistributionList = 2, Group = 3, GroupMessage = 4 (or alternatively remove the hard-coded numeric values and adjust the XML comments to not claim specific pipeline IDs) and ensure the enum declarations for Group, GroupMessage, and DistributionList reflect the corrected mapping to keep code and docs consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile`:
- Around line 54-63: The ENV lines for the Resgrid relay variables (e.g.
RESGRID__RELAY__Resgrid__BaseUrl, RESGRID__RELAY__Resgrid__ApiVersion,
RESGRID__RELAY__Resgrid__ClientId, RESGRID__RELAY__Resgrid__ClientSecret,
RESGRID__RELAY__Resgrid__RefreshToken, RESGRID__RELAY__Resgrid__Scope,
RESGRID__RELAY__Resgrid__TokenCachePath, RESGRID__RELAY__Resgrid__GrantType,
RESGRID__RELAY__Resgrid__SystemApiKey, RESGRID__RELAY__Resgrid__DepartmentId)
currently include inline “#” comments which become part of the variable values;
remove those inline comments, place any explanatory text on separate comment
lines above each ENV (use e.g. ## or #), and set empty/required variables to
explicit empty strings (e.g. RESGRID__RELAY__Resgrid__BaseUrl="" and
RESGRID__RELAY__Resgrid__ClientId="") so values are unambiguous; repeat the same
change for the other commented ENV blocks (e.g. the block around lines 95–99) to
prevent configuration binding failures.
In `@Providers/Resgrid.Providers.ApiClient/V4/DispatchListBuilder.cs`:
- Around line 94-102: The Build method in DispatchListBuilder
(Build(IEnumerable<DispatchCode> dispatchCodes, string departmentDispatchPrefix
= "G")) is including DispatchCodeType.GroupMessage recipients; update the LINQ
filter to also exclude x.Type == DispatchCodeType.GroupMessage so group-message
targets are not formatted or included in the returned tokens (i.e., add a
condition like && x.Type != DispatchCodeType.GroupMessage to the Where clause
before Select/Distinct); keep using Format(x, departmentDispatchPrefix) and the
existing Distinct behavior after the filter.
In `@Resgrid.Audio.Relay.Console/Program.cs`:
- Around line 106-110: The configuration builder currently only adds environment
variables with the "RESGRID__RELAY__" prefix so legacy RELAY_* env vars are
ignored; update the ConfigurationBuilder that produces the configuration
variable to also call AddEnvironmentVariables("RELAY_") before the RESGRID call
so existing RELAY_* variables are picked up but are overridden by
RESGRID__RELAY__ when present; locate the ConfigurationBuilder block in
Program.cs (the variable named configuration) and add the legacy
AddEnvironmentVariables("RELAY_") call immediately before the existing
AddEnvironmentVariables("RESGRID__RELAY__") call.
- Around line 151-160: When SMTP mode is selected add a startup validation that
if options.Smtp.RedisCache.Enabled is true but
options.Smtp.RedisCache.ConnectionString (or equivalent) is null/empty, you
should add an error to errors (same collection used for domain validation) and
prevent startup; specifically update the SMTP validation block around
options.Mode == "smtp" to check options.Smtp.RedisCache.Enabled and
string.IsNullOrWhiteSpace(options.Smtp.RedisCache.ConnectionString) and push a
clear message like "Redis cache enabled but
RESGRID__RELAY__Smtp__RedisCache__ConnectionString is not set" so
RedisDispatchLookupCache won't repeatedly attempt ConnectAsync at runtime.
In `@Resgrid.Audio.Relay.Console/Smtp/RedisDispatchLookupCache.cs`:
- Around line 36-43: Concurrent callers of GetDatabaseAsync can race past the
_db != null check and each call ConnectionMultiplexer.ConnectAsync, leaking
sockets; serialize connection creation by introducing an async lock (e.g., a
private SemaphoreSlim or AsyncLock) and use double-checked locking inside
GetDatabaseAsync: if _db != null return it, otherwise await the lock, check _db
again, then call ConnectionMultiplexer.ConnectAsync(_options.ConnectionString)
and set _redis and _db only once (and finally release the lock); reference the
GetDatabaseAsync method and the fields _db, _redis,
ConnectionMultiplexer.ConnectAsync and _options.ConnectionString when making
this change.
In `@Resgrid.Audio.Relay.Console/Smtp/SmtpDispatchAddressParser.cs`:
- Around line 42-48: The loop that adds parsed recipients collapses entries by
dispatchCode.Code only and stores a single result.DepartmentId, causing
cross-department/Type collisions; update the logic in the block that calls
TryParse (and mutates result.DispatchCodes and result.DepartmentId) to enforce
uniqueness on the tuple (departmentId, dispatchCode.Type, dispatchCode.Code)
instead of Code alone, and if result.DepartmentId is already set and a new
parsed departmentId differs, reject the input (or return an error) when only a
single department is allowed. Locate TryParse, the local variables dispatchCode
and departmentId, and the places writing result.DispatchCodes and
result.DepartmentId to implement the tuple-based comparison and the
mixed-department rejection.
In `@Resgrid.Audio.Relay.Console/Smtp/SmtpRelayInfrastructure.cs`:
- Around line 187-198: The current branches that check
dispatchResult.HasGroupMessageTargets and
dispatchResult.HasDistributionListTargets call
_telemetry.MessageProcessingStarted which later conflicts with
_telemetry.UnroutableMessage when there are no call targets; change these to
emit a dedicated unsupported-target signal (e.g., add/invoke
_telemetry.UnsupportedTarget(context, messageSummary) or similar) instead of
_telemetry.MessageProcessingStarted, or alternatively defer the single
_telemetry.MessageProcessingStarted call until after you confirm
dispatchResult.HasCallTargets is true; update references to
dispatchResult.HasGroupMessageTargets,
dispatchResult.HasDistributionListTargets, _telemetry.MessageProcessingStarted,
_telemetry.UnroutableMessage, HasCallTargets, messageSummary and context
accordingly.
---
Outside diff comments:
In `@Providers/Resgrid.Providers.ApiClient/V4/ResgridApiClientOptions.cs`:
- Around line 74-100: The current validation in ResgridApiClientOptions always
enforces ClientId and ClientSecret even when GrantType ==
ResgridAuthGrantType.SystemApiKey; update the validator so that the checks for
ClientId and ClientSecret are skipped when GrantType is SystemApiKey (aligning
with ResgridV4ApiClient behavior), and ensure Program.ValidateOptions is changed
similarly so both validators allow configurations that use SystemApiKey without
requiring OAuth client credentials.
---
Nitpick comments:
In `@Providers/Resgrid.Providers.ApiClient/V4/DispatchListBuilder.cs`:
- Around line 18-39: The enum explicit values for DistributionList, Group, and
GroupMessage in DispatchListBuilder.cs do not match their XML docs (docs claim
DistributionList=2, Group=3, GroupMessage=4) which causes misclassification;
update the enum numeric assignments so DistributionList = 2, Group = 3,
GroupMessage = 4 (or alternatively remove the hard-coded numeric values and
adjust the XML comments to not claim specific pipeline IDs) and ensure the enum
declarations for Group, GroupMessage, and DistributionList reflect the corrected
mapping to keep code and docs consistent.
🪄 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: 7cbbbc46-1fa8-4b46-be06-e68cfa770a60
📒 Files selected for processing (22)
DockerfileProviders/Resgrid.Providers.ApiClient/V4/CallsApi.csProviders/Resgrid.Providers.ApiClient/V4/DispatchListBuilder.csProviders/Resgrid.Providers.ApiClient/V4/LookupsApi.csProviders/Resgrid.Providers.ApiClient/V4/Models/CallModels.csProviders/Resgrid.Providers.ApiClient/V4/Models/LookupModels.csProviders/Resgrid.Providers.ApiClient/V4/ResgridApiClientOptions.csProviders/Resgrid.Providers.ApiClient/V4/ResgridV4ApiClient.csResgrid.Audio.Relay.Console/Configuration/RelayHostOptions.csResgrid.Audio.Relay.Console/Program.csResgrid.Audio.Relay.Console/Resgrid.Audio.Relay.Console.csprojResgrid.Audio.Relay.Console/Smtp/CachedLookupsService.csResgrid.Audio.Relay.Console/Smtp/IDispatchLookupCache.csResgrid.Audio.Relay.Console/Smtp/NullDispatchLookupCache.csResgrid.Audio.Relay.Console/Smtp/RedisDispatchLookupCache.csResgrid.Audio.Relay.Console/Smtp/SmtpDispatchAddressParser.csResgrid.Audio.Relay.Console/Smtp/SmtpRelayInfrastructure.csResgrid.Audio.Relay.Console/Smtp/SmtpTelemetry.csResgrid.Audio.Relay.Console/appsettings.jsonResgrid.Audio.Tests/DispatchListBuilderTests.csResgrid.Audio.Tests/SmtpDispatchAddressParserTests.csdocker-entrypoint.sh
Comment on lines
106
to
110
| var configuration = new ConfigurationBuilder() | ||
| .SetBasePath(AppContext.BaseDirectory) | ||
| .AddJsonFile("appsettings.json", optional: true) | ||
| .AddEnvironmentVariables("RELAY_") | ||
| .AddEnvironmentVariables("RESGRID__RELAY__") | ||
| .Build(); |
There was a problem hiding this comment.
Keep the legacy RELAY_ binding during the migration.
This drops the prefix the live worker has been using, so existing deployments that still inject RELAY_* will silently fall back to defaults or fail validation. Bind both prefixes and let RESGRID__RELAY__* win while configs migrate.
Suggested fix
var configuration = new ConfigurationBuilder()
.SetBasePath(AppContext.BaseDirectory)
.AddJsonFile("appsettings.json", optional: true)
+ .AddEnvironmentVariables("RELAY_")
.AddEnvironmentVariables("RESGRID__RELAY__")
.Build();Based on learnings: Program.cs in Resgrid.Audio.Relay.Console is the live worker entrypoint that selects smtp or audio mode, loads appsettings.json plus RELAY_ environment variables into RelayHostOptions, and resolves relative paths from AppContext.BaseDirectory.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Resgrid.Audio.Relay.Console/Program.cs` around lines 106 - 110, The
configuration builder currently only adds environment variables with the
"RESGRID__RELAY__" prefix so legacy RELAY_* env vars are ignored; update the
ConfigurationBuilder that produces the configuration variable to also call
AddEnvironmentVariables("RELAY_") before the RESGRID call so existing RELAY_*
variables are picked up but are overridden by RESGRID__RELAY__ when present;
locate the ConfigurationBuilder block in Program.cs (the variable named
configuration) and add the legacy AddEnvironmentVariables("RELAY_") call
immediately before the existing AddEnvironmentVariables("RESGRID__RELAY__")
call.
Member
Author
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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
New Features
Configuration
RELAY_toRESGRID__RELAY__.