Conversation
📝 WalkthroughWalkthroughThe changes add empty solution validation in the API layer, refactor riddle eligibility checks in the business logic to use a "nextRiddles" set-based approach, and introduce a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.kt (2)
202-206: Eligibility check ordering is inconsistent withsubmitRiddleForGroup.Here the submission is fetched on line 202 before the
NOT_SOLVABLEcheck on lines 204–205, so a request for a non-eligible riddle still incurs a DB lookup.submitRiddleForGroupalready does the check first (lines 290–291) and only queriesfindMappingByOwnerGroupIdAndRiddleIdafterwards on line 293. Move the check above the fetch to match.♻️ Proposed reorder
- val submission = riddleCacheManager.findMappingByOwnerUserIdAndRiddleId(user.id, riddleId) - - if (riddle.id !in listRiddlesForUser(user).flatMap { it.nextRiddles }.map { it.id }) - return RiddleSubmissionView(status = RiddleSubmissionStatus.NOT_SOLVABLE) - + if (riddle.id !in listRiddlesForUser(user).flatMap { it.nextRiddles }.map { it.id }) + return RiddleSubmissionView(status = RiddleSubmissionStatus.NOT_SOLVABLE) + + val submission = riddleCacheManager.findMappingByOwnerUserIdAndRiddleId(user.id, riddleId)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.kt` around lines 202 - 206, The eligibility check is done after fetching the submission which causes an unnecessary lookup; move the riddle eligibility check (the if that uses listRiddlesForUser(user).flatMap { it.nextRiddles }...) to run before calling riddleCacheManager.findMappingByOwnerUserIdAndRiddleId(user.id, riddleId) so you only query when the riddle is eligible—mirror the ordering used in submitRiddleForGroup (check eligibility first, then call findMappingByOwnerGroupIdAndRiddleId).
204-206: Scope the eligibility lookup to the riddle's category; the inner null-return becomes dead code.Same concern applies to
submitRiddleForGrouplines 290–291. Two points:
Use the single-category helpers.
listRiddlesForUser(user)recomputesfindNextTofor every category just to flatten and check one id.getNextRiddlesUser(user, riddle)already exists and does exactly the right query for the riddle's category. Same forgetNextRiddlesGroup(groupId, riddle)in the group variant. This shrinks the work from O(categories × riddles_per_category) to O(riddles_in_one_category) on every submit.Now-redundant inner check. Once the new guard passes, the
elsebranch'sif (nextRiddles.find { it.id == riddle.id } == null) return null(line 232–233 for user, 319–320 for group) can never fire — the riddle is guaranteed eligible for its category. You can drop it (and reuse the freshly computed list) to avoid the duplicate work.♻️ Suggested shape (user variant; mirror for group)
- if (riddle.id !in listRiddlesForUser(user).flatMap { it.nextRiddles }.map { it.id }) - return RiddleSubmissionView(status = RiddleSubmissionStatus.NOT_SOLVABLE) + val nextRiddlesForCategory = getNextRiddlesUser(user, riddle) + if (nextRiddlesForCategory.none { it.id == riddle.id }) + return RiddleSubmissionView(status = RiddleSubmissionStatus.NOT_SOLVABLE) @@ - } else { - val nextRiddles = getNextRiddlesUser(user, riddle) - if (nextRiddles.find { it.id == riddle.id } == null) - return null - val userEntity = userService.getById(user.internalId) + } else { + val userEntity = userService.getById(user.internalId)Note: this also implicitly rejects re-submissions to already-solved riddles (since
findNextToexcludes completed ones) — which matches the PR's stated intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.kt` around lines 204 - 206, Replace the broad eligibility check that flattens listRiddlesForUser(user) with a category-scoped check using getNextRiddlesUser(user, riddle) (and mirror the change in submitRiddleForGroup to use getNextRiddlesGroup(groupId, riddle)); specifically, call getNextRiddlesUser/getNextRiddlesGroup to compute nextRiddles for the riddle's category, verify riddle.id is present there (return NOT_SOLVABLE if not), then reuse that nextRiddles list for the subsequent logic and remove the redundant inner null-check (the if nextRiddles.find { it.id == riddle.id } == null) since it can no longer occur.backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleApiController.kt (1)
106-108: ConsiderisBlank()to also reject whitespace-only solutions.
body.solution == ""lets whitespace-only payloads (e.g.," ","\t") through. SincecheckSolutionIsWrongstrips whitespace before comparison (RiddleBusinessLogicService.ktlines 365–378), such submissions effectively become empty downstream and would just be counted as aWRONGattempt rather than rejected at the API edge. UsingisBlank()keeps the controller guard consistent with the service-side normalization.♻️ Proposed change
- if (body.solution == "") - return ResponseEntity.status(HttpStatus.BAD_REQUEST).build() + if (body.solution.isBlank()) + return ResponseEntity.status(HttpStatus.BAD_REQUEST).build()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleApiController.kt` around lines 106 - 108, In RiddleApiController update the input validation to reject whitespace-only solutions by using Kotlin's isBlank(): replace the current check that compares body.solution == "" with body.solution.isBlank() so the controller rejects empty or whitespace-only payloads (keeping the same ResponseEntity.status(HttpStatus.BAD_REQUEST).build() behavior); this keeps the controller consistent with the normalization performed in RiddleBusinessLogicService.checkSolutionIsWrong.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.kt`:
- Around line 70-76: Remove the redundant multi-category precheck that calls
listRiddlesForUser(user) inside getRiddleForUser and likewise in
getRiddleForGroup; instead rely solely on the local eligibility check already
done by computing submissions via
riddleCacheManager.findAllMappingByOwnerUserIdAndRiddleCategoryId(...) and using
findNextTo(riddle.categoryId, submissions). Concretely, delete the conditional
that checks riddle.id against listRiddlesForUser(...).flatMap { it.nextRiddles
}... so the method simply computes submissions, calls findNextTo, and returns
nextRiddles.find { it.id == riddle.id }; apply the same removal in
getRiddleForGroup to avoid the expensive multi-category traversal and to
preserve access behavior for already-solved riddles.
In
`@backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleSubmissionView.kt`:
- Around line 10-11: The frontend RiddleSubmissionStatus union is missing
NOT_SOLVABLE which the backend (submitRiddleForUser / submitRiddleForGroup) can
return; add NOT_SOLVABLE to the TypeScript union in RiddleSubmissionStatus (in
riddle.view.ts) and update the submission handler and skip handler in
riddle.page.tsx to handle the NOT_SOLVABLE case by showing a localized message
(e.g., "this riddle is not currently available") via the same toast/localization
mechanism used for other statuses and ensuring any navigation/flow is consistent
with other non-success outcomes.
---
Nitpick comments:
In
`@backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleApiController.kt`:
- Around line 106-108: In RiddleApiController update the input validation to
reject whitespace-only solutions by using Kotlin's isBlank(): replace the
current check that compares body.solution == "" with body.solution.isBlank() so
the controller rejects empty or whitespace-only payloads (keeping the same
ResponseEntity.status(HttpStatus.BAD_REQUEST).build() behavior); this keeps the
controller consistent with the normalization performed in
RiddleBusinessLogicService.checkSolutionIsWrong.
In
`@backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.kt`:
- Around line 202-206: The eligibility check is done after fetching the
submission which causes an unnecessary lookup; move the riddle eligibility check
(the if that uses listRiddlesForUser(user).flatMap { it.nextRiddles }...) to run
before calling riddleCacheManager.findMappingByOwnerUserIdAndRiddleId(user.id,
riddleId) so you only query when the riddle is eligible—mirror the ordering used
in submitRiddleForGroup (check eligibility first, then call
findMappingByOwnerGroupIdAndRiddleId).
- Around line 204-206: Replace the broad eligibility check that flattens
listRiddlesForUser(user) with a category-scoped check using
getNextRiddlesUser(user, riddle) (and mirror the change in submitRiddleForGroup
to use getNextRiddlesGroup(groupId, riddle)); specifically, call
getNextRiddlesUser/getNextRiddlesGroup to compute nextRiddles for the riddle's
category, verify riddle.id is present there (return NOT_SOLVABLE if not), then
reuse that nextRiddles list for the subsequent logic and remove the redundant
inner null-check (the if nextRiddles.find { it.id == riddle.id } == null) since
it can no longer occur.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 57d95f9d-2e6c-4255-b7ae-1cc68674f4d4
📒 Files selected for processing (3)
backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleApiController.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleSubmissionView.kt
| if (riddle.id !in listRiddlesForUser(user).flatMap { it.nextRiddles }.map { it.id }) | ||
| return null | ||
|
|
||
| val submissions = riddleCacheManager.findAllMappingByOwnerUserIdAndRiddleCategoryId(user.id, riddle.categoryId) | ||
| return getRiddleIfAllowedToRead(riddle, submissions) | ||
| val nextRiddles = findNextTo(riddle.categoryId, submissions) | ||
| return nextRiddles.find { it.id == riddle.id } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether the frontend ever opens /riddle/solve/{id} for a riddle that may already be completed
fd -t f -e ts -e tsx | xargs rg -nP -C3 "/riddle/solve/"
# And whether the history view links into the solve route
rg -nP -C2 'riddle.*history|history.*riddle' --type=ts --type=tsxRepository: kir-dev/cmsch
Length of output: 85
🏁 Script executed:
#!/bin/bash
# Search for /riddle/solve/ references in TypeScript files (without tsx type specifier)
fd -t f -e ts | xargs rg -nP -C3 "/riddle/solve/" 2>/dev/null || echo "No matches found with .ts extension"
# Also search more broadly for riddle/solve patterns
rg -nP "riddle.*solve" --type=typescript --type=javascript 2>/dev/null || echo "Pattern search complete"
# Check for history-related views
rg -nP "history|completion" --type=typescript --type=javascript -i | grep -i riddle | head -20Repository: kir-dev/cmsch
Length of output: 170
🏁 Script executed:
#!/bin/bash
# List main directories to understand repo structure
ls -la | head -20
# Find all frontend/client code directories
find . -maxdepth 2 -type d -name "frontend" -o -name "client" -o -name "src" 2>/dev/null | head -10
# Search for any file containing /riddle/solve pattern regardless of extension
rg "/riddle/solve" --no-type-list 2>/dev/null | head -30
# Search for history endpoint usage patterns
rg "riddle.*history|solve.*history" --no-type-list -i 2>/dev/null | head -20Repository: kir-dev/cmsch
Length of output: 866
🏁 Script executed:
#!/bin/bash
# Search within frontend directory for /riddle/solve route patterns
rg "/riddle/solve" ./frontend/ 2>/dev/null | head -30
# Search for history-related components/views
rg "history.*riddle|riddle.*history" ./frontend/ -i 2>/dev/null | head -20
# Search for route definitions
rg "path.*riddle|riddle.*path|Route.*riddle" ./frontend/ -i -C2 2>/dev/null | head -50
# Look for navigation after completion
rg "complete|solved|finished" ./frontend/ -i -C2 2>/dev/null | grep -i "riddle\|navigate\|redirect" | head -20Repository: kir-dev/cmsch
Length of output: 7825
🏁 Script executed:
#!/bin/bash
# Check the getRiddleForUser and getRiddleForGroup methods
rg -A 15 "fun getRiddleForUser\|fun getRiddleForGroup" ./backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.kt
# Check the findNextTo implementation at line 420
rg -B 5 -A 10 "fun findNextTo" ./backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.kt
# Check what listRiddlesForUser does
rg -A 20 "fun listRiddlesForUser" ./backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.kt | head -40Repository: kir-dev/cmsch
Length of output: 1738
Redundant precheck with performance overhead; also prevents viewing already-solved riddles.
Two concerns with the guard on lines 70–76 (also applies to getRiddleForGroup lines 87–93):
-
Redundant and expensive. The precheck calls
listRiddlesForUser(user)to computenextRiddlesfor every category, then filters the result to a single id—pure overhead. Lines 73–75 already perform the same eligibility check locally for just the relevant category viafindNextTo(riddle.categoryId, submissions)andnextRiddles.find { it.id == riddle.id }returnsnullif the riddle isn't in the result. The precheck adds a multi-category traversal for no functional change. -
Blocks access to completed riddles.
findNextTo(line 85) filters out riddles whereit.completed == true, so after solving a riddle, any direct GET to/riddle/solve/{riddleId}will returnnull/404. Current frontend flows navigate to the next riddle or main list on completion, so this doesn't break normal UX. However, if users ever bookmark, deep-link, or history-nav to a solved riddle's solve view, it will 404. If blocking solve-view post-completion is intentional, this is fine; otherwise it's a usability regression.
♻️ Proposed simplification (drop the redundant precheck)
override fun getRiddleForUser(user: CmschUser, riddleId: Int): RiddleView? {
val riddle = riddleCacheManager.getRiddleById(riddleId) ?: return null
riddleCacheManager.findCategoryByCategoryIdAndVisibleTrueAndMinRoleAtMost(riddle.categoryId, user.role)
?: return null
- if (riddle.id !in listRiddlesForUser(user).flatMap { it.nextRiddles }.map { it.id })
- return null
-
val submissions = riddleCacheManager.findAllMappingByOwnerUserIdAndRiddleCategoryId(user.id, riddle.categoryId)
val nextRiddles = findNextTo(riddle.categoryId, submissions)
return nextRiddles.find { it.id == riddle.id }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.kt`
around lines 70 - 76, Remove the redundant multi-category precheck that calls
listRiddlesForUser(user) inside getRiddleForUser and likewise in
getRiddleForGroup; instead rely solely on the local eligibility check already
done by computing submissions via
riddleCacheManager.findAllMappingByOwnerUserIdAndRiddleCategoryId(...) and using
findNextTo(riddle.categoryId, submissions). Concretely, delete the conditional
that checks riddle.id against listRiddlesForUser(...).flatMap { it.nextRiddles
}... so the method simply computes submissions, calls findNextTo, and returns
nextRiddles.find { it.id == riddle.id }; apply the same removal in
getRiddleForGroup to avoid the expensive multi-category traversal and to
preserve access behavior for already-solved riddles.
| SUBMITTER_BANNED, | ||
| NOT_SOLVABLE |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the frontend enum and any pattern-matching against riddle submission statuses
fd -t f -e ts -e tsx | xargs rg -nP -C2 '\bRiddleSubmissionStatus\b'
# Find places that switch on status values
rg -nP -C2 "'(CORRECT|WRONG|CANNOT_SKIP|SUBMITTER_BANNED)'"Repository: kir-dev/cmsch
Length of output: 5127
🏁 Script executed:
#!/bin/bash
# Verify backend returns NOT_SOLVABLE status
rg -nP -C3 'NOT_SOLVABLE' backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/
# Check full riddle.page.tsx for any catch-all or error handling
fd -t f -name 'riddle.page.tsx' | xargs cat -n | head -200Repository: kir-dev/cmsch
Length of output: 3017
🏁 Script executed:
#!/bin/bash
# Get full riddle.page.tsx to check for error handling of unknown statuses
wc -l frontend/src/pages/riddle/riddle.page.tsx
cat -n frontend/src/pages/riddle/riddle.page.tsxRepository: kir-dev/cmsch
Length of output: 10356
Frontend RiddleSubmissionStatus is out of sync with backend enum — NOT_SOLVABLE not handled.
Backend returns NOT_SOLVABLE from submitRiddleForUser and submitRiddleForGroup (lines 205, 291 in RiddleBusinessLogicService.kt), but the frontend TypeScript union in frontend/src/util/views/riddle.view.ts only defines CORRECT, WRONG, CANNOT_SKIP, SUBMITTER_BANNED.
The riddle.page.tsx submission handler (lines 54–87) and skip handler (lines 102–120) have no case for NOT_SOLVABLE. When this status is received, all conditional checks fail and the user sees no toast, no navigation, and no feedback — a silent failure with confusing UX.
Add NOT_SOLVABLE to the TS union in riddle.view.ts and provide a localized message in both submission and skip handlers (e.g., "this riddle is not currently available").
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleSubmissionView.kt`
around lines 10 - 11, The frontend RiddleSubmissionStatus union is missing
NOT_SOLVABLE which the backend (submitRiddleForUser / submitRiddleForGroup) can
return; add NOT_SOLVABLE to the TypeScript union in RiddleSubmissionStatus (in
riddle.view.ts) and update the submission handler and skip handler in
riddle.page.tsx to handle the NOT_SOLVABLE case by showing a localized message
(e.g., "this riddle is not currently available") via the same toast/localization
mechanism used for other statuses and ensuring any navigation/flow is consistent
with other non-success outcomes.
1 participant
Implemented checks for solved and unavailable riddles when submitting a riddle.
Summary by CodeRabbit
Bug Fixes
Refactor