fix(web): keep an assignment's row in place on write - #755
Conversation
The edit, lock, and close writers rebuilt assignments.json as everything
except the slug plus the updated entry, moving the row to the end. The
CLI replaces in place ("Position preserved on replace"), and its
`assignment list` prints file order, so a web-side lock reordered the
teacher's listing and made every flag flip a whole-row diff.
There was a problem hiding this comment.
Pull request overview
This PR fixes an ordering/diff-hygiene issue in the web app’s writers for <classroom>/assignments.json by updating existing assignment entries in-place rather than removing and re-appending them (which previously moved the touched row to the end of the array). This aligns web write behavior with the CLI contract (“Position preserved on replace; new slugs append”) and improves the auditability of config repo diffs and CLI list output order.
Changes:
- Added a
replaceAssignmentEntryhelper to replace the first matching assignment row by slug while preserving array position. - Updated
editAssignment,setAssignmentLock, andsetAssignmentClosedto use the helper instead offilter + push. - Added regression tests ensuring row position is preserved (and that duplicate-slug manifests only rewrite the first match), plus updated a related comment in
rename.ts.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| web/src/domain/assignments/createEdit.ts | Introduces replaceAssignmentEntry and uses it in the three affected writers to preserve row position. |
| web/src/domain/assignments/rename.ts | Updates an explanatory comment to remove the now-stale “filter+push” reference. |
| web/src/domain/assignments.test.ts | Adds regression tests covering position preservation for edit/lock/close, plus a duplicate-slug scenario. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Map in place so the entry keeps its position in the array: a rename | ||
| // shouldn't reorder the manifest (see replaceAssignmentEntry). |
There was a problem hiding this comment.
Reworded — the comment now names createEdit.ts and the CLI's UpsertAssignment instead of a symbol that isn't in this file. 14dc5bd
rongxin-liu
added
enhancement
Summary
editAssignment,setAssignmentLock, andsetAssignmentClosedrebuilt<classroom>/assignments.jsonas "every entry except this slug, then the updated entry", so touching an assignment moved its row to the end of the array. They now go through a smallreplaceAssignmentEntryhelper that replaces the row where it sits.That matches the CLI, which is the other side of this contract and writes the rule down:
UpsertAssignment— "Position preserved on replace; new slugs append" — andgh teacher assignment lockassigns back into the same index. It also matches the web app's ownrenameAssignment, which already maps in place;deleteAssignmentis a plain filter andcreateAssignment/copyAssignmentappend new slugs, both already correct.What the old shape cost teachers:
gh teacher assignment listprints entries in file order (no sort), so lockinghw1in the web app moved it belowhw9in the terminal and in--jsonoutput.git log -p assignments.jsonno longer showed what actually changed.No reader is affected — every consumer looks entries up by slug — so this is an ordering and diff-hygiene fix, not a behavior change for students or grading.
The helper replaces the first match only, like
UpsertAssignment: a hand-edited manifest can hold two rows for one slug, and each caller builds its entry from the first of them, so replacing both would overwrite the second row's own content.Also updates the comment at
rename.ts:286, which cited "the lock flip's filter+push" as the counter-example for mapping in place — that pattern no longer exists after this change.Closes #754
Type of change
Checklist
cd web && npm run check(web only — no Go or Python change).schemas/*.schema.jsonand every mirror (Go / Python / TypeScript), and the parity tests pass. — n/a: no schema or field change; this aligns the web app with the CLI's existing write behavior.Each of the three writers gets a regression test asserting the surrounding rows keep their positions, plus one for the duplicate-slug manifest; all three position tests fail on
main.