Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
|
Warning Review limit reached
Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds shared Sentry filtering for recognized scanner-related 404 transactions across three web hosts and centralizes terminal failure logging after TTS prompt refresh retries are exhausted. ChangesSentry transaction filtering
TTS terminal failure reporting
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant WebHost
participant SentryTransactionFilter
participant Sentry
WebHost->>SentryTransactionFilter: Filter(transaction)
SentryTransactionFilter->>SentryTransactionFilter: Normalize target and check NotFound scanner path
SentryTransactionFilter-->>Sentry: Return transaction or null
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
Core/Resgrid.Framework/SentryTransactionFilter.cs (3)
118-121: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the modern C# range operator.
As per coding guidelines to "Use modern C# features appropriately", you can simplify this substring extraction by using the range operator.
♻️ Proposed refactor
var separatorIndex = transactionName.IndexOf(' '); return separatorIndex >= 0 && separatorIndex < transactionName.Length - 1 - ? transactionName.Substring(separatorIndex + 1) + ? transactionName[(separatorIndex + 1)..] : transactionName;🤖 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 `@Core/Resgrid.Framework/SentryTransactionFilter.cs` around lines 118 - 121, Update the transaction-name extraction in SentryTransactionFilter to use the modern C# range operator instead of Substring, while preserving the existing separator and boundary checks and returning the original transactionName when no trailing portion exists.Source: Coding guidelines
134-135: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid allocating a new array on every 404 request.
Passing
new[] { '?', '#' }inline allocates a new array each time a request target is parsed. Consider caching this in astatic readonly char[]field at the class level to reduce memory traffic.⚡ Proposed optimization
Define a static field in the class:
private static readonly char[] QueryAndFragmentChars = { '?', '#' };And update the method:
- var suffixIndex = value.IndexOfAny(new[] { '?', '#' }); + var suffixIndex = value.IndexOfAny(QueryAndFragmentChars);🤖 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 `@Core/Resgrid.Framework/SentryTransactionFilter.cs` around lines 134 - 135, Cache the delimiter array used by the suffix lookup in the surrounding method, such as a private static readonly char[] field named QueryAndFragmentChars, and update the IndexOfAny call to reuse it instead of allocating new[] { '?', '#' } for each request.
87-89: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid string allocation in scanner path prefix checks.
The expression
prefix + "/"allocates a new string on each loop iteration. You can prevent this allocation by checking the prefix length and the next character directly.⚡ Proposed optimization
- if (path.Equals(prefix, StringComparison.OrdinalIgnoreCase) || - path.StartsWith(prefix + "/", StringComparison.OrdinalIgnoreCase)) + if (path.StartsWith(prefix, StringComparison.OrdinalIgnoreCase) && + (path.Length == prefix.Length || path[prefix.Length] == '/'))🤖 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 `@Core/Resgrid.Framework/SentryTransactionFilter.cs` around lines 87 - 89, Update the path prefix check in the scanner loop to avoid constructing prefix + "/" on each iteration: retain the exact path.Equals(prefix) match, then validate that path is longer than prefix and that the character immediately after the prefix is '/'.
🤖 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 `@Core/Resgrid.Framework/SentryTransactionFilter.cs`:
- Around line 118-121: Update the transaction-name extraction in
SentryTransactionFilter to use the modern C# range operator instead of
Substring, while preserving the existing separator and boundary checks and
returning the original transactionName when no trailing portion exists.
- Around line 134-135: Cache the delimiter array used by the suffix lookup in
the surrounding method, such as a private static readonly char[] field named
QueryAndFragmentChars, and update the IndexOfAny call to reuse it instead of
allocating new[] { '?', '#' } for each request.
- Around line 87-89: Update the path prefix check in the scanner loop to avoid
constructing prefix + "/" on each iteration: retain the exact
path.Equals(prefix) match, then validate that path is longer than prefix and
that the character immediately after the prefix is '/'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0712d3e2-f4bc-4b85-bf4a-342d76087d0f
⛔ Files ignored due to path filters (1)
Tests/Resgrid.Tests/Framework/SentryTransactionFilterTests.csis excluded by!**/Tests/**
📒 Files selected for processing (4)
Core/Resgrid.Framework/SentryTransactionFilter.csWeb/Resgrid.Web.Eventing/Program.csWeb/Resgrid.Web.Services/Program.csWeb/Resgrid.Web/Program.cs
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 `@Workers/Resgrid.Workers.Console/Tasks/TtsStaticPromptRefreshTask.cs`:
- Around line 89-97: Update ReportTerminalFailure to accept and forward caller
information to Resgrid.Framework.Logging.LogException, using CallerMemberName,
CallerFilePath, and CallerLineNumber defaults so the original failure site is
preserved. Remove the _logger.LogError call entirely, leaving
Resgrid.Framework.Logging as the sole logging path.
🪄 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: 9542f905-9c8a-48a7-8239-24b4c79b3501
⛔ Files ignored due to path filters (1)
Tests/Resgrid.Tests/Workers/Console/Tasks/TtsStaticPromptRefreshTaskTests.csis excluded by!**/Tests/**
📒 Files selected for processing (1)
Workers/Resgrid.Workers.Console/Tasks/TtsStaticPromptRefreshTask.cs
| protected virtual void ReportTerminalFailure(Exception exception) | ||
| { | ||
| Resgrid.Framework.Logging.LogException( | ||
| exception, | ||
| "TtsStaticPromptRefresh::Failed to refresh static prompts"); | ||
| _logger.LogError( | ||
| exception, | ||
| "TtsStaticPromptRefresh::Failed to refresh static prompts"); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Preserve caller information and remove non-compliant logger.
Wrapping LogException inside ReportTerminalFailure obscures the actual source of the failure. LogException will automatically capture ReportTerminalFailure as the caller via its default attributes, losing the context of the original failure site (e.g., ProcessAsync). Pass the caller attributes through to preserve the original call site.
Additionally, as per coding guidelines, all logging should exclusively use Resgrid.Framework.Logging static methods. The _logger.LogError call violates this and results in duplicate logging, since LogException already emits to the logger.
🛠️ Proposed fix to pass caller info and use the standard logger
- protected virtual void ReportTerminalFailure(Exception exception)
+ protected virtual void ReportTerminalFailure(
+ Exception exception,
+ [System.Runtime.CompilerServices.CallerFilePath] string callerFilePath = "",
+ [System.Runtime.CompilerServices.CallerMemberName] string callerMemberName = "",
+ [System.Runtime.CompilerServices.CallerLineNumber] int callerLineNumber = 0)
{
Resgrid.Framework.Logging.LogException(
exception,
- "TtsStaticPromptRefresh::Failed to refresh static prompts");
- _logger.LogError(
- exception,
- "TtsStaticPromptRefresh::Failed to refresh static prompts");
+ "TtsStaticPromptRefresh::Failed to refresh static prompts",
+ callerFilePath: callerFilePath,
+ callerMemberName: callerMemberName,
+ callerLineNumber: callerLineNumber);
}📝 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.
| protected virtual void ReportTerminalFailure(Exception exception) | |
| { | |
| Resgrid.Framework.Logging.LogException( | |
| exception, | |
| "TtsStaticPromptRefresh::Failed to refresh static prompts"); | |
| _logger.LogError( | |
| exception, | |
| "TtsStaticPromptRefresh::Failed to refresh static prompts"); | |
| } | |
| protected virtual void ReportTerminalFailure( | |
| Exception exception, | |
| [System.Runtime.CompilerServices.CallerFilePath] string callerFilePath = "", | |
| [System.Runtime.CompilerServices.CallerMemberName] string callerMemberName = "", | |
| [System.Runtime.CompilerServices.CallerLineNumber] int callerLineNumber = 0) | |
| { | |
| Resgrid.Framework.Logging.LogException( | |
| exception, | |
| "TtsStaticPromptRefresh::Failed to refresh static prompts", | |
| callerFilePath: callerFilePath, | |
| callerMemberName: callerMemberName, | |
| callerLineNumber: callerLineNumber); | |
| } |
🤖 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 `@Workers/Resgrid.Workers.Console/Tasks/TtsStaticPromptRefreshTask.cs` around
lines 89 - 97, Update ReportTerminalFailure to accept and forward caller
information to Resgrid.Framework.Logging.LogException, using CallerMemberName,
CallerFilePath, and CallerLineNumber defaults so the original failure site is
preserved. Remove the _logger.LogError call entirely, leaving
Resgrid.Framework.Logging as the sole logging path.
Source: Coding guidelines
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
1 participant
Summary by CodeRabbit