Open
Conversation
rob93c
added
enhancement
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:
📝 WalkthroughWalkthroughReplaces ANSI console formatting with structured JSON logging: adds logstash encoder and jackson-core deps, introduces Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/github/stickerifier/stickerify/bot/Stickerify.java`:
- Line 82: The current binding of ScopedValue where(USER_ID,
request.getUserId()) can NPE for channel messages because request.getUserId()
may be null; before binding, check whether request.getUserId() (or the
underlying Message.from) is non-null and only call ScopedValue.where(...) when
present, otherwise call answer(request) without the USER_ID binding (or bind an
absent/optional marker if your code expects it). Update the logic around
ScopedValue.where/answer(request) to branch: if request.getUserId() != null then
ScopedValue.where(USER_ID, request.getUserId()).run(() -> answer(request)); else
just call answer(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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4f7c0585-2644-45a3-a017-41c076183fd7
📒 Files selected for processing (14)
build.gradlegradle/libs.versions.tomlsrc/main/java/com/github/stickerifier/stickerify/bot/Stickerify.javasrc/main/java/com/github/stickerifier/stickerify/logger/ExceptionHighlighter.javasrc/main/java/com/github/stickerifier/stickerify/logger/HighlightHelper.javasrc/main/java/com/github/stickerifier/stickerify/logger/MessageHighlighter.javasrc/main/java/com/github/stickerifier/stickerify/logger/StructuredLogger.javasrc/main/java/com/github/stickerifier/stickerify/media/MediaHelper.javasrc/main/java/com/github/stickerifier/stickerify/telegram/model/TelegramRequest.javasrc/main/resources/logback.xmlsrc/test/java/com/github/stickerifier/stickerify/junit/TempFilesCleanupExtension.javasrc/test/java/com/github/stickerifier/stickerify/logger/ExceptionHighlighterTest.javasrc/test/java/com/github/stickerifier/stickerify/logger/LoggingEvent.javasrc/test/java/com/github/stickerifier/stickerify/logger/MessageHighlighterTest.java
💤 Files with no reviewable changes (6)
- src/test/java/com/github/stickerifier/stickerify/logger/MessageHighlighterTest.java
- src/main/java/com/github/stickerifier/stickerify/logger/HighlightHelper.java
- src/main/java/com/github/stickerifier/stickerify/logger/ExceptionHighlighter.java
- src/test/java/com/github/stickerifier/stickerify/logger/ExceptionHighlighterTest.java
- src/test/java/com/github/stickerifier/stickerify/logger/LoggingEvent.java
- src/main/java/com/github/stickerifier/stickerify/logger/MessageHighlighter.java
src/main/java/com/github/stickerifier/stickerify/bot/Stickerify.java
Outdated
Show resolved
Hide resolved
rob93c
force-pushed
the
use-structured-logging
branch
from
7d5fc5c to
21b8daa
Compare
rob93c
force-pushed
the
use-structured-logging
branch
from
21b8daa to
888a05b
Compare
rob93c
force-pushed
the
use-structured-logging
branch
from
efa0653 to
38ef8c4
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/java/com/github/stickerifier/stickerify/media/MediaHelper.java (2)
85-88: Include the exception cause in the error log for consistency.Other error and warning logs in this file (e.g., lines 347, 501) use
.setCause(e)to capture the exception details. Without it, theIOExceptionstack trace and message are lost from the structured log output.Proposed fix
} catch (IOException e) { - LOGGER.at(Level.ERROR).addKeyValue("file_name", file.getName()).log("Unable to retrieve MIME type"); + LOGGER.at(Level.ERROR).setCause(e).addKeyValue("file_name", file.getName()).log("Unable to retrieve MIME type"); throw new MediaException(e); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/github/stickerifier/stickerify/media/MediaHelper.java` around lines 85 - 88, In the catch(IOException e) block in MediaHelper where LOGGER.at(Level.ERROR).addKeyValue("file_name", file.getName()).log("Unable to retrieve MIME type") is called, include the exception as the cause on the logger (e.g., call .setCause(e) before .log(...)) so the IOException stack/message is captured, then rethrow new MediaException(e) as before.
256-258: Consider logging the exception details before discarding.The
IOExceptionis caught and discarded (using_), but the error log doesn't capture the exception cause. This makes it harder to diagnose why the gzip content couldn't be retrieved. If the exception is intentionally swallowed, consider logging the cause or downgrading toWARNlevel.Proposed fix to capture exception details
- } catch (IOException _) { - LOGGER.at(Level.ERROR).addKeyValue("file_name", file.getName()).log("Unable to retrieve gzip content"); + } catch (IOException e) { + LOGGER.at(Level.WARN).setCause(e).addKeyValue("file_name", file.getName()).log("Unable to retrieve gzip content"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/github/stickerifier/stickerify/media/MediaHelper.java` around lines 256 - 258, The catch block in MediaHelper swallows the IOException and discards its details; change the catch from "catch (IOException _)" to "catch (IOException e)" and include the exception in the log call (e.g., use LOGGER.at(Level.ERROR).withThrowable(e).addKeyValue("file_name", file.getName()).log("Unable to retrieve gzip content")) or, if intentionally non-fatal, downgrade to WARN level while still attaching the exception; update the catch site to reference the new exception variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/com/github/stickerifier/stickerify/media/MediaHelper.java`:
- Around line 85-88: In the catch(IOException e) block in MediaHelper where
LOGGER.at(Level.ERROR).addKeyValue("file_name", file.getName()).log("Unable to
retrieve MIME type") is called, include the exception as the cause on the logger
(e.g., call .setCause(e) before .log(...)) so the IOException stack/message is
captured, then rethrow new MediaException(e) as before.
- Around line 256-258: The catch block in MediaHelper swallows the IOException
and discards its details; change the catch from "catch (IOException _)" to
"catch (IOException e)" and include the exception in the log call (e.g., use
LOGGER.at(Level.ERROR).withThrowable(e).addKeyValue("file_name",
file.getName()).log("Unable to retrieve gzip content")) or, if intentionally
non-fatal, downgrade to WARN level while still attaching the exception; update
the catch site to reference the new exception variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d4f5df97-90fa-4e34-8cf7-644c5c9bc596
📒 Files selected for processing (2)
src/main/java/com/github/stickerifier/stickerify/bot/Stickerify.javasrc/main/java/com/github/stickerifier/stickerify/media/MediaHelper.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/github/stickerifier/stickerify/bot/Stickerify.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/com/github/stickerifier/stickerify/bot/Stickerify.java (1)
88-90: Attach the throwable to the unexpected-failure log.Line 89 only records
exception_message, so you lose the exception type and stack trace exactly when diagnostics matter most.🔧 Proposed fix
- LOGGER.at(Level.ERROR).addKeyValue("exception_message", e.getMessage()).log("There was an unexpected failure"); + LOGGER.at(Level.ERROR) + .setCause(e) + .addKeyValue("exception_message", e.getMessage()) + .log("There was an unexpected failure");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/github/stickerifier/stickerify/bot/Stickerify.java` around lines 88 - 90, The error log in Stickerify.onException currently only records the exception message and omits the throwable and stack trace; update the LOGGER invocation in the onException(TelegramException e) method to attach the actual throwable (e) to the error entry so the exception type and stack trace are recorded — for example use the logger's withThrowable/withCause (or equivalent) fluent method or pass the throwable as the log call's throwable parameter when calling LOGGER.at(Level.ERROR)...log(...) so the full exception is captured rather than only addKeyValue("exception_message", ...).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/github/stickerifier/stickerify/bot/Stickerify.java`:
- Around line 172-193: processFailure short-circuits incorrectly: when an
exception is a TelegramApiException you call processTelegramFailure(...) but
then continue and may log/answer again; change the flow so handled Telegram
reply failures don't fall through — either make processTelegramFailure return a
boolean (e.g., handled) and in processFailure return early when handled, or have
processTelegramFailure throw/mark handled and then in processFailure check that
and return before the CorruptedFileException/else branches; update references to
processTelegramFailure and processFailure accordingly so cases like "message
deleted" or "user blocked the bot" do not lead to the extra
LOGGER.at(Level.WARN)... and answerText(ERROR, request) call.
---
Nitpick comments:
In `@src/main/java/com/github/stickerifier/stickerify/bot/Stickerify.java`:
- Around line 88-90: The error log in Stickerify.onException currently only
records the exception message and omits the throwable and stack trace; update
the LOGGER invocation in the onException(TelegramException e) method to attach
the actual throwable (e) to the error entry so the exception type and stack
trace are recorded — for example use the logger's withThrowable/withCause (or
equivalent) fluent method or pass the throwable as the log call's throwable
parameter when calling LOGGER.at(Level.ERROR)...log(...) so the full exception
is captured rather than only addKeyValue("exception_message", ...).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fa78104c-d6e0-4a2f-b352-5048e4a38d28
📒 Files selected for processing (2)
src/main/java/com/github/stickerifier/stickerify/bot/Stickerify.javasrc/main/java/com/github/stickerifier/stickerify/media/MediaHelper.java
rob93c
force-pushed
the
use-structured-logging
branch
from
38ef8c4 to
c78195a
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/java/com/github/stickerifier/stickerify/telegram/model/TelegramRequest.java (1)
97-120:⚠️ Potential issue | 🔴 CriticalHandle messages without
fromwhen buildingRequestDetails.
toRequestDetails()now dereferencesmessage.from().id()unconditionally. Telegram updates such as channel posts can arrive without sender data, so the new structured-log binding insrc/main/java/com/github/stickerifier/stickerify/bot/Stickerify.javaLine 80 can fail before the request is handled.💡 Proposed fix
public RequestDetails toRequestDetails() { - return new RequestDetails(getUserId(), isNewUser()); + var sender = message.from(); + return new RequestDetails(sender != null ? sender.id() : null, isNewUser()); } -public record RequestDetails(`@JsonProperty`("user_id") Long userId, `@JsonProperty`("new_user") boolean isNewUser) {} +public record RequestDetails(`@JsonProperty`("user_id") `@Nullable` Long userId, + `@JsonProperty`("new_user") boolean isNewUser) {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/github/stickerifier/stickerify/telegram/model/TelegramRequest.java` around lines 97 - 120, toRequestDetails() can throw an NPE when message.from() is absent (channel posts), so change it to avoid dereferencing message.from().id(); compute a nullable userId (e.g. Long userId = Optional.ofNullable(message).map(m -> m.from()).map(from -> from.id()).orElse(null)) and pass that into new RequestDetails(userId, isNewUser()), ensuring RequestDetails and isNewUser() accept/handle a null user id rather than assuming message.from() is always present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@src/main/java/com/github/stickerifier/stickerify/telegram/model/TelegramRequest.java`:
- Around line 97-120: toRequestDetails() can throw an NPE when message.from() is
absent (channel posts), so change it to avoid dereferencing message.from().id();
compute a nullable userId (e.g. Long userId = Optional.ofNullable(message).map(m
-> m.from()).map(from -> from.id()).orElse(null)) and pass that into new
RequestDetails(userId, isNewUser()), ensuring RequestDetails and isNewUser()
accept/handle a null user id rather than assuming message.from() is always
present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9dccd2bb-a51c-4f28-9623-973bd030d41c
📒 Files selected for processing (5)
src/main/java/com/github/stickerifier/stickerify/bot/Stickerify.javasrc/main/java/com/github/stickerifier/stickerify/logger/StructuredLogger.javasrc/main/java/com/github/stickerifier/stickerify/media/MediaHelper.javasrc/main/java/com/github/stickerifier/stickerify/telegram/model/TelegramRequest.javasrc/test/java/com/github/stickerifier/stickerify/junit/TempFilesCleanupExtension.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/java/com/github/stickerifier/stickerify/junit/TempFilesCleanupExtension.java
- src/main/java/com/github/stickerifier/stickerify/logger/StructuredLogger.java
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
Refactor
Chores