Implement room screen logic#26
Conversation
|
Warning Review limit reached
More reviews will be available in 14 minutes and 9 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds OpenAPI API contracts, Room DB schema for assignments/submissions, role-aware auth and client wiring, media caching/storage, RoomRepository, Room UI and ViewModels with pagination, Coil avatar rendering, and an instrumented DB test. ChangesAssignment & Room Integration
Sequence Diagram(s)sequenceDiagram
participant HomeVM
participant ConnRepo
participant ApiServer
participant RoomRepo
participant DB
HomeVM->>ConnRepo: syncConnectionsPage(role,page,size)
ConnRepo->>ApiServer: getMyConnections(page,size)
ApiServer-->>ConnRepo: connections page DTO
ConnRepo->>DB: insertConnections(entities with avatar bytes)
HomeVM->>RoomRepo: syncAssignmentsPage(connectionId,page,size)
RoomRepo->>ApiServer: getAssignments(connectionId,page,size)
RoomRepo->>DB: insert/update AssignmentEntity & SubmissionResultEntity
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai make full rescan of this PR, then create a list of improvements and vulnerabilities/bugs in ENG and RUS |
🧩 Analysis chain🏁 Script executed: Length of output: 40296
🔴 Vulnerabilities & Security Issues / Уязвимости и проблемы безопасности1. Cleartext HTTP Traffic Enabled / Разрешён незашифрованный HTTP-трафикEN: RU: 2. Tokens Stored in Plain DataStore / Токены хранятся в незашифрованном DataStoreEN: RU: В 3. User Role Not Cleared on Logout / Роль пользователя не удаляется при выходеEN: RU: Метод // Fix / Исправление:
suspend fun clearTokens() {
context.dataStore.edit { preferences ->
preferences.remove(ACCESS_TOKEN)
preferences.remove(REFRESH_TOKEN)
preferences.remove(USER_ROLE) // ← add this / добавить это
}
}4.
|
| # | Severity / Критичность | Category / Категория | Short Description / Краткое описание |
|---|---|---|---|
| 1 | 🔴 Critical | Security | Cleartext HTTP globally enabled |
| 2 | 🔴 Critical | Security | Tokens in plain DataStore |
| 3 | 🔴 Critical | Bug | USER_ROLE not cleared on logout |
| 4 | 🔴 Critical | Bug | runBlocking in OkHttp interceptor → ANR |
| 5 | 🔴 High | Security | allowBackup exposes data |
| 6 | 🔴 High | Bug | references.localhost replace never matches |
| 7 | 🟠 High | Bug | ByteArray in data class breaks equality |
| 8 | 🟠 High | Bug | DB v1→v6, destructive migration = data loss |
| 9 | 🟡 Medium | Bug | createdAt overwritten with Instant.now() |
| 10 | 🟡 Medium | Bug | Transient null token → spurious Login redirect |
| 11 | 🟡 Medium | Bug | Unbounded coroutines in collect |
| 12 | 🟡 Medium | Bug | isLoading stuck true on register failure |
| 13 | 🟠 High | Improvement | 10.0.2.2 emulator address in production code |
| 14 | 🟡 Medium | Improvement | Avatar bytes as BLOB in Room |
| 15 | 🟡 Medium | Improvement | No DI framework |
| 16 | 🟡 Medium | Improvement | No HTTP timeouts on audio client |
| 17 | 🟡 Medium | Improvement | Corrupted files on partial download |
| 18 | 🟢 Low | Improvement | Hardcoded status strings instead of enum |
| 19 | 🟢 Low | Improvement | submissionsJobs map not cleaned up |
| 20 | 🟢 Low | Improvement | New ApiClient per token refresh |
| 21 | 🟢 Low | Improvement | .wav hardcoded in AudioFileStore |
| 22 | 🟢 Low | Improvement | Mixed RU/EN error messages |
✅ Actions performed
Full review triggered.
Satlykovs
added
Moblie
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 28
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
mobile/app/src/main/java/com/smartjam/app/data/local/entity/ConnectionEntity.kt (1)
9-17: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winByteArray equality in
ConnectionEntity.peerAvatarBytes/Connection.peerAvatarBytesis reference-based
ConnectionEntityand the mapped domainConnectionaredata classes that containByteArray?. Kotlin’s generatedequals/hashCodecomparesByteArrayby reference (not byte content), which will be wrong if these objects are ever compared directly, used in sets/maps, or fed into list diffing that relies onequals. Current UI rendering uses ComposeLazyColumn(items(state.connections))and doesn’t show DiffUtil/areContentsTheSame, so this may not affect behavior right now. Consider switching tocontentEquals/contentHashCode(or excluding the byte array from equality) if correctness matters for future comparisons.🤖 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/entity/ConnectionEntity.kt` around lines 9 - 17, The data classes ConnectionEntity and Connection currently hold peerAvatarBytes: ByteArray?, so their auto-generated equals/hashCode compare the byte arrays by reference; to fix, implement custom equals(other: Any?) and hashCode() in both ConnectionEntity and Connection that compare peerAvatarBytes using peerAvatarBytes?.contentEquals(other.peerAvatarBytes) (handling nulls) and compute hashCode with peerAvatarBytes?.contentHashCode() combined with other properties, or alternatively change peerAvatarBytes to an immutable type that compares by content (e.g., List<Byte> or a ByteString) and update any mapping code that converts between ConnectionEntity and Connection accordingly.mobile/app/src/main/java/com/smartjam/app/ui/screens/register/RegisterViewModel.kt (2)
84-86:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winError message does not reflect actual password requirements.
The regex at line 40 requires at least one special character from
[#?!@$%^&*-], but the error message only mentions "латинские буквы и цифры" (Latin letters and digits). Users won't know they need a special character.📝 Suggested fix
if (!passwordRegex.matches(currentState.passwordInput)) { - _state.update { it.copy(errorMessage = "Пароль: мин. 8 символов, латинские буквы и цифры") } + _state.update { it.copy(errorMessage = "Пароль: 8-20 символов, заглавные и строчные буквы, цифра и спецсимвол (#?!@\$%^&*-)") } return }🤖 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 84 - 86, The error text shown when passwordRegex fails is misleading: passwordRegex (defined near line 40) requires at least one special character from [#?!@$%^&*-] but the _state.update call sets errorMessage to only mention letters and digits; update the errorMessage in the failing branch (where _state.update is called) to list the actual requirements (e.g., minimum 8 characters, Latin letters, digits, and at least one special character like #?!@$%^&*-) so it matches passwordRegex and users know to include a special character.
104-110:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
isLoadingnot reset on registration failure — UI will be stuck in loading state.When registration fails, the failure branch only updates
errorMessagebut does not resetisLoadingtofalse. This leaves the UI stuck showing the loading indicator with no way for the user to retry.🐛 Proposed fix
if (result.isSuccess) { _state.update { it.copy(isLoading = false) } eventChannel.send(RegisterEvent.NavigateToHome) } else { val error = result.exceptionOrNull()?.message ?: "Ошибка регистрации" - _state.update { it.copy(errorMessage = error) } + _state.update { it.copy(errorMessage = error, isLoading = false) } }Alternatively, use a
finallyblock like inLoginViewModel:+ try { val result = authRepository.register(...) if (result.isSuccess) { - _state.update { it.copy(isLoading = false) } eventChannel.send(RegisterEvent.NavigateToHome) } else { val error = result.exceptionOrNull()?.message ?: "Ошибка регистрации" _state.update { it.copy(errorMessage = error) } } + } 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 104 - 110, In RegisterViewModel the failure branch after checking result.isSuccess only updates errorMessage and never resets isLoading, causing the UI to remain in a loading state; update the failure branch (the else after if (result.isSuccess)) to also set _state.update { it.copy(isLoading = false, errorMessage = error) } or move the isLoading=false logic into a shared finally/cleanup section (similar to LoginViewModel) so _state.isLoading is always cleared whether registration succeeds (and you send RegisterEvent.NavigateToHome) or fails.
🤖 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 `@mobile/app/src/main/AndroidManifest.xml`:
- Line 9: The manifest enables cleartext globally via
android:usesCleartextTraffic="true", which weakens transport security; remove
that global attribute and instead create a network security config (e.g.,
res/xml/network_security_config.xml) that enables cleartext only for debug
builds and disallows it for release, then reference that config from the
<application> tag using
android:networkSecurityConfig="`@xml/network_security_config`" via
build-variant-specific manifests or manifest placeholders so cleartext is
allowed only in debug and not in production.
In `@mobile/app/src/main/java/com/smartjam/app/api.yaml`:
- Around line 531-534: The roles array in UserResponse is typed as plain strings
which allows invalid values; change the items definition to reference the
existing enum schema by replacing the current items: { type: string } with an
item $ref to the UserRole schema (referencing components.schemas.UserRole, e.g.
$ref: '`#/components/schemas/UserRole`') so UserResponse.roles uses the UserRole
enum.
In `@mobile/app/src/main/java/com/smartjam/app/common-models.yaml`:
- Around line 44-59: The four timestamp properties teacherStartTime,
teacherEndTime, studentStartTime, and studentEndTime allow negative values;
update their OpenAPI schema entries to enforce non-negative values by adding
"minimum: 0" to each property in the common-models YAML(s) so both copies of the
schema are hardened (ensure you modify the definitions for teacherStartTime,
teacherEndTime, studentStartTime and studentEndTime).
In `@mobile/app/src/main/java/com/smartjam/app/data/api/AuthAuthenticator.kt`:
- Around line 86-90: The toApiRole function is catching Exception too broadly;
narrow it to only catch IllegalArgumentException thrown by
com.smartjam.app.model.UserRole.valueOf(role) so invalid enum names return null
without masking other errors—update the try/catch in toApiRole to catch
IllegalArgumentException (or use runCatching { UserRole.valueOf(role)
}.getOrNull()) and keep the current null-on-failure behavior.
In `@mobile/app/src/main/java/com/smartjam/app/data/api/InstantAdapter.kt`:
- Around line 24-27: InstantAdapter.read() currently swallows all exceptions
from Instant.parse(s) and returns null, which can silently produce non-null Room
fields (AssignmentEntity, ConnectionEntity, SubmissionResultEntity); modify
InstantAdapter.read() to fail fast by not returning null on parse errors—catch
the specific parse exception (e.g., DateTimeParseException) or catch Exception
and rethrow a JsonDataException/IOException with a clear message including the
offending string, so deserialization fails immediately instead of propagating
nulls.
In `@mobile/app/src/main/java/com/smartjam/app/data/local/AudioFileStore.kt`:
- Around line 8-11: The AudioFileStore initialization currently ignores mkdirs()
failures; update the baseDir setup in AudioFileStore (the private val baseDir:
File = File(context.filesDir, "assignment_audio").apply { ... }) to fail fast:
after checking if (!exists()) call mkdirs() and if it returns false or the path
still isn't a directory, throw an IllegalStateException (or similar) with a
clear message including the directory path so the class does not continue with
an invalid baseDir.
In
`@mobile/app/src/main/java/com/smartjam/app/data/local/dao/SubmissionResultDao.kt`:
- Around line 16-17: The one-shot DAO query getResultsForAssignmentOnce returns
results in unspecified order while the flow variant orders by createdAt DESC;
update the SQL in the `@Query` for getResultsForAssignmentOnce (table
submission_results) to include "ORDER BY createdAt DESC" so both one-shot and
flow methods return results in the same createdAt descending order.
In `@mobile/app/src/main/java/com/smartjam/app/data/local/TokenStorage.kt`:
- Around line 23-29: saveToken currently leaves a stale USER_ROLE when role is
null and clearTokens doesn't remove USER_ROLE; update TokenStorage.kt so
saveToken(accessToken: String, refreshToken: String, role: String? = null)
deletes the USER_ROLE preference when role == null (instead of leaving the old
value) and ensure clearTokens() also explicitly removes USER_ROLE from
preferences along with ACCESS_TOKEN and REFRESH_TOKEN; reference the
ACCESS_TOKEN, REFRESH_TOKEN and USER_ROLE preference keys and the saveToken and
clearTokens functions to locate where to edit.
In `@mobile/app/src/main/java/com/smartjam/app/domain/model/Connection.kt`:
- Around line 4-10: The generated equals()/hashCode() for data class Connection
is wrong for the peerAvatarBytes array; override equals(other: Any?) and
hashCode() in Connection to compare peerAvatarBytes with
ByteArray.contentEquals(other.peerAvatarBytes) (handling nulls) and compute its
hash using peerAvatarBytes.contentHashCode() (or 0 when null), while keeping the
existing comparisons for id, peerId, peerName and peerAvatarUrl so two
Connection instances with identical avatar bytes are considered equal.
In
`@mobile/app/src/main/java/com/smartjam/app/domain/repository/AuthRepository.kt`:
- Around line 70-72: In AuthRepository's login flow remove the redundant
immediate call to refreshWithRole(role) after saving fresh tokens (the call
right after saveTokens / token persist), since valid tokens were just obtained
and the refresh adds latency and potential failure; either delete that
invocation from the login method or, if the intent was to fetch role-scoped
claims, replace it with a documented conditional or explicit separate call
(e.g., a new method getRoleClaims or a boolean flag that login accepts) so the
intent is clear—update any unit tests that assumed the refresh side‑effect
accordingly.
- Around line 143-147: The catch block in refreshWithRole() currently swallows
all Exceptions and clears tokens which breaks structured concurrency; update the
catch to rethrow CancellationException (or check if e is CancellationException
then throw) before handling other exceptions, so keep tokenStorage.clearTokens()
and apiClient.setBearerToken("") only for non-cancellation errors and return
false afterwards; reference the refreshWithRole() function and the
tokenStorage.clearTokens() / apiClient.setBearerToken("") calls when making the
change.
In
`@mobile/app/src/main/java/com/smartjam/app/domain/repository/ConnectionRepository.kt`:
- Line 29: avatarClient is created without timeouts which can cause avatar
downloads to hang; update the OkHttpClient.Builder() used for avatarClient in
ConnectionRepository.kt to set sensible timeouts (e.g., connect, read and write
timeouts) so requests fail fast on slow/unresponsive servers; modify the
avatarClient initialization (the OkHttpClient.Builder() that constructs
avatarClient) to call timeout configuration methods on the builder before
build().
In
`@mobile/app/src/main/java/com/smartjam/app/domain/repository/RoomRepository.kt`:
- Around line 41-44: The OkHttpClient instance named httpClient in
RoomRepository.kt lacks timeouts and can hang on large audio uploads/downloads;
configure sensible timeouts by updating the OkHttpClient.Builder used for
httpClient to set connectTimeout, readTimeout, writeTimeout (and optionally
callTimeout) with appropriate durations (e.g., seconds) to cover slow servers,
and keep followRedirects/followSslRedirects as-is so the client enforces bounded
wait times for audio transfer operations.
- Around line 281-284: The URL replacement logic in RoomRepository.kt
incorrectly replaces "localhost" before "references.localhost", causing
"references.localhost" to be transformed into "references.10.0.2.2" and
preventing the intended match; update the replacement order for the fixedUrl
construction (the variable fixedUrl in the repository) so the more specific
pattern "references.localhost" is replaced first, then replace "localhost" and
"127.0.0.1" afterwards (or use explicit exact-match replacements/regex to avoid
substring collisions).
- Around line 193-207: The code overwrites the server-created timestamp by using
Instant.now() for createdAt; instead, parse and use the timestamp from the
response DTO (e.g. dto.createdAt) when building SubmissionResultEntity, falling
back to Instant.now() only if the DTO timestamp is null or unparsable; update
the creation of SubmissionResultEntity (and remove the local created variable
usage) in the block that calls submissionResultDao.insertAll(...) so createdAt
is set from the DTO (after converting to java.time.Instant if necessary).
- Around line 249-254: The current download writes directly to target which can
leave partial/corrupt files if interrupted; change the logic in
RoomRepository.kt so you write the response.body?.byteStream() into a temporary
file in the same directory (e.g., target.resolveSibling(target.name + ".tmp") or
similar) using the same use/copyTo pattern, ensure streams are flushed/closed,
then atomically move/rename the temp file to the final target (using Files.move
with ATOMIC_MOVE/REPLACE_EXISTING or File.renameTo fallback) and delete the temp
on any failure; reference the existing response.body?.byteStream() read block
and the current target.outputStream() usage to locate where to swap in the
temp-file write + atomic rename logic.
In `@mobile/app/src/main/java/com/smartjam/app/MainActivity.kt`:
- Around line 103-107: The collector on tokenStorage.accessToken only sets
apiClient.setBearerToken(newToken) for non-null tokens, leaving a stale token
after logout; update the collector inside lifecycleScope.launch so that when
newToken is null you explicitly clear the client's token (e.g., call
apiClient.clearBearerToken()), otherwise set the new token with
apiClient.setBearerToken(newToken); if clearBearerToken() doesn't exist use
apiClient.setBearerToken(null) to ensure the bearer is removed.
- Around line 59-61: The OkHttp logging interceptor in MainActivity is set to
HttpLoggingInterceptor.Level.BODY unconditionally; change the configuration in
the OkHttpClient builder (where HttpLoggingInterceptor is created) to use
BuildConfig.DEBUG to choose the level (e.g., Level.BODY when BuildConfig.DEBUG
is true, otherwise Level.NONE or Level.BASIC for release) so full
request/response bodies (and sensitive headers like Authorization) are not
logged in production.
- Around line 62-65: The interceptor in MainActivity.kt currently uses
runBlocking to read tokenStorage.accessToken inside the OkHttp .addInterceptor
block; remove runBlocking and instead read from an in-memory cached token that
the DataStore collector already updates (e.g., the value set via
apiClient.setBearerToken(...) or a shared volatile/cachedToken variable updated
by the collector). Update the interceptor to check that cached token (e.g.,
cachedToken or apiClient.getBearerToken()) and only add the Authorization header
when that in-memory token is non-null and original.header("Authorization") ==
null, ensuring no blocking calls on OkHttp threads and keeping token updates
handled by the collector logic in MainActivity.
In `@mobile/app/src/main/java/com/smartjam/app/ui/navigation/NavGraph.kt`:
- Around line 107-113: The computed but unused animated value navBarPhase
(created from navBarTransition via rememberInfiniteTransition and animateFloat
in NavGraph.kt) should be removed or applied; either delete the whole
rememberInfiniteTransition/navBarPhase block to avoid unnecessary CPU work, or
reference navBarPhase in the composition where the nav bar is drawn (for example
apply it to a Modifier rotation, background brush angle, or shader) so the
animation drives a visible property; update the code that renders the nav bar to
use navBarPhase (or remove the transition) and remove any imports related to the
unused animation if you choose deletion.
- Around line 198-215: The composable for Screen.Room risks crashing on
malformed args because UUID.fromString and UserRole.valueOf can throw and
currently nulls silently return; wrap parsing of connectionIdStr and roleStr in
validation and a try/catch around UUID.fromString(...) and
UserRole.valueOf(...), and on failure either navigate back
(navController.popBackStack()) or show an error/fallback UI via RoomScreen
(e.g., pass an error state) rather than returning silently; also log the
exception. Locate the logic in the composable block that creates the
RoomViewModel with RoomViewModelFactory and calls RoomScreen to implement these
checks and error handling.
In
`@mobile/app/src/main/java/com/smartjam/app/ui/screens/login/LoginViewModel.kt`:
- Around line 30-34: The constructor parameter tokenStorage in LoginViewModel is
injected but never used; remove it from the LoginViewModel primary constructor
and from any corresponding parameter in LoginViewModelFactory (and update its
create/instantiate call sites) so the ViewModel and factory signatures match;
also remove the unused import for com.smartjam.app.data.local.TokenStorage and
rebuild to ensure no remaining references to tokenStorage remain.
In `@mobile/app/src/main/java/com/smartjam/app/ui/screens/room/RoomScreen.kt`:
- Around line 382-406: The division by zero happens in ErrorTimelineChart where
maxEnd is computed from teacherEndTime; guard against maxEnd being 0.0 by using
a non-zero fallback (e.g., maxEndSafe = if (maxEnd <= 0.0) 1.0 else maxEnd) or
skip drawing when maxEnd is zero, then use maxEndSafe in the calculations for
startX/endX that reference teacherStartTime and teacherEndTime so no
NaN/Infinity is produced; update the references to use the new safe denominator
(and optionally coerce startX/endX within 0..width) in the drawRect logic.
- Around line 63-77: Wrap the file copy and related operations inside the
assignmentPicker callback (the rememberLauncherForActivityResult lambda that
creates File(context.cacheDir, "temp_assignment_upload.wav")) in a try-catch so
IOExceptions (from context.contentResolver.openInputStream or
file.outputStream().use or input.copyTo) are caught; only call
viewModel.uploadAssignment when the copy succeeds, reset
pendingAssignmentTitle/Description in the success path, and in the catch block
log or surface the error (e.g., via viewModel error handler or a Toast) and
ensure any partially written temp file is deleted to avoid leaking corrupt
files.
In `@mobile/app/src/main/java/com/smartjam/app/ui/screens/room/RoomViewModel.kt`:
- Around line 180-194: The code spawns unbounded coroutines in
submissions.forEach (inside viewModelScope.launch) causing duplicate concurrent
repository.getSubmissionResult calls and racey updates to
_uiState.feedbackBySubmission; fix by adding an in-flight tracker (e.g., a
mutable Set like inFlightSubmissionFetches) checked before launching a fetch for
a submission.id, add the id to the set immediately when starting the fetch and
remove it in a finally block (or on completion/error) so duplicate launches are
skipped while a request is outstanding, and ensure the add/remove are done on
the same CoroutineScope/dispatcher (or guarded by a Mutex) to avoid races when
updating the in-flight set and when updating _uiState.
- Line 40: The submissionsJobs map holds Job references that aren't cleaned up;
override RoomViewModel.onCleared() to iterate submissionsJobs.values, call
cancel() on each Job, then clear the submissionsJobs map to avoid leaks;
implement the override inside the RoomViewModel class so any running coroutines
are cancelled and submissionsJobs.reset when the ViewModel is destroyed.
- Around line 210-213: Replace the fragile string comparison of dto.status.name
with direct enum comparisons against AudioProcessingStatus: inside the launch
block in RoomViewModel where SubmissionResultResponse dto is handled, change the
checks that test status == "COMPLETED" || status == "FAILED" to compare
dto.status to AudioProcessingStatus.COMPLETED and AudioProcessingStatus.FAILED
(or use a when/contains check) so the logic uses the enum values directly.
---
Outside diff comments:
In
`@mobile/app/src/main/java/com/smartjam/app/data/local/entity/ConnectionEntity.kt`:
- Around line 9-17: The data classes ConnectionEntity and Connection currently
hold peerAvatarBytes: ByteArray?, so their auto-generated equals/hashCode
compare the byte arrays by reference; to fix, implement custom equals(other:
Any?) and hashCode() in both ConnectionEntity and Connection that compare
peerAvatarBytes using peerAvatarBytes?.contentEquals(other.peerAvatarBytes)
(handling nulls) and compute hashCode with peerAvatarBytes?.contentHashCode()
combined with other properties, or alternatively change peerAvatarBytes to an
immutable type that compares by content (e.g., List<Byte> or a ByteString) and
update any mapping code that converts between ConnectionEntity and Connection
accordingly.
In
`@mobile/app/src/main/java/com/smartjam/app/ui/screens/register/RegisterViewModel.kt`:
- Around line 84-86: The error text shown when passwordRegex fails is
misleading: passwordRegex (defined near line 40) requires at least one special
character from [#?!@$%^&*-] but the _state.update call sets errorMessage to only
mention letters and digits; update the errorMessage in the failing branch (where
_state.update is called) to list the actual requirements (e.g., minimum 8
characters, Latin letters, digits, and at least one special character like
#?!@$%^&*-) so it matches passwordRegex and users know to include a special
character.
- Around line 104-110: In RegisterViewModel the failure branch after checking
result.isSuccess only updates errorMessage and never resets isLoading, causing
the UI to remain in a loading state; update the failure branch (the else after
if (result.isSuccess)) to also set _state.update { it.copy(isLoading = false,
errorMessage = error) } or move the isLoading=false logic into a shared
finally/cleanup section (similar to LoginViewModel) so _state.isLoading is
always cleared whether registration succeeds (and you send
RegisterEvent.NavigateToHome) or fails.
🪄 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: 2bf4c4ef-aad0-4556-8052-aa86b342aa7d
📒 Files selected for processing (37)
mobile/app/build.gradle.ktsmobile/app/src/androidTest/java/com/smartjam/app/ConnectionSeedInstrumentedTest.ktmobile/app/src/main/AndroidManifest.xmlmobile/app/src/main/java/com/smartjam/app/MainActivity.ktmobile/app/src/main/java/com/smartjam/app/api.yamlmobile/app/src/main/java/com/smartjam/app/common-models.yamlmobile/app/src/main/java/com/smartjam/app/data/api/AuthAuthenticator.ktmobile/app/src/main/java/com/smartjam/app/data/api/InstantAdapter.ktmobile/app/src/main/java/com/smartjam/app/data/local/AudioFileStore.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/AssignmentDao.ktmobile/app/src/main/java/com/smartjam/app/data/local/dao/ConnectionDao.ktmobile/app/src/main/java/com/smartjam/app/data/local/dao/SubmissionResultDao.ktmobile/app/src/main/java/com/smartjam/app/data/local/entity/AssignmentEntity.ktmobile/app/src/main/java/com/smartjam/app/data/local/entity/ConnectionEntity.ktmobile/app/src/main/java/com/smartjam/app/data/local/entity/SubmissionResultEntity.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/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/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/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/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/gradlew
💤 Files with no reviewable changes (7)
- mobile/app/src/main/java/com/smartjam/app/data/model/LoginResponse.kt
- mobile/app/src/main/java/com/smartjam/app/data/model/ConnectionModels.kt
- mobile/app/src/main/java/com/smartjam/app/data/model/RefreshRequest.kt
- mobile/app/src/main/java/com/smartjam/app/data/model/LoginRequest.kt
- mobile/app/src/main/java/com/smartjam/app/data/model/CommentModels.kt
- mobile/app/src/main/java/com/smartjam/app/data/model/TaskModels.kt
- mobile/app/src/main/java/com/smartjam/app/data/model/RegisterRequest.kt
| <uses-permission android:name="android.permission.ACCESS_NETWORK_STATE" /> | ||
|
|
||
| <application | ||
| android:usesCleartextTraffic="true" |
There was a problem hiding this comment.
Do not enable cleartext traffic globally.
android:usesCleartextTraffic="true" permits HTTP for all app traffic and materially weakens credential/token transport security.
Suggested hardening
- android:usesCleartextTraffic="true"
+ android:usesCleartextTraffic="false"Use a debug-only network security config if local HTTP is required during development.
🤖 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/AndroidManifest.xml` at line 9, The manifest enables
cleartext globally via android:usesCleartextTraffic="true", which weakens
transport security; remove that global attribute and instead create a network
security config (e.g., res/xml/network_security_config.xml) that enables
cleartext only for debug builds and disallows it for release, then reference
that config from the <application> tag using
android:networkSecurityConfig="`@xml/network_security_config`" via
build-variant-specific manifests or manifest placeholders so cleartext is
allowed only in debug and not in production.
| type: array | ||
| items: | ||
| type: string | ||
| example: [ "STUDENT" ] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="mobile/app/src/main/java/com/smartjam/app/api.yaml"
# 1) Show the exact region around the reported lines
echo "---- Context around lines 520-560 ----"
nl -ba "$FILE" | sed -n '500,580p'
# 2) Locate the UserResponse schema section and the specific "roles" field
echo
echo "---- Find UserResponse and roles field ----"
rg -n "UserResponse" "$FILE"
rg -n "roles:" "$FILE"
# Print the blocks near each roles occurrence
echo
echo "---- Context around each 'roles:' occurrence ----"
rg -n -C 6 "roles:" "$FILE"
# 3) Check whether UserRole schema exists and what it looks like
echo
echo "---- Find UserRole schema definition ----"
rg -n "UserRole" "$FILE"
echo
echo "---- Context around '`#/components/schemas/UserRole`' refs (if any) ----"
rg -n -C 4 "`#/components/schemas/UserRole`" "$FILE" || true
echo
echo "---- Show components/schemas section where UserRole might live ----"
# Try to find components/schemas: and then show a window around it
rg -n "components:" "$FILE"
rg -n -C 4 "schemas:" "$FILE"
# Show a reasonable window after schemas: (limit output)
start=$(rg -n "components:" "$FILE" | head -n1 | cut -d: -f1)
if [[ -n "${start:-}" ]]; then
echo "---- Window around components->schemas ----"
nl -ba "$FILE" | sed -n "$((start-10)),$((start+200))p" | head -n 260
fiRepository: Satlykovs/SmartJam
Length of output: 162
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="mobile/app/src/main/java/com/smartjam/app/api.yaml"
num_lines() { awk '{printf "%d\t%s\n", NR, $0}'; }
echo "---- Context around lines 500-580 (with line numbers) ----"
num_lines < "$FILE" | sed -n '500,580p'
echo
echo "---- Find UserResponse schema section ----"
rg -n "UserResponse" "$FILE" || true
echo
echo "---- Find 'roles:' occurrences ----"
rg -n "roles:" "$FILE" || true
echo
echo "---- Context around each 'roles:' occurrence ----"
# limit context output volume
rg -n -C 6 "roles:" "$FILE" || true
echo
echo "---- Find UserRole definition and refs ----"
rg -n "UserRole" "$FILE" || true
echo
rg -n -C 4 "`#/components/schemas/UserRole`" "$FILE" || true
echo
echo "---- Locate components/schemas region ----"
rg -n "components:" "$FILE" || true
rg -n "schemas:" "$FILE" || true
# Print a window after first schemas: occurrence
schemas_line=$(rg -n "schemas:" "$FILE" | head -n1 | cut -d: -f1 || true)
if [[ -n "${schemas_line:-}" ]]; then
start=$((schemas_line-20))
end=$((schemas_line+240))
echo
echo "---- Window around components/schemas (approx $start-$end) ----"
num_lines < "$FILE" | sed -n "${start},${end}p"
fiRepository: Satlykovs/SmartJam
Length of output: 10969
Use UserRole enum for UserResponse.roles items.
UserResponse.roles is currently array<string> (items: { type: string }), which allows invalid role values; components.schemas.UserRole already defines enum: [STUDENT, TEACHER]. Update roles.items to reference it.
Contract fix
roles:
type: array
items:
- type: string
+ $ref: '`#/components/schemas/UserRole`'📝 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.
| type: array | |
| items: | |
| type: string | |
| example: [ "STUDENT" ] | |
| type: array | |
| items: | |
| $ref: '`#/components/schemas/UserRole`' | |
| example: [ "STUDENT" ] |
🧰 Tools
🪛 Checkov (3.2.529)
[medium] 531-537: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🪛 YAMLlint (1.38.0)
[error] 534-534: too many spaces inside brackets
(brackets)
[error] 534-534: too many spaces inside brackets
(brackets)
🤖 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/api.yaml` around lines 531 - 534,
The roles array in UserResponse is typed as plain strings which allows invalid
values; change the items definition to reference the existing enum schema by
replacing the current items: { type: string } with an item $ref to the UserRole
schema (referencing components.schemas.UserRole, e.g. $ref:
'`#/components/schemas/UserRole`') so UserResponse.roles uses the UserRole enum.
| teacherStartTime: | ||
| type: number | ||
| format: double | ||
| description: Start time of the event in the teacher's reference track (seconds). | ||
| teacherEndTime: | ||
| type: number | ||
| format: double | ||
| description: End time of the event in the teacher's reference track (seconds). | ||
| studentStartTime: | ||
| type: number | ||
| format: double | ||
| description: Start time of the event in the student's submission (seconds). | ||
| studentEndTime: | ||
| type: number | ||
| format: double | ||
| description: End time of the event in the student's submission (seconds). |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="mobile/app/src/main/java/com/smartjam/app/common-models.yaml"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE"
exit 1
fi
echo "=== file head (first 120 lines) ==="
nl -ba "$FILE" | head -n 120
echo "=== file lines 30-90 ==="
nl -ba "$FILE" | sed -n '30,90p'Repository: Satlykovs/SmartJam
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="mobile/app/src/main/java/com/smartjam/app/common-models.yaml"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE"
exit 1
fi
echo "=== file lines 30-90 (with numbers via awk) ==="
awk 'NR>=30 && NR<=90 {printf "%5d\t%s\n", NR, $0}' "$FILE"
echo "=== locate time field keys ==="
rg -n "teacherStartTime|teacherEndTime|studentStartTime|studentEndTime" "$FILE" || true
echo "=== locate existing numeric bounds keywords ==="
rg -n "minimum:|exclusiveMinimum:|nonNegative|minimum\\b" "$FILE" || trueRepository: Satlykovs/SmartJam
Length of output: 1973
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "teacherStartTime|teacherEndTime|studentStartTime|studentEndTime" . || true
echo "=== search for negative timestamp checks ==="
rg -n "negative|must be (>=|greater than|at least)|exclusiveMinimum|nonNegative|min(imum)?\\s*:\\s*[0-9]" mobile/app/src/main/java/com/smartjam/app/common-models.yaml . || trueRepository: Satlykovs/SmartJam
Length of output: 3163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== openapi-spec/common-models.yaml lines 30-80 ==="
FILE="openapi-spec/common-models.yaml"
awk 'NR>=30 && NR<=80 {printf "%5d\t%s\n", NR, $0}' "$FILE"
echo "=== openapi-spec/api.yaml lines 340-420 ==="
FILE="openapi-spec/api.yaml"
awk 'NR>=340 && NR<=420 {printf "%5d\t%s\n", NR, $0}' "$FILE"Repository: Satlykovs/SmartJam
Length of output: 4095
🏁 Script executed:
#!/bin/bash
set -euo pipefail
A="mobile/app/src/main/java/com/smartjam/app/common-models.yaml"
B="openapi-spec/common-models.yaml"
echo "=== compare FeedbackEvent time field blocks (mobile vs openapi-spec) ==="
python3 - <<'PY'
import itertools
A="mobile/app/src/main/java/com/smartjam/app/common-models.yaml"
B="openapi-spec/common-models.yaml"
def extract(path):
out=[]
with open(path,'r',encoding='utf-8') as f:
lines=f.readlines()
# grab around teacherStartTime through studentEndTime
for i,l in enumerate(lines):
if "teacherStartTime:" in l:
start=i-1
break
else:
raise SystemExit("teacherStartTime not found in "+path)
# extend until after studentEndTime description block
end=None
for j in range(start+1,len(lines)):
if "studentEndTime:" in lines[j]:
# find next 2-level indent end by stopping after description line following it
# just take until next key at same indent level ("type:" for event) if present
pass
# simpler: take until line containing "type:" with two more spaces than others (event schema field)
for j in range(start,len(lines)):
if lines[j].startswith(" type:") or lines[j].startswith(" type:"):
end=j
break
if end is None:
end=start+40
return "".join(lines[start:end])
print("--- mobile ---")
print(extract(A))
print("--- openapi-spec ---")
print(extract(B))
PYRepository: Satlykovs/SmartJam
Length of output: 1493
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "teacherStartTime\\(|teacherEndTime\\(|studentStartTime\\(|studentEndTime\\(" backend mobile || true
echo "=== search for numeric guards on time fields ==="
rg -n "(StartTime|EndTime).*\\b(>=|>|greater than|non-negative|nonnegative|>=\\s*0|<\\s*0|negative)" backend mobile || true
rg -n "\\bminimum\\b|exclusiveMinimum|nonNegative" backend mobile openapi-spec || trueRepository: Satlykovs/SmartJam
Length of output: 1361
Add non-negative constraints for feedback timestamp fields
FeedbackEvent.teacherStartTime, teacherEndTime, studentStartTime, and studentEndTime are defined without any minimum, so negative timestamps are allowed by the OpenAPI schema. Ensure the same change is applied to both mobile/app/src/main/java/com/smartjam/app/common-models.yaml and openapi-spec/common-models.yaml.
Schema hardening
teacherStartTime:
type: number
format: double
+ minimum: 0
teacherEndTime:
type: number
format: double
+ minimum: 0
studentStartTime:
type: number
format: double
+ minimum: 0
studentEndTime:
type: number
format: double
+ minimum: 0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| teacherStartTime: | |
| type: number | |
| format: double | |
| description: Start time of the event in the teacher's reference track (seconds). | |
| teacherEndTime: | |
| type: number | |
| format: double | |
| description: End time of the event in the teacher's reference track (seconds). | |
| studentStartTime: | |
| type: number | |
| format: double | |
| description: Start time of the event in the student's submission (seconds). | |
| studentEndTime: | |
| type: number | |
| format: double | |
| description: End time of the event in the student's submission (seconds). | |
| teacherStartTime: | |
| type: number | |
| format: double | |
| minimum: 0 | |
| description: Start time of the event in the teacher's reference track (seconds). | |
| teacherEndTime: | |
| type: number | |
| format: double | |
| minimum: 0 | |
| description: End time of the event in the teacher's reference track (seconds). | |
| studentStartTime: | |
| type: number | |
| format: double | |
| minimum: 0 | |
| description: Start time of the event in the student's submission (seconds). | |
| studentEndTime: | |
| type: number | |
| format: double | |
| minimum: 0 | |
| description: End time of the event in the student's submission (seconds). |
🤖 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/common-models.yaml` around lines 44
- 59, The four timestamp properties teacherStartTime, teacherEndTime,
studentStartTime, and studentEndTime allow negative values; update their OpenAPI
schema entries to enforce non-negative values by adding "minimum: 0" to each
property in the common-models YAML(s) so both copies of the schema are hardened
(ensure you modify the definitions for teacherStartTime, teacherEndTime,
studentStartTime and studentEndTime).
| private fun toApiRole(role: String): com.smartjam.app.model.UserRole? { | ||
| return try { | ||
| com.smartjam.app.model.UserRole.valueOf(role) | ||
| } catch (e: Exception) { | ||
| null |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Narrow exception handling in toApiRole.
Catching Exception is too broad here; valueOf should be guarded with IllegalArgumentException only.
Refactor
private fun toApiRole(role: String): com.smartjam.app.model.UserRole? {
return try {
com.smartjam.app.model.UserRole.valueOf(role)
- } catch (e: Exception) {
+ } catch (e: IllegalArgumentException) {
null
}
}📝 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.
| private fun toApiRole(role: String): com.smartjam.app.model.UserRole? { | |
| return try { | |
| com.smartjam.app.model.UserRole.valueOf(role) | |
| } catch (e: Exception) { | |
| null | |
| private fun toApiRole(role: String): com.smartjam.app.model.UserRole? { | |
| return try { | |
| com.smartjam.app.model.UserRole.valueOf(role) | |
| } catch (e: IllegalArgumentException) { | |
| null | |
| } | |
| } |
🧰 Tools
🪛 detekt (1.23.8)
[warning] 89-89: The caught exception is too generic. Prefer catching specific exceptions to the case that is currently handled.
(detekt.exceptions.TooGenericExceptionCaught)
[warning] 89-89: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🤖 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/AuthAuthenticator.kt`
around lines 86 - 90, The toApiRole function is catching Exception too broadly;
narrow it to only catch IllegalArgumentException thrown by
com.smartjam.app.model.UserRole.valueOf(role) so invalid enum names return null
without masking other errors—update the try/catch in toApiRole to catch
IllegalArgumentException (or use runCatching { UserRole.valueOf(role)
}.getOrNull()) and keep the current null-on-failure behavior.
| return try { | ||
| Instant.parse(s) | ||
| } catch (e: Exception) { | ||
| null |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="mobile/app/src/main/java/com/smartjam/app/data/api/InstantAdapter.kt"
echo "== File exists =="
ls -l "$FILE" || true
echo "== Relevant lines (20-60) =="
nl -ba "$FILE" | sed -n '1,120p' | sed -n '20,60p' || true
echo "== Full file (if <=200 lines) =="
LINES=$(wc -l < "$FILE" 2>/dev/null || echo 0)
echo "Line count: $LINES"
if [ "$LINES" -le 200 ]; then
nl -ba "$FILE"
fi
echo "== Search for InstantAdapter usage =="
rg -n "InstantAdapter" -S mobile/app/src/main/java || true
echo "== Search for parse failure handling patterns for adapters =="
rg -n "catch \\(e: Exception\\)" mobile/app/src/main/java/com/smartjam/app/data/api || true
rg -n "return null" mobile/app/src/main/java/com/smartjam/app/data/api || trueRepository: Satlykovs/SmartJam
Length of output: 378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="mobile/app/src/main/java/com/smartjam/app/data/api/InstantAdapter.kt"
echo "== File exists =="
ls -l "$FILE" || true
echo "== File contents with line numbers (entire file) =="
awk '{printf "%4d:%s\n", NR, $0}' "$FILE"
echo "== Search for InstantAdapter usage =="
rg -n "InstantAdapter" mobile/app/src/main/java || true
echo "== Search for parse failure handling patterns for adapters in data/api =="
rg -n "catch \\(e: Exception\\)" mobile/app/src/main/java/com/smartjam/app/data/api || true
rg -n "return null" mobile/app/src/main/java/com/smartjam/app/data/api || trueRepository: Satlykovs/SmartJam
Length of output: 2197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "registerTypeAdapter\\(Instant::class.java, InstantAdapter\\(" -S mobile/app/src/main/java || true
rg -n "\\bInstant\\b" mobile/app/src/main/java --glob='*.kt' || true
echo "== MainActivity snippet around Gson builder =="
FILE="mobile/app/src/main/java/com/smartjam/app/MainActivity.kt"
if [ -f "$FILE" ]; then
awk 'NR>=1 && NR<=120 {printf "%4d:%s\n", NR, $0}' "$FILE"
fi
echo "== AuthAuthenticator snippet around existing catch blocks =="
FILE="mobile/app/src/main/java/com/smartjam/app/data/api/AuthAuthenticator.kt"
if [ -f "$FILE" ]; then
wc -l "$FILE"
awk 'NR>=1 && NR<=140 {printf "%4d:%s\n", NR, $0}' "$FILE"
fiRepository: Satlykovs/SmartJam
Length of output: 11995
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find all occurrences of 'Instant' in model/api DTOs =="
rg -n "\\bInstant\\b" mobile/app/src/main/java/com/smartjam/app/model mobile/app/src/main/java/com/smartjam/app/api mobile/app/src/main/java/com/smartjam/app/data/api || true
echo "== List Kotlin files in model that mention Instant (for targeted reading) =="
fd -e kt ".*" mobile/app/src/main/java/com/smartjam/app/model 2>/dev/null | head -n 200 | wc -l || true
rg -l "\\bInstant\\b" mobile/app/src/main/java/com/smartjam/app/model --glob="*.kt" || true
echo "== Inspect any model property declarations with Instant around those files =="
for f in $(rg -l "\\bInstant\\b" mobile/app/src/main/java/com/smartjam/app/model --glob="*.kt"); do
echo "--- $f ---"
# print just regions around "Instant" lines
rg -n "\\bInstant\\b" "$f" -n
# show ~20 lines around first few matches
for ln in $(rg -n "\\bInstant\\b" "$f" | head -n 5 | cut -d: -f1); do
start=$((ln-10))
end=$((ln+10))
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e {printf "%4d:%s\n", NR, $0}' "$f"
done
doneRepository: Satlykovs/SmartJam
Length of output: 1216
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Occurrences of java.time.Instant in repo (checked-in sources) =="
rg -n "java\\.time\\.Instant" -S . --glob='*.kt' || true
echo "== Occurrences of 'Instant?' (nullable) in checked-in sources =="
rg -n "\\bInstant\\?" -S mobile/app/src/main/java --glob='*.kt' || true
echo "== Occurrences of 'Instant' token in checked-in sources (kt only) =="
rg -n "\\bInstant\\b" -S mobile/app/src/main/java --glob='*.kt' || true
echo "== Search for any Json TypeAdapter that swallows parse errors =="
rg -n "TypeAdapter<|catch \\(e: Exception\\)|return null" mobile/app/src/main/java/com/smartjam/app/data/api --glob='*.kt' || trueRepository: Satlykovs/SmartJam
Length of output: 4720
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Tree (top 4 levels) under mobile/app/src/main/java/com/smartjam/app =="
find mobile/app/src/main/java/com/smartjam/app -maxdepth 4 -type d | sed -n '1,200p'
echo "== Find files under mobile/app/src/main/java/com/smartjam/app/api (if exists) =="
if [ -d "mobile/app/src/main/java/com/smartjam/app/api" ]; then
find mobile/app/src/main/java/com/smartjam/app/api -maxdepth 6 -type f | sed -n '1,200p'
else
echo "No directory: mobile/app/src/main/java/com/smartjam/app/api"
fi
echo "== Search for DTO fields named createdAt containing Instant or Instant? =="
rg -n "createdAt" mobile/app/src/main/java/com/smartjam/app --glob='*.kt' --glob='*.java' || true
echo "== Search for any Kotlin/Java types declaring Instant fields =="
rg -n "Instant" mobile/app/src/main/java/com/smartjam/app --glob='*.kt' --glob='*.java' || trueRepository: Satlykovs/SmartJam
Length of output: 5301
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for createdAt property declarations (type included) =="
rg -n "createdAt\\s*[:=]" mobile/app/src/main/java/com/smartjam/app --glob='*.kt' --glob='*.java'
echo "== Search for DTO/domain types containing createdAt usage =="
rg -n "dto\\.createdAt" mobile/app/src/main/java/com/smartjam/app --glob='*.kt' --glob='*.java'
echo "== Read RoomRepository around dto.createdAt assignments =="
FILE="mobile/app/src/main/java/com/smartjam/app/domain/repository/RoomRepository.kt"
if [ -f "$FILE" ]; then
# show around the matches lines 50-120, 140-220, 240-290 where createdAt appears
awk 'NR>=40 && NR<=120 {printf "%4d:%s\n", NR, $0}' "$FILE"
echo "----"
awk 'NR>=140 && NR<=220 {printf "%4d:%s\n", NR, $0}' "$FILE"
echo "----"
awk 'NR>=230 && NR<=310 {printf "%4d:%s\n", NR, $0}' "$FILE"
fi
echo "== Read ConnectionRepository around dto.createdAt assignments =="
FILE="mobile/app/src/main/java/com/smartjam/app/domain/repository/ConnectionRepository.kt"
if [ -f "$FILE" ]; then
awk 'NR>=1 && NR<=120 {printf "%4d:%s\n", NR, $0}' "$FILE"
fiRepository: Satlykovs/SmartJam
Length of output: 18888
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
mobile/app/src/main/java/com/smartjam/app/data/local/entity/AssignmentEntity.kt \
mobile/app/src/main/java/com/smartjam/app/data/local/entity/ConnectionEntity.kt \
mobile/app/src/main/java/com/smartjam/app/data/local/entity/SubmissionResultEntity.kt; do
echo "==== $f ===="
awk 'NR>=1 && NR<=120 {printf "%4d:%s\n", NR, $0}' "$f"
doneRepository: Satlykovs/SmartJam
Length of output: 2155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find DTO response types mentioning createdAt =="
rg -n "createdAt" mobile/app/src/main/java/com/smartjam/app/data/model --glob='*.kt' --glob='*.java' || true
rg -n "createdAt" mobile/app/src/main/java/com/smartjam/app/domain/model --glob='*.kt' --glob='*.java' || true
echo "== Find AssignmentResponse* definitions =="
rg -n "class AssignmentResponse|data class AssignmentResponse|AssignmentResponse" mobile/app/src/main/java/com/smartjam/app --glob='*.kt' --glob='*.java' || true
echo "== Find AssignmentResponseDetailed definitions =="
rg -n "AssignmentResponseDetailed" mobile/app/src/main/java/com/smartjam/app --glob='*.kt' --glob='*.java' || true
echo "== Find SubmissionResultResponse definitions =="
rg -n "SubmissionResultResponse" mobile/app/src/main/java/com/smartjam/app --glob='*.kt' --glob='*.java' || true
echo "== Find Connection response DTO definitions mentioning createdAt =="
rg -n "createdAt\\s*[:=]" mobile/app/src/main/java/com/smartjam/app/data/model --glob='*.kt' --glob='*.java' || trueRepository: Satlykovs/SmartJam
Length of output: 1172
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate definitions for AssignmentResponseDetailed / SubmissionResultResponse =="
rg -n "data class AssignmentResponseDetailed|class AssignmentResponseDetailed|AssignmentResponseDetailed\\b" -S mobile/app/src/main/java --glob='*.kt' --glob='*.java' || true
rg -n "data class SubmissionResultResponse|class SubmissionResultResponse|SubmissionResultResponse\\b" -S mobile/app/src/main/java --glob='*.kt' --glob='*.java' || true
echo "== Locate package com.smartjam.app.model =="
rg -n "^package com\\.smartjam\\.app\\.model\\b" -S mobile/app/src/main/java --glob='*.kt' --glob='*.java' || true
echo "== List directories under mobile/app/src/main/java/com/smartjam/app =="
find mobile/app/src/main/java/com/smartjam/app -maxdepth 2 -type d | sort
echo "== Search for 'createdAt' in any com/smartjam/app/model files (if present elsewhere) =="
rg -n "createdAt\\s*[:=]" -S mobile/app/src/main/java --glob='*.kt' --glob='*.java' | head -n 200Repository: Satlykovs/SmartJam
Length of output: 2630
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for AssignmentResponseDetailed symbol across repo =="
rg -n "AssignmentResponseDetailed" -S . --glob='*.kt' --glob='*.java' --glob='*.g.kt' || true
echo "== Search for SubmissionResultResponse symbol across repo =="
rg -n "SubmissionResultResponse" -S . --glob='*.kt' --glob='*.java' --glob='*.g.kt' || true
echo "== Search for createdAt in any *response* types =="
rg -n "createdAt" -S . --glob='*.kt' --glob='*.java' --glob='*.g.kt' --glob='*Response*' || true
echo "== List java/kotlin sources under mobile/app/src =="
find mobile/app/src -type d -maxdepth 4 | sort | head -n 200Repository: Satlykovs/SmartJam
Length of output: 6241
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the backend DTO model classes
rg -n "class AssignmentResponseDetailed" backend -S --glob='*.java' || true
rg -n "class SubmissionResultResponse" backend -S --glob='*.java' || true
echo "== Show AssignmentResponseDetailed.java (first 200 lines) =="
FILE_A=$(rg -l "class AssignmentResponseDetailed" backend --glob='*.java' | head -n 1 || true)
if [ -n "${FILE_A:-}" ] && [ -f "$FILE_A" ]; then
echo "$FILE_A"
head -n 200 "$FILE_A"
fi
echo "== Show SubmissionResultResponse.java (first 200 lines) =="
FILE_S=$(rg -l "class SubmissionResultResponse" backend --glob='*.java' | head -n 1 || true)
if [ -n "${FILE_S:-}" ] && [ -f "$FILE_S" ]; then
echo "$FILE_S"
head -n 200 "$FILE_S"
fiRepository: Satlykovs/SmartJam
Length of output: 182
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find files containing AssignmentResponseDetailed.java =="
find backend -type f -name '*AssignmentResponseDetailed*' -maxdepth 10 2>/dev/null || true
echo "== Find files containing SubmissionResultResponse.java =="
find backend -type f -name '*SubmissionResultResponse*' -maxdepth 10 2>/dev/null || true
echo "== Locate com.smartjam.api.model.AssignmentResponseDetailed source =="
rg -n "package com\\.smartjam\\.api\\.model" backend --glob='*.java' || true
rg -n "AssignmentResponseDetailed" backend --glob='*.java' | head -n 50 || true
echo "== Locate com.smartjam.api.model.SubmissionResultResponse source =="
rg -n "SubmissionResultResponse" backend --glob='*.java' | head -n 50 || trueRepository: Satlykovs/SmartJam
Length of output: 1943
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== AssignmentsService.java relevant section =="
FILE="backend/smartjam-api/src/main/java/com/smartjam/smartjamapi/service/AssignmentsService.java"
if [ -f "$FILE" ]; then
awk 'NR>=1 && NR<=140 {printf "%4d:%s\n", NR, $0}' "$FILE" | sed -n '50,120p'
fi
echo "== SubmissionsService.java relevant section =="
FILE="backend/smartjam-api/src/main/java/com/smartjam/smartjamapi/service/SubmissionsService.java"
if [ -f "$FILE" ]; then
awk 'NR>=1 && NR<=160 {printf "%4d:%s\n", NR, $0}' "$FILE" | sed -n '40,130p'
fi
echo "== Inspect other instantiations for createdAt in those files =="
rg -n "createdAt" backend/smartjam-api/src/main/java/com/smartjam/smartjamapi/service/{AssignmentsService,SubmissionsService}.java || trueRepository: Satlykovs/SmartJam
Length of output: 7269
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search backend for AssignmentResponse / SubmissionResponse / Connection response DTOs =="
rg -n "class (AssignmentResponse|SubmissionResponse|ConnectionResponse)\\b" backend --glob='*.java' || true
echo "== Search backend for 'createdAt' fields in response model classes =="
rg -n "createdAt" backend --glob='*.java' | head -n 200
echo "== List files under backend/smartjam-api/src/main/java/com/smartjam/api/model (if exists) =="
if [ -d "backend/smartjam-api/src/main/java/com/smartjam/api/model" ]; then
find backend/smartjam-api/src/main/java/com/smartjam/api/model -type f -maxdepth 3 | head -n 200
else
echo "No such directory: backend/smartjam-api/src/main/java/com/smartjam/api/model"
fi
echo "== Search for com.smartjam.api.model package declarations in backend =="
rg -n "^package com\\.smartjam\\.api\\.model" backend --glob='*.java' || trueRepository: Satlykovs/SmartJam
Length of output: 1739
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find directories matching *com/smartjam/api/model* =="
find backend -type d -path '*com/smartjam/api/model*' 2>/dev/null | head -n 200 || true
echo "== Search for 'package com.smartjam.api.model' across backend =="
rg -n "^package com\\.smartjam\\.api\\.model" backend --glob='*.java' --glob='*.kt' || true
echo "== Search for class names without restricting to module path =="
rg -n "AssignmentResponseDetailed|SubmissionResultResponse" backend --glob='*.java' --glob='*.kt' || true
echo "== Search for model sources under backend (directories) =="
find backend -maxdepth 5 -type d -name model 2>/dev/null | head -n 200 || trueRepository: Satlykovs/SmartJam
Length of output: 1932
Fail fast on invalid Instant JSON payloads in InstantAdapter.read()
InstantAdapter.read()currently catchesExceptionand returnsnull, masking malformed datetime payloads; this can propagate into non-null RoomcreatedAtfields (AssignmentEntity,ConnectionEntity,SubmissionResultEntity).
Fail-fast adapter behavior
+import com.google.gson.JsonParseException
+import java.time.format.DateTimeParseException
...
return try {
Instant.parse(s)
- } catch (e: Exception) {
- null
+ } catch (e: DateTimeParseException) {
+ throw JsonParseException("Invalid Instant: $s", e)
}📝 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.
| return try { | |
| Instant.parse(s) | |
| } catch (e: Exception) { | |
| null | |
| return try { | |
| Instant.parse(s) | |
| } catch (e: DateTimeParseException) { | |
| throw JsonParseException("Invalid Instant: $s", e) | |
| } |
🧰 Tools
🪛 detekt (1.23.8)
[warning] 26-26: The caught exception is too generic. Prefer catching specific exceptions to the case that is currently handled.
(detekt.exceptions.TooGenericExceptionCaught)
[warning] 26-26: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🤖 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/InstantAdapter.kt` around
lines 24 - 27, InstantAdapter.read() currently swallows all exceptions from
Instant.parse(s) and returns null, which can silently produce non-null Room
fields (AssignmentEntity, ConnectionEntity, SubmissionResultEntity); modify
InstantAdapter.read() to fail fast by not returning null on parse errors—catch
the specific parse exception (e.g., DateTimeParseException) or catch Exception
and rethrow a JsonDataException/IOException with a clear message including the
offending string, so deserialization fails immediately instead of propagating
nulls.
| val assignmentPicker = rememberLauncherForActivityResult( | ||
| contract = ActivityResultContracts.GetContent() | ||
| ) { uri: Uri? -> | ||
| uri?.let { | ||
| val file = File(context.cacheDir, "temp_assignment_upload.wav") | ||
| context.contentResolver.openInputStream(it)?.use { input -> | ||
| file.outputStream().use { output -> | ||
| input.copyTo(output) | ||
| } | ||
| } | ||
| viewModel.uploadAssignment(file, pendingAssignmentTitle, pendingAssignmentDescription.ifBlank { null }) | ||
| pendingAssignmentTitle = "" | ||
| pendingAssignmentDescription = "" | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing error handling for file copy operations.
If openInputStream or file write fails (e.g., disk full, permission denied), the exception propagates uncaught. Consider wrapping in a try-catch or validating the result before calling uploadAssignment.
🛡️ Suggested defensive approach
val assignmentPicker = rememberLauncherForActivityResult(
contract = ActivityResultContracts.GetContent()
) { uri: Uri? ->
uri?.let {
- val file = File(context.cacheDir, "temp_assignment_upload.wav")
- context.contentResolver.openInputStream(it)?.use { input ->
- file.outputStream().use { output ->
- input.copyTo(output)
+ try {
+ val file = File(context.cacheDir, "temp_assignment_upload.wav")
+ context.contentResolver.openInputStream(it)?.use { input ->
+ file.outputStream().use { output ->
+ input.copyTo(output)
+ }
+ } ?: return@let
+ viewModel.uploadAssignment(file, pendingAssignmentTitle, pendingAssignmentDescription.ifBlank { null })
+ } catch (e: Exception) {
+ // Handle error, e.g. show toast or set error state
+ } finally {
+ pendingAssignmentTitle = ""
+ pendingAssignmentDescription = ""
}
- }
- viewModel.uploadAssignment(file, pendingAssignmentTitle, pendingAssignmentDescription.ifBlank { null })
- pendingAssignmentTitle = ""
- pendingAssignmentDescription = ""
}
}🤖 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 63 - 77, Wrap the file copy and related operations inside the
assignmentPicker callback (the rememberLauncherForActivityResult lambda that
creates File(context.cacheDir, "temp_assignment_upload.wav")) in a try-catch so
IOExceptions (from context.contentResolver.openInputStream or
file.outputStream().use or input.copyTo) are caught; only call
viewModel.uploadAssignment when the copy succeeds, reset
pendingAssignmentTitle/Description in the success path, and in the catch block
log or surface the error (e.g., via viewModel error handler or a Toast) and
ensure any partially written temp file is deleted to avoid leaking corrupt
files.
| @Composable | ||
| private fun ErrorTimelineChart(feedback: List<FeedbackEvent>) { | ||
| val maxEnd = feedback.maxOfOrNull { it.teacherEndTime } ?: 1.0 | ||
| val height = 40.dp | ||
|
|
||
| GlassContainer { | ||
| Column(modifier = Modifier.padding(12.dp)) { | ||
| Text("График ошибок", color = Color.White, fontWeight = FontWeight.Bold) | ||
| Spacer(modifier = Modifier.height(8.dp)) | ||
| Canvas(modifier = Modifier.fillMaxWidth().height(height)) { | ||
| val width = size.width | ||
| feedback.forEach { event -> | ||
| val startX = (event.teacherStartTime / maxEnd).toFloat() * width | ||
| val endX = (event.teacherEndTime / maxEnd).toFloat() * width | ||
| val color = when (event.type) { | ||
| FeedbackType.WRONG_NOTE -> Color(0xFFFF5252) | ||
| FeedbackType.WRONG_RHYTHM -> Color(0xFFFFD166) | ||
| } | ||
| drawRect( | ||
| color = color, | ||
| topLeft = androidx.compose.ui.geometry.Offset(startX, 0f), | ||
| size = androidx.compose.ui.geometry.Size((endX - startX).coerceAtLeast(2f), size.height) | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Division by zero when all teacherEndTime values are zero.
If all feedback events have teacherEndTime = 0.0, then maxEnd = 0.0, and the division at lines 394-395 produces NaN or Infinity, causing incorrect rectangle positions.
🐛 Proposed fix
`@Composable`
private fun ErrorTimelineChart(feedback: List<FeedbackEvent>) {
- val maxEnd = feedback.maxOfOrNull { it.teacherEndTime } ?: 1.0
+ val maxEnd = (feedback.maxOfOrNull { it.teacherEndTime } ?: 1.0).coerceAtLeast(0.001)
val height = 40.dp📝 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.
| @Composable | |
| private fun ErrorTimelineChart(feedback: List<FeedbackEvent>) { | |
| val maxEnd = feedback.maxOfOrNull { it.teacherEndTime } ?: 1.0 | |
| val height = 40.dp | |
| GlassContainer { | |
| Column(modifier = Modifier.padding(12.dp)) { | |
| Text("График ошибок", color = Color.White, fontWeight = FontWeight.Bold) | |
| Spacer(modifier = Modifier.height(8.dp)) | |
| Canvas(modifier = Modifier.fillMaxWidth().height(height)) { | |
| val width = size.width | |
| feedback.forEach { event -> | |
| val startX = (event.teacherStartTime / maxEnd).toFloat() * width | |
| val endX = (event.teacherEndTime / maxEnd).toFloat() * width | |
| val color = when (event.type) { | |
| FeedbackType.WRONG_NOTE -> Color(0xFFFF5252) | |
| FeedbackType.WRONG_RHYTHM -> Color(0xFFFFD166) | |
| } | |
| drawRect( | |
| color = color, | |
| topLeft = androidx.compose.ui.geometry.Offset(startX, 0f), | |
| size = androidx.compose.ui.geometry.Size((endX - startX).coerceAtLeast(2f), size.height) | |
| ) | |
| } | |
| } | |
| `@Composable` | |
| private fun ErrorTimelineChart(feedback: List<FeedbackEvent>) { | |
| val maxEnd = (feedback.maxOfOrNull { it.teacherEndTime } ?: 1.0).coerceAtLeast(0.001) | |
| val height = 40.dp | |
| GlassContainer { | |
| Column(modifier = Modifier.padding(12.dp)) { | |
| Text("График ошибок", color = Color.White, fontWeight = FontWeight.Bold) | |
| Spacer(modifier = Modifier.height(8.dp)) | |
| Canvas(modifier = Modifier.fillMaxWidth().height(height)) { | |
| val width = size.width | |
| feedback.forEach { event -> | |
| val startX = (event.teacherStartTime / maxEnd).toFloat() * width | |
| val endX = (event.teacherEndTime / maxEnd).toFloat() * width | |
| val color = when (event.type) { | |
| FeedbackType.WRONG_NOTE -> Color(0xFFFF5252) | |
| FeedbackType.WRONG_RHYTHM -> Color(0xFFFFD166) | |
| } | |
| drawRect( | |
| color = color, | |
| topLeft = androidx.compose.ui.geometry.Offset(startX, 0f), | |
| size = androidx.compose.ui.geometry.Size((endX - startX).coerceAtLeast(2f), size.height) | |
| ) | |
| } | |
| } |
🤖 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 382 - 406, The division by zero happens in ErrorTimelineChart where
maxEnd is computed from teacherEndTime; guard against maxEnd being 0.0 by using
a non-zero fallback (e.g., maxEndSafe = if (maxEnd <= 0.0) 1.0 else maxEnd) or
skip drawing when maxEnd is zero, then use maxEndSafe in the calculations for
startX/endX that reference teacherStartTime and teacherEndTime so no
NaN/Infinity is produced; update the references to use the new safe denominator
(and optionally coerce startX/endX within 0..width) in the drawRect logic.
| private val _uiState = kotlinx.coroutines.flow.MutableStateFlow(RoomUiState()) | ||
| val uiState = _uiState.asStateFlow() | ||
|
|
||
| private var submissionsJobs: MutableMap<UUID, Job> = mutableMapOf() |
There was a problem hiding this comment.
submissionsJobs map is never cleared, causing potential memory leak.
When the ViewModel is cleared, the stored Job references in submissionsJobs are not cleaned up. Override onCleared() to cancel all jobs and clear the map.
🐛 Proposed fix
+ override fun onCleared() {
+ super.onCleared()
+ submissionsJobs.values.forEach { it.cancel() }
+ submissionsJobs.clear()
+ }🤖 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`
at line 40, The submissionsJobs map holds Job references that aren't cleaned up;
override RoomViewModel.onCleared() to iterate submissionsJobs.values, call
cancel() on each Job, then clear the submissionsJobs map to avoid leaks;
implement the override inside the RoomViewModel class so any running coroutines
are cancelled and submissionsJobs.reset when the ViewModel is destroyed.
| submissions.forEach { submission -> | ||
| val hasFeedback = _uiState.value.feedbackBySubmission.containsKey(submission.id) | ||
| val needsDetailFetch = submission.pitchScore == null || submission.rhythmScore == null || !hasFeedback | ||
| if (needsDetailFetch) { | ||
| viewModelScope.launch { | ||
| val res = repository.getSubmissionResult(submission.id, assignmentId) | ||
| if (res.isSuccess) { | ||
| val dto = res.getOrNull()!! | ||
| val feedback = dto.feedback ?: emptyList() | ||
| _uiState.update { st -> | ||
| st.copy(feedbackBySubmission = st.feedbackBySubmission + (submission.id to feedback)) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Unbounded coroutine spawning can cause duplicate concurrent requests.
Every time the submissions flow emits, a new coroutine is launched for each submission needing detail fetch—even if a request is already in flight for that submission. This can lead to redundant API calls and race conditions updating feedbackBySubmission.
Track in-flight requests to avoid duplicates:
🐛 Suggested approach
+ private val fetchingSubmissionDetails = mutableSetOf<UUID>()
+
private fun observeSubmissions(assignmentId: UUID) {
submissionsJobs[assignmentId]?.cancel()
submissionsJobs[assignmentId] = viewModelScope.launch {
repository.getSubmissionsFlow(assignmentId).collect { submissions ->
_uiState.update { state ->
state.copy(
submissionsByAssignment = state.submissionsByAssignment + (assignmentId to submissions)
)
}
submissions.forEach { submission ->
val hasFeedback = _uiState.value.feedbackBySubmission.containsKey(submission.id)
val needsDetailFetch = submission.pitchScore == null || submission.rhythmScore == null || !hasFeedback
- if (needsDetailFetch) {
+ if (needsDetailFetch && fetchingSubmissionDetails.add(submission.id)) {
viewModelScope.launch {
- val res = repository.getSubmissionResult(submission.id, assignmentId)
- if (res.isSuccess) {
- val dto = res.getOrNull()!!
- val feedback = dto.feedback ?: emptyList()
- _uiState.update { st ->
- st.copy(feedbackBySubmission = st.feedbackBySubmission + (submission.id to feedback))
+ try {
+ val res = repository.getSubmissionResult(submission.id, assignmentId)
+ if (res.isSuccess) {
+ val dto = res.getOrNull()!!
+ val feedback = dto.feedback ?: emptyList()
+ _uiState.update { st ->
+ st.copy(feedbackBySubmission = st.feedbackBySubmission + (submission.id to feedback))
+ }
}
+ } finally {
+ fetchingSubmissionDetails.remove(submission.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 `@mobile/app/src/main/java/com/smartjam/app/ui/screens/room/RoomViewModel.kt`
around lines 180 - 194, The code spawns unbounded coroutines in
submissions.forEach (inside viewModelScope.launch) causing duplicate concurrent
repository.getSubmissionResult calls and racey updates to
_uiState.feedbackBySubmission; fix by adding an in-flight tracker (e.g., a
mutable Set like inFlightSubmissionFetches) checked before launching a fetch for
a submission.id, add the id to the set immediately when starting the fetch and
remove it in a finally block (or on completion/error) so duplicate launches are
skipped while a request is outstanding, and ensure the add/remove are done on
the same CoroutineScope/dispatcher (or guarded by a Mutex) to avoid races when
updating the in-flight set and when updating _uiState.
| val status = dto.status.name | ||
| if (status == "COMPLETED" || status == "FAILED") { | ||
| return@launch | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what type dto.status is and if SubmissionStatus enum exists
ast-grep --pattern 'enum class SubmissionStatus {
$$$
}'
rg -n 'SubmissionStatus' --type kotlinRepository: Satlykovs/SmartJam
Length of output: 44
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="mobile/app/src/main/java/com/smartjam/app/ui/screens/room/RoomViewModel.kt"
echo "== Context around target lines =="
# Print around line 190-230 (adjust after seeing actual file size if needed)
nl -ba "$FILE" | sed -n '180,240p'
echo
echo "== Search for dto.status usage =="
rg -n "dto\.status" mobile/app/src/main/java/com/smartjam/app/ui/screens/room/RoomViewModel.kt
echo
echo "== Search for the exact comparison =="
rg -n 'status == "COMPLETED"|status == "FAILED"|dto\.status\.name' mobile/app/src/main/java/com/smartjam/app/ui/screens/room/RoomViewModel.kt
echo
echo "== Search for occurrences of 'COMPLETED'/'FAILED' with dto.status =="
rg -n '"COMPLETED"|"FAILED"' mobile/app/src/main/java/com/smartjam/app -SRepository: Satlykovs/SmartJam
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="mobile/app/src/main/java/com/smartjam/app/ui/screens/room/RoomViewModel.kt"
echo "== File length =="
wc -l "$FILE"
echo
echo "== Context around target lines (180-240) =="
# Print with line numbers using awk
awk 'NR>=180 && NR<=240 {printf "%d:%s\n", NR, $0}' "$FILE"
echo
echo "== Search for dto.status usage in RoomViewModel.kt =="
rg -n "dto\.status" "$FILE" || true
echo
echo "== Search for the exact comparison (status == \"COMPLETED\"/\"FAILED\") =="
rg -n 'status == "COMPLETED"|status == "FAILED"|dto\.status\.name' "$FILE" || true
echo
echo "== Search for occurrences of \"COMPLETED\"/\"FAILED\" with dto.status in app =="
rg -n '"COMPLETED"|"FAILED"' mobile/app/src/main/java/com/smartjam/app -S || trueRepository: Satlykovs/SmartJam
Length of output: 3058
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate getSubmissionResult definition and its return type
rg -n "fun getSubmissionResult" -S mobile/app/src/main/java/com/smartjam/app || true
rg -n "getSubmissionResult\\(" -S mobile/app/src/main/java/com/smartjam/app || true
echo "----"
# Also search for a model/DTO containing ".status"
rg -n "\\.status\\.name" -S mobile/app/src/main/java/com/smartjam/app || true
rg -n "val status:" -S mobile/app/src/main/java/com/smartjam/app || true
rg -n "status\\s*:\\s*.*" -S mobile/app/src/main/java/com/smartjam/app || trueRepository: Satlykovs/SmartJam
Length of output: 2650
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show RoomRepository.kt around getSubmissionResult and relevant mappings
FILE="mobile/app/src/main/java/com/smartjam/app/domain/repository/RoomRepository.kt"
echo "== RoomRepository.kt length =="
wc -l "$FILE"
echo
echo "== RoomRepository.kt lines 150-240 =="
awk 'NR>=150 && NR<=240 {printf "%d:%s\n", NR, $0}' "$FILE"
echo
echo "== SubmissionResultEntity.kt =="
SUBE="mobile/app/src/main/java/com/smartjam/app/data/local/entity/SubmissionResultEntity.kt"
wc -l "$SUBE"
awk 'NR>=1 && NR<=120 {printf "%d:%s\n", NR, $0}' "$SUBE"
echo
echo "== Find SubmissionResultResponse type =="
rg -n "SubmissionResultResponse" mobile/app/src/main/java/com/smartjam/app -S
echo
echo "== Search for status property type definitions near models =="
# Try to locate the model file that declares SubmissionResultResponse
# We'll list files containing "class SubmissionResultResponse" or "data class SubmissionResultResponse"
rg -n "class SubmissionResultResponse|data class SubmissionResultResponse" mobile/app/src/main/java/com/smartjam/app -S || true
echo
echo "== Search for enums with COMPLETED/FAILED =="
rg -n "COMPLETED|FAILED" mobile/app/src/main/java/com/smartjam/app -S || trueRepository: Satlykovs/SmartJam
Length of output: 6503
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the model file path for SubmissionResultResponse, if present
echo "== Locate SubmissionResultResponse model files =="
rg -n "class SubmissionResultResponse|data class SubmissionResultResponse|interface SubmissionResultResponse" -S mobile/app/src/main/java/com/smartjam/app || true
echo
echo "== Locate usage of SubmissionResultResponse.status =="
rg -n "SubmissionResultResponse\\s*\\.|\\.status\\.name|val status\\s*=" -S mobile/app/src/main/java/com/smartjam/app || true
echo
echo "== Search for enum(s) containing COMPLETED/FAILED =="
rg -n "enum class .*COMPLETED|COMPLETED\\s*,|FAILED\\s*," -S mobile/app/src/main/java/com/smartjam/app || true
echo
echo "== Show common-models.yaml enum definition around COMPLETED/FAILED =="
YAML="mobile/app/src/main/java/com/smartjam/app/common-models.yaml"
if [ -f "$YAML" ]; then
rg -n "COMPLETED|FAILED" "$YAML"
echo
# Print a small window around those lines
awk 'NR>=1 && NR<=80 {print}' "$YAML"
fiRepository: Satlykovs/SmartJam
Length of output: 3019
🏁 Script executed:
#!/bin/bash
set -euo pipefail
YAML="mobile/app/src/main/java/com/smartjam/app/api.yaml"
echo "== api.yaml: find SubmissionResultResponse schema section =="
# Print around the SubmissionResultResponse block
# We'll locate the line number first, then print a window.
ln=$(rg -n "SubmissionResultResponse:" "$YAML" | head -n 1 | cut -d: -f1 || true)
if [ -n "${ln:-}" ]; then
start=$((ln-20))
end=$((ln+60))
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e {printf "%d:%s\n", NR, $0}' "$YAML"
else
echo "SubmissionResultResponse not found in api.yaml"
fi
echo
echo "== Search for AudioProcessingStatus usage in Kotlin sources =="
rg -n "AudioProcessingStatus" mobile/app/src/main/java/com/smartjam/app -S || true
rg -n "status\\s*=\\s*.*AudioProcessingStatus|\\.status\\.name" mobile/app/src/main/java/com/smartjam/app -S || true
echo
echo "== Search for 'status' property types referencing AudioProcessingStatus in repo =="
rg -n "status\\s*:\\s*.*AudioProcessingStatus|\\$ref: '`#/components/schemas/AudioProcessingStatus`'" "$YAML" -S || trueRepository: Satlykovs/SmartJam
Length of output: 4199
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="mobile/app/src/main/java/com/smartjam/app/domain/repository/RoomRepository.kt"
echo "== RoomRepository.kt imports (lines 1-60) =="
awk 'NR>=1 && NR<=70 {printf "%d:%s\n", NR, $0}' "$FILE"
echo
echo "== Search Kotlin for AudioProcessingStatus symbol (non-YAML) =="
rg -n "AudioProcessingStatus" mobile/app/src/main/java/com/smartjam/app -S --type kotlin || true
echo
echo "== Search Kotlin for '.status' enum comparison patterns =="
rg -n "\.status(\s*==|\.name)" mobile/app/src/main/java/com/smartjam/app -S --type kotlin || trueRepository: Satlykovs/SmartJam
Length of output: 3975
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Try to find any file mentioning AudioProcessingStatus/COMPLETED/FAILED in Kotlin sources.
echo "== fd: candidate files containing AudioProcessingStatus in name =="
fd -a -t f "AudioProcessingStatus*" mobile/app/src/main/java || true
echo
echo "== rg: Kotlin text for 'AudioProcessingStatus' across repo =="
rg -n "AudioProcessingStatus" mobile/app/src/main/java -S || true
echo
echo "== rg: Kotlin text for 'COMPLETED'/'FAILED' enums (rough) =="
rg -n "enum class .*COMPLETED|AudioProcessingStatus\\..*(COMPLETED|FAILED)|AudioProcessingStatus\\b|COMPLETED\\b|FAILED\\b" mobile/app/src/main/java -S --type kotlin || true
echo
echo "== Locate model package directory contents (top level) =="
ls -R mobile/app/src/main/java/com/smartjam/app/model 2>/dev/null | head -n 200 || trueRepository: Satlykovs/SmartJam
Length of output: 1097
Prefer enum comparison for dto.status instead of string literals
SubmissionResultResponse.status is defined as AudioProcessingStatus, but the code compares dto.status.name to "COMPLETED"/"FAILED", which is fragile. Compare the enum values directly.
♻️ Suggested approach
- val status = dto.status.name
- if (status == "COMPLETED" || status == "FAILED") {
+ if (dto.status == AudioProcessingStatus.COMPLETED || dto.status == AudioProcessingStatus.FAILED) {
return@launch
}📝 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.
| val status = dto.status.name | |
| if (status == "COMPLETED" || status == "FAILED") { | |
| return@launch | |
| } | |
| if (dto.status == AudioProcessingStatus.COMPLETED || dto.status == AudioProcessingStatus.FAILED) { | |
| return@launch | |
| } |
🤖 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 210 - 213, Replace the fragile string comparison of dto.status.name
with direct enum comparisons against AudioProcessingStatus: inside the launch
block in RoomViewModel where SubmissionResultResponse dto is handled, change the
checks that test status == "COMPLETED" || status == "FAILED" to compare
dto.status to AudioProcessingStatus.COMPLETED and AudioProcessingStatus.FAILED
(or use a when/contains check) so the logic uses the enum values directly.
3 participants
This pull request introduces several significant improvements and new features to the mobile app, focusing on expanding local data storage, enhancing authentication, and improving API and network handling. The update includes a database schema upgrade to support assignments and submission results, adds new DAOs and entities, improves token handling with user roles, and enhances API client configuration. Additionally, new utility classes and dependencies are introduced, and a comprehensive OpenAPI contract is added for feedback events.
Database and Local Storage Enhancements:
SmartJamDatabaseto version 6, adding support forAssignmentEntityandSubmissionResultEntity, and included corresponding DAOs (AssignmentDao,SubmissionResultDao). [1] [2]AudioFileStoreutility for managing assignment and submission audio files on disk.Authentication and Token Management:
TokenStorageto persist user roles alongside tokens, and provided methods to save and retrieve the role. [1] [2]AuthAuthenticatorto send the stored user role during token refresh and to save the role with new tokens. [1] [2]API Client and Networking Improvements:
MainActivityby adding a customInstantAdapterfor date/time serialization, attaching logging and authorization interceptors, and dynamically updating the bearer token when it changes. [1] [2] [3]AssignmentsApi,SubmissionsApi) and repositories, and ensured navigation to login on missing refresh token. [1] [2] [3]coil-compose).2.7.0-alpha11.Testing and Permissions:
AndroidManifest.xmlto request necessary network permissions and allow cleartext traffic.API Contract and Serialization:
common-models.yamlOpenAPI contract for feedback event schemas, supporting future integration and validation.InstantAdapterfor proper handling ofInstantserialization with Gson.These changes collectively improve the app's robustness, scalability, and maintainability, especially around data management and authentication.
References: [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16] [17] [18]