Integrating the push notification system into all parts of the system#24
Open
Satlykovs wants to merge 17 commits into
Open
Integrating the push notification system into all parts of the system#24Satlykovs wants to merge 17 commits into
Satlykovs wants to merge 17 commits into
Conversation
|
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 analysis-finished event DTOs and ports, updates analyzer to publish events and handle fatal errors, creates Kafka publisher and topic, adds a notification service consuming events and sending FCM pushes, and wires device API/DB plus client FCM integration. ChangesEvent-Driven Analysis and Notification
Sequence Diagram (high-level) sequenceDiagram
participant Analyzer as SmartjamAnalyzer
participant Kafka as KafkaBroker
participant Notif as smartjam-notification (AnalysisResultListener)
participant DB as RecipientDB (RecipientPersistenceAdapter)
participant FCM as Firebase (FcmPushAdapter)
Analyzer->>Kafka: publish AnalysisFinishedEvent(topic=analysis-results, key=targetId)
Kafka->>Notif: deliver AnalysisFinishedEvent
Notif->>DB: findOwnerId(targetId,type) / findFcmToken(userId)
Notif->>FCM: sendPush(fcmToken, message)
Estimated code review effort: 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs:
Suggested reviewers:
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 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 @.gitignore:
- Line 7: Update the .gitignore entry so the credentials file is ignored
anywhere in the repo by replacing the literal filename pattern
"firebase-adminsdk.json" with the glob "**/firebase-adminsdk.json"; ensure the
resource name referenced by FirebaseConfig (firebase-adminsdk.json) remains
unchanged, and if the credentials file was already committed, remove it from
tracking (e.g., git rm --cached <path-to-file>) and commit the .gitignore update
so the file is no longer tracked.
In
`@backend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/infrastructure/messaging/KafkaAnalysisEventPublisher.java`:
- Around line 18-20: The publish method in KafkaAnalysisEventPublisher currently
calls kafkaTemplate.send(TOPIC, event.targetId().toString(), event) and drops
the returned CompletableFuture, so broker/serialization failures are ignored;
update publish to attach a completion callback (e.g., whenComplete/handle or
addCallback on the returned future) that logs any exception and relevant context
(TOPIC, event.targetId()) and optionally retries or propagates the error as
needed so send failures are not silently swallowed.
In
`@backend/smartjam-common/src/main/java/com/smartjam/common/dto/analysis/AnalysisFinishedEvent.java`:
- Around line 8-10: Add a canonical compact constructor to the
AnalysisFinishedEvent record to validate invariants: ensure targetId, type and
status are non-null (throw NPE/IllegalArgumentException), require totalScore
non-null when status == COMPLETED and require errorMessage non-null when status
== FAILED, and enforce that totalScore is null for non-COMPLETED statuses and
errorMessage is null for non-FAILED statuses; put this validation inside the
AnalysisFinishedEvent(...) canonical constructor so the `@Builder` and normal
constructors both get the checks and throw descriptive
IllegalArgumentException/NPE when invariants fail.
In
`@backend/smartjam-notification/src/main/java/com/smartjam/notification/application/ProcessAnalysisResultUseCase.java`:
- Around line 43-45: The catch block in ProcessAnalysisResultUseCase currently
logs and swallows all Exceptions (log.error("Failed to send push notification
for {}: {}", event.targetId(), e.getMessage())), which causes the Kafka listener
to ack messages and lose retries; change the handler so that after logging you
propagate the failure (rethrow the caught exception or wrap it in a
runtime/processing exception) instead of suppressing it, ensuring the Kafka
listener will see the exception and allow message retry/ DLQ handling — update
the catch in the method that calls event.targetId()/sends push notifications to
log the error with context and then throw the exception onward.
In
`@backend/smartjam-notification/src/main/java/com/smartjam/notification/infrastructure/fcm/DebugLoggingPushAdapter.java`:
- Line 14: Replace the direct FCM token logging in DebugLoggingPushAdapter (the
log.info call that prints fcmToken) with a masked token: add a private
maskToken(String token) helper (as suggested) that returns "***" for null/short
tokens and otherwise returns token.substring(0,6) + "***" +
token.substring(token.length()-4), then call log.info with maskToken(fcmToken)
instead of the raw fcmToken so only the masked fragment appears in debug logs.
In
`@backend/smartjam-notification/src/main/java/com/smartjam/notification/infrastructure/fcm/FcmPushAdapter.java`:
- Line 34: The log in FcmPushAdapter currently prints the full fcmToken; replace
that with a masked token by adding a private helper (e.g., maskToken(String
token)) and call it in the error log (use the masked value instead of fcmToken),
ensuring it returns a safe placeholder for null/short tokens; update the
log.error invocation in FcmPushAdapter to use maskToken(fcmToken) so only a
partially redacted token is logged.
- Around line 33-35: The catch in FcmPushAdapter currently swallows exceptions;
update the PushPublisher contract to propagate failures by introducing a custom
checked exception (e.g., PushPublishException) and declare it on the
PushPublisher method signature, then change FcmPushAdapter's method (the one
containing the catch) to rethrow a PushPublishException after logging the error
(include fcmToken and original exception); alternatively, if you prefer a
non-exception API, change PushPublisher's return type to a boolean or Result<T>
and return a failure result from FcmPushAdapter instead—ensure you modify both
the PushPublisher interface and FcmPushAdapter implementation accordingly so
callers can perform retries/DLT/alerting.
In
`@backend/smartjam-notification/src/main/java/com/smartjam/notification/infrastructure/fcm/FirebaseConfig.java`:
- Around line 23-27: In FirebaseConfig's `@PostConstruct` initialization method
(the block that reads serviceAccount), stop silently returning on missing
serviceAccount; instead, if the app is not running in a debug profile, throw an
IllegalStateException with a clear message so Spring fails fast; apply the same
change to the later initialization/failure check (the block around the
initialization that currently logs and returns). Use the
Environment/ActiveProfiles check (or the existing profile flag in
FirebaseConfig) to detect debug mode, and throw IllegalStateException
referencing serviceAccount and the FirebaseConfig init method so startup fails
in non-debug environments.
In `@backend/smartjam-notification/src/main/resources/application.yaml`:
- Around line 2-3: Remove the hard-coded production profile and plaintext DB
credentials in application.yaml: delete or comment out the profiles.active: prod
entry and replace the literal spring.datasource.username /
spring.datasource.password values with environment-backed placeholders (e.g. use
${SPRING_DATASOURCE_USERNAME} and ${SPRING_DATASOURCE_PASSWORD} or secret/value
references) so profile selection is driven by SPRING_PROFILES_ACTIVE at runtime
and credentials are injected from env/secret management rather than committed
defaults.
- Line 27: Replace the open wildcard for Spring JSON deserialization by setting
spring.json.trusted.packages to the concrete DTO package(s) used for messaging
(e.g., com.smartjam.common.dto or com.smartjam.common.dto.analysis) instead of
"*"; update the application.yaml property spring.json.trusted.packages
accordingly so only the packages containing AnalysisFinishedEvent and related
DTOs are trusted for deserialization.
In
`@backend/smartjam-notification/src/test/java/com/smartjam/notification/SmartjamNotificationApplicationTests.java`:
- Around line 6-10: The context-load integration test (class
SmartjamNotificationApplicationTests with method contextLoads and the
`@SpringBootTest` annotation) currently runs with production defaults and may hit
real infra; annotate the test class to activate a dedicated test profile (e.g.,
add `@ActiveProfiles`("test") or set spring.profiles.active="test" on
`@SpringBootTest`) and ensure you have a test profile configuration that provides
in-memory or mocked beans for Postgres/Kafka/Firebase so the test runs
deterministically in CI.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 50f7c44b-20e6-4a24-8368-f4ac9bc7e2a9
📒 Files selected for processing (65)
.gitignorebackend/compose-messaging.yamlbackend/settings.gradlebackend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/SmartjamAnalyzerApplication.javabackend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/api/kafka/S3StorageListener.javabackend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/application/AudioAnalysisUseCase.javabackend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/domain/exception/AnalysisFatalException.javabackend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/domain/model/AnalysisResult.javabackend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/domain/model/FeatureSequence.javabackend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/domain/port/AnalysisEventPublisher.javabackend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/domain/port/AudioConverter.javabackend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/domain/port/AudioStorage.javabackend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/domain/port/DebugVisualizer.javabackend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/domain/port/FeatureExtractor.javabackend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/domain/port/PerformanceEvaluator.javabackend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/domain/port/ReferenceRepository.javabackend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/domain/port/ResultRepository.javabackend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/domain/port/Workspace.javabackend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/domain/port/WorkspaceFactory.javabackend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/domain/service/DtwPerformanceEvaluator.javabackend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/infrastructure/analysis/DspProperties.javabackend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/infrastructure/analysis/DtwConfig.javabackend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/infrastructure/analysis/TarsosFeatureExtractor.javabackend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/infrastructure/converter/FFmpegConfig.javabackend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/infrastructure/converter/FfmpegAudioConverter.javabackend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/infrastructure/messaging/KafkaAnalysisEventPublisher.javabackend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/infrastructure/persistence/adapter/AssignmentPersistenceAdapter.javabackend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/infrastructure/persistence/adapter/SubmissionPersistenceAdapter.javabackend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/infrastructure/persistence/entity/AssignmentEntity.javabackend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/infrastructure/persistence/entity/SubmissionEntity.javabackend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/infrastructure/persistence/repository/JpaAssignmentRepository.javabackend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/infrastructure/persistence/repository/JpaSubmissionRepository.javabackend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/infrastructure/storage/MinioAudioStorage.javabackend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/infrastructure/storage/MinioConfig.javabackend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/infrastructure/utils/FeatureBinarySerializer.javabackend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/infrastructure/utils/FsWorkspace.javabackend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/infrastructure/utils/FsWorkspaceFactory.javabackend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/infrastructure/visualizer/ImageIoDebugVisualizer.javabackend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/infrastructure/visualizer/NoOpDebugVisualizer.javabackend/smartjam-analyzer/src/main/java/com/smartjam/smartjamanalyzer/domain/port/PerformanceEvaluator.javabackend/smartjam-analyzer/src/test/java/com/smartjam/analyzer/SmartjamAnalyzerApplicationTests.javabackend/smartjam-analyzer/src/test/java/com/smartjam/analyzer/api/listener/S3StorageListenerTest.javabackend/smartjam-analyzer/src/test/java/com/smartjam/analyzer/application/AudioAnalysisIntegrationTest.javabackend/smartjam-analyzer/src/test/java/com/smartjam/analyzer/application/AudioAnalysisUseCaseTest.javabackend/smartjam-analyzer/src/test/java/com/smartjam/analyzer/domain/service/DtwPerformanceEvaluatorTest.javabackend/smartjam-analyzer/src/test/java/com/smartjam/analyzer/infrastructure/converter/FfmpegAudioConverterTest.javabackend/smartjam-analyzer/src/test/java/com/smartjam/analyzer/infrastructure/storage/MinioAudioStorageTest.javabackend/smartjam-analyzer/src/test/java/com/smartjam/analyzer/infrastructure/utils/FeatureBinarySerializerTest.javabackend/smartjam-analyzer/src/test/java/com/smartjam/analyzer/infrastructure/utils/FsWorkspaceTest.javabackend/smartjam-common/src/main/java/com/smartjam/common/dto/analysis/AnalysisFinishedEvent.javabackend/smartjam-common/src/main/java/com/smartjam/common/dto/analysis/AnalysisType.javabackend/smartjam-common/src/main/java/com/smartjam/common/dto/s3/S3Event.javabackend/smartjam-common/src/test/java/common/dto/s3/S3EventTest.javabackend/smartjam-notification/build.gradlebackend/smartjam-notification/src/main/java/com/smartjam/notification/SmartjamNotificationApplication.javabackend/smartjam-notification/src/main/java/com/smartjam/notification/api/kafka/AnalysisResultListener.javabackend/smartjam-notification/src/main/java/com/smartjam/notification/application/ProcessAnalysisResultUseCase.javabackend/smartjam-notification/src/main/java/com/smartjam/notification/domain/port/PushPublisher.javabackend/smartjam-notification/src/main/java/com/smartjam/notification/domain/port/RecipientResolver.javabackend/smartjam-notification/src/main/java/com/smartjam/notification/infrastructure/fcm/DebugLoggingPushAdapter.javabackend/smartjam-notification/src/main/java/com/smartjam/notification/infrastructure/fcm/FcmPushAdapter.javabackend/smartjam-notification/src/main/java/com/smartjam/notification/infrastructure/fcm/FirebaseConfig.javabackend/smartjam-notification/src/main/java/com/smartjam/notification/infrastructure/persistence/adapter/RecipientPersistenceAdapter.javabackend/smartjam-notification/src/main/resources/application.yamlbackend/smartjam-notification/src/test/java/com/smartjam/notification/SmartjamNotificationApplicationTests.java
💤 Files with no reviewable changes (1)
- backend/smartjam-analyzer/src/main/java/com/smartjam/smartjamanalyzer/domain/port/PerformanceEvaluator.java
Comment on lines
+18
to
+20
| public void publish(AnalysisFinishedEvent event) { | ||
| kafkaTemplate.send(TOPIC, event.targetId().toString(), event); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "KafkaAnalysisEventPublisher.java" | head -5Repository: Satlykovs/SmartJam
Length of output: 184
🏁 Script executed:
cat -n backend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/infrastructure/messaging/KafkaAnalysisEventPublisher.javaRepository: Satlykovs/SmartJam
Length of output: 938
🏁 Script executed:
# Search for other KafkaTemplate usage in the codebase
rg "kafkaTemplate\.send" --type java -A 2 -B 2Repository: Satlykovs/SmartJam
Length of output: 810
🏁 Script executed:
# Search for error handling patterns with Kafka in the codebase
rg "whenComplete|addCallback|get\(\)" --type java -A 2 -B 2 | head -50Repository: Satlykovs/SmartJam
Length of output: 44
🏁 Script executed:
# Look for other event publishers or listeners to understand error handling patterns
rg "EventPublisher|Listener" --type java -l | head -10Repository: Satlykovs/SmartJam
Length of output: 1087
🏁 Script executed:
# Check how the publish method is called
rg "\.publish\(" --type java -B 3 -A 3Repository: Satlykovs/SmartJam
Length of output: 3250
🏁 Script executed:
# Search for error handling, logging, or exception handling patterns
rg "log\.|catch|throws|RuntimeException" --type java backend/smartjam-analyzer/src/main/java/ | head -30Repository: Satlykovs/SmartJam
Length of output: 5345
🏁 Script executed:
# Get more context on how publish is called
cat -n backend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/application/AudioAnalysisUseCase.java | head -100Repository: Satlykovs/SmartJam
Length of output: 4670
🏁 Script executed:
# Check the AnalysisEventPublisher interface definition
cat -n backend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/domain/port/AnalysisEventPublisher.javaRepository: Satlykovs/SmartJam
Length of output: 310
🏁 Script executed:
# Look for how AnalysisResultListener handles events (consumer side)
cat -n backend/smartjam-notification/src/main/java/com/smartjam/notification/api/kafka/AnalysisResultListener.javaRepository: Satlykovs/SmartJam
Length of output: 1714
Add error handling to Kafka send operation instead of ignoring the async result.
At line 19, kafkaTemplate.send() returns a CompletableFuture that is ignored, causing broker and serialization failures to be silently dropped. Add a callback to log failures:
Suggested fix
import com.smartjam.analyzer.domain.port.AnalysisEventPublisher;
import com.smartjam.common.dto.analysis.AnalysisFinishedEvent;
import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
import org.springframework.kafka.core.KafkaTemplate;
import org.springframework.stereotype.Component;
`@Component`
+@Slf4j
`@RequiredArgsConstructor`
public class KafkaAnalysisEventPublisher implements AnalysisEventPublisher {
@@
`@Override`
public void publish(AnalysisFinishedEvent event) {
- kafkaTemplate.send(TOPIC, event.targetId().toString(), event);
+ kafkaTemplate.send(TOPIC, event.targetId().toString(), event)
+ .whenComplete((result, ex) -> {
+ if (ex != null) {
+ log.error("Failed to publish analysis event for {}", event.targetId(), ex);
+ }
+ });
}
}📝 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 void publish(AnalysisFinishedEvent event) { | |
| kafkaTemplate.send(TOPIC, event.targetId().toString(), event); | |
| } | |
| import com.smartjam.analyzer.domain.port.AnalysisEventPublisher; | |
| import com.smartjam.common.dto.analysis.AnalysisFinishedEvent; | |
| import lombok.RequiredArgsConstructor; | |
| import lombok.extern.slf4j.Slf4j; | |
| import org.springframework.kafka.core.KafkaTemplate; | |
| import org.springframework.stereotype.Component; | |
| `@Component` | |
| `@Slf4j` | |
| `@RequiredArgsConstructor` | |
| public class KafkaAnalysisEventPublisher implements AnalysisEventPublisher { | |
| private static final String TOPIC = "analysis-events"; | |
| private final KafkaTemplate<String, AnalysisFinishedEvent> kafkaTemplate; | |
| `@Override` | |
| public void publish(AnalysisFinishedEvent event) { | |
| kafkaTemplate.send(TOPIC, event.targetId().toString(), event) | |
| .whenComplete((result, ex) -> { | |
| if (ex != null) { | |
| log.error("Failed to publish analysis event for {}", event.targetId(), ex); | |
| } | |
| }); | |
| } | |
| } |
🤖 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
`@backend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/infrastructure/messaging/KafkaAnalysisEventPublisher.java`
around lines 18 - 20, The publish method in KafkaAnalysisEventPublisher
currently calls kafkaTemplate.send(TOPIC, event.targetId().toString(), event)
and drops the returned CompletableFuture, so broker/serialization failures are
ignored; update publish to attach a completion callback (e.g.,
whenComplete/handle or addCallback on the returned future) that logs any
exception and relevant context (TOPIC, event.targetId()) and optionally retries
or propagates the error as needed so send failures are not silently swallowed.
Comment on lines
+8
to
+10
| @Builder | ||
| public record AnalysisFinishedEvent( | ||
| UUID targetId, AnalysisType type, AudioProcessingStatus status, Double totalScore, String errorMessage) {} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Enforce event payload invariants in the record constructor.
@Builder currently allows inconsistent event states. On Line 10, add validation so required fields are always present and status agrees with totalScore/errorMessage.
Proposed fix
`@Builder`
public record AnalysisFinishedEvent(
- UUID targetId, AnalysisType type, AudioProcessingStatus status, Double totalScore, String errorMessage) {}
+ UUID targetId, AnalysisType type, AudioProcessingStatus status, Double totalScore, String errorMessage) {
+ public AnalysisFinishedEvent {
+ if (targetId == null || type == null || status == null) {
+ throw new IllegalArgumentException("targetId, type, and status are required");
+ }
+ if (status == AudioProcessingStatus.COMPLETED && totalScore == null) {
+ throw new IllegalArgumentException("totalScore is required for COMPLETED status");
+ }
+ if (status == AudioProcessingStatus.FAILED && (errorMessage == null || errorMessage.isBlank())) {
+ throw new IllegalArgumentException("errorMessage is required for FAILED status");
+ }
+ }
+}📝 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
| @Builder | |
| public record AnalysisFinishedEvent( | |
| UUID targetId, AnalysisType type, AudioProcessingStatus status, Double totalScore, String errorMessage) {} | |
| `@Builder` | |
| public record AnalysisFinishedEvent( | |
| UUID targetId, AnalysisType type, AudioProcessingStatus status, Double totalScore, String errorMessage) { | |
| public AnalysisFinishedEvent { | |
| if (targetId == null || type == null || status == null) { | |
| throw new IllegalArgumentException("targetId, type, and status are required"); | |
| } | |
| if (status == AudioProcessingStatus.COMPLETED && totalScore == null) { | |
| throw new IllegalArgumentException("totalScore is required for COMPLETED status"); | |
| } | |
| if (status == AudioProcessingStatus.FAILED && (errorMessage == null || errorMessage.isBlank())) { | |
| throw new IllegalArgumentException("errorMessage is required for FAILED status"); | |
| } | |
| } | |
| } |
🤖 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
`@backend/smartjam-common/src/main/java/com/smartjam/common/dto/analysis/AnalysisFinishedEvent.java`
around lines 8 - 10, Add a canonical compact constructor to the
AnalysisFinishedEvent record to validate invariants: ensure targetId, type and
status are non-null (throw NPE/IllegalArgumentException), require totalScore
non-null when status == COMPLETED and require errorMessage non-null when status
== FAILED, and enforce that totalScore is null for non-COMPLETED statuses and
errorMessage is null for non-FAILED statuses; put this validation inside the
AnalysisFinishedEvent(...) canonical constructor so the `@Builder` and normal
constructors both get the checks and throw descriptive
IllegalArgumentException/NPE when invariants fail.
Comment on lines
+43
to
+45
| } catch (Exception e) { | ||
| log.error("Failed to send push notification for {}: {}", event.targetId(), e.getMessage()); | ||
| } |
There was a problem hiding this comment.
Do not swallow processing exceptions here.
Lines 43-45 log and suppress errors, so the Kafka listener treats processing as successful and acknowledges the message. That silently loses notifications on transient failures.
Suggested fix
} catch (Exception e) {
- log.error("Failed to send push notification for {}: {}", event.targetId(), e.getMessage());
+ log.error("Failed to send push notification for {}", event.targetId(), e);
+ throw new IllegalStateException("Push notification processing failed", e);
}🤖 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
`@backend/smartjam-notification/src/main/java/com/smartjam/notification/application/ProcessAnalysisResultUseCase.java`
around lines 43 - 45, The catch block in ProcessAnalysisResultUseCase currently
logs and swallows all Exceptions (log.error("Failed to send push notification
for {}: {}", event.targetId(), e.getMessage())), which causes the Kafka listener
to ack messages and lose retries; change the handler so that after logging you
propagate the failure (rethrow the caught exception or wrap it in a
runtime/processing exception) instead of suppressing it, ensuring the Kafka
listener will see the exception and allow message retry/ DLQ handling — update
the catch in the method that calls event.targetId()/sends push notifications to
log the error with context and then throw the exception onward.
Comment on lines
+23
to
+27
| if (serviceAccount == null) { | ||
| log.warn("firebase-adminsdk.json file not found. Push notifications will not " + "work!"); | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "FirebaseConfig.java" -type fRepository: Satlykovs/SmartJam
Length of output: 173
🏁 Script executed:
cat -n ./backend/smartjam-notification/src/main/java/com/smartjam/notification/infrastructure/fcm/FirebaseConfig.javaRepository: Satlykovs/SmartJam
Length of output: 1697
🏁 Script executed:
rg -t java "FirebaseConfig|FirebaseApp" --max-count 20Repository: Satlykovs/SmartJam
Length of output: 671
🏁 Script executed:
find ./backend/smartjam-notification -name "*.java" -type f | head -20Repository: Satlykovs/SmartJam
Length of output: 1205
🏁 Script executed:
cat -n ./backend/smartjam-notification/src/main/java/com/smartjam/notification/infrastructure/fcm/FcmPushAdapter.javaRepository: Satlykovs/SmartJam
Length of output: 1587
🏁 Script executed:
cat -n ./backend/smartjam-notification/src/main/java/com/smartjam/notification/infrastructure/fcm/DebugLoggingPushAdapter.javaRepository: Satlykovs/SmartJam
Length of output: 685
🏁 Script executed:
cat -n ./backend/smartjam-notification/src/main/java/com/smartjam/notification/SmartjamNotificationApplication.javaRepository: Satlykovs/SmartJam
Length of output: 503
Fail fast when Firebase initialization fails in non-debug profiles.
The current code silently degrades when firebase-adminsdk.json is missing or initialization fails, deferring failure to runtime when push notifications are attempted. In non-debug profiles (production/staging), this configuration error should stop startup immediately via @PostConstruct.
Throw IllegalStateException instead of logging warnings and returning, allowing Spring to fail the application context during startup rather than leaving the app in a broken state.
Suggested fix
if (serviceAccount == null) {
- log.warn("firebase-adminsdk.json file not found. Push notifications will not " + "work!");
-
- return;
+ throw new IllegalStateException("firebase-adminsdk.json not found for !debug profile");
}
@@
} catch (Exception e) {
- log.error("Error during Firebase initialization: {}", e.getMessage(), e);
+ throw new IllegalStateException("Error during Firebase initialization", e);
}Also applies to: 37-39
🤖 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
`@backend/smartjam-notification/src/main/java/com/smartjam/notification/infrastructure/fcm/FirebaseConfig.java`
around lines 23 - 27, In FirebaseConfig's `@PostConstruct` initialization method
(the block that reads serviceAccount), stop silently returning on missing
serviceAccount; instead, if the app is not running in a debug profile, throw an
IllegalStateException with a clear message so Spring fails fast; apply the same
change to the later initialization/failure check (the block around the
initialization that currently logs and returns). Use the
Environment/ActiveProfiles check (or the existing profile flag in
FirebaseConfig) to detect debug mode, and throw IllegalStateException
referencing serviceAccount and the FirebaseConfig init method so startup fails
in non-debug environments.
Comment on lines
+2
to
+3
| profiles: | ||
| active: prod |
There was a problem hiding this comment.
Remove forced prod profile and plaintext DB credentials from committed defaults.
Line 3 forces prod for every startup, and Lines 13-14 commit credentials directly. This is risky for local/test environments and leaks secrets. Move profile activation to runtime (SPRING_PROFILES_ACTIVE) and read DB credentials from environment/secret storage.
Suggested config adjustment
spring:
- profiles:
- active: prod
application:
name: smartjam-notification
@@
datasource:
url: jdbc:postgresql://localhost:5432/smartjam
- username: admin
- password: admin
+ username: ${DB_USERNAME}
+ password: ${DB_PASSWORD}Also applies to: 11-15
🤖 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 `@backend/smartjam-notification/src/main/resources/application.yaml` around
lines 2 - 3, Remove the hard-coded production profile and plaintext DB
credentials in application.yaml: delete or comment out the profiles.active: prod
entry and replace the literal spring.datasource.username /
spring.datasource.password values with environment-backed placeholders (e.g. use
${SPRING_DATASOURCE_USERNAME} and ${SPRING_DATASOURCE_PASSWORD} or secret/value
references) so profile selection is driven by SPRING_PROFILES_ACTIVE at runtime
and credentials are injected from env/secret management rather than committed
defaults.
| key-deserializer: org.apache.kafka.common.serialization.StringDeserializer | ||
| value-deserializer: org.springframework.kafka.support.serializer.JacksonJsonDeserializer | ||
| properties: | ||
| spring.json.trusted.packages: "*" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Spring Kafka JsonDeserializer trusted.packages security recommendation, and whether using "*" is discouraged in production.
💡 Result:
The spring.json.trusted.packages configuration in Spring Kafka is a critical security feature designed to prevent arbitrary class deserialization [1][2][3]. When using the JsonDeserializer, Spring Kafka relies on type information (often provided in message headers) to determine which Java class to instantiate [2]. Without restrictions, a malicious actor could manipulate these headers to force the application to deserialize unexpected or malicious classes present on the classpath, potentially leading to remote code execution (RCE) or other deserialization-based attacks [1][2]. Using "" for spring.json.trusted.packages is strongly discouraged in production environments [4]. Setting this value to "" effectively disables the security protection by trusting all classes on the classpath for deserialization [5][6]. This exposes the application to the very vulnerabilities the feature was designed to mitigate [1][2]. Security Recommendations: 1. Explicitly define trusted packages: Only include the specific packages that contain the data transfer objects (DTOs) or classes you expect to deserialize [3][7]. 2. Use the principle of least privilege: By limiting the scope of trusted classes, you significantly reduce the attack surface [1]. 3. Avoid "" in production: Reserve the use of "" only for isolated development or testing scenarios where security risks are managed and understood [4]. 4. Keep dependencies updated: Ensure you are using recent versions of spring-kafka to benefit from security patches, such as those addressing CVE-2023-34040, which involved deserialization vulnerabilities [8][9]. While trusted packages provide an important layer of defense, they are not a complete solution against all threats, such as header spoofing [1]. Always ensure that your Kafka infrastructure is secured and that you trust the sources publishing to your topics [1][4].
Citations:
- 1: https://www.baeldung.com/spring-kafka-trusted-packages-feature
- 2: https://stackoverflow.com/questions/75573979/reason-behind-spring-json-trusted-packages
- 3: http://homedutech.com/faq/java/spring-kafka--the-class-is-not-in-the-trusted-packages.html
- 4: https://stackoverflow.com/questions/74001130/is-it-a-problem-if-i-set-spring-json-trusted-packages-while-consuming-data
- 5: How to properly configure `spring.json.trusted.packages` for Kafka Streams? spring-projects/spring-kafka#2193
- 6: https://stackoverflow.com/questions/71623576/how-to-configure-spring-json-trusted-packages-for-kafka-streams-spring
- 7: https://codingtechroom.com/tutorial/java-spring-kafka-trusted-packages-feature
- 8: https://security.snyk.io/vuln/SNYK-JAVA-ORGSPRINGFRAMEWORKKAFKA-5855818
- 9: https://github.com/advisories/GHSA-crqf-q9fp-hwjw/dependabot
🏁 Script executed:
# First, locate and read the application.yaml file
cat -n backend/smartjam-notification/src/main/resources/application.yaml | head -50Repository: Satlykovs/SmartJam
Length of output: 1017
🏁 Script executed:
# Search for DTO classes in the smartjam-notification module
find backend/smartjam-notification -type f \( -name "*.java" -o -name "*.kt" \) | head -20Repository: Satlykovs/SmartJam
Length of output: 1185
🏁 Script executed:
# Search for Kafka consumer configurations and DTO package references
rg -i "dto|message|deserializ" backend/smartjam-notification/src --type java -A 3 -B 1Repository: Satlykovs/SmartJam
Length of output: 12688
Restrict spring.json.trusted.packages to concrete DTO packages instead of using "*".
Line 27 uses "*" which weakens deserialization safety by trusting all classes on the classpath. This enables potential remote code execution through malicious class instantiation via message headers. Restrict this to the specific packages containing your DTOs. Based on the code, com.smartjam.common.dto (or more specifically com.smartjam.common.dto.analysis) should be sufficient for the AnalysisFinishedEvent deserialization.
🤖 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 `@backend/smartjam-notification/src/main/resources/application.yaml` at line
27, Replace the open wildcard for Spring JSON deserialization by setting
spring.json.trusted.packages to the concrete DTO package(s) used for messaging
(e.g., com.smartjam.common.dto or com.smartjam.common.dto.analysis) instead of
"*"; update the application.yaml property spring.json.trusted.packages
accordingly so only the packages containing AnalysisFinishedEvent and related
DTOs are trusted for deserialization.
Comment on lines
+6
to
+10
| @SpringBootTest | ||
| class SmartjamNotificationApplicationTests { | ||
|
|
||
| @Test | ||
| void contextLoads() {} |
There was a problem hiding this comment.
Isolate context test from production profile/infrastructure.
With Line 6 using plain @SpringBootTest and default config activating prod, this test can require real Postgres/Kafka/Firebase and fail nondeterministically in CI. Add a dedicated test profile for context-load tests.
Suggested test annotation change
import org.junit.jupiter.api.Test;
import org.springframework.boot.test.context.SpringBootTest;
+import org.springframework.test.context.ActiveProfiles;
`@SpringBootTest`
+@ActiveProfiles("test")
class SmartjamNotificationApplicationTests {🤖 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
`@backend/smartjam-notification/src/test/java/com/smartjam/notification/SmartjamNotificationApplicationTests.java`
around lines 6 - 10, The context-load integration test (class
SmartjamNotificationApplicationTests with method contextLoads and the
`@SpringBootTest` annotation) currently runs with production defaults and may hit
real infra; annotate the test class to activate a dedicated test profile (e.g.,
add `@ActiveProfiles`("test") or set spring.profiles.active="test" on
`@SpringBootTest`) and ensure you have a test profile configuration that provides
in-memory or mocked beans for Postgres/Kafka/Firebase so the test runs
deterministically in CI.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
backend/smartjam-notification/src/main/java/com/smartjam/notification/application/ProcessAnalysisResultUseCase.java (1)
47-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not suppress notification delivery failures.
The catch block logs and swallows errors, so the caller cannot react to delivery failures.
Suggested fix
} catch (Exception e) { - log.error("Failed to send push notification for {}: {}", event.targetId(), e.getMessage()); + log.error("Failed to send push notification for {}", event.targetId(), e); + throw new IllegalStateException("Push notification processing failed", e); }🤖 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 `@backend/smartjam-notification/src/main/java/com/smartjam/notification/application/ProcessAnalysisResultUseCase.java` around lines 47 - 49, The catch in ProcessAnalysisResultUseCase currently logs and swallows exceptions when sending notifications (catch(Exception e) { log.error(...); }), preventing callers from handling delivery failures; update the catch to not suppress the error by rethrowing it (either rethrow the caught Exception or wrap it in a domain-specific unchecked/checked exception such as NotificationDeliveryException) after logging so the caller can react; ensure the method signature permits propagation (add a throws clause if using a checked wrapper) and reference the existing log and event.targetId() usage when implementing the change.backend/smartjam-notification/src/main/java/com/smartjam/notification/infrastructure/fcm/FcmPushAdapter.java (1)
37-42:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate push-send errors instead of logging-only handling.
This catch block currently absorbs send failures, making delivery errors invisible to upstream orchestration.
Suggested fix
} catch (Exception e) { @@ String maskedToken = (fcmToken != null && fcmToken.length() > 10) ? fcmToken.substring(0, 10) + "..." : "***"; log.error("Firebase cloud messaging error for token {}: {}", maskedToken, e.getMessage(), e); + throw new IllegalStateException("FCM push sending failed", e); }🤖 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 `@backend/smartjam-notification/src/main/java/com/smartjam/notification/infrastructure/fcm/FcmPushAdapter.java` around lines 37 - 42, The catch in FcmPushAdapter that currently logs the maskedToken and swallows exceptions must propagate push-send failures to callers; update the catch in the sending method (the block that builds maskedToken) to rethrow the original exception or wrap it in a descriptive unchecked exception (e.g., PushSendException) instead of only logging, or alter the method signature to return/emit a failure result so upstream orchestration can react; preserve the existing log call but then throw the exception (or return an error object) so delivery errors are not silently absorbed.
🤖 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
`@backend/smartjam-notification/src/main/java/com/smartjam/notification/application/ProcessAnalysisResultUseCase.java`:
- Around line 32-50: Change the current status check so the use case also
handles failed analyses (not only AudioProcessingStatus.COMPLETED) by expanding
the conditional to include AudioProcessingStatus.FAILED; inside the try block
generate a different message for the FAILED case (e.g., a failure notification
that includes any available failure details from the event) while keeping the
existing submission vs non-submission branching for COMPLETED, then call
pushPublisher.sendPush(token, message) as before and retain the catch that logs
failures; update references around event.status(), AudioProcessingStatus.FAILED,
event.type(), AnalysisType.SUBMISSION, and pushPublisher.sendPush(token,
message).
In
`@backend/smartjam-notification/src/main/java/com/smartjam/notification/infrastructure/fcm/FirebaseConfig.java`:
- Around line 25-35: The code in FirebaseConfig leaves InputStream
serviceAccount open; wrap the resource acquisition in a try-with-resources block
around getResourceAsStream("firebase-adminsdk.json") so the stream is always
closed (e.g., try (InputStream serviceAccount = ...) { ... }), and pass that
stream into GoogleCredentials.fromStream() inside the block; additionally
consider switching the call to ServiceAccountCredentials.fromStream() where
appropriate to avoid the deprecated GoogleCredentials.fromStream() usage. Ensure
this change is applied inside the FirebaseConfig initialization method so the
stream is closed on all paths.
---
Duplicate comments:
In
`@backend/smartjam-notification/src/main/java/com/smartjam/notification/application/ProcessAnalysisResultUseCase.java`:
- Around line 47-49: The catch in ProcessAnalysisResultUseCase currently logs
and swallows exceptions when sending notifications (catch(Exception e) {
log.error(...); }), preventing callers from handling delivery failures; update
the catch to not suppress the error by rethrowing it (either rethrow the caught
Exception or wrap it in a domain-specific unchecked/checked exception such as
NotificationDeliveryException) after logging so the caller can react; ensure the
method signature permits propagation (add a throws clause if using a checked
wrapper) and reference the existing log and event.targetId() usage when
implementing the change.
In
`@backend/smartjam-notification/src/main/java/com/smartjam/notification/infrastructure/fcm/FcmPushAdapter.java`:
- Around line 37-42: The catch in FcmPushAdapter that currently logs the
maskedToken and swallows exceptions must propagate push-send failures to
callers; update the catch in the sending method (the block that builds
maskedToken) to rethrow the original exception or wrap it in a descriptive
unchecked exception (e.g., PushSendException) instead of only logging, or alter
the method signature to return/emit a failure result so upstream orchestration
can react; preserve the existing log call but then throw the exception (or
return an error object) so delivery errors are not silently absorbed.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 0f9bdcec-4142-49ae-8532-30f4f8526517
📒 Files selected for processing (9)
backend/smartjam-analyzer/src/main/java/com/smartjam/analyzer/domain/exception/AnalysisFatalException.javabackend/smartjam-notification/src/main/java/com/smartjam/notification/api/kafka/AnalysisResultListener.javabackend/smartjam-notification/src/main/java/com/smartjam/notification/application/ProcessAnalysisResultUseCase.javabackend/smartjam-notification/src/main/java/com/smartjam/notification/domain/port/PushPublisher.javabackend/smartjam-notification/src/main/java/com/smartjam/notification/domain/port/RecipientResolver.javabackend/smartjam-notification/src/main/java/com/smartjam/notification/infrastructure/fcm/DebugLoggingPushAdapter.javabackend/smartjam-notification/src/main/java/com/smartjam/notification/infrastructure/fcm/FcmPushAdapter.javabackend/smartjam-notification/src/main/java/com/smartjam/notification/infrastructure/fcm/FirebaseConfig.javabackend/smartjam-notification/src/main/java/com/smartjam/notification/infrastructure/persistence/adapter/RecipientPersistenceAdapter.java
Comment on lines
+32
to
+50
| if (event.status() == AudioProcessingStatus.COMPLETED) { | ||
| try { | ||
| UUID userId = recipientResolver.findOwnerId(event.targetId(), event.type()); | ||
|
|
||
| String token = recipientResolver.findFcmToken(userId); | ||
|
|
||
| if (token == null || token.isEmpty()) { | ||
| log.warn("User {} does not have fcm token, push will not be sent", userId); | ||
| return; | ||
| } | ||
|
|
||
| String message = (event.type() == AnalysisType.SUBMISSION) | ||
| ? "Твоя игра " + "проанализирована! Балл: " + event.totalScore() | ||
| : "Твоя запись " + "обработана!"; | ||
| pushPublisher.sendPush(token, message); | ||
| } catch (Exception e) { | ||
| log.error("Failed to send push notification for {}: {}", event.targetId(), e.getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle FAILED analysis results, not only COMPLETED.
Right now notifications are skipped for non-COMPLETED statuses, so users won’t receive feedback for failed analysis results.
Suggested fix
- if (event.status() == AudioProcessingStatus.COMPLETED) {
+ if (event.status() == AudioProcessingStatus.COMPLETED
+ || event.status() == AudioProcessingStatus.FAILED) {
try {
UUID userId = recipientResolver.findOwnerId(event.targetId(), event.type());
@@
- String message = (event.type() == AnalysisType.SUBMISSION)
- ? "Твоя игра " + "проанализирована! Балл: " + event.totalScore()
- : "Твоя запись " + "обработана!";
+ String message;
+ if (event.status() == AudioProcessingStatus.FAILED) {
+ message = "Не удалось обработать аудио. Попробуй снова позже.";
+ } else {
+ message = (event.type() == AnalysisType.SUBMISSION)
+ ? "Твоя игра проанализирована! Балл: " + event.totalScore()
+ : "Твоя запись обработана!";
+ }
pushPublisher.sendPush(token, message);📝 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
| if (event.status() == AudioProcessingStatus.COMPLETED) { | |
| try { | |
| UUID userId = recipientResolver.findOwnerId(event.targetId(), event.type()); | |
| String token = recipientResolver.findFcmToken(userId); | |
| if (token == null || token.isEmpty()) { | |
| log.warn("User {} does not have fcm token, push will not be sent", userId); | |
| return; | |
| } | |
| String message = (event.type() == AnalysisType.SUBMISSION) | |
| ? "Твоя игра " + "проанализирована! Балл: " + event.totalScore() | |
| : "Твоя запись " + "обработана!"; | |
| pushPublisher.sendPush(token, message); | |
| } catch (Exception e) { | |
| log.error("Failed to send push notification for {}: {}", event.targetId(), e.getMessage()); | |
| } | |
| } | |
| if (event.status() == AudioProcessingStatus.COMPLETED | |
| || event.status() == AudioProcessingStatus.FAILED) { | |
| try { | |
| UUID userId = recipientResolver.findOwnerId(event.targetId(), event.type()); | |
| String token = recipientResolver.findFcmToken(userId); | |
| if (token == null || token.isEmpty()) { | |
| log.warn("User {} does not have fcm token, push will not be sent", userId); | |
| return; | |
| } | |
| String message; | |
| if (event.status() == AudioProcessingStatus.FAILED) { | |
| message = "Не удалось обработать аудио. Попробуй снова позже."; | |
| } else { | |
| message = (event.type() == AnalysisType.SUBMISSION) | |
| ? "Твоя игра проанализирована! Балл: " + event.totalScore() | |
| : "Твоя запись обработана!"; | |
| } | |
| pushPublisher.sendPush(token, message); | |
| } catch (Exception e) { | |
| log.error("Failed to send push notification for {}: {}", event.targetId(), e.getMessage()); | |
| } | |
| } |
🤖 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
`@backend/smartjam-notification/src/main/java/com/smartjam/notification/application/ProcessAnalysisResultUseCase.java`
around lines 32 - 50, Change the current status check so the use case also
handles failed analyses (not only AudioProcessingStatus.COMPLETED) by expanding
the conditional to include AudioProcessingStatus.FAILED; inside the try block
generate a different message for the FAILED case (e.g., a failure notification
that includes any available failure details from the event) while keeping the
existing submission vs non-submission branching for COMPLETED, then call
pushPublisher.sendPush(token, message) as before and retain the catch that logs
failures; update references around event.status(), AudioProcessingStatus.FAILED,
event.type(), AnalysisType.SUBMISSION, and pushPublisher.sendPush(token,
message).
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
backend/smartjam-notification/src/test/java/com/smartjam/notification/SmartjamNotificationApplicationTests.java (1)
7-8:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not disable the only context-load smoke test.
Disabling this class removes CI coverage for wiring/config regressions. Keep it enabled with a dedicated
testprofile and test-only infra config.Minimal annotation fix
-import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.test.context.ActiveProfiles; -@Disabled("TODO: Enable after configuring Testcontainers for Postgres/Kafka in CI/CD") `@SpringBootTest` +@ActiveProfiles("test") class SmartjamNotificationApplicationTests {🤖 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 `@backend/smartjam-notification/src/test/java/com/smartjam/notification/SmartjamNotificationApplicationTests.java` around lines 7 - 8, The test class SmartjamNotificationApplicationTests is incorrectly disabled via the `@Disabled` annotation; remove that annotation and re-enable the context-load smoke test, and annotate the class to use a test-specific configuration (e.g., add `@ActiveProfiles`("test") or equivalent test profile annotation and/or test-only Testcontainers setup annotations such as `@Testcontainers`) so the test runs in CI with dedicated test infra for Postgres/Kafka rather than being globally suppressed.
🤖 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
`@backend/smartjam-common/src/main/resources/db/changelog/changes/04-add-user-devices.sql`:
- Around line 4-12: The migration drops users.fcm_token and will lose existing
tokens; instead create user_devices first, then backfill tokens from users into
user_devices and only then drop the users.fcm_token column. Concretely: create
the user_devices table (as shown), then INSERT INTO user_devices (fcm_token,
user_id, device_type, updated_at) SELECT fcm_token, id, 'ANDROID', NOW() FROM
users WHERE fcm_token IS NOT NULL ON CONFLICT DO NOTHING (or use WHERE NOT
EXISTS to avoid PK conflicts), and finally ALTER TABLE users DROP COLUMN IF
EXISTS fcm_token; perform all steps in a transaction so failures roll back;
references: users.fcm_token, users.id, user_devices(fcm_token, user_id,
device_type, updated_at).
In `@openapi-spec/api.yaml`:
- Around line 173-175: Remove the consecutive blank lines in the OpenAPI YAML so
there are no extra empty lines between entries/sections; collapse multiple blank
lines down to a single newline and re-run the YAML linter to ensure the file
(openapi spec) passes without the excessive empty-line error.
- Around line 818-826: The DeviceRegistrationRequest schema currently only marks
token as required but allows empty strings; update the DeviceRegistrationRequest
-> properties -> token schema to enforce non-empty values (e.g., add minLength:
1 or an appropriate regex pattern) so the OpenAPI contract rejects empty tokens;
modify the token property inside DeviceRegistrationRequest to include this
constraint and keep the example/description unchanged.
---
Duplicate comments:
In
`@backend/smartjam-notification/src/test/java/com/smartjam/notification/SmartjamNotificationApplicationTests.java`:
- Around line 7-8: The test class SmartjamNotificationApplicationTests is
incorrectly disabled via the `@Disabled` annotation; remove that annotation and
re-enable the context-load smoke test, and annotate the class to use a
test-specific configuration (e.g., add `@ActiveProfiles`("test") or equivalent
test profile annotation and/or test-only Testcontainers setup annotations such
as `@Testcontainers`) so the test runs in CI with dedicated test infra for
Postgres/Kafka rather than being globally suppressed.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: e5c1d618-a9f0-41d7-aac5-e14c1a4c7ca6
📒 Files selected for processing (5)
backend/smartjam-analyzer/src/test/java/com/smartjam/analyzer/application/AudioAnalysisUseCaseTest.javabackend/smartjam-common/src/main/resources/db/changelog/changes/04-add-user-devices.sqlbackend/smartjam-common/src/main/resources/db/changelog/db.changelog-master.yamlbackend/smartjam-notification/src/test/java/com/smartjam/notification/SmartjamNotificationApplicationTests.javaopenapi-spec/api.yaml
Comment on lines
+4
to
+12
| ALTER TABLE users DROP COLUMN IF EXISTS fcm_token; | ||
|
|
||
| -- changeset sanjar:18 | ||
| CREATE TABLE user_devices ( | ||
| fcm_token VARCHAR(255) PRIMARY KEY, | ||
| user_id UUID NOT NULL REFERENCES users(id) ON DELETE CASCADE, | ||
| device_type VARCHAR(50) DEFAULT 'ANDROID', | ||
| updated_at TIMESTAMPTZ DEFAULT NOW() | ||
| ); |
There was a problem hiding this comment.
Preserve existing tokens before dropping users.fcm_token.
This migration drops current token data without backfilling into user_devices, so existing users will silently lose push delivery after deploy.
Suggested migration order
-- liquibase formatted sql
-- changeset sanjar:17
-ALTER TABLE users DROP COLUMN IF EXISTS fcm_token;
-
--- changeset sanjar:18
CREATE TABLE user_devices (
fcm_token VARCHAR(255) PRIMARY KEY,
user_id UUID NOT NULL REFERENCES users(id) ON DELETE CASCADE,
device_type VARCHAR(50) DEFAULT 'ANDROID',
updated_at TIMESTAMPTZ DEFAULT NOW()
);
--- changeset sanjar:19
+-- changeset sanjar:18
+INSERT INTO user_devices (fcm_token, user_id, device_type, updated_at)
+SELECT fcm_token, id, 'ANDROID', NOW()
+FROM users
+WHERE fcm_token IS NOT NULL
+ON CONFLICT (fcm_token) DO NOTHING;
+
+-- changeset sanjar:19
+ALTER TABLE users DROP COLUMN IF EXISTS fcm_token;
+
+-- changeset sanjar:20
CREATE INDEX idx_user_devices_user_id ON user_devices(user_id);🤖 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
`@backend/smartjam-common/src/main/resources/db/changelog/changes/04-add-user-devices.sql`
around lines 4 - 12, The migration drops users.fcm_token and will lose existing
tokens; instead create user_devices first, then backfill tokens from users into
user_devices and only then drop the users.fcm_token column. Concretely: create
the user_devices table (as shown), then INSERT INTO user_devices (fcm_token,
user_id, device_type, updated_at) SELECT fcm_token, id, 'ANDROID', NOW() FROM
users WHERE fcm_token IS NOT NULL ON CONFLICT DO NOTHING (or use WHERE NOT
EXISTS to avoid PK conflicts), and finally ALTER TABLE users DROP COLUMN IF
EXISTS fcm_token; perform all steps in a transaction so failures roll back;
references: users.fcm_token, users.id, user_devices(fcm_token, user_id,
device_type, updated_at).
Comment on lines
+173
to
+175
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Remove extra blank lines to satisfy YAML lint.
There are more consecutive empty lines than allowed by the linter.
🧰 Tools
🪛 YAMLlint (1.38.0)
[error] 175-175: too many blank lines (4 > 2)
(empty-lines)
🤖 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 `@openapi-spec/api.yaml` around lines 173 - 175, Remove the consecutive blank
lines in the OpenAPI YAML so there are no extra empty lines between
entries/sections; collapse multiple blank lines down to a single newline and
re-run the YAML linter to ensure the file (openapi spec) passes without the
excessive empty-line error.
Satlykovs
added
Moblie
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
`@backend/smartjam-api/src/main/java/com/smartjam/smartjamapi/service/DeviceService.java`:
- Around line 29-36: Device tokens saved by DeviceService.register (DeviceEntity
via deviceRepository) are not used by the notification path because
RecipientPersistenceAdapter (and ProcessAnalysisResultUseCase token resolution)
still reads users.fcm_token; update the system so both paths align: either
modify DeviceService.register to also persist the token into the Users table
(update users.fcm_token via the User repository or UserService after saving
DeviceEntity) or change RecipientPersistenceAdapter to resolve tokens from the
user_devices table (join/read DeviceEntity records) so
ProcessAnalysisResultUseCase picks up registered tokens; pick one approach,
implement the corresponding repository/service update, and ensure unique
identifiers (DeviceService.register, DeviceEntity, deviceRepository,
RecipientPersistenceAdapter, ProcessAnalysisResultUseCase) are consistently used
and tested.
- Around line 29-47: Reject blank or whitespace-only fcmToken values in
DeviceService.register and DeviceService.unregister by validating the input at
the start of each method (e.g., if fcmToken is null or
fcmToken.trim().isEmpty()) and throwing an IllegalArgumentException (or other
appropriate runtime exception) rather than proceeding to repository operations;
also trim the token (use the trimmed value when building DeviceEntity in
register and when calling deviceRepository.deleteByFcmTokenAndUserId in
unregister) so stored/looked-up tokens are normalized.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 7f9d72f3-514d-4a5f-9d2f-64e9bb8d40ba
📒 Files selected for processing (6)
backend/smartjam-api/src/main/java/com/smartjam/smartjamapi/controller/DevicesController.javabackend/smartjam-api/src/main/java/com/smartjam/smartjamapi/entity/DeviceEntity.javabackend/smartjam-api/src/main/java/com/smartjam/smartjamapi/enums/DeviceType.javabackend/smartjam-api/src/main/java/com/smartjam/smartjamapi/repository/DeviceRepository.javabackend/smartjam-api/src/main/java/com/smartjam/smartjamapi/service/DeviceService.javaopenapi-spec/api.yaml
Satlykovs
changed the title
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
.github/workflows/mobile-ci.yml (1)
13-15:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSet explicit least-privilege
permissionsfor the workflow/job.This workflow relies on default token permissions; define a minimal
permissions:block at workflow or job level to reduce blast radius.🤖 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 @.github/workflows/mobile-ci.yml around lines 13 - 15, Add an explicit least-privilege permissions block to the workflow (or at the job level under jobs -> build-android) instead of relying on defaults; identify the exact tokens your Android build needs (e.g., read access to repo contents and only the specific write scopes required such as packages or id-token if you perform publishing/oidc) and add a minimal permissions: mapping (for example contents: read and any required specific scopes) under the build-android job so the workflow no longer uses broad default token permissions.mobile/app/src/main/java/com/smartjam/app/domain/repository/RoomRepository.kt (1)
1-2:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove this empty Kotlin file or add the intended repository declaration.
This file currently has no package and no symbols, which creates avoidable static-analysis noise and confusion.
🤖 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 `@mobile/app/src/main/java/com/smartjam/app/domain/repository/RoomRepository.kt` around lines 1 - 2, This file is empty and should either be removed or contain the intended repository declaration; fix by either deleting RoomRepository.kt or adding a proper package declaration (package com.smartjam.app.domain.repository) and the repository symbol (e.g., interface RoomRepository or class RoomRepositoryImpl) with its expected methods/signatures so static analysis stops flagging an empty file; ensure the file defines the RoomRepository symbol and appropriate imports/visibility.mobile/app/src/main/java/com/smartjam/app/ui/screens/room/RoomScreen.kt (1)
1-2:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix broken Room navigation / empty Room screen implementation
RoomScreen.kt(andRoomViewModel.kt) are empty, butNavGraph.ktstill definesScreen.Room("room_screen") and navigates to it fromHomeScreen—there is nocomposable(route = Screen.Room.route)destination, so navigation will be broken at runtime.Restore/implement the
RoomScreen(+RoomViewModel) and add the missingcomposable(route = Screen.Room.route)wiring inNavGraph.kt, or removeScreen.Roomand theonNavigateToRoomnavigation if the Room screen is not intended.🤖 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 `@mobile/app/src/main/java/com/smartjam/app/ui/screens/room/RoomScreen.kt` around lines 1 - 2, Nav navigation is broken because Screen.Room ("room_screen") is declared and HomeScreen calls onNavigateToRoom but there is no composable(route = Screen.Room.route) destination and RoomScreen/RoomViewModel are empty; either implement RoomScreen and RoomViewModel and add a composable(route = Screen.Room.route) entry in NavGraph (wiring in a NavHost destination that constructs RoomViewModel and calls RoomScreen) or remove Screen.Room and all calls to onNavigateToRoom in HomeScreen and NavGraph; update references to use the existing Screen sealed class and ensure NavGraph's NavHost provides the destination that calls RoomScreen(route = Screen.Room.route) and obtains RoomViewModel (e.g., via hiltViewModel() or provided factory).mobile/app/src/main/java/com/smartjam/app/ui/screens/room/RoomViewModel.kt (1)
1-2:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove this empty source file or restore the missing ViewModel implementation.
Line 1 leaves a Kotlin file with no declarations in the main source set. This is usually an incomplete commit and should be deleted or populated.
🤖 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 `@mobile/app/src/main/java/com/smartjam/app/ui/screens/room/RoomViewModel.kt` around lines 1 - 2, The file RoomViewModel.kt is empty and must be removed or restored; either delete the empty RoomViewModel.kt from the main source set, or re-create the missing ViewModel by adding a class named RoomViewModel that extends androidx.lifecycle.ViewModel (or your project's BaseViewModel), implement the expected public members used by the UI (e.g., LiveData/StateFlow properties and methods like loadRoom(), joinRoom(), leaveRoom() referenced by RoomFragment/RoomActivity), and ensure the file is placed in the same package as the consuming UI so the DI/instantiation code can find RoomViewModel.mobile/app/src/main/java/com/smartjam/app/ui/screens/register/RegisterViewModel.kt (1)
96-111:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle exceptions around
register()and always clear loading state.Lines 96-111 assume
authRepository.register(...)never throws. If it throws,isLoadingis never reset and the coroutine fails without surfacing a stable UI error.Suggested resilient flow
- viewModelScope.launch { - _state.update { it.copy(isLoading = true, errorMessage = null) } - - val result = - authRepository.register( - email = currentState.emailInput, - password = currentState.passwordInput, - username = currentState.usernameInput, - role = currentState.selectedRole, - ) - - _state.update { it.copy(isLoading = false) } - - if (result.isSuccess) { - eventChannel.send(RegisterEvent.NavigateToHome) - } else { - val error = result.exceptionOrNull()?.message ?: "Ошибка регистрации" - _state.update { it.copy(errorMessage = error) } - } - } + viewModelScope.launch { + _state.update { it.copy(isLoading = true, errorMessage = null) } + try { + val result = + authRepository.register( + email = currentState.emailInput, + password = currentState.passwordInput, + username = currentState.usernameInput, + role = currentState.selectedRole, + ) + + if (result.isSuccess) { + eventChannel.send(RegisterEvent.NavigateToHome) + } else { + val error = result.exceptionOrNull()?.message ?: "Ошибка регистрации" + _state.update { it.copy(errorMessage = error) } + } + } catch (e: Exception) { + _state.update { it.copy(errorMessage = e.message ?: "Ошибка регистрации") } + } finally { + _state.update { it.copy(isLoading = false) } + } + }🤖 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 `@mobile/app/src/main/java/com/smartjam/app/ui/screens/register/RegisterViewModel.kt` around lines 96 - 111, In RegisterViewModel wrap the call to authRepository.register(...) in a try/catch/finally so any thrown exception is caught and converted into a user-facing error (set via _state.update { it.copy(errorMessage = ...) }) and ensure _state.update { it.copy(isLoading = false) } runs in finally; keep the existing success path that calls eventChannel.send(RegisterEvent.NavigateToHome) on result.isSuccess and the existing failure path that reads result.exceptionOrNull() when no exception was thrown.
🤖 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 @.github/workflows/mobile-ci.yml:
- Line 25: Replace the mutable reference "uses: actions/setup-java@v5" with a
pinned commit SHA for the actions/setup-java action; locate the workflow step
that contains the literal string uses: actions/setup-java@v5 and update it to
uses: actions/setup-java@<full-commit-sha> (obtain the latest stable commit SHA
from the actions/setup-java repository and replace v5 with that full SHA).
- Around line 24-27: The CI workflow uses actions/setup-java@v5 with
java-version: '25' which is not an AGP-supported JDK; change the java-version
value to '17' (update the step named "Setup Java 25" to set java-version: '17')
so the workflow uses the AGP/Gradle-compatible JDK, or alternatively gate this
change with explicit compatibility checks for your AGP and Gradle versions
before keeping '25'.
In `@mobile/app/src/main/java/com/smartjam/app/data/api/FcmPushService.kt`:
- Line 53: The Log.d call in FcmPushService that logs the raw notification body
(Log.d("SmartjJam_FCM", "Push notification received: $body")) must be changed to
avoid writing PII/content to logs; update the onMessageReceived handler in
FcmPushService to log only metadata (e.g., message ID, sender, timestamp) or a
fixed "payload received" message, or gate detailed logging behind a secure debug
flag and/or sanitize/truncate any content before logging, ensuring the literal
$body is never emitted to logs.
- Around line 26-45: FcmPushService.onNewToken and the call to
devicesApi.registerDevice(DeviceRegistrationRequest) need a guaranteed retry
path: persist the FCM token (or a "pendingPushToken" flag) to local storage when
registration fails and schedule a reliable retry (e.g., WorkManager/RetryWorker)
that attempts devicesApi.registerDevice until it succeeds (also ensure
AuthRepository.register/login triggers the same retry logic rather than only
logging failures); additionally, remove any logging of
message.notification?.body in FcmPushService.onMessageReceived and replace with
non-sensitive logs (e.g., log only notification presence or message ID) to avoid
logging notification bodies.
In `@mobile/app/src/main/java/com/smartjam/app/data/local/Converters.kt`:
- Around line 27-32: The catch in Converters.fromURI is too broad; replace
catching Exception with catching IllegalArgumentException so only
URI.create(String) parse errors are swallowed. In the fromURI function (which
uses URI.create(value) inside value?.let { ... }), change the catch clause to
catch IllegalArgumentException and keep the existing Log.e call and null return
behavior intact.
In `@mobile/app/src/main/java/com/smartjam/app/data/local/TokenStorage.kt`:
- Around line 13-16: The code currently exposes Context.dataStore created via
preferencesDataStore and is persisting raw access/refresh tokens (lines around
the current token save/read methods); change this to store encrypted values:
create a MasterKey (AndroidX Security Crypto) and either (a) switch to
EncryptedSharedPreferences for the auth keys or (b) encrypt token strings with
AES-GCM using the MasterKey before writing into the existing DataStore and
decrypt on read. Update the save/persist methods that write the access/refresh
tokens to call the encrypt routine and update the retrieval methods to decrypt,
and keep Context.dataStore/preferencesDataStore usage only for encrypted blobs
or non-sensitive prefs.
In
`@mobile/app/src/main/java/com/smartjam/app/domain/repository/ConnectionRepository.kt`:
- Around line 84-85: The respondToRequest function currently always returns
Result.success(Unit) without using connectionId or accept; replace the stub with
a real implementation in ConnectionRepository.respondToRequest that calls the
appropriate remote data-source / API method to accept/reject the connection
using the connectionId and accept flag, handles HTTP/errors with try/catch,
updates any local cache or DAO state as needed, and returns Result.success(Unit)
on success or Result.failure(exception) on error (propagate meaningful error
messages to the caller).
In `@mobile/app/src/main/java/com/smartjam/app/MainActivity.kt`:
- Line 73: MainActivity currently blocks the main thread in onCreate by calling
runBlocking to read tokenStorage.accessToken.first(); change this to a
non-blocking coroutine launched on the Main lifecycle scope (e.g., use
lifecycleScope.launch or lifecycleScope.launchWhenCreated) and perform
tokenStorage.accessToken.first() inside that coroutine, then update UI or
proceed with initialization after the token is received; ensure you remove
runBlocking and any blocking behavior so onCreate returns immediately and any UI
updates from the coroutine run on the main thread.
- Around line 40-46: The code currently calls
ActivityCompat.requestPermissions(...) for
Manifest.permission.POST_NOTIFICATIONS on every launch; instead, before calling
requestPermissions in MainActivity, check the current grant status using
ContextCompat.checkSelfPermission(this, Manifest.permission.POST_NOTIFICATIONS)
and only call ActivityCompat.requestPermissions(...) when that check does not
equal PackageManager.PERMISSION_GRANTED (keep the existing Build.VERSION.SDK_INT
>= Build.VERSION_CODES.TIRAMISU guard). This change prevents asking for
POST_NOTIFICATIONS if already granted while preserving the API 33+ conditional.
In `@mobile/app/src/main/java/com/smartjam/app/ui/screens/login/LoginScreen.kt`:
- Around line 130-134: The GoldenStringsButton is currently always clickable and
can trigger viewModel.onLoginClicked() repeatedly while state.isLoading is true;
update the button to be disabled during loading by wiring its
enabled/interactive property to state.isLoading (eg. enabled = !state.isLoading
or equivalent) so clicks are ignored while the login request is in-flight, and
keep the existing text logic that shows "Загрузка..." when state.isLoading is
true.
---
Outside diff comments:
In @.github/workflows/mobile-ci.yml:
- Around line 13-15: Add an explicit least-privilege permissions block to the
workflow (or at the job level under jobs -> build-android) instead of relying on
defaults; identify the exact tokens your Android build needs (e.g., read access
to repo contents and only the specific write scopes required such as packages or
id-token if you perform publishing/oidc) and add a minimal permissions: mapping
(for example contents: read and any required specific scopes) under the
build-android job so the workflow no longer uses broad default token
permissions.
In
`@mobile/app/src/main/java/com/smartjam/app/domain/repository/RoomRepository.kt`:
- Around line 1-2: This file is empty and should either be removed or contain
the intended repository declaration; fix by either deleting RoomRepository.kt or
adding a proper package declaration (package com.smartjam.app.domain.repository)
and the repository symbol (e.g., interface RoomRepository or class
RoomRepositoryImpl) with its expected methods/signatures so static analysis
stops flagging an empty file; ensure the file defines the RoomRepository symbol
and appropriate imports/visibility.
In
`@mobile/app/src/main/java/com/smartjam/app/ui/screens/register/RegisterViewModel.kt`:
- Around line 96-111: In RegisterViewModel wrap the call to
authRepository.register(...) in a try/catch/finally so any thrown exception is
caught and converted into a user-facing error (set via _state.update {
it.copy(errorMessage = ...) }) and ensure _state.update { it.copy(isLoading =
false) } runs in finally; keep the existing success path that calls
eventChannel.send(RegisterEvent.NavigateToHome) on result.isSuccess and the
existing failure path that reads result.exceptionOrNull() when no exception was
thrown.
In `@mobile/app/src/main/java/com/smartjam/app/ui/screens/room/RoomScreen.kt`:
- Around line 1-2: Nav navigation is broken because Screen.Room ("room_screen")
is declared and HomeScreen calls onNavigateToRoom but there is no
composable(route = Screen.Room.route) destination and RoomScreen/RoomViewModel
are empty; either implement RoomScreen and RoomViewModel and add a
composable(route = Screen.Room.route) entry in NavGraph (wiring in a NavHost
destination that constructs RoomViewModel and calls RoomScreen) or remove
Screen.Room and all calls to onNavigateToRoom in HomeScreen and NavGraph; update
references to use the existing Screen sealed class and ensure NavGraph's NavHost
provides the destination that calls RoomScreen(route = Screen.Room.route) and
obtains RoomViewModel (e.g., via hiltViewModel() or provided factory).
In `@mobile/app/src/main/java/com/smartjam/app/ui/screens/room/RoomViewModel.kt`:
- Around line 1-2: The file RoomViewModel.kt is empty and must be removed or
restored; either delete the empty RoomViewModel.kt from the main source set, or
re-create the missing ViewModel by adding a class named RoomViewModel that
extends androidx.lifecycle.ViewModel (or your project's BaseViewModel),
implement the expected public members used by the UI (e.g., LiveData/StateFlow
properties and methods like loadRoom(), joinRoom(), leaveRoom() referenced by
RoomFragment/RoomActivity), and ensure the file is placed in the same package as
the consuming UI so the DI/instantiation code can find RoomViewModel.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 7b4291ab-28e5-467d-b4d3-6f0028f1b825
📒 Files selected for processing (47)
.github/workflows/mobile-ci.yml.gitignoremobile/app/build.gradle.ktsmobile/app/src/androidTest/java/com/smartjam/app/ExampleInstrumentedTest.ktmobile/app/src/main/AndroidManifest.xmlmobile/app/src/main/java/com/smartjam/app/MainActivity.ktmobile/app/src/main/java/com/smartjam/app/data/api/AuthAuthenticator.ktmobile/app/src/main/java/com/smartjam/app/data/api/FcmPushService.ktmobile/app/src/main/java/com/smartjam/app/data/local/Converters.ktmobile/app/src/main/java/com/smartjam/app/data/local/SmartJamDatabase.ktmobile/app/src/main/java/com/smartjam/app/data/local/TokenStorage.ktmobile/app/src/main/java/com/smartjam/app/data/local/dao/ConnectionDao.ktmobile/app/src/main/java/com/smartjam/app/data/local/entity/ConnectionEntity.ktmobile/app/src/main/java/com/smartjam/app/data/model/CommentModels.ktmobile/app/src/main/java/com/smartjam/app/data/model/ConnectionModels.ktmobile/app/src/main/java/com/smartjam/app/data/model/LoginRequest.ktmobile/app/src/main/java/com/smartjam/app/data/model/LoginResponse.ktmobile/app/src/main/java/com/smartjam/app/data/model/LoginState.ktmobile/app/src/main/java/com/smartjam/app/data/model/RefreshRequest.ktmobile/app/src/main/java/com/smartjam/app/data/model/RegisterRequest.ktmobile/app/src/main/java/com/smartjam/app/data/model/TaskModels.ktmobile/app/src/main/java/com/smartjam/app/domain/model/Connection.ktmobile/app/src/main/java/com/smartjam/app/domain/model/UserRole.ktmobile/app/src/main/java/com/smartjam/app/domain/repository/AuthRepository.ktmobile/app/src/main/java/com/smartjam/app/domain/repository/ConnectionRepository.ktmobile/app/src/main/java/com/smartjam/app/domain/repository/RoomRepository.ktmobile/app/src/main/java/com/smartjam/app/ui/components/Backgrounds.ktmobile/app/src/main/java/com/smartjam/app/ui/components/Buttons.ktmobile/app/src/main/java/com/smartjam/app/ui/components/Containers.ktmobile/app/src/main/java/com/smartjam/app/ui/components/TextFields.ktmobile/app/src/main/java/com/smartjam/app/ui/navigation/NavGraph.ktmobile/app/src/main/java/com/smartjam/app/ui/screens/home/HomeScreen.ktmobile/app/src/main/java/com/smartjam/app/ui/screens/home/HomeViewModel.ktmobile/app/src/main/java/com/smartjam/app/ui/screens/login/LoginScreen.ktmobile/app/src/main/java/com/smartjam/app/ui/screens/login/LoginViewModel.ktmobile/app/src/main/java/com/smartjam/app/ui/screens/register/RegisterScreen.ktmobile/app/src/main/java/com/smartjam/app/ui/screens/register/RegisterViewModel.ktmobile/app/src/main/java/com/smartjam/app/ui/screens/room/RoomScreen.ktmobile/app/src/main/java/com/smartjam/app/ui/screens/room/RoomViewModel.ktmobile/app/src/main/java/com/smartjam/app/ui/theme/Color.ktmobile/app/src/main/java/com/smartjam/app/ui/theme/Theme.ktmobile/app/src/main/java/com/smartjam/app/ui/theme/Type.ktmobile/app/src/test/java/com/smartjam/app/ExampleUnitTest.ktmobile/build.gradle.ktsmobile/gradle/libs.versions.tomlmobile/gradle/wrapper/gradle-wrapper.propertiesopenapi-spec/api.yaml
💤 Files with no reviewable changes (1)
- mobile/app/src/main/java/com/smartjam/app/ui/theme/Color.kt
Comment on lines
+24
to
+27
| - name: Setup Java 25 | ||
| uses: actions/setup-java@v5 | ||
| with: | ||
| java-version: '17' | ||
| java-version: '25' |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What Java versions are officially supported by the current Android Gradle Plugin and Gradle for Android builds, and is JDK 25 supported?
💡 Result:
Answer (as of 2026-05-25): 1) Officially supported Java versions for Android Gradle Plugin (AGP) and Gradle builds - AGP docs state that the minimum required JDK to run AGP depends on the AGP version, and provides a compatibility table in the AGP release notes (example given: AGP 8.x requires JDK 17 to run) [1]. - AGP 9.2.0 release notes explicitly list JDK 17 as both the minimum and default JDK for that AGP release [2]. - Therefore, for current AGP (9.2.0 and the contemporary AGP line), the supported JDK to run the build is JDK 17 (and higher, subject to the toolchain choosing behavior), with the exact minimum pinned per AGP version via release-note compatibility tables [1][2]. 2) What Gradle (for Android builds) is officially supported with (JDK side) - Gradle’s tested/compatibility matrix is about which Gradle versions work with which Android Gradle Plugin versions; it doesn’t enumerate JDK 25 as supported in the snippet we retrieved [3][4]. - For the purpose of “is JDK 25 supported?”, the key point is whether Gradle itself supports running/compiling with JDK 25. A Gradle issue discussing JDK 25 support notes that the current compatibility matrix shows there is no toolchain support for JDK 25 (at least at the time of that request) [5], and another notes that a warning about Kotlin/JDK 25 compatibility can be resolved on newer Gradle (e.g., 9.2.0) [6]. 3) Is JDK 25 supported? - For Android builds specifically: JDK 25 is not listed as an officially supported JDK in the Android/AGP JDK documentation we retrieved; the AGP 9.2.0 release notes show JDK 17 as the required/default JDK [2]. So, in the current official AGP guidance, JDK 25 is not presented as a supported “build JDK” the way JDK 17 is. - For Gradle itself: there are explicit reports that (at least in some versions of Gradle) the compatibility matrix indicated no toolchain support for JDK 25 [5], and later Gradle versions/workarounds address compilation issues [6]. Based on these sources alone, we cannot claim “JDK 25 is officially supported by current AGP+Gradle” as a definitive, fully sanctioned baseline; the authoritative AGP release-note baseline we have points to JDK 17 [2]. Practical conclusion: - Use JDK 17 for current Android Gradle Plugin (example: AGP 9.2.0 requires JDK 17) [2]. - I do not have authoritative evidence from the AGP/Android official JDK guidance that JDK 25 is officially supported as a build JDK today; Gradle community sources indicate that JDK 25 support varies by Gradle version and has had compatibility gaps [5][6].
Citations:
- 1: https://developer.android.com/build/jdks
- 2: https://developer.android.com/build/releases/agp-9-2-0-release-notes
- 3: https://docs.gradle.org/9.4.0/userguide/compatibility.html
- 4: https://docs.gradle.org/current/userguide/compatibility.html
- 5: Support compiling and testing on Java 25 gradle/gradle#33310
- 6: Getting warning about Kotlin being incompatible with JDK 25 with Gradle 9.1.0, JDK 25, and Kotlin DSL gradle/gradle#35204
Pin CI to an AGP-supported JDK (JDK 17), or justify JDK 25 explicitly
File: .github/workflows/mobile-ci.yml (lines 24-27)
- name: Setup Java 25
uses: actions/setup-java@v5
with:
java-version: '25'AGP 9.2.0’s official guidance specifies JDK 17 as the minimum/default JDK, and the AGP/Gradle JDK compatibility information does not show JDK 25 as an officially supported build JDK. Switching CI to Java 25 can therefore break the Android toolchain if Gradle/Kotlin/AGP reject newer JDKs.
- Set
java-version: '17'(matching the AGP compatibility baseline) or gate/upgrade only after confirming JDK 25 compatibility for the exact AGP + Gradle versions used.
🧰 Tools
🪛 zizmor (1.25.2)
[error] 25-25: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/mobile-ci.yml around lines 24 - 27, The CI workflow uses
actions/setup-java@v5 with java-version: '25' which is not an AGP-supported JDK;
change the java-version value to '17' (update the step named "Setup Java 25" to
set java-version: '17') so the workflow uses the AGP/Gradle-compatible JDK, or
alternatively gate this change with explicit compatibility checks for your AGP
and Gradle versions before keeping '25'.
|
|
||
| - name: Setup Java 17 | ||
| - name: Setup Java 25 | ||
| uses: actions/setup-java@v5 |
There was a problem hiding this comment.
Pin GitHub Action to a commit SHA.
Using actions/setup-java@v5 at Line 25 is mutable and weakens CI supply-chain guarantees. Pin this action to a full commit SHA.
🧰 Tools
🪛 zizmor (1.25.2)
[error] 25-25: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/mobile-ci.yml at line 25, Replace the mutable reference
"uses: actions/setup-java@v5" with a pinned commit SHA for the
actions/setup-java action; locate the workflow step that contains the literal
string uses: actions/setup-java@v5 and update it to uses:
actions/setup-java@<full-commit-sha> (obtain the latest stable commit SHA from
the actions/setup-java repository and replace v5 with that full SHA).
Comment on lines
+26
to
+45
| scope.launch { | ||
| try { | ||
| val tokenStorage = TokenStorage(applicationContext) | ||
| val baseUrl = BuildConfig.BASE_URL | ||
| val authenticator = AuthAuthenticator(tokenStorage, baseUrl) | ||
|
|
||
| val okHttpClientBuilder = OkHttpClient.Builder().authenticator(authenticator) | ||
|
|
||
| val apiClient = | ||
| ApiClient(baseUrl = baseUrl, okHttpClientBuilder = okHttpClientBuilder) | ||
| authenticator.apiClient = apiClient | ||
|
|
||
| val devicesApi = apiClient.createService(DevicesApi::class.java) | ||
|
|
||
| devicesApi.registerDevice(DeviceRegistrationRequest(token = token)) | ||
| Log.i("SmartJam_FCM", "FCM token updated successfully") | ||
| } catch (e: Exception) { | ||
| Log.e("SmartJam_FCM", "Error during FCM token update", e) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether registerDevice is invoked anywhere besides FcmPushService.
# Expected: at least one authenticated retry path outside onNewToken.
rg -nP --type=kt -C2 '\bregisterDevice\s*\('
rg -nP --type=kt -C2 '\bonNewToken\s*\('Repository: Satlykovs/SmartJam
Length of output: 116
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Locate and inspect the target service around the reported lines (and any logging around ~line 53).
TARGET="mobile/app/src/main/java/com/smartjam/app/data/api/FcmPushService.kt"
if [ -f "$TARGET" ]; then
echo "=== FILE: $TARGET ==="
nl -ba "$TARGET" | sed -n '1,200p'
else
echo "Missing file: $TARGET" >&2
fi
# 2) Find all call sites of registerDevice(...) in Kotlin.
echo "=== registerDevice call sites (kotlin) ==="
rg -n "registerDevice\s*\(" -t kotlin
# 3) Find all onNewToken overrides in Kotlin.
echo "=== onNewToken overrides (kotlin) ==="
rg -n "\bonNewToken\b" -t kotlin
# 4) Find DeviceRegistrationRequest usage.
echo "=== DeviceRegistrationRequest usage (kotlin) ==="
rg -n "DeviceRegistrationRequest" -t kotlin
# 5) Inspect TokenStorage implementation for any pending-token/retry mechanisms.
echo "=== TokenStorage class/usage ==="
rg -n "class TokenStorage\b|TokenStorage\(" -t kotlin
# If TokenStorage exists, print it for manual check.
TOKEN_FILE=$(fd -a "TokenStorage" mobile/app/src/main/java/com/smartjam/app -t f 2>/dev/null | head -n 1 || true)
if [ -n "${TOKEN_FILE:-}" ] && [ -f "$TOKEN_FILE" ]; then
echo "=== TokenStorage file: $TOKEN_FILE ==="
nl -ba "$TOKEN_FILE" | sed -n '1,220p'
fi
# 6) Search for logs that include notification body content.
echo "=== Potential notification body logging (kotlin) ==="
rg -n "Log\.(i|d|e|w)\(.*(body|notification.*body|message.*body|notification\.body|payload)" -t kotlinRepository: Satlykovs/SmartJam
Length of output: 206
🏁 Script executed:
#!/bin/bash
set -euo pipefail
TARGET="mobile/app/src/main/java/com/smartjam/app/data/api/FcmPushService.kt"
if [ -f "$TARGET" ]; then
echo "=== FILE: $TARGET (first 220 lines) ==="
cat -n "$TARGET" | sed -n '1,220p'
else
echo "Missing file: $TARGET" >&2
fi
echo "=== registerDevice call sites (kotlin) ==="
rg -n "registerDevice\s*\(" -t kotlin
echo "=== onNewToken overrides (kotlin) ==="
rg -n "\bonNewToken\b" -t kotlin
echo "=== DeviceRegistrationRequest usage (kotlin) ==="
rg -n "DeviceRegistrationRequest" -t kotlin
echo "=== TokenStorage class/usage (kotlin) ==="
rg -n "class TokenStorage\b|TokenStorage\(" -t kotlin
TOKEN_FILE=$(fd -a "TokenStorage" mobile/app/src/main/java/com/smartjam/app -t f 2>/dev/null | head -n 1 || true)
if [ -n "${TOKEN_FILE:-}" ] && [ -f "$TOKEN_FILE" ]; then
echo "=== TokenStorage file: $TOKEN_FILE (first 260 lines) ==="
cat -n "$TOKEN_FILE" | sed -n '1,260p'
fi
echo "=== Potential notification body logging (kotlin) ==="
rg -n "Log\.(i|d|e|w)\(.*(body|notification.*body|message.*body|notification\.body|payload)" -t kotlinRepository: Satlykovs/SmartJam
Length of output: 6986
🏁 Script executed:
#!/bin/bash
set -euo pipefail
AUTH_REPO="mobile/app/src/main/java/com/smartjam/app/domain/repository/AuthRepository.kt"
echo "=== AuthRepository.kt (relevant sections) ==="
# Show imports and around likely registerDevice usage lines
cat -n "$AUTH_REPO" | sed -n '1,220p'
echo "=== Search: fcmToken / FirebaseMessaging token usage in AuthRepository ==="
rg -n "fcmToken|FirebaseMessaging|getInstance\(\)\.token|getToken\(|onNewToken" -t kotlin "$AUTH_REPO"Repository: Satlykovs/SmartJam
Length of output: 7261
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search: fcm / FirebaseMessaging / registerDevicePushToken usage (kotlin) ==="
rg -n "FirebaseMessaging|getInstance\(\)\.token|registerDevicePushToken|registerDevice\(|unregisterDevice\(|fcmToken|FCM" -t kotlin mobile/app/src/main/java
echo "=== Search: pending retry / datastore keys related to FCM ==="
rg -n "pending|retry|retries|backoff|workmanager|WorkManager" -t kotlin mobile/app/src/main/java
echo "=== Search: TokenStorage keys/fields mentioning FCM ==="
rg -n "fcm|FCM" -t kotlin mobile/app/src/main/java/com/smartjam/app/data/localRepository: Satlykovs/SmartJam
Length of output: 2338
Add a guaranteed retry path for failed token registration (and stop logging notification bodies).
- Reliability:
FcmPushService.onNewTokenregisters viadevicesApi.registerDevice(...)but only logs exceptions with no persisted retry; whileAuthRepository.register/logindoes callregisterDevicePushToken()after auth, failures there are also only logged—if either fails once (network/auth issues), the device can remain unregistered until a new FCM token arrives or the user logs in again successfully. - Privacy:
FcmPushService.onMessageReceivedlogsmessage.notification?.body, which can expose sensitive push content to logs.
🧰 Tools
🪛 detekt (1.23.8)
[warning] 42-42: The caught exception is too generic. Prefer catching specific exceptions to the case that is currently handled.
(detekt.exceptions.TooGenericExceptionCaught)
🤖 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 `@mobile/app/src/main/java/com/smartjam/app/data/api/FcmPushService.kt` around
lines 26 - 45, FcmPushService.onNewToken and the call to
devicesApi.registerDevice(DeviceRegistrationRequest) need a guaranteed retry
path: persist the FCM token (or a "pendingPushToken" flag) to local storage when
registration fails and schedule a reliable retry (e.g., WorkManager/RetryWorker)
that attempts devicesApi.registerDevice until it succeeds (also ensure
AuthRepository.register/login triggers the same retry logic rather than only
logging failures); additionally, remove any logging of
message.notification?.body in FcmPushService.onMessageReceived and replace with
non-sensitive logs (e.g., log only notification presence or message ID) to avoid
logging notification bodies.
|
|
||
| val body = message.notification?.body | ||
|
|
||
| Log.d("SmartjJam_FCM", "Push notification received: $body") |
There was a problem hiding this comment.
Do not log raw push message body content.
Line 53 logs notification payload text, which can contain user content/PII. Keep logs metadata-only (or debug-gated without body data).
🔒 Suggested fix
- val body = message.notification?.body
-
- Log.d("SmartjJam_FCM", "Push notification received: $body")
+ Log.d("SmartJam_FCM", "Push notification received")🤖 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 `@mobile/app/src/main/java/com/smartjam/app/data/api/FcmPushService.kt` at line
53, The Log.d call in FcmPushService that logs the raw notification body
(Log.d("SmartjJam_FCM", "Push notification received: $body")) must be changed to
avoid writing PII/content to logs; update the onMessageReceived handler in
FcmPushService to log only metadata (e.g., message ID, sender, timestamp) or a
fixed "payload received" message, or gate detailed logging behind a secure debug
flag and/or sanitize/truncate any content before logging, ensuring the literal
$body is never emitted to logs.
Comment on lines
+13
to
+16
| private val Context.dataStore: DataStore<Preferences> by | ||
| preferencesDataStore( // TODO: make encrypted storage | ||
| name = "auth_preferences" | ||
| ) | ||
| class TokenStorage(private val context: Context) { | ||
| private companion object Keys{ | ||
| val ACCESS_TOKEN = stringPreferencesKey("access_token") | ||
| val REFRESH_TOKEN = stringPreferencesKey("refresh_token") | ||
| } | ||
| ) |
There was a problem hiding this comment.
Encrypt auth tokens at rest instead of plain preferences storage.
Line 14 notes this already, and Lines 24–27 currently persist raw access/refresh tokens. That is a security/compliance gap if device storage is compromised. Please move to encrypted at-rest storage (or encrypt values before DataStore persistence), especially for the refresh token.
Also applies to: 24-27
🤖 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 `@mobile/app/src/main/java/com/smartjam/app/data/local/TokenStorage.kt` around
lines 13 - 16, The code currently exposes Context.dataStore created via
preferencesDataStore and is persisting raw access/refresh tokens (lines around
the current token save/read methods); change this to store encrypted values:
create a MasterKey (AndroidX Security Crypto) and either (a) switch to
EncryptedSharedPreferences for the auth keys or (b) encrypt token strings with
AES-GCM using the MasterKey before writing into the existing DataStore and
decrypt on read. Update the save/persist methods that write the access/refresh
tokens to call the encrypt routine and update the retrieval methods to decrypt,
and keep Context.dataStore/preferencesDataStore usage only for encrypted blobs
or non-sensitive prefs.
Comment on lines
+84
to
+85
| suspend fun respondToRequest(connectionId: String, accept: Boolean): Result<Unit> = | ||
| Result.success(Unit) |
There was a problem hiding this comment.
respondToRequest is a stub and falsely reports success.
On Line 84, the method ignores connectionId/accept and always returns success, so UI toasts and refreshes can claim a request was processed when no API call happened.
🤖 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
`@mobile/app/src/main/java/com/smartjam/app/domain/repository/ConnectionRepository.kt`
around lines 84 - 85, The respondToRequest function currently always returns
Result.success(Unit) without using connectionId or accept; replace the stub with
a real implementation in ConnectionRepository.respondToRequest that calls the
appropriate remote data-source / API method to accept/reject the connection
using the connectionId and accept flag, handles HTTP/errors with try/catch,
updates any local cache or DAO state as needed, and returns Result.success(Unit)
on success or Result.failure(exception) on error (propagate meaningful error
messages to the caller).
Comment on lines
+40
to
+46
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { | ||
| ActivityCompat.requestPermissions( | ||
| this, | ||
| arrayOf(Manifest.permission.POST_NOTIFICATIONS), | ||
| 101, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Gate notification permission request by current grant status.
Lines 40-46 request POST_NOTIFICATIONS on every launch for API 33+, including when already granted.
Suggested guard
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) {
- ActivityCompat.requestPermissions(
- this,
- arrayOf(Manifest.permission.POST_NOTIFICATIONS),
- 101,
- )
+ if (
+ ContextCompat.checkSelfPermission(
+ this,
+ Manifest.permission.POST_NOTIFICATIONS
+ ) != PackageManager.PERMISSION_GRANTED
+ ) {
+ ActivityCompat.requestPermissions(
+ this,
+ arrayOf(Manifest.permission.POST_NOTIFICATIONS),
+ 101,
+ )
+ }
}🤖 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 `@mobile/app/src/main/java/com/smartjam/app/MainActivity.kt` around lines 40 -
46, The code currently calls ActivityCompat.requestPermissions(...) for
Manifest.permission.POST_NOTIFICATIONS on every launch; instead, before calling
requestPermissions in MainActivity, check the current grant status using
ContextCompat.checkSelfPermission(this, Manifest.permission.POST_NOTIFICATIONS)
and only call ActivityCompat.requestPermissions(...) when that check does not
equal PackageManager.PERMISSION_GRANTED (keep the existing Build.VERSION.SDK_INT
>= Build.VERSION_CODES.TIRAMISU guard). This change prevents asking for
POST_NOTIFICATIONS if already granted while preserving the API 33+ conditional.
Comment on lines
130
to
134
| GoldenStringsButton( | ||
| text = if (state.isLoading) "Загрузка..." else "Войти", | ||
| onClick = { viewModel.onLoginClicked() }, | ||
| modifier = Modifier.fillMaxWidth() | ||
| modifier = Modifier.fillMaxWidth(), | ||
| ) |
There was a problem hiding this comment.
Disable login action while loading to prevent duplicate submissions.
On Line 130, GoldenStringsButton is always enabled, so users can repeatedly trigger onLoginClicked() during an in-flight request.
Proposed fix
GoldenStringsButton(
text = if (state.isLoading) "Загрузка..." else "Войти",
onClick = { viewModel.onLoginClicked() },
+ enabled = !state.isLoading,
modifier = Modifier.fillMaxWidth(),
)📝 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
| GoldenStringsButton( | |
| text = if (state.isLoading) "Загрузка..." else "Войти", | |
| onClick = { viewModel.onLoginClicked() }, | |
| modifier = Modifier.fillMaxWidth() | |
| modifier = Modifier.fillMaxWidth(), | |
| ) | |
| GoldenStringsButton( | |
| text = if (state.isLoading) "Загрузка..." else "Войти", | |
| onClick = { viewModel.onLoginClicked() }, | |
| enabled = !state.isLoading, | |
| modifier = Modifier.fillMaxWidth(), | |
| ) |
🤖 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 `@mobile/app/src/main/java/com/smartjam/app/ui/screens/login/LoginScreen.kt`
around lines 130 - 134, The GoldenStringsButton is currently always clickable
and can trigger viewModel.onLoginClicked() repeatedly while state.isLoading is
true; update the button to be disabled during loading by wiring its
enabled/interactive property to state.isLoading (eg. enabled = !state.isLoading
or equivalent) so clicks are ignored while the login request is in-flight, and
keep the existing text logic that shows "Загрузка..." when state.isLoading is
true.
3142c88 to
2018a96
Compare
…ame smartjam-analyzer module package
… jdbc recipient resolver and use case
…app. Add spotless & ktfmt code formatting # Conflicts: # mobile/app/build.gradle.kts # mobile/app/src/main/AndroidManifest.xml # mobile/app/src/main/java/com/smartjam/app/MainActivity.kt # mobile/app/src/main/java/com/smartjam/app/data/api/AuthAuthenticator.kt # mobile/app/src/main/java/com/smartjam/app/data/local/SmartJamDatabase.kt # mobile/app/src/main/java/com/smartjam/app/data/local/TokenStorage.kt # mobile/app/src/main/java/com/smartjam/app/data/local/entity/ConnectionEntity.kt # mobile/app/src/main/java/com/smartjam/app/data/model/CommentModels.kt # mobile/app/src/main/java/com/smartjam/app/data/model/ConnectionModels.kt # mobile/app/src/main/java/com/smartjam/app/data/model/LoginRequest.kt # mobile/app/src/main/java/com/smartjam/app/data/model/LoginResponse.kt # mobile/app/src/main/java/com/smartjam/app/data/model/RefreshRequest.kt # mobile/app/src/main/java/com/smartjam/app/data/model/RegisterRequest.kt # mobile/app/src/main/java/com/smartjam/app/data/model/TaskModels.kt # mobile/app/src/main/java/com/smartjam/app/domain/model/Connection.kt # mobile/app/src/main/java/com/smartjam/app/domain/repository/AuthRepository.kt # mobile/app/src/main/java/com/smartjam/app/domain/repository/ConnectionRepository.kt # mobile/app/src/main/java/com/smartjam/app/domain/repository/RoomRepository.kt # mobile/app/src/main/java/com/smartjam/app/ui/navigation/NavGraph.kt # mobile/app/src/main/java/com/smartjam/app/ui/screens/home/HomeScreen.kt # mobile/app/src/main/java/com/smartjam/app/ui/screens/home/HomeViewModel.kt # mobile/app/src/main/java/com/smartjam/app/ui/screens/login/LoginScreen.kt # mobile/app/src/main/java/com/smartjam/app/ui/screens/login/LoginViewModel.kt # mobile/app/src/main/java/com/smartjam/app/ui/screens/room/RoomScreen.kt # mobile/app/src/main/java/com/smartjam/app/ui/screens/room/RoomViewModel.kt
Satlykovs
force-pushed
the
satlykovs/feat/notification
branch
from
2018a96 to
0083deb
Compare
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
2 participants
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.
Цель изменений
Реализация системы асинхронных уведомлений пользователей о результатах анализа аудио. Создание инфраструктуры для доставки фидбека от анализатора до конечного пользователя через Firebase Cloud Messaging (FCM) и интеграция всех звеньев системы.
Что сделано
Создание микросервиса
smartjam-notificationAnalysisResultListener, который слушает события о завершении анализа.debug/prod) для переключения между логированием в консоль и реальной отправкой в Google.Рефакторинг и доработка
smartjam-analyzerAnalysisFatalException) и технические сбои.FAILEDстатус и уведомляет систему, не спамя в очередь.AnalysisFinishedEventв Kafka.План на будущее
smartjam-api