Repository navigation
Conversation
20ab546 to
ecbaf88
Compare
| rememberSettings(); | ||
| } | ||
|
|
||
| nodes.presetOpen.addEventListener("click", () => { |
There was a problem hiding this comment.
Closing the dialog with Escape bypasses presetClose, and nothing here resets the form. A user who clicks "Save current", types half a name and presses Escape reopens the dialog to the stale name form (presetForm.hidden is still false, the input keeps its text and any custom validity). Move the reset into a close listener on nodes.presetDialog so every dismissal path (button, Escape) hides the form, clears the name and validity, and returns focus to presetOpen. The presetClose handler can then just call close(). The keyboard test only covers the button path, so add an Escape case.
|
Add screenshots to the pull request description to illustrate your work, as @ColtenOuO suggests. Example: #349 |
| (sessionPresets.length ? nodes.presetSelect : nodes.presetSave).focus(); | ||
| }); | ||
|
|
||
| nodes.presetClose.addEventListener("click", () => { |
There was a problem hiding this comment.
Closing the dialog with the name form open (Close or Escape) leaves presetForm visible, with the old name and the old status line. The next open shows a stale form that a single Enter submits. Reset the form in a close listener on presetDialog, which covers both ways out. A native modal dialog already returns focus to the opener, so the explicit presetOpen.focus() can go. Add a test that opens the form, presses Escape, and reopens.
| rememberSettings(); | ||
| } | ||
|
|
||
| nodes.presetOpen.addEventListener("click", () => { |
There was a problem hiding this comment.
Closing the dialog with Escape fires close but never reaches the presetClose handler, and reopening only resets the select. If presetForm was open when the dialog was dismissed, it reopens still visible with the old name typed in and the previous status text (for example "Saving x.") still in the live region. Reset the form, name, validity and status in a close listener on presetDialog instead of in the button handler, so every way of closing goes through the same path.
Candidates who switch between practice formats currently have to reconfigure the lobby each time. Preserve multiple browser-local setups and restore them without changing the selected problem, difficulty filters, or optional interview context. Cover saving, updating, deleting, reloading, and compatibility with controls added after a setup was saved. Close sysprog21#591 Change-Id: If15c9302b6e7df7d61eb01b51b8cf71a8990a845
ecbaf88 to
ffa5f00
Compare
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
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:582">
P2: Saving over an existing setup replaces its record with only the current known settings, dropping forward-compatible fields such as `futureControl` and `revision`. Preserve unknown fields from the existing record when updating it.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
View guided diff | Re-trigger cubic
| (preset) => normalizePreset(preset)?.name.toLocaleLowerCase() === key, | ||
| ); | ||
| const next = [...stored]; | ||
| const candidate = savedPreset(name, settings); |
There was a problem hiding this comment.
P2: Saving over an existing setup replaces its record with only the current known settings, dropping forward-compatible fields such as futureControl and revision. Preserve unknown fields from the existing record when updating it.
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 582:
<comment>Saving over an existing setup replaces its record with only the current known settings, dropping forward-compatible fields such as `futureControl` and `revision`. Preserve unknown fields from the existing record when updating it.</comment>
<file context>
@@ -613,17 +573,13 @@ nodes.presetForm.addEventListener("submit", async (event) => {
- name,
- settings,
- );
+ const candidate = savedPreset(name, settings);
if (!candidate) return false;
if (index < 0) next.push(candidate);
</file context>
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.
| }); | ||
|
|
||
| nodes.presetForm.addEventListener("submit", async (event) => { | ||
| event.preventDefault(); |
There was a problem hiding this comment.
The submit handler never checks presetMutationPending, and syncPresetControls disables "Save current" but not the form's own Save button or the name field. Pressing Enter twice, or submitting while a delete waits on the lock, queues a second mutation. The first one to finish clears presetMutationPending and re-enables the controls while the other is still queued, so the status line and the select can show a state that the queued write then changes. Refuse the submit while a mutation is pending, and disable the form's submit button in syncPresetControls alongside presetSave.
| event.preventDefault(); | |
| event.preventDefault(); | |
| if (presetMutationPending) return; |
|
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:
Addressed at the current head; reply with what changed and resolve:
Checked at ffa5f00. |
|
The
Each refusal and its fix: https://github.com/sysprog21/codetrial/blob/main/docs/large-change-explanation.md#troubleshooting |
Problem and behavior
Candidates who switch between practice formats currently have to reconfigure
the lobby each time because CodeTrial preserves only the most recently used
settings.
This change lets candidates save multiple browser-local session setups under
different names. Selecting a saved setup immediately restores its session
length, interview format, and editor options without changing the selected
problem, difficulty filters, or optional interview context.
Candidates can also update an existing setup by saving under the same name and
delete setups they no longer need. Saved setups remain available after
reloading the lobby.
Approach
Store all named setups as individual records under one local storage key.
The implementation reuses
selectedEditorOptionsandsaveEditorOptionsinstead of maintaining a separate editor-option list. Missing fields are left
unchanged when applying an older setup, while fields unknown to the current
version are preserved when records are rewritten.
Save and delete operations re-read storage inside a Web Lock so simultaneous
changes from different lobby tabs do not overwrite each other.
At the time this pull request was opened, #584 and #590 were still open and
were therefore not included. If a saved setup does not contain a control added
later, applying it keeps that control's current value.
Screenshots
Lobby
Save setups
Select setups
Validation
node --test tests/browser/lobby.test.js: 281 tests passed.lobby from working.
fields added by newer versions.
below the offered durations.
git diff --check.Close #591
Summary by cubic
Lets candidates save named session setups in their browser and restore them later, instead of reconfiguring session length, interview format, and editor options each time.
Written for commit ffa5f00. Summary will update on new commits.