Repository navigation
Conversation
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="web/app.js">
<violation number="1" location="web/app.js:1028">
P2: Topic-filter redraws in random mode can select the current problem again because `filterSelectionChanged()` clears `avoidedProblem` before `recommend()`. Preserve the current card as the exclusion on random-mode redraws.</violation>
</file>
Reply to a comment to ask cubic a question or push back. It learns from your replies.
Turn on auto-fix | Re-trigger cubic
| undefined, | ||
| avoidedProblem, | ||
| cards, | ||
| pickerMode, |
There was a problem hiding this comment.
P2: Topic-filter redraws in random mode can select the current problem again because filterSelectionChanged() clears avoidedProblem before recommend(). Preserve the current card as the exclusion on random-mode redraws.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At web/app.js, line 1028:
<comment>Topic-filter redraws in random mode can select the current problem again because `filterSelectionChanged()` clears `avoidedProblem` before `recommend()`. Preserve the current card as the exclusion on random-mode redraws.</comment>
<file context>
@@ -1021,6 +1025,7 @@ function recommend(note = "") {
undefined,
avoidedProblem,
cards,
+ pickerMode,
);
// Nothing to offer is still an answer, and it has to go through `setProblem`
</file context>
| keptDraw = null; | ||
| roll = Math.random(); | ||
| avoidedProblem = undefined; | ||
| pickerMode = "random"; |
There was a problem hiding this comment.
A level change is not an explicit random draw, but this switches it to uniform selection anyway. With a passed Hard problem and unseen Hard ones, checking Hard and unchecking the current level can now offer the passed problem first. That undoes "a problem already passed is not what gets recommended next", and the description does not mention it. If the goal is only to stop reviews crossing the selected levels, keep "recommend" here and restrict the review pool to the selected difficulties instead.
| undefined, | ||
| avoidedProblem, | ||
| cards, | ||
| pickerMode, |
There was a problem hiding this comment.
pickerMode is set to "random" and never goes back to "recommend", so after one click every later recommend() draws uniformly: filterSelectionChanged(), sign-in, report deletion and every settle(). Concrete case: click Random and get X, finish X, press Back. The bfcache restore refetches reports that now record X as passed, yet roll, avoidedProblem and the pool are unchanged, so settle() offers X again (and silently drops the reason keptDraw is cleared there). Reset to "recommend" wherever avoidedProblem is reset to undefined, and in settle() whenever the reports differ from the ones the random draw was made from (track them the way keptDraw does).
| return; | ||
| } | ||
| setProblem(choice.picked); | ||
| // Editing begins: When randomly selecting questions, display the difficulty level next to the question title. |
There was a problem hiding this comment.
"Editing begins:" reads like an editor marker. A comment here should say why random mode skips the review sentence, e.g. that a random draw is never a review and the level is named because it may differ from the suggested one.
| await setLevel(page, "Hard", true); | ||
| await setLevel(page, "Medium", false); | ||
| assert.equal((await cardInfo(page, (await snapshot(page)).card)).level, "Hard"); |
There was a problem hiding this comment.
The difficulty half of this test only runs after the Random click has already set pickerMode = "random", so deleting the assignment in the level handler still passes. It also checks only the level, not that the note lacks Review due. Exercise the level change from a fresh lobby with overdue reviews, as a separate test, and assert both.
Random draws previously prioritized overdue reviews, preventing unseen problems from being selected when due reviews were available. Select uniformly from the chosen difficulty and topic pool so every eligible problem has an equal chance, excluding the current problem when another is available. Preserve review priority for initial recommendations and show the drawn problem's difficulty so users can identify its level. Refs sysprog21#210 Co-authored-by: Jim Huang <jserv.tw@gmail.com>
jserv
left a comment
There was a problem hiding this comment.
Check CONTRIBUTING.md carefully and enforce the rules. In particular, the manner for writing git commit messages.
Explicit random selection currently shares the spaced-review recommendation policy. When multiple problems are overdue, repeated clicks can cycle through those problems indefinitely, excluding unseen problems and selecting reviews outside the chosen difficulties.
Separate explicit random selection from automatic recommendations. Random draws uniformly sample the problems matching the selected difficulties and topic filter, excluding the current problem whenever an alternative exists. Initial recommendations retain overdue-review priority across the broader review pool. Random selections also display the problem's difficulty.
Add regression tests for unseen-problem eligibility, difficulty filtering, current-problem exclusion, single-problem and empty pools, and the distinction between filtered practice and broader review recommendations. Update the lobby's expected selection text and add browser coverage for random selection, page restoration, and difficulty changes.
Validation:
node --test tests/browser/problem-picker.test.js tests/browser/lobby.test.jscompleted with 43 tests passing, no failures, and 112 browser tests skipped because Playwright Chromium was unavailable. The JavaScript syntax check andgit diff --cached --checkpassed. The required./scripts/test.shgate has not been run, and browser interaction remains unverified locally.Closes #210
Summary by cubic
Fixes #210 by separating explicit random selection from the spaced-review recommendation policy. Random draws now sample uniformly from the problems matching the selected difficulties and topic filter instead of cycling through overdue reviews, which previously left unseen problems unreachable.
Written for commit ac3968c. Summary will update on new commits.