Offload discord postings to background worker #2003
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Redis-stream background jobs with server enqueue API, a background worker project (handlers for Discord and GitHub), failure/dead-letter handling, settings and logging, and updates game viewmodels, Docker/compose, env vars, and deployment docs. ChangesBackground job worker + bug reporting
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@SWLOR.Game.Server/Feature/GuiDefinition/ViewModel/BugReportViewModel.cs`:
- Line 65: Change the timestamp format used when building the dateReported
string in BugReportViewModel to use the 24-hour token and invariant culture:
replace the current format "yyyy-MM-dd hh:mm:ss" with "yyyy-MM-dd HH:mm:ss" and
pass CultureInfo.InvariantCulture into the ToString call where dateReported is
assigned so the UTC timestamp is unambiguous and culture-stable.
- Around line 87-138: SubmitBugReportToGitHub creates a new HttpClient per call
which can exhaust sockets; replace that with a shared static HttpClient
initialized once using a SocketsHttpHandler configured with
PooledConnectionLifetime (e.g. 2 minutes) and any desirable timeout, and stop
adding Authorization to DefaultRequestHeaders—instead set the Bearer token on
the individual HttpRequestMessage used for PostAsync so token rotation works;
update SubmitBugReportToGitHub to build an HttpRequestMessage (with per-request
Authorization, User-Agent and Accept headers) and call
staticHttpClient.SendAsync(request).
🪄 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: c3af811f-270c-4dcd-b11a-ccbd78031337
📒 Files selected for processing (3)
SWLOR.Game.Server/Docker/swlor.envSWLOR.Game.Server/Feature/GuiDefinition/ViewModel/BugReportViewModel.csSWLOR.Game.Server/Readmes/Deployment.md
Owner
Author
|
@codex review all comments and make fixes |
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Owner
Author
|
@codex fix the issues raised by CodeRabbit |
Summary
Testing
|
…ssages. Update existing Discord connections (bug report, broadcast, etc.) to use this via Redis.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
SWLOR.BackgroundServices/Configuration/BackgroundServiceSettings.cs (1)
63-69: 💤 Low value
GetIntsilently falls back to the default when0is explicitly configured
parsed > 0treats zero as invalid and returns the default. This means operators can't distinguish between "env var not set" and "env var set to 0" — for example, settingSWLOR_BACKGROUND_EMPTY_READ_DELAY_SECONDS=0would silently use 1 second. At minimum, consider logging a warning when a non-positive value is encountered.💡 Proposed improvement
private static int GetInt(string name, int defaultValue) { var value = Environment.GetEnvironmentVariable(name); - return int.TryParse(value, out var parsed) && parsed > 0 - ? parsed - : defaultValue; + if (string.IsNullOrWhiteSpace(value)) + return defaultValue; + if (int.TryParse(value, out var parsed) && parsed > 0) + return parsed; + // Non-positive or unparseable value — fall back to default silently. + // Consider logging a warning here if a logger is accessible. + return defaultValue; }🤖 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 `@SWLOR.BackgroundServices/Configuration/BackgroundServiceSettings.cs` around lines 63 - 69, GetInt currently rejects zero because it uses parsed > 0; change it to accept zero by returning parsed when int.TryParse succeeds and parsed >= 0, and only fall back to default when parse fails or parsed < 0. Also emit a warning (use your ILogger/processLogger) when a negative value is provided so operators know the env var was read but invalid (e.g. SWLOR_BACKGROUND_EMPTY_READ_DELAY_SECONDS set to -1). Update the GetInt method implementation and add the warning/log line inside that method when parsed < 0.
🤖 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 `@SWLOR.BackgroundServices/BackgroundJobs/BackgroundJobFailureHandler.cs`:
- Around line 43-55: MoveToDeadLetterAsync has the same non-atomic pattern as
HandleFailureAsync: it XADDs the dead-letter entry via StreamAddAsync then
separately calls AcknowledgeAsync, risking duplicate dead-letter entries if ACK
fails; replace this with an atomic Redis Lua script that performs XADD to
BackgroundJobQueueNames.DeadLetterStreamName and XACK for the original entry.Id
in one EVAL call (use the Redis scripting/ScriptEvaluateAsync entry-point used
elsewhere or add a helper like ExecuteAtomicallyAsync), then remove the separate
AcknowledgeAsync call from MoveToDeadLetterAsync; if you cannot change runtime
behavior now, add explicit documentation on MoveToDeadLetterAsync and
BackgroundJobQueueNames that the dead-letter stream is at-most-once and
consumers must dedupe/handle duplicate entries.
- Around line 28-40: The retry enqueue currently omits the stream max length and
performs XADD then AcknowledgeAsync separately, which can overflow the stream
and cause duplicate requeues if AcknowledgeAsync fails; modify the
StreamAddAsync call in BackgroundJobFailureHandler to pass the same max
length/approximate options used on initial enqueue (e.g., MaxLenApprox = 10000)
and replace the separate StreamAddAsync + AcknowledgeAsync pair with a single
Redis Lua script (EVAL) that performs XADD with MAXLEN (or ~MAXLEN) and XACK
atomically, using the same field set (type, payload, attempt, createdUtc,
lastError) and the job.Id for XACK; keep AcknowledgeAsync logic but change
BackgroundJobFailureHandler to call the new Lua-based helper so retry
enqueue+ack are atomic and bounded.
In `@SWLOR.BackgroundServices/BackgroundJobs/BackgroundJobProcessor.cs`:
- Around line 38-43: The call sequence awaiting handler.HandleAsync(...)
immediately followed by database.StreamAcknowledgeAsync(...) is non-atomic: if
StreamAcknowledgeAsync fails the job will be re-delivered and the handler (e.g.,
in HandleAsync implementations) may run twice and then be routed through
HandleFailureAsync/BackgroundJobFailureHandler incorrectly; add a clear code
comment in BackgroundJobProcessor (near the HandleAsync → StreamAcknowledgeAsync
block) stating the system provides at-least-once delivery and that handler
implementations must be idempotent (or perform their own deduplication, e.g.,
GitHub issue title existence check), and optionally suggest patterns (retry ack
logic, persistent outbox, or idempotency keys) for future refactors.
- Around line 33-36: The code currently throws InvalidOperationException when
_handlers.TryGetValue(backgroundJob.Type, out var handler) fails, which funnels
into HandleFailureAsync and causes pointless retries; instead, immediately
dead-letter the job using the same dead-letter logic used in the TryCreate
failure path (do not throw or call HandleFailureAsync). Replace the throw with a
call to the existing dead-letter routine (use the same method/logic invoked when
TryCreate fails) passing the backgroundJob and an explanatory message about
unknown job type so the entry is moved to the dead-letter stream immediately.
In `@SWLOR.BackgroundServices/BackgroundJobs/BackgroundJobWorker.cs`:
- Line 25: The code logs the raw Redis connection string via
_logger.Info($"Connecting to Redis at {_settings.RedisConnection}...") which may
expose credentials; update the BackgroundJobWorker startup log to redact
passwords by parsing _settings.RedisConnection (look for "password=" or "pwd="
key/value or a URI userinfo) and replace the value with a placeholder like
"[REDACTED]" before passing it to _logger.Info, or log only the host:port
portion; ensure you modify the logging call that references
_settings.RedisConnection so no plaintext password is emitted.
In
`@SWLOR.BackgroundServices/BackgroundJobs/Handlers/DiscordWebhookJobHandler.cs`:
- Around line 74-87: ResolveThreadName currently returns job.Title when Title is
non-empty even if CreateThread is false, causing threads to be auto-created;
update ResolveThreadName (in DiscordWebhookJobHandler) so it only returns
job.Title when job.CreateThread is true and Title is non-empty (i.e., change the
condition to require job.CreateThread && !string.IsNullOrWhiteSpace(job.Title)),
otherwise return string.Empty unless job.ThreadName is explicitly provided; this
ensures the handler's check that only creates a thread when CreateThread is true
behaves correctly.
In `@SWLOR.BackgroundServices/Dockerfile`:
- Around line 10-13: The runtime stage runs the app as root; create a non-root
user and ensure app files are owned by that user, then switch to it before
ENTRYPOINT. In the Dockerfile's runtime stage (the lines with WORKDIR /app, COPY
--from=build /app/publish ., and ENTRYPOINT ["dotnet",
"SWLOR.BackgroundServices.dll"]), add commands to create a user/group (e.g.,
appuser), chown the /app directory to that user, and set USER to that account so
the container runs the dotnet process non-root. Ensure the created UID/GID are
consistent and that ownership is applied to the copied publish files.
In `@SWLOR.BackgroundServices/Program.cs`:
- Line 22: The code calls worker.RunAsync with CancellationToken.None which
ignores SIGTERM/SIGINT and prevents graceful shutdown; replace
CancellationToken.None by a cancellation token that is signalled on process stop
(e.g., create a CancellationTokenSource and cancel it from
Console.CancelKeyPress and AppDomain.ProcessExit, or use the host's
IHostApplicationLifetime.ApplicationStopping token) and pass that token into
worker.RunAsync; locate the call to worker.RunAsync in Program.cs and change it
to use the new token (e.g., cts.Token or appLifetime.ApplicationStopping) so the
worker can exit cleanly and close Redis connections on container stop.
In `@SWLOR.Game.Server/Feature/ChatCommandDefinition/DMChatCommand.cs`:
- Around line 954-957: The call to BackgroundJob.EnqueueDiscordWebhook in
DMChatCommand currently ignores failures; capture its result and handle errors
explicitly: call BackgroundJob.EnqueueDiscordWebhook(url, authorName, message,
15105570) into a variable, check for a false/failed result or catch exceptions
around the call, and on failure emit a Serilog error (include url, authorName
and message for context) and optionally notify the DM sender or retry/fallback
as appropriate; ensure you update only the DMChatCommand code path surrounding
the enqueue so failures are not silently dropped.
In `@SWLOR.Game.Server/Service/BackgroundJob.cs`:
- Around line 51-73: Enqueue<TPayload> currently calls the blocking DB.StreamAdd
inside the NWN game thread; change it to call an asynchronous variant
(DB.StreamAddAsync) and fire-and-forget it from a background Task or dedicated
thread so the game thread never blocks, or if no async API exists, offload the
blocking call to Task.Run with a bounded timeout on the Redis client and a
cancellation token; update the Enqueue method (and any callers) to start the
background operation and immediately return, and enhance the Log.Write error
message to include the BackgroundJobType (type) plus any player/context info
available so failures are actionable (references: Enqueue<TPayload>,
DB.StreamAdd/StreamAddAsync, MaxStreamLength, useApproximateMaxLength,
Log.Write).
---
Nitpick comments:
In `@SWLOR.BackgroundServices/Configuration/BackgroundServiceSettings.cs`:
- Around line 63-69: GetInt currently rejects zero because it uses parsed > 0;
change it to accept zero by returning parsed when int.TryParse succeeds and
parsed >= 0, and only fall back to default when parse fails or parsed < 0. Also
emit a warning (use your ILogger/processLogger) when a negative value is
provided so operators know the env var was read but invalid (e.g.
SWLOR_BACKGROUND_EMPTY_READ_DELAY_SECONDS set to -1). Update the GetInt method
implementation and add the warning/log line inside that method when parsed < 0.
🪄 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: f3f72984-e94d-440d-974d-d32ea5e18124
📒 Files selected for processing (30)
SWLOR.BackgroundServices/BackgroundJobs/BackgroundJob.csSWLOR.BackgroundServices/BackgroundJobs/BackgroundJobFailureHandler.csSWLOR.BackgroundServices/BackgroundJobs/BackgroundJobProcessor.csSWLOR.BackgroundServices/BackgroundJobs/BackgroundJobQueueNames.csSWLOR.BackgroundServices/BackgroundJobs/BackgroundJobTypes.csSWLOR.BackgroundServices/BackgroundJobs/BackgroundJobWorker.csSWLOR.BackgroundServices/BackgroundJobs/Handlers/DiscordWebhookJobHandler.csSWLOR.BackgroundServices/BackgroundJobs/Handlers/GitHubIssueJobHandler.csSWLOR.BackgroundServices/BackgroundJobs/IBackgroundJobHandler.csSWLOR.BackgroundServices/BackgroundJobs/Models/DiscordWebhookPayload.csSWLOR.BackgroundServices/BackgroundJobs/Models/GitHubIssuePayload.csSWLOR.BackgroundServices/Configuration/BackgroundServiceSettings.csSWLOR.BackgroundServices/DockerfileSWLOR.BackgroundServices/Infrastructure/ConsoleAppLogger.csSWLOR.BackgroundServices/Infrastructure/IAppLogger.csSWLOR.BackgroundServices/Infrastructure/JsonHttpContent.csSWLOR.BackgroundServices/Program.csSWLOR.BackgroundServices/SWLOR.BackgroundServices.csprojSWLOR.Game.Server.slnSWLOR.Game.Server/Docker/docker-compose.ymlSWLOR.Game.Server/Docker/swlor.envSWLOR.Game.Server/Feature/ChatCommandDefinition/DMChatCommand.csSWLOR.Game.Server/Feature/GuiDefinition/ViewModel/BugReportViewModel.csSWLOR.Game.Server/Feature/GuiDefinition/ViewModel/HoloNetViewModel.csSWLOR.Game.Server/SWLOR.Game.Server.csprojSWLOR.Game.Server/Service/BackgroundJob.csSWLOR.Game.Server/Service/BackgroundJobService/BackgroundJobType.csSWLOR.Game.Server/Service/BackgroundJobService/DiscordWebhookPayload.csSWLOR.Game.Server/Service/BackgroundJobService/GitHubIssuePayload.csSWLOR.Game.Server/Service/DB.cs
💤 Files with no reviewable changes (1)
- SWLOR.Game.Server/SWLOR.Game.Server.csproj
✅ Files skipped from review due to trivial changes (9)
- SWLOR.BackgroundServices/BackgroundJobs/BackgroundJobTypes.cs
- SWLOR.BackgroundServices/BackgroundJobs/IBackgroundJobHandler.cs
- SWLOR.BackgroundServices/BackgroundJobs/BackgroundJobQueueNames.cs
- SWLOR.Game.Server/Service/BackgroundJobService/BackgroundJobType.cs
- SWLOR.BackgroundServices/Infrastructure/JsonHttpContent.cs
- SWLOR.Game.Server/Service/DB.cs
- SWLOR.BackgroundServices/SWLOR.BackgroundServices.csproj
- SWLOR.Game.Server/Service/BackgroundJobService/GitHubIssuePayload.cs
- SWLOR.BackgroundServices/BackgroundJobs/Models/GitHubIssuePayload.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- SWLOR.Game.Server/Docker/swlor.env
Owner
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 949b6319ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Owner
Author
|
@coderabbitai review now that rate limits are over |
Contributor
|
✅ Actions performedFull review triggered. |
Owner
Author
|
@coderabbitai review now that rate limits are over |
Contributor
|
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
SWLOR.Game.Server/Feature/GuiDefinition/ViewModel/BugReportViewModel.cs (1)
46-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBug reports are still routed to Discord, not GitHub Issues.
The submit path currently depends on
SWLOR_BUG_DISCORD_WEBHOOK_URLandEnqueueDiscordWebhook(...), so it does not implement the PR’s stated GitHub-issue migration behavior.💡 Suggested direction
- var discordWebhookUrl = Environment.GetEnvironmentVariable("SWLOR_BUG_DISCORD_WEBHOOK_URL"); - if (string.IsNullOrWhiteSpace(discordWebhookUrl)) { ... } + var githubRepository = Environment.GetEnvironmentVariable("SWLOR_BUG_GITHUB_REPOSITORY"); + if (string.IsNullOrWhiteSpace(githubRepository)) { ... } - if (!SubmitBugReportToDiscord(discordWebhookUrl, ...)) { ... } + if (!SubmitBugReportToGitHub(githubRepository, ...)) { ... } - return BackgroundJob.EnqueueDiscordWebhook(...); + return BackgroundJob.EnqueueGitHubIssue(githubRepository, title, body);Also applies to: 84-121
🤖 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 `@SWLOR.Game.Server/Feature/GuiDefinition/ViewModel/BugReportViewModel.cs` around lines 46 - 75, The code currently always sends bug reports via the Discord webhook check and SubmitBugReportToDiscord; update the flow to prefer creating a GitHub issue when the new GitHub submission path is available: add/check a GitHub target environment/config (e.g., a GitHub repo or token env) and call a new SubmitBugReportToGitHubIssue(...) method (or refactor existing submission code) using the same metadata (authorName, areaName, areaTag, areaResref, positionGroup, dateReported, playerId, message); if GitHub submission fails or the GitHub config is absent, then fall back to the existing SubmitBugReportToDiscord(...) path, preserving the error messages (SendMessageToPC) and rate-limiting behavior (nextReportAllowed). Do the same replacement for the secondary block referenced (lines ~84-121) so both code paths use GitHub first and Discord as fallback, and ensure any EnqueueDiscordWebhook(...) usages are only invoked when Discord is the chosen fallback.
🧹 Nitpick comments (3)
SWLOR.BackgroundServices/Infrastructure/IAppLogger.cs (1)
3-8: ⚡ Quick winConsider replacing
IAppLoggerwithSerilog.ILoggerorMicrosoft.Extensions.Logging.ILogger.Rolling a custom logger interface duplicates what both Serilog and MEL already provide (including structured logging, log levels, and a rich ecosystem of sinks). The codebase's coding guideline mandates Serilog; wiring the standard
Serilog.ILoggerthrough the worker program would satisfy that requirement without the bespoke abstraction.🤖 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 `@SWLOR.BackgroundServices/Infrastructure/IAppLogger.cs` around lines 3 - 8, The custom IAppLogger interface (IAppLogger with Info, Error(string), Error(string, Exception)) duplicates existing logging abstractions; replace usages and registration of IAppLogger with the standard Serilog.ILogger (or Microsoft.Extensions.Logging.ILogger<T>) by removing IAppLogger, updating consumers to accept/consume Serilog.ILogger (or ILogger<T>), and wiring the chosen logger in the host/worker registration so dependency injection provides the real Serilog logger instance; ensure calls currently targeting Info and Error map to the appropriate ILogger methods (e.g., Information, Error) and update any exception overload usage accordingly.SWLOR.Game.Server/Docker/docker-compose.yml (1)
38-44: Consider adding a Redis readiness health check to avoid a startup race.
depends_on: redisonly guarantees the Redis container has started, not that it is ready to accept connections. The background worker may attempt to connect before Redis is listening and fail. A health-check condition makes this robust:⚙️ Suggested health-check dependency
background-services: hostname: background-services image: zunath/swlor-background-worker:latest env_file: ${PWD-.}/swlor.env depends_on: - - redis + redis: + condition: service_healthy restart: unless-stoppedAdd a health check to the
redisservice:redis: hostname: redis image: redislabs/redismod:latest volumes: - ${PWD-.}/redis:/data restart: unless-stopped + healthcheck: + test: ["CMD", "redis-cli", "ping"] + interval: 5s + timeout: 3s + retries: 5🤖 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 `@SWLOR.Game.Server/Docker/docker-compose.yml` around lines 38 - 44, The redis service needs a healthcheck and the background-services should depend on redis being healthy; add a healthcheck block to the redis service (use a command like "redis-cli PING" and set interval, timeout and retries) and update background-services' depends_on to wait for the redis service_healthy condition so the background worker (background-services) only starts after Redis is ready to accept connections.SWLOR.BackgroundServices/BackgroundJobs/BackgroundJobProcessor.cs (1)
24-27: ⚡ Quick winAdd a log entry when malformed jobs are dead-lettered.
Line 26 dead-letters parse failures, but that path is currently silent. Add a direct error log with
entry.Idanderrorbefore returning so malformed payloads are traceable in production.Proposed change
if (!BackgroundJob.TryCreate(entry, out var job, out var error)) { await _failureHandler.MoveToDeadLetterAsync(database, entry, error); + _logger.Error($"Background job {entry.Id} dead-lettered due to malformed payload: {error}"); return; }As per coding guidelines, "Use comprehensive logging via Serilog framework throughout the codebase".
🤖 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 `@SWLOR.BackgroundServices/BackgroundJobs/BackgroundJobProcessor.cs` around lines 24 - 27, Add a Serilog error log right before returning when BackgroundJob.TryCreate fails: log the entry.Id and the parsed error so malformed payloads are traceable in production. Update the failure path in BackgroundJobProcessor where BackgroundJob.TryCreate(entry, out var job, out var error) returns false to call the logger (include entry.Id and error) immediately before invoking _failureHandler.MoveToDeadLetterAsync(database, entry, error) or at least before the return. Ensure the log uses the same logger instance used in the class and formats the message with structured properties (entry.Id and error).
🤖 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 `@SWLOR.BackgroundServices/BackgroundJobs/BackgroundJob.cs`:
- Around line 40-44: The ParseAttempt method currently returns whatever
int.TryParse yields, which may be negative; update ParseAttempt to clamp any
parsed value to a non-negative number (zero or greater) before returning—e.g.,
after parsing in ParseAttempt(string? value) ensure the returned attempt is
Math.Max(0, attempt) so negative inputs become 0 and retry accounting remains
correct.
In `@SWLOR.BackgroundServices/Infrastructure/ConsoleAppLogger.cs`:
- Around line 3-18: ConsoleAppLogger currently writes directly to the console in
Info and Error methods; replace those calls with Serilog so logs are consistent
and structured: add the Serilog NuGet and update the ConsoleAppLogger class to
call Serilog.Log.Information for Info(string message), Serilog.Log.Error for
Error(string message) and Serilog.Log.Error(exception, message) for the
Error(string message, Exception exception) overload, removing direct
Console/Console.Error usage and relying on Serilog configuration (timestamp,
sinks, enrichment) like other services that use the Log service.
In `@SWLOR.Game.Server/Service/BackgroundJob.cs`:
- Around line 64-67: The method currently fire-and-forgets the Redis write by
calling Task.Run(() => EnqueueAsync(type, entries, context)) and returns true
immediately, causing callers to get false success; change the public enqueue
methods to await the background enqueue operation and propagate its
success/failure: call and await EnqueueAsync(type, entries, context) (or change
the method signature to return Task<bool> if not already) and return the actual
boolean result from EnqueueAsync (or catch exceptions and return false after
logging), ensuring the caller only receives true when the Redis enqueue
succeeded; locate usages of Enqueue* and EnqueueAsync in BackgroundJob.cs to
update their signatures and callers accordingly.
- Around line 91-99: BuildLogContext currently includes free-form fields (Title,
Description, AuthorName) that may contain sensitive user-supplied text; update
the switch in BuildLogContext to stop logging truncated message content and only
emit non-sensitive identifiers. For DiscordWebhookPayload return a context
string containing only threadId and createThread (e.g., threadId and
CreateThread flag) and omit AuthorName/Title/Description, and for
GitHubIssuePayload return only repository (omit issue Title); adjust the
returned strings in the BuildLogContext method to remove Truncate(...) usage for
free-form fields and keep only safe identifiers.
In `@SWLOR.Game.Server/Service/BackgroundJobService/GitHubIssuePayload.cs`:
- Around line 8-14: The GitHubIssuePayload DTO's non-nullable string properties
(Repository, Title, Body) are not initialized which can cause null-state drift;
update the GitHubIssuePayload class to initialize each property (Repository,
Title, Body) to an empty string (or a sensible default) in their declarations to
ensure they are never null at runtime and avoid nullability warnings.
---
Outside diff comments:
In `@SWLOR.Game.Server/Feature/GuiDefinition/ViewModel/BugReportViewModel.cs`:
- Around line 46-75: The code currently always sends bug reports via the Discord
webhook check and SubmitBugReportToDiscord; update the flow to prefer creating a
GitHub issue when the new GitHub submission path is available: add/check a
GitHub target environment/config (e.g., a GitHub repo or token env) and call a
new SubmitBugReportToGitHubIssue(...) method (or refactor existing submission
code) using the same metadata (authorName, areaName, areaTag, areaResref,
positionGroup, dateReported, playerId, message); if GitHub submission fails or
the GitHub config is absent, then fall back to the existing
SubmitBugReportToDiscord(...) path, preserving the error messages
(SendMessageToPC) and rate-limiting behavior (nextReportAllowed). Do the same
replacement for the secondary block referenced (lines ~84-121) so both code
paths use GitHub first and Discord as fallback, and ensure any
EnqueueDiscordWebhook(...) usages are only invoked when Discord is the chosen
fallback.
---
Nitpick comments:
In `@SWLOR.BackgroundServices/BackgroundJobs/BackgroundJobProcessor.cs`:
- Around line 24-27: Add a Serilog error log right before returning when
BackgroundJob.TryCreate fails: log the entry.Id and the parsed error so
malformed payloads are traceable in production. Update the failure path in
BackgroundJobProcessor where BackgroundJob.TryCreate(entry, out var job, out var
error) returns false to call the logger (include entry.Id and error) immediately
before invoking _failureHandler.MoveToDeadLetterAsync(database, entry, error) or
at least before the return. Ensure the log uses the same logger instance used in
the class and formats the message with structured properties (entry.Id and
error).
In `@SWLOR.BackgroundServices/Infrastructure/IAppLogger.cs`:
- Around line 3-8: The custom IAppLogger interface (IAppLogger with Info,
Error(string), Error(string, Exception)) duplicates existing logging
abstractions; replace usages and registration of IAppLogger with the standard
Serilog.ILogger (or Microsoft.Extensions.Logging.ILogger<T>) by removing
IAppLogger, updating consumers to accept/consume Serilog.ILogger (or
ILogger<T>), and wiring the chosen logger in the host/worker registration so
dependency injection provides the real Serilog logger instance; ensure calls
currently targeting Info and Error map to the appropriate ILogger methods (e.g.,
Information, Error) and update any exception overload usage accordingly.
In `@SWLOR.Game.Server/Docker/docker-compose.yml`:
- Around line 38-44: The redis service needs a healthcheck and the
background-services should depend on redis being healthy; add a healthcheck
block to the redis service (use a command like "redis-cli PING" and set
interval, timeout and retries) and update background-services' depends_on to
wait for the redis service_healthy condition so the background worker
(background-services) only starts after Redis is ready to accept connections.
🪄 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: 70e15db8-7b33-4f2d-89f1-43da6d060f91
📒 Files selected for processing (31)
SWLOR.BackgroundServices/BackgroundJobs/BackgroundJob.csSWLOR.BackgroundServices/BackgroundJobs/BackgroundJobFailureHandler.csSWLOR.BackgroundServices/BackgroundJobs/BackgroundJobProcessor.csSWLOR.BackgroundServices/BackgroundJobs/BackgroundJobQueueNames.csSWLOR.BackgroundServices/BackgroundJobs/BackgroundJobTypes.csSWLOR.BackgroundServices/BackgroundJobs/BackgroundJobWorker.csSWLOR.BackgroundServices/BackgroundJobs/Handlers/DiscordWebhookJobHandler.csSWLOR.BackgroundServices/BackgroundJobs/Handlers/GitHubIssueJobHandler.csSWLOR.BackgroundServices/BackgroundJobs/IBackgroundJobHandler.csSWLOR.BackgroundServices/BackgroundJobs/Models/DiscordWebhookPayload.csSWLOR.BackgroundServices/BackgroundJobs/Models/GitHubIssuePayload.csSWLOR.BackgroundServices/Configuration/BackgroundServiceSettings.csSWLOR.BackgroundServices/DockerfileSWLOR.BackgroundServices/Infrastructure/ConsoleAppLogger.csSWLOR.BackgroundServices/Infrastructure/IAppLogger.csSWLOR.BackgroundServices/Infrastructure/JsonHttpContent.csSWLOR.BackgroundServices/Program.csSWLOR.BackgroundServices/SWLOR.BackgroundServices.csprojSWLOR.Game.Server.slnSWLOR.Game.Server/Docker/docker-compose.ymlSWLOR.Game.Server/Docker/swlor.envSWLOR.Game.Server/Feature/ChatCommandDefinition/DMChatCommand.csSWLOR.Game.Server/Feature/GuiDefinition/ViewModel/BugReportViewModel.csSWLOR.Game.Server/Feature/GuiDefinition/ViewModel/HoloNetViewModel.csSWLOR.Game.Server/Readmes/Deployment.mdSWLOR.Game.Server/SWLOR.Game.Server.csprojSWLOR.Game.Server/Service/BackgroundJob.csSWLOR.Game.Server/Service/BackgroundJobService/BackgroundJobType.csSWLOR.Game.Server/Service/BackgroundJobService/DiscordWebhookPayload.csSWLOR.Game.Server/Service/BackgroundJobService/GitHubIssuePayload.csSWLOR.Game.Server/Service/DB.cs
💤 Files with no reviewable changes (1)
- SWLOR.Game.Server/SWLOR.Game.Server.csproj
Comment on lines
+40
to
+44
| private static int ParseAttempt(string? value) | ||
| { | ||
| return int.TryParse(value, out var attempt) | ||
| ? attempt | ||
| : 0; |
Contributor
There was a problem hiding this comment.
Clamp parsed attempt to a non-negative value.
int.TryParse allows negative numbers, which can skew retry accounting.
🛠️ Proposed fix
private static int ParseAttempt(string? value)
{
- return int.TryParse(value, out var attempt)
- ? attempt
- : 0;
+ return int.TryParse(value, out var attempt) && attempt >= 0
+ ? attempt
+ : 0;
}📝 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
| private static int ParseAttempt(string? value) | |
| { | |
| return int.TryParse(value, out var attempt) | |
| ? attempt | |
| : 0; | |
| private static int ParseAttempt(string? value) | |
| { | |
| return int.TryParse(value, out var attempt) && attempt >= 0 | |
| ? attempt | |
| : 0; | |
| } |
🤖 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 `@SWLOR.BackgroundServices/BackgroundJobs/BackgroundJob.cs` around lines 40 -
44, The ParseAttempt method currently returns whatever int.TryParse yields,
which may be negative; update ParseAttempt to clamp any parsed value to a
non-negative number (zero or greater) before returning—e.g., after parsing in
ParseAttempt(string? value) ensure the returned attempt is Math.Max(0, attempt)
so negative inputs become 0 and retry accounting remains correct.
Comment on lines
+3
to
+18
| public sealed class ConsoleAppLogger : IAppLogger | ||
| { | ||
| public void Info(string message) | ||
| { | ||
| Console.WriteLine($"{DateTime.UtcNow:O} [INF] {message}"); | ||
| } | ||
|
|
||
| public void Error(string message) | ||
| { | ||
| Console.Error.WriteLine($"{DateTime.UtcNow:O} [ERR] {message}"); | ||
| } | ||
|
|
||
| public void Error(string message, Exception exception) | ||
| { | ||
| Console.Error.WriteLine($"{DateTime.UtcNow:O} [ERR] {message}{Environment.NewLine}{exception}"); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify Serilog usage and package presence before refactor
fd -e csproj | xargs -I{} sh -c 'echo "--- {} ---"; rg -n "Serilog|PackageReference.*Serilog" "{}"'
rg -n --type cs "Log\.Information|Log\.Error|ILogger|Serilog"Repository: zunath/SWLOR_NWN
Length of output: 810
🏁 Script executed:
# Check the actual content of the BackgroundServices .csproj file
cat SWLOR.BackgroundServices/SWLOR.BackgroundServices.csprojRepository: zunath/SWLOR_NWN
Length of output: 469
🏁 Script executed:
# Examine the Log.cs file in SWLOR.Game.Server to understand the logging pattern
cat SWLOR.Game.Server/Service/Log.csRepository: zunath/SWLOR_NWN
Length of output: 3588
🏁 Script executed:
# Find where ConsoleAppLogger is being used
rg -n "ConsoleAppLogger" --type csRepository: zunath/SWLOR_NWN
Length of output: 251
🏁 Script executed:
# Check the IAppLogger interface to understand its contract
fd -name "IAppLogger*" --type fRepository: zunath/SWLOR_NWN
Length of output: 291
🏁 Script executed:
# Find IAppLogger interface
rg -n "interface IAppLogger" --type csRepository: zunath/SWLOR_NWN
Length of output: 148
🏁 Script executed:
# Find all usages of IAppLogger
rg -n "IAppLogger" --type csRepository: zunath/SWLOR_NWN
Length of output: 977
🏁 Script executed:
# Check what Program.cs does with the logger
cat -n SWLOR.BackgroundServices/Program.csRepository: zunath/SWLOR_NWN
Length of output: 1367
🏁 Script executed:
# View the IAppLogger interface definition
cat SWLOR.BackgroundServices/Infrastructure/IAppLogger.csRepository: zunath/SWLOR_NWN
Length of output: 286
Replace direct console writes with Serilog-backed logging.
This implementation bypasses the project logging standard and loses structured log metadata consistency across services. The SWLOR.BackgroundServices project should add the Serilog NuGet package and update ConsoleAppLogger to use Serilog's comprehensive logging capabilities, consistent with how SWLOR.Game.Server implements logging via the Log service.
🤖 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 `@SWLOR.BackgroundServices/Infrastructure/ConsoleAppLogger.cs` around lines 3 -
18, ConsoleAppLogger currently writes directly to the console in Info and Error
methods; replace those calls with Serilog so logs are consistent and structured:
add the Serilog NuGet and update the ConsoleAppLogger class to call
Serilog.Log.Information for Info(string message), Serilog.Log.Error for
Error(string message) and Serilog.Log.Error(exception, message) for the
Error(string message, Exception exception) overload, removing direct
Console/Console.Error usage and relying on Serilog configuration (timestamp,
sinks, enrichment) like other services that use the Log service.
Comment on lines
+64
to
+67
| _ = Task.Run(() => EnqueueAsync(type, entries, context)); | ||
|
|
||
| return true; | ||
| } |
Contributor
There was a problem hiding this comment.
Enqueue* reports success before the job is actually enqueued.
Line 64 offloads Redis write to fire-and-forget, and Line 66 returns true immediately. If Redis write fails later (Line 85+), callers still think the job was accepted. That creates false-positive UX and silent loss for bug reports.
Also applies to: 75-88
🤖 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 `@SWLOR.Game.Server/Service/BackgroundJob.cs` around lines 64 - 67, The method
currently fire-and-forgets the Redis write by calling Task.Run(() =>
EnqueueAsync(type, entries, context)) and returns true immediately, causing
callers to get false success; change the public enqueue methods to await the
background enqueue operation and propagate its success/failure: call and await
EnqueueAsync(type, entries, context) (or change the method signature to return
Task<bool> if not already) and return the actual boolean result from
EnqueueAsync (or catch exceptions and return false after logging), ensuring the
caller only receives true when the Redis enqueue succeeded; locate usages of
Enqueue* and EnqueueAsync in BackgroundJob.cs to update their signatures and
callers accordingly.
Comment on lines
+91
to
+99
| private static string BuildLogContext<TPayload>(TPayload payload) | ||
| { | ||
| switch (payload) | ||
| { | ||
| case DiscordWebhookPayload discord: | ||
| return $"authorName='{Truncate(discord.AuthorName)}', title='{Truncate(discord.Title)}', description='{Truncate(discord.Description)}', threadId='{Truncate(discord.ThreadId)}', createThread='{discord.CreateThread}'"; | ||
| case GitHubIssuePayload gitHub: | ||
| return $"repository='{Truncate(gitHub.Repository)}', title='{Truncate(gitHub.Title)}'"; | ||
| default: |
Contributor
There was a problem hiding this comment.
Failure logs are including user-supplied message content.
BuildLogContext logs truncated description/title. Those fields can contain sensitive data from bug reports and end up in error logs. Remove free-form content from log context and keep only non-sensitive identifiers.
Proposed change
case DiscordWebhookPayload discord:
- return $"authorName='{Truncate(discord.AuthorName)}', title='{Truncate(discord.Title)}', description='{Truncate(discord.Description)}', threadId='{Truncate(discord.ThreadId)}', createThread='{discord.CreateThread}'";
+ return $"authorName='{Truncate(discord.AuthorName)}', threadId='{Truncate(discord.ThreadId)}', createThread='{discord.CreateThread}'";
case GitHubIssuePayload gitHub:
- return $"repository='{Truncate(gitHub.Repository)}', title='{Truncate(gitHub.Title)}'";
+ return $"repository='{Truncate(gitHub.Repository)}'";Also applies to: 70-70, 87-87
🤖 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 `@SWLOR.Game.Server/Service/BackgroundJob.cs` around lines 91 - 99,
BuildLogContext currently includes free-form fields (Title, Description,
AuthorName) that may contain sensitive user-supplied text; update the switch in
BuildLogContext to stop logging truncated message content and only emit
non-sensitive identifiers. For DiscordWebhookPayload return a context string
containing only threadId and createThread (e.g., threadId and CreateThread flag)
and omit AuthorName/Title/Description, and for GitHubIssuePayload return only
repository (omit issue Title); adjust the returned strings in the
BuildLogContext method to remove Truncate(...) usage for free-form fields and
keep only safe identifiers.
Comment on lines
+8
to
+14
| public string Repository { get; set; } | ||
|
|
||
| [JsonProperty("title")] | ||
| public string Title { get; set; } | ||
|
|
||
| [JsonProperty("body")] | ||
| public string Body { get; set; } |
Contributor
There was a problem hiding this comment.
Initialize non-nullable DTO strings to prevent null-state drift.
Repository, Title, and Body are non-nullable but currently uninitialized. This can surface nullability warnings and null values at runtime on partial/malformed payloads.
Proposed fix
public class GitHubIssuePayload
{
[JsonProperty("repository")]
- public string Repository { get; set; }
+ public string Repository { get; set; } = string.Empty;
[JsonProperty("title")]
- public string Title { get; set; }
+ public string Title { get; set; } = string.Empty;
[JsonProperty("body")]
- public string Body { get; set; }
+ public string Body { get; set; } = string.Empty;
}Based on learnings: "In C# files within the SWLOR.Game.Server and SWLOR.NWN.API projects, avoid using acronyms in variable names; prefer descriptive names..."
📝 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 string Repository { get; set; } | |
| [JsonProperty("title")] | |
| public string Title { get; set; } | |
| [JsonProperty("body")] | |
| public string Body { get; set; } | |
| public string Repository { get; set; } = string.Empty; | |
| [JsonProperty("title")] | |
| public string Title { get; set; } = string.Empty; | |
| [JsonProperty("body")] | |
| public string Body { get; set; } = string.Empty; |
🤖 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 `@SWLOR.Game.Server/Service/BackgroundJobService/GitHubIssuePayload.cs` around
lines 8 - 14, The GitHubIssuePayload DTO's non-nullable string properties
(Repository, Title, Body) are not initialized which can cause null-state drift;
update the GitHubIssuePayload class to initialize each property (Repository,
Title, Body) to an empty string (or a sensible default) in their declarations to
ensure they are never null at runtime and avoid nullability warnings.
zunath
changed the title
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
Updates
New Features
Documentation