Repository navigation
fix: read pmd's wrapped single-project answer in the Projects re-reads - #95
Conversation
#94) pmd answers GET /v1/projects/{slug} as {"project": {...}, "items": [...]}. The Projects dialog parsed the whole answer as a row, so the re-read after 409 stale_version and after a timed-out create always failed as malformed. Project::one_from_json takes the project object and refuses anything else. The Projects test fake now answers in pmd's shape. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@sfl review |
There was a problem hiding this comment.
✅ Verdict: PASS • Round 1
Confidence: 9/10
Review done by claude-opus-5-5 (same family: same-family: codex gated: codex Weekly 167% SUS 60 🔴 pause, defer)
The PR adds Project::one_from_json, which unwraps pmd's {"project": {...}, "items": [...]} answer to GET /v1/projects/{slug}. The Projects dialog's re-read (after 409 stale_version and after a create that timed out but landed) now uses it, the test fake answers in pmd's shape, and the PR adds core, UI and capture tests. I found no evidenced defect: the wrapped shape matches pmd's handler and golden file at 30bf244, and the change is limited to the one re-read caller.
🔨 Full-Spectrum Review Summary
Effort: high
Review time: 1m 51s
Token count: Unavailable
| Pass | 🔴 Critical | 🟠 High | 🟡 Medium | ⚪ Low |
|---|---|---|---|---|
| 🔒 Security | 0 | 0 | 0 | 0 |
| ✅ Accuracy | 0 | 0 | 0 | 0 |
| 🧰 Quality & Maintainability | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 |
Key Findings
No material defects found.
Coverage
- Files reviewed:
crates/dashboard-core/src/projects.rscrates/dashboard-app/src/manage/projects.rscrates/dashboard-app/src/projects_tests.rscrates/dashboard-app/src/captures.rscrates/dashboard-core/tests/projects.rscrates/dashboard-core/tests/fixtures/pmd-project-codexbar-ios-30bf244.json
- Context read:
AGENTS.mdcrates/dashboard-core/src/write.rscrates/dashboard-core/src/edit.rscrates/dashboard-app/src/main.rscrates/dashboard-app/src/manage.rscrates/dashboard-app/src/manage/work_items.rscrates/dashboard-core/src/pmd.rscrates/dashboard-core/tests/support/stub.rshemsoft-dev/project-manager internal/api/api.go@30bf244hemsoft-dev/project-manager internal/api/projects.go@30bf244hemsoft-dev/project-manager internal/api/testdata/project.golden.json@30bf244
- Verified:
- The compare's
base_commit.shaisfa8ffb7af7a4b1310e0c69e9247655713351f210,merge_base_commit.shamatches it, its last commit ise604380b909d01aed99b27b7712e3f65cff1ba74and it isahead_by1. PR #95's head and base match the reserved SHAs. - pmd's
API.projecthandler (api.go:443) writes{"project": p, "items": items}forGET /v1/projects/{slug}, which confirms the wrapped shape thatone_from_jsonexpects. - The new fixture is byte-identical to pmd's
internal/api/testdata/project.golden.jsonat30bf244. one_from_jsonreturnsMalformedwhenprojectis missing, null, an array or not an object, and also for a bare row; otherwise it delegates to the unchangedfrom_json, so the required slug, repo, tracked and version checks still apply.- manage/projects.rs:640 is the only caller that re-reads a single project; the PUT response parsing at :572 and the list parsing at :289 (
list_from_json) are unchanged. write.rscreate-check (CreateCheck::Exists) only checks that the read succeeded and never parses the body, so the wrapped answer has no effect on it.- On a malformed re-read, the stale-version branch calls
reread_failed, which keeps the user's edits and resends nothing. The create branch falls back toTimeout{Unknown}plus a list reload. Both paths are unchanged. - The symbols the tests use (
editor_form,visible_slugs,conflict_state,differences,is_dirty,version,message,Form::value,Folder::config_dir,FakeProjects::new,Project: PartialEq+Debug) exist at the head SHA with the needed visibility.capturesandprojects_testsare bothcfg(test). - Each fake read increments the counter once, and the fake's
writenever callsread, so the asserted read counts of 1 andround + 1match the code paths. - The PR head's check runs
advisories,coreandrust(ubuntu-latest and windows-latest) completed withsuccess.
- The compare's
- Not verified:
- Runtime behavior, including GPUI rendering and capture PNGs; execution was forbidden.
- The stub's behavior on a delayed PUT is inferred from the test, not read in full in
support/stub.rs. - pmd at any revision newer than
30bf244; the shape was checked only at the revision the fixture names.
- Tests run: None. Reviewer execution is disabled.
HemSoft full-spectrum review • 3 evidence-based passes • Reviewed head e604380b909d01aed99b27b7712e3f65cff1ba74 • SFL v0
Risk level: 2/10 (project-dashboard)
Worked on by: claude-opus-5-5 on laptop, for Chief dispatch 17533a35
Summary
pmd answers
GET /v1/projects/{slug}as{"project": {...}, "items": [...]}. Manage > Projects parsed that whole answer as a project row, so the re-read after409 stale_version, and after a create that timed out but landed, always failed as malformed. The Projects test fake answered with a bare row, so the tests never saw pmd's shape.dashboard-core/src/projects.rs:Project::one_from_jsontakes theprojectobject out of the answer. An answer without one is malformed, a bare row included.manage/projects.rs:640: the one re-read that both paths share now calls it. ThePUTanswer, which pmd sends as a bare row, still goes throughProject::from_json(line 572).projects_tests.rs:FakeProjects::readanswers in pmd's wrapped shape, can be told to send one other answer, and counts reads.tests/fixtures/pmd-project-codexbar-ios-30bf244.json: project-manager'sinternal/api/testdata/project.golden.jsonat30bf244, byte for byte (sha2566a5dcc4e…c55c744).Not changed: write semantics,
If-Match/If-None-Match, no automatic retry, the error messages, Work items and Fleet (pmd sends their single rows bare), and what Reapply sends.Acceptance Criteria
PUTwith the re-read version, and Discard loads pmd's values. Evidence:changed_elsewhere_shows_both_values_and_reapplies_on_the_new_version(2 writes, the secondIf-Match: 6) andchanged_elsewhere_can_discard_mine_for_the_reread_values(goal and notes from pmd, version 6, not dirty; the next save sendsIf-Match: 6). Alsoa_risk_level_changed_elsewhere_shows_both_values_and_reapplies, botha_priority_changed_elsewhere_*tests, and corea_stale_version_rereads_pmds_wrapped_project_into_the_conflictover the loopback stub (PUT If-Match 3,GETwith the read token,PUT If-Match 4).a_create_that_timed_out_but_landed_becomes_an_edit_without_a_second_create(titleyahtzee (hemsoft-dev/yahtzee), no dialog message, version 2, one read, one write, listed after Back) and corea_create_that_timed_out_reads_back_the_project_pmd_stored(PUT,GET,GET; never resent).a_malformed_reread_says_so_and_nothing_is_sent_until_save({"items": []}, then a bare row: each shows "Changed elsewhere, and reloading it failed: pmd returned malformed data.", keeps the edit and the stale version 2, one write per Save and one read per conflict) and corea_single_project_read_takes_pmds_project_object_and_nothing_else(pmd's golden parses; five malformed shapes are refused).read("v1/projects/{slug}")in pmd's wrapped shape.work_items_tests::changed_elsewhere_shows_both_values_then_reapplies_on_the_new_version,work_items_tests::changed_elsewhere_can_discard_mineand the Fleet conflict tests infleet_tests.rs.Definition of Done
dashboard-corewith unit tests against pmd's golden and malformed shapes.cargo deny --locked check advisories:advisories oklocally, and CI'sadvisoriesjob passed.e604380: advisories,core (ubuntu-latest),core (windows-latest),rust (ubuntu-latest)andrust (windows-latest)(run 42350135376).e604380b909d01aed99b27b7712e3f65cff1ba74.Validation
Local runs on Windows 11 (Laptop), not CI.
RED, at the real app seam: this head's tests and fake, with only
manage/projects.rs:640put back to the baseProject::from_json.cargo test -p dashboard-app --locked projects_tests: 16 passed, 7 failed.changed_elsewhere_shows_both_values_and_reapplies_on_the_new_version,a_risk_level_changed_elsewhere_shows_both_values_and_reappliesandchanged_elsewhere_can_discard_mine_for_the_reread_values: noconflict-*element.a_priority_changed_elsewhere_moves_the_row_after_the_conflictanda_priority_changed_elsewhere_keeps_the_project_selected_after_the_conflict: noconflict-discard.a_create_that_timed_out_but_landed_becomes_an_edit_without_a_second_create: the title staysNew project.a_malformed_reread_says_so_and_nothing_is_sent_until_save: round 0 ({"items": []}) passes there too, as a control. Round 1 fails because the base accepts a bare row.The core tests call the new
one_from_json, so they don't build on the base.GREEN at this head:
cargo test -p dashboard-app --locked projects_tests: 23 passed.cargo fmt --all --check: pass.cargo clippy --workspace --all-targets --locked -- -D warnings: pass.cargo test --workspace --locked: pass. dashboard-app 209 passed (captures are compiled out on Windows); every dashboard-core suite passed,projects20.CARGO_TARGET_DIR:cargo check -p dashboard-core --lockedandcargo test -p dashboard-core --locked: pass.cargo deny --locked check advisories(cargo-deny 0.20.2):advisories ok.Screenshots
Rendered offscreen on Linux by the required
rust (ubuntu-latest)job of CI run 42350135376 at this heade604380, step Screenshots:CAPTURE_DIR="$GITHUB_WORKSPACE/captures" cargo test -p dashboard-app --locked captures -- --ignored(19 passed, includingcaptures_projects_reread). They come from that run'scapturesartifact, whosehead_shaise604380b909d01aed99b27b7712e3f65cff1ba74. Default 1100×760 window and theme.captures_projects_rereadsaves each frame and then checks it: the conflict's differences, onePUTwithIf-Match: 6, the stored project and the list.Stale version, wrapped re-read. gh-x is saved at P1 while someone else changed its goal (version 6). The conflict panel shows Yours next to Now in pmd, and Reapply mine has focus.
After Reapply mine. One
PUTwithIf-Match: 6; gh-x is saved at P1, version 7. Reapply sends every form value, so the goal is the user's again. That is existing behaviour and out of this issue's scope.A create that timed out but landed. The read back finds yahtzee, and the editor opens as the stored project (P3, risk 5). There is no "checking for the row failed" message and no second create.
No recording: GPUI's headless renderer gives still frames, and opening a desktop window for a screen recording was out of bounds for this run.
Risks and deferred work
one_from_jsonrefuses a bare row. If a future pmd answeredGET /v1/projects/{slug}without the wrapper, re-reads would fail visibly with the existing "malformed data" message, as they do today.edit.rstests pin this), and fix: keep a Work item selected when its save leaves it past the first page #93's deferred Work items items.Closes #94
🤖 Generated with Claude Code