Repository navigation
Conversation
bangyou0912
left a comment
There was a problem hiding this comment.
Two questions about undoing a selection and confirming it, and one about what the lobby test can tell apart.
|
Some review comments on this pull request are not settled yet. Please refine the change per the review comments listed below, reply on each thread with what you changed or why you did not, and mark each thread resolved yourself once it is settled (Resolve conversation, at the bottom of the thread). The pull request stays blocked while review threads are open. Not addressed at the current head:
Checked at 83c1742. |
83c1742 to
c5ad1b7
Compare
There was a problem hiding this comment.
All reported issues were addressed across 24 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
|
Some review comments on this pull request are not settled yet. Please refine the change per the review comments listed below, reply on each thread with what you changed or why you did not, and mark each thread resolved yourself once it is settled (Resolve conversation, at the bottom of the thread). The pull request stays blocked while review threads are open. Not addressed at the current head:
Checked at c5ad1b7. |
c5ad1b7 to
e052d08
Compare
Carry selected report improvements safely through interviews and replay while preserving account ownership and lobby settings. Change-Id: I974546bfeb9ffc69db5be0bab32220f46b647727
e052d08 to
db3562f
Compare
|
The
Each refusal and its fix: https://github.com/sysprog21/codetrial/blob/main/docs/large-change-explanation.md#troubleshooting |
jserv
left a comment
There was a problem hiding this comment.
Rebase the current branch onto the upstream default branch and resolve any conflicts. Run /codetrial-review in AI agents (use $codetrial-review in Codex) and resolve all identified issues. Rework the series into minimal, logically coherent commits, squash closely related commits, and ensure all commit messages follow the project’s rules.
| try { | ||
| const currentSession = await fetchJson("/api/session"); | ||
| const currentOwner = practiceFocusOwner(currentSession); | ||
| if (!currentOwner || currentOwner !== selectedPracticeFocusOwner) { |
There was a problem hiding this comment.
This check also runs for the automatic carry-forward focus, which has no owner of its own, so it changes the existing share path. Signed out with login required: tick Share this focus with Jim, type a GitHub username and press Start. recordGitHubLogin signs the page in, /api/session now answers with an account id, currentOwner is account:... while selectedPracticeFocusOwner is still device, and the box is unchecked and nothing is shared, with no message. The same happens when this /api/session call fails. Compare owners only when selectedPracticeFocus is set, and leave the automatic focus on its old path.
| } | ||
| } | ||
|
|
||
| export function readSelectedPracticeFocus(storage, owner) { |
There was a problem hiding this comment.
Nothing under web/ calls this; the lobby uses readSelectedPracticeFocusSelection, and only tests/browser/problem-picker.test.js imports this wrapper. Those tests therefore pin a function the page never runs. Delete it and point the tests at readSelectedPracticeFocusSelection(...)?.focus.
jserv
left a comment
There was a problem hiding this comment.
Rebase onto latest main branch and resolve conflicts.
|
#603 changes the lobby Start handler to handle blocked session storage and adds a rule against bare storage globals. This pull request changes the same handler to carry a selected practice focus. After #603 merges, please rebase onto |
| let Some(accounts) = state.accounts.as_ref() else { | ||
| return state.accounts_error(); | ||
| }; | ||
| let account_id = account_owner_id(accounts, user.account_identity); |
There was a problem hiding this comment.
current_user only returns Ok(Some(user)) when state.accounts is Some, so this else never runs, and binding accounts here is what pushes the whole json! body one level deeper. Leaving the arm as it was and computing the field in place keeps the diff to the one new line:
"user": {
"id": state.accounts.as_ref()
.map(|accounts| account_owner_id(accounts, user.account_identity)),
"login": user.login,
"avatarUrl": user.avatar_url
}| typeof data?.accountId !== "string" || | ||
| data.accountId !== expectedAccountId | ||
| ) { | ||
| if (generation !== historyLoad) return; |
There was a problem hiding this comment.
This repeats the generation check just above with no await in between, so it can never fire.
| if (generation !== historyLoad) return; |
| selectedPracticeFocus = focus; | ||
| selectedPracticeFocusSourceReportId = attempt.id || null; | ||
| storeSelectedPracticeFocus( | ||
| selectedPracticeFocusStorage, | ||
| selectedPracticeFocusOwner, | ||
| item, | ||
| attempt.interviewMode, | ||
| selectedPracticeFocusSourceReportId, | ||
| ); |
There was a problem hiding this comment.
The result of storeSelectedPracticeFocus is dropped here. When setItem throws, the lobby shows the new focus, but a reload restores whatever selection was stored before, or none. The interview and replay paths check the result and show "Could not save this practice focus."; do the same here. Those five lines of status handling now exist three times, so a helper beside storeSelectedPracticeFocus in problem-picker.js would keep them in one place.
| selectedPracticeFocus = focus; | |
| selectedPracticeFocusSourceReportId = attempt.id || null; | |
| storeSelectedPracticeFocus( | |
| selectedPracticeFocusStorage, | |
| selectedPracticeFocusOwner, | |
| item, | |
| attempt.interviewMode, | |
| selectedPracticeFocusSourceReportId, | |
| ); | |
| if ( | |
| !storeSelectedPracticeFocus( | |
| selectedPracticeFocusStorage, | |
| selectedPracticeFocusOwner, | |
| item, | |
| attempt.interviewMode, | |
| attempt.id || null, | |
| ) | |
| ) { | |
| const status = button.parentElement?.querySelector( | |
| "[data-practice-focus-status]", | |
| ); | |
| if (status) { | |
| status.hidden = false; | |
| status.textContent = "Could not save this practice focus."; | |
| } | |
| return; | |
| } | |
| selectedPracticeFocus = focus; | |
| selectedPracticeFocusSourceReportId = attempt.id || null; |
|
Some review comments on this pull request are not settled yet. Please refine the change per the review comments listed below, reply on each thread with what you changed or why you did not, and mark each thread resolved yourself once it is settled (Resolve conversation, at the bottom of the thread). The pull request stays blocked while review threads are open. Not addressed at the current head:
Checked at db3562f. |
Problem and behavior
Reports already suggest concrete items under Practice next, but the candidate could not choose one of those items to carry into the next interview. The lobby instead derived its practice focus automatically from report history.
This adds a Practice this action to each valid improvement-plan item. Choosing one carries that exact phase, weakness, drill, and success criterion into the lobby and gives it precedence over the automatic carry-forward focus for the next interview. If the candidate does not choose an item, the existing automatic behavior is unchanged.
The selection changes only the practice focus. It does not change the selected problem, difficulty, interview duration, format, or editor settings, and the report's
durationMinremains guidance for the improvement item rather than becoming the next interview's duration. Sharing the focus with Jim also remains a separate opt-in and is unchecked when a focus is selected.Approach
The selected improvement is kept in session storage as
selectedPracticeFocus. It survives lobby reloads and takes precedence overpracticeFocus(reports)until the candidate starts the next interview, at which point the selection is consumed. If no explicit selection exists, the existing automatic carry-forward path is unchanged.Only
phase,weakness,drill, andsuccessCriterionare carried. The selection also retains its source report id so deleting an unrelated or already-missing report does not discard it, while deleting its source report or all saved history does.The same selection path is available from saved lobby reports, the completed interview report, and replay reports. Interview navigation stores the selection only after the existing leave safeguards permit navigation, and storage failures keep the candidate on the report with feedback instead of silently navigating with a lost selection.
The selection is also bound to the account that created it.
/api/sessionexposes the browser-facing owner asuser.id, while/api/reportsand signed-in/api/tokenresponses expose the same value asaccountId. These are opaque values derived from the account's stable identity rather than the recyclable sequentialusers.iddatabase key. Temporary-account cleanup therefore cannot transfer an old browser owner to a new account even if SQLite reuses its row id, while the same GitHub account retains a stable owner across sign-ins.Before Start hands a selected weakness to the interview, the lobby re-reads
/api/sessionand drops the focus if the account owner changed. Sharing remains a separate opt-in, and its checkbox is checked again immediately before the handoff is stored.The report HTML gains the action, while Markdown report output remains unchanged.
Validation
./scripts/test.shpasses on the final head: 2971 tests, 2944 passed, 0 failed, and 27 environment-dependent tests skipped.Browser regression coverage checks the exact selected improvement, persistence across lobby reloads and consumption on Start, automatic fallback, the unchecked sharing control, source-report deletion without clearing selections from unrelated or already-missing reports, persisted-page reconciliation, interview leave cancellation and duplicate-leave protection, storage-failure feedback, replay selection, cross-account sharing protection, last-moment Share consent, and Strict report selection. The lobby test also verifies that choosing Practice this does not change the problem, difficulty, duration, interview format, or editor settings.
HTTP regression coverage verifies that
/api/session,/api/reports, and signed-in/api/tokenexpose the same opaque owner for an account, guest token responses expose no account owner, and cleaning up a temporary account does not recycle its browser owner when SQLite later reuses the sameusers.id.The required interactive review for the 2846-line pull-request diff was completed on the final head. No live provider behavior or desktop-specific behavior was exercised; this change is limited to the existing web report and lobby practice-focus flow.
Screenshots
Practice this in the report
Selected focus in the lobby
Markdown report unchanged
Close #580
Summary by cubic
Adds a Practice this action to each report improvement item so a candidate can carry that exact phase, weakness, drill, and success criterion into the lobby as the practice focus. A selected focus overrides the automatic carry-forward for the next interview; without a selection, existing automatic behavior is unchanged.
devicefor signed-out and guest sessions); switching accounts, signing out, deleting the source report, or starting the interview clears it./api/session,/api/reports,/api/token) now include a signedaccountIdso the lobby discards report rows that belong to another account.Written for commit db3562f. Summary will update on new commits.