You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
[Frontend][Review] Revision-metadata residuals from PR #2565 review: invalid gate does not cover the disputed number, failure-toast noise after a proven save, clear-half coverage, unbounded history map #2579
Non-blocking residuals from the two fresh-context reviews of PR #2565 (refs #2524, revision metadata truth in useProposalRevisions.ts), tracked here rather than fix-cycled. File references are at that PR's head b0421100d.
MEDIUM, needs a self-contradicting server.history.invalid is cleared by any response that is a self-complete chain 1..max (loaded.size === highestLoaded, ~160), which does not require the response to cover the number that was disputed. Scenario: GET [#1, #2]; GET [#1, #2, #2-alt] sets invalid and stores nothing; a stale pre-save GET [#1] is self-complete for max 1, clears invalid, and the composable republishes (2, rev-2, true) as authoritative for the very number the server contradicted. Same shape with an empty list after a conflict at number 1. Every trigger requires the backend to contradict itself, and ProposalRevisionConfiguration.cs:44 makes (ProposalId, RevisionNumber) unique, so the precondition is DB or serialization corruption. Fix direction: record the disputed number(s) instead of a boolean and require the clearing response to cover them, or soften the doc comment at ~135-138 (which currently claims the disputed number is covered) to match the implemented proxy.
LOW, toast noise. Because publishPersistedRevisionMetadata no longer bumps loadGeneration, a GET that main silently discarded on the generation check now reaches the failure toast (~286-288) even when the published metadata is correct and authoritative: a reviewer can see "Revision saved" immediately followed by "Failed to load revision history". The read did fail, so the toast is not false, but it may make a reviewer distrust a good save. No spec asserts the toast on this path.
LOW, coverage. Only the retain half of the new catch rule is pinned. The "non-authoritative, do clear" half is unpinned: the #2215 M-1 resync-failure spec asserts revisionsLoaded === false and no toast but not revisionCount === 0; one extra assertion there closes it. Deleting the two assignments inside the guard would likely keep the suite green.
Non-blocking residuals from the two fresh-context reviews of PR #2565 (refs #2524, revision metadata truth in
useProposalRevisions.ts), tracked here rather than fix-cycled. File references are at that PR's headb0421100d.history.invalidis cleared by any response that is a self-complete chain1..max(loaded.size === highestLoaded, ~160), which does not require the response to cover the number that was disputed. Scenario: GET[#1, #2]; GET[#1, #2, #2-alt]sets invalid and stores nothing; a stale pre-save GET[#1]is self-complete for max 1, clears invalid, and the composable republishes(2, rev-2, true)as authoritative for the very number the server contradicted. Same shape with an empty list after a conflict at number 1. Every trigger requires the backend to contradict itself, andProposalRevisionConfiguration.cs:44makes(ProposalId, RevisionNumber)unique, so the precondition is DB or serialization corruption. Fix direction: record the disputed number(s) instead of a boolean and require the clearing response to cover them, or soften the doc comment at ~135-138 (which currently claims the disputed number is covered) to match the implemented proxy.publishPersistedRevisionMetadatano longer bumpsloadGeneration, a GET that main silently discarded on the generation check now reaches the failure toast (~286-288) even when the published metadata is correct and authoritative: a reviewer can see "Revision saved" immediately followed by "Failed to load revision history". The read did fail, so the toast is not false, but it may make a reviewer distrust a good save. No spec asserts the toast on this path.#2215 M-1resync-failure spec assertsrevisionsLoaded === falseand no toast but notrevisionCount === 0; one extra assertion there closes it. Deleting the two assignments inside the guard would likely keep the suite green.revisionHistoryByProposalis unbounded across a queue walk.Refs #2524, #2458, #2215, PR #2565.