Offer the reply suggestion of a ticket to the clerk - #1291
Conversation
|
Five review passes were needed before both review dimensions came back clean. What they found and what was changed:
Two points were raised and deliberately not changed:
Every touched file is at 100 % statements, branches, functions and lines; the screen and the hook were at 0 % before. The suite runs green repeatedly (918 tests, six consecutive runs). |
|
Four review passes were needed before both a conformance review and a correctness review came back Pass 1 — a decision the server refuses, because it has already been taken by another clerk or in Pass 2
Pass 3 — the test around the refused decision asserted the state after the reconciliation, which Pass 4 — no findings. Two reported points were declined, with reasons:
Also worth a reviewer's decision: the three Playwright baselines are Side note from the test work: the screen's tests emitted 45 "not wrapped in act" warnings, from All checks are green. Coverage on every touched file is 100 % on all four metrics. |
|
@mara-steiner please check |
|
Thank you — Blocker 1 was right, and it turned out to be the entry point to a whole class rather than Blocker 1 — the ticket on screen and the ticket being written toFixed both ways you offered, because they answer different halves of it. Your reading of how this PR made it worse was accurate: Blocker 2 — baselinesDone as asked: the three Major — a failed reconciliation re-enabling the buttonsFixed. Major — two polls sharing one epochFixed. Every suggestion fetch takes a number, and only the newest one may write. The epoch stays for The rest of your list
What the follow-up passes foundBlocker 1 was one instance of "work started on one ticket writing to the screen showing another", so
And two that no guard can help with, because the state was already on screen: the thread of the It is one helper now: take the token where the work starts, ask before writing. The ticket change Coverage stays at 100 % on all four touched files, and the screen's own tests went from 83 to 107 — |
|
@mara-steiner please check |
The support issue screen shows the newest suggestion still awaiting a decision above the composer, together with a warning when the conversation has moved past it. Accepting copies the text into the composer, where it is edited and sent as the clerk's own message; discarding records the decision. Suggestions are never written here — they enter through the API alone. Also splits the default message author out of the ticket load. It read the clerk list, so the list arriving a moment later reloaded the whole ticket and put the screen back behind its loading spinner: a second request and a visible flash on every ticket that was opened.
Closing a preview that is not open, polling a ticket that has no uid, and unmounting with a preview still open were the paths the screen tests had not reached yet.
The default author is resolved from the ticket and the clerk list, which arrive independently. Whichever lands after the clerk picked an author by hand reset the selection back to the default — visible in CI, where the clerk list is slower than it is locally.
A change event on a select whose options have not arrived yet is dropped, so the two tests picking a clerk read the default back. The clerk list now resolves a tick late in every test, which reproduces the ordering CI runs into, and the two tests wait for the options they pick from.
The default author is resolved in an effect over the ticket and the clerk list. A run scheduled before the clerk picks an author carries the state of the render it was scheduled in, so the state flag guarding it was still false and the pick was overwritten. A ref is set synchronously and holds. The screen tests now mock the dashboard hook as one stable object, the way its useMemo behaves in the app: a fresh object per render changed the identity of every callback and drove the screen through reload rounds no production render performs — which is why this only ever failed in CI.
The five guards left uncovered were unreachable rather than untested, so they are gone instead of pinned: the account id, the ticket id and the attachment now come from the render that has them, and clearing the file selection remounts the input rather than writing to a ref that is never null. The message list hands its file handler a message whose fileName is settled, which is what removes the last of those guards from the screen; its own branches — a thread without ids, without a file, without a handler — are covered by a spec of their own.
The screen had no baseline at all, so this creates one: a spec that drives it through the three states the suggestion panel has — waiting for a decision, outdated, absent — plus the metadata entry that titles the flow. Unlike the older staff specs, everything is mocked: the session token is synthetic (the frontend only decodes a JWT, it never verifies it) and every API call is answered from fixtures. The baselines are deterministic, carry no production data, and need neither a running api nor a seeded database.
Found by review: the handbook recognised only `darwin` baselines, so the three
new ones — taken on Linux — reached the page with neither a project nor a
platform badge, which is precisely the case where that badge carries
information. The snapshot path template is `{projectName}-{platform}`, so both
parts are now read from the name.
The spec says where its baselines came from, and how the macOS variant is
added: Playwright compares against the baseline of the platform it runs on, so
a macOS run writes its own alongside these.
Three findings from review: The 15-second poll refetched the suggestion even while an accept or discard was in flight. Its answer carries the state from before that decision, so the decided suggestion could reappear and invite a second click, which the API then refuses as a conflict. The poll now skips the suggestion while a decision is running. react-router keeps this component mounted when only the route parameter changes, and neither the offered suggestion nor the "author was picked by hand" marker were tied to the ticket. Opening another ticket now drops both. The screen tests resolved the clerk list through a real timer while waiting on a fixed number of microtask ticks, which is not a wait at all: the file failed roughly one run in eight, in tests unrelated to whatever had actually raced. The list now resolves when the test says so, and the waits are on conditions.
Found by review: skipping the poll while a decision runs only stops a new fetch from starting. One that was already in flight answers with the state from before the decision — and the busy flag is false again by then, so it put the decided suggestion back on screen, where the next click earned a conflict from the API. The same held for a fetch of the ticket that was open before another one was opened. Both are now qualified by a token that is raised when a decision starts and when the ticket changes; an answer whose token no longer matches is dropped, error included. The reset runs ahead of the loading effects, so raising the token qualifies the fetch that follows instead of invalidating it.
Found by review: opening another ticket while an accept or discard was still running left its continuation unguarded — the accepted text landed in the composer of the ticket the clerk had moved to, the suggestion loaded there was cleared, and a failure of the old decision was reported on the new screen. The decision now carries the same token as a fetch does.
Found by review: the flag that blocks the buttons while a decision runs was not tied to the ticket. Opening another one while a decision was still in flight left the new ticket's Accept and Discard disabled and its suggestion poll paused — until the request of the ticket that was left finally settled, which in the worst case is never. Opening a ticket now clears the flag, and only the decision the screen is still on may clear it, so a late answer cannot unblock a decision that has since started here. The success-path test also asserts what it had only claimed: the suggestion of the ticket now open survives the late answer.
The API stores a suggestion on the message it answers rather than in a table of its own, so it no longer has an id separate from that message.
A decision the server has already taken — by another clerk, or in another tab — came back as an error and left the panel standing with its buttons live, so the same decision could be sent into the same refusal until the poll happened to reconcile it up to 15 seconds later. The error now comes with what the server actually holds. The tests around it grew the waits the actions they trigger deserve: an update reloads the ticket, a send reloads the thread, and a test that ended on the first of those left the rest to land after it. That is what produced most of this file's "not wrapped in act" warnings, 45 of them; 8 remain, all in tests whose fetch fails and settles between two awaits.
Three points from review. The busy flag was cleared as soon as the refused decision returned, which re-enabled Accept and Discard while the suggestion the server had already decided was still on screen — one round trip in which the same decision could be sent again. It now waits for the reconciliation. The reload reports its own failures the same way, so it overwrote the refusal the clerk actually needed to read. The decision's error is set after it. The poll effect took the busy flag as a dependency, so every decision tore the interval down and set it up again — which also postponed the message poll the same interval carries. It reads a ref now.
The test around the refused decision asserted the state after the reconciliation, which a version that clears the buttons right away would also pass. It now holds the reload open and looks at the buttons while it is running.
Review found that the ticket load wrote whatever came back, while everything around it was guarded. Two loads can be in flight at once — the screen stays mounted when only the route parameter changes — so a slow answer for the ticket that was left replaced the one on screen, silently, because the spinner is long gone by then. This branch had made that worse rather than inherited it: it moved the update write from the route parameter onto the loaded ticket and added the two decisions beside it. The load now carries the same token as everything else, and the three writes take the ticket from the route again. What else the ticket change now drops: the composer and its attachment. A draft is written for one customer, an accepted suggestion most of all, and carrying it into the next ticket is how it reaches someone it was never meant for. The reset also moved ahead of the default-author effect, which is what lets the default apply again when two tickets share a clerk — the case where nothing else changes for that effect to react to. Two suggestion fetches of one ticket carry the same token, so it could not order them against each other: a poll outliving the 15-second interval let the next one start, and the slower answer won. Each fetch takes a number now. And a reconciliation that fails leaves nothing behind: if the reload after a refused decision fails too, the suggestion goes off screen instead of standing there with live buttons.
An adversarial pass over the concurrency work found the same class of defect in four more places, and a second pass found a fifth. All of them share a shape: work started on one ticket writing to the screen showing another. - The reload after an update ran from a stale closure, so an update that answered after the switch fetched the ticket it belonged to and put it back on screen, form fields included — the next update would then carry those values. - The thread and its poll had no guard at all: the messages of the ticket that was left replaced the ones on screen. - A send that answered after the switch cleared the composer of the ticket the clerk had moved to, taking a draft written for someone else. - The file preview, sticky by design, kept showing one customer's document over another customer's ticket. - The default author read the ticket on screen while it was still the previous one, so its clerk became the default for a ticket that has no clerk of its own. The guard is one helper now, taken where the work starts and asked before it writes, and the ticket change drops the author, the preview and both busy flags along with what it already dropped.
Two further passes over the same question. The guards stop a late answer from writing to the wrong ticket, but they cannot help with what is already on screen: the thread of the ticket that was left stayed until the new one's own fetch came back, and that fetch only starts once the ticket has brought its uid. For that moment one customer's conversation sat under another customer's ticket. The same for what the screen reports. A failed ticket load blocks this screen entirely and was never cleared, so one failure blocked every ticket opened afterwards until the page was reloaded; an action error stayed as a banner over a ticket it had nothing to do with. Both are pre-existing, both belong to the reset this change is built around. Also dropped the manual revoke of the preview url — the effect watching that state already does it — and formatted the two new test files.
A third pass walked every state and every await of the screen. Two were still open, both reproduced before they were fixed: The file preview wrote whatever came back. An attachment opened on one ticket and answering after the switch put one customer's document on another customer's ticket, on a panel that stays until it is closed by hand. The template picker was worse. Nothing ever closed it, so it reopened itself over the next ticket as soon as that ticket had loaded — and when its account lookup answered after the switch, the templates were filled from the account of the customer whose ticket the clerk had left. Both take the guard now, and the ticket change closes the picker along with everything else it drops.
The reset closes it, but nothing held that: removing the line left all 107 tests green while the picker stayed open over the next ticket, showing the transaction list of the account that was left.
The account id is required, so the disabled state of the template button could only ever be decided by the loading flag — the other half was left over from an optional chain this branch unwrapped, and the test that covered it constructed an account the API cannot return. Both are gone, like the five unreachable guards before them. The suggestion type also loses the two fields the screen never reads.
d4fa58f to
c11fb88
Compare
|
Thank you — and a note on timing before the points: this review landed on Blocker 1 — no baseline is committedDeclared as a deviation in the description, under the section You already have them. Your run wrote all five and passed against them on the second pass — if you Blocker 2 — the description described a different branchFixed. It now names the five variants, states that no baselines are committed, and declares the Rebased onto Major 3 — the message threadFixed in Both of your reproductions have a test now: the late answer is dropped, and the thread is off screen Major 4 — the file previewFixed in the same two commits. The same pass found the template picker in the same condition — nothing ever closed it, so it Since these are pre-existing and you offered the exception: they are fixed here rather than deferred. Minor 5 —
|
|
@mara-steiner please check |
1 participant
Consumes the reply suggestions added in DFXswiss/api#4761 — merge that one first.
What the clerk sees
Above the composer on the support issue screen, whenever a suggestion is waiting:
message, exactly like a template,
Only the newest suggestion awaiting a decision is offered; earlier ones stay on record on the server
and are never shown. A suggestion is addressed by the id of the message it answers, which is where the
API stores it. There is no control for writing a suggestion, by design: they enter through the
API alone. A suggestion that arrives while the ticket is open is picked up by the existing 15-second
poll, so a clerk sitting on the screen does not have to reload.
Thirteen defects in the screen this uncovered
loadIssueread the clerk list to pick the default message author,which made it depend on that list. The list arrives a moment after the ticket, so every ticket
that was opened was loaded twice, and the second round put the screen back behind its loading
spinner — a wasted request and a visible flash. The author is now resolved in its own effect.
already scheduled when the clerk picks an author carries the state of the render it was scheduled
in — so it reset the selection back to the default. A state flag cannot fix that (it is exactly
the stale value that run reads); a ref is set synchronously and holds.
runs only stops a new fetch; one already in flight still answered with the state from before
the decision and put the decided suggestion back on screen. The same held for a fetch belonging
to the ticket that was left, and for the decision's own continuation — which wrote the accepted
text into the composer of the ticket the clerk had moved to. Fetch and decision now carry a
token that is raised when a decision starts and when the ticket changes.
its Accept and Discard disabled until the abandoned request settled — in the worst case never.
dependency, so each decision tore it down and set it up again — which also restarted the clock
for the message poll the same interval carries, and could postpone new customer messages by up
to another 15 seconds. It reads a ref now.
recognised only
-chromium-darwin. The snapshot path template is{projectName}-{platform}, sothe platform is now read from the name.
Review found a seventh, and it turned out to be the entry to a class rather than a single case: the
ticket load itself wrote whatever came back, while everything around it was guarded — and this
branch had moved the update write from the route parameter onto that loaded ticket. Walking every
state and every
awaitof the screen for the same shape turned up six more:on screen, silently — the spinner is long gone by then. The three writes take the id from the
route again, so a write never depends on a load having been guarded correctly.
putting it back on screen, form fields included — the next update would have carried those values.
ticket either: one customer's conversation sat under another customer's ticket until the new
thread answered, which only starts once the ticket has brought its uid.
moved to, taking a draft written for someone else.
hand, and its own fetch could put it there after the switch.
account lookup filled templates from the customer whose ticket had been left.
stands: a single failure blocked every ticket opened afterwards until the page was reloaded.
Everything tied to the ticket now takes one token where the work starts and asks before it writes,
and the ticket change drops the author, the thread, the preview, the picker, the errors and both
busy flags along with the draft. Numbers 9 to 13 predate this pull request; they sit in the code it
rewrites, and are fixed here rather than deferred.
A refused decision is reconciled rather than left standing: the server refuses a decision it has
already taken — by another clerk, or in another tab — and the panel would otherwise keep offering
that same decision into that same refusal. Accept and Discard stay disabled until what the server
actually holds is on screen, and the refusal is the message the clerk reads.
One case is deliberately left to the server: leaving a ticket and returning to it before the server
has answered a decision shows the suggestion again with usable buttons, so a second decision can be
sent. "A decision is taken once" is enforced there — the write is addressed to a suggestion that is
still pending — and a second attempt is answered with a conflict — rebuilding that in the client would duplicate state the server holds, for
a window the size of one round trip.
Tests
Every touched file is at 100 % on all four metrics:
src/screens/support-dashboard-issue.screen.tsxsrc/hooks/support-dashboard.hook.tssrc/components/support/reply-suggestion-panel.tsxsrc/components/support/info-panel.tsxThe screen and the hook were at 0 % before this pull request.
Reaching 100 % on the screen meant removing six guards rather than testing them: they were
unreachable, not untested, and CONTRIBUTING asks for exactly that. The account id, the ticket id and
the attachment now come from the render that has them; clearing the file selection remounts the
input instead of writing to a ref that is never null; and the message list hands its file handler a
message whose
fileNameis settled; and the template button's account check was the remnant of anoptional chain this branch unwrapped, on a field the API always sends.
info-panel.tsxis covered bya spec of its own — a thread without ids, without a file, without a handler.
Handbook
The screen had no baseline at all, so this creates the spec for one:
e2e/support-dashboard-issue.spec.tsdrives it through the five visual states this changeintroduces — a suggestion waiting for a decision, one the conversation has moved past, none at all,
a decision in flight with both buttons closed, and an accepted suggestion sitting in the composer —
plus the
metadata.jsonentry that titles the flow.Unlike the older staff specs, this one mocks everything: the session token is synthetic (the
frontend only decodes a JWT, it never verifies it) and every API call is answered from fixtures. The
baselines are deterministic, carry no production data, and need neither a running api nor a seeded
database.
Declared deviation: no baselines are committed
Per Deviating from these guidelines, this pull request departs from Handbook — "a committed
Playwright baseline under
e2e/screenshots/baseline/, covering each visual variant the changeintroduces". The spec and the metadata entry are here; the five
.pngfiles are not.The reason is that they belong on macOS, where the other 148 baselines in that directory were taken,
and no macOS machine was available to produce them. Playwright only ever compares against the
baseline of the platform it runs on, so Linux captures would be read by nobody on the platform the
rest of the directory targets — and renaming one to
-darwinwould be a claim about its origin thatthe file cannot keep, since font rendering differs between the two.
What closes it:
npx playwright test e2e/support-dashboard-issue.spec.ts --update-snapshotson aMac writes all five, and committing them here removes the deviation. The reviewer has already run
the spec on macOS and confirmed it passes against the baselines that run wrote.