docs(feedback): pin what actually keeps a comment from outliving its vote - #440
Open
eugenia-scandit wants to merge 4 commits into
Open
eugenia-scandit wants to merge 4 commits into
eugenia-scandit wants to merge 4 commits into
Conversation
…vote submit() flushes a held vote before capturing the comment. Nothing in the file said why that line has to stay, so a refactor could reasonably gate it on the comment succeeding, or delete it as redundant with the retry poll. Either reopens a gap that fails silently. The poll gives up after RETRY_LIMIT * RETRY_MS, and pagehide and unmount can both be missed. A reader who votes while PostHog is still absent, idles past 30s, then writes a comment can - without this flush - land the comment and lose the vote. Reporting takes vote counts from docs_page_feedback alone, so they would leave a comment behind no countable vote, with no exception, no failed send and no undelivered notice. An earlier draft of this comment credited the statement ORDER for that, and that was wrong. Three of capturePostHogEvent's four exits - no window, no capture function, opted out - are shared state read synchronously one line apart, so swapping the two calls cannot change whether the vote is accepted. The fourth is a throw; note has_opted_out_capturing sits inside the try, so a throw drops both calls when its cause is the call itself, and it can never drop the vote alone, because the vote's properties are a subset of the comment's. Order only keeps the pair in sequence. The flush being unconditional and present is what protects the data, and documenting the inert constraint while leaving the load-bearing one unstated would have licensed exactly the edits it was meant to prevent. Phrasing is deliberate on two further points. "Accepted, not received": capturePostHogEvent returns true when PostHog takes the event, which the helper's own docstring is careful to distinguish from delivery. And this buys at-least-once for a commenter's vote, not exactly-once - not being double-counted rests on the sessionStorage guard, which is per-tab and swallows its own failures by design. Follow-up to #422, which merged before this landed. Comment only, no behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Review found three problems in the paragraph that justifies the invariant. All three are fair, and the second is the kind of error that discredits the rest of a comment. It said the flush is what stops a comment arriving with no vote "because the poll gives up". It is not. Clearing the interval does not unregister the `pagehide` listener or the unmount `lastChance()` - the effect does not re-run - so after poll expiry a held vote is still flushed on an in-site route change or on unload. This line is decisive in the narrower case where neither fires: the tab discarded or the process killed after the comment lands. Claiming more than that invites a reader to check, find the fallbacks still armed, and discard the whole comment - including the half that is load-bearing. It also contradicted itself about gating on `delivered`. Having established that three of the four false-exits are shared state read one line apart, it then warned that gating reopens the idle scenario - but for those three exits, a false `delivered` means the vote would not have been accepted either, so the gate changes nothing there. The case the gate really does break is the FOURTH exit, the throw: if ph.capture throws on the comment payload but would not have on the smaller vote payload - and the vote's properties are a subset, so that is the direction that can happen - the gate drops a vote that would have gone through. The warning now names that case instead of the one it does not apply to. And the premise the whole invariant rests on - that vote counts come from `docs_page_feedback` alone - is not checkable from this repo. That event name appears in no other file: no dashboard, query or digest config. It is now marked as an external premise to verify rather than stated as fact, so that if the reporting changes to count both events the comment does not go on asserting a rationale that has quietly stopped applying. Still comment-only: zero non-comment added lines against main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous rewrite over-corrected. Having found that `pagehide` and the unmount `lastChance()` stay armed after the poll expires, it concluded this flush only matters when the tab is discarded, and called that "Rare" - which is precisely the sentence a future editor would quote while deleting the line. Both those paths call the same helper, which returns true on ACCEPTED rather than received, and both fire at unload, which is the least reliable moment there is for getting a request out of a browser. They are a fallback, not cover. This flush is the only one that happens while the page is alive. The argument against gating on `delivered` no longer rests on reproducing a payload-dependent throw, which a future editor would try, fail, and then dismiss the whole paragraph over. It rests on something checkable from this file alone: the gate can only ever LOSE a vote and can never gain one. Where `delivered` is true the vote goes either way; where it is false the vote may still have been accepted. There is no case in which adding it helps. The throw is now a supporting detail rather than the load-bearing claim. And the reporting premise is sharper: `docs_page_feedback` is a strict PREFIX of `docs_page_feedback_comment`, so an insight matching by contains or by regex rather than exact equality already counts both - in which case the rationale is wrong today, not merely at risk. The comment now says to confirm the filter is exact-match rather than asserting the conclusion. Still comment-only: zero non-comment added lines against main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous rewrite lumped them together as "both fire at the least reliable moment there is". That is true of `pagehide` and false of the unmount `lastChance()`: on this SPA the widget unmounts on a client-side route change, which is how most readers leave a docs page, and the page is fully alive when it runs. The existing comment on that cleanup, twenty lines above, says exactly this - so the new text contradicted its own neighbour, and in the direction that invites deleting the MORE reliable path. The two are now described separately, with the unmount one marked as the better bet rather than collateral in an argument about unload. Also dropped "the only flush that happens while the page is alive". The poll flushes on a live page too, for the first RETRY_LIMIT * RETRY_MS - a reader who votes and types a short comment submits well inside that window, and this line is then a no-op. The claim now carries the qualifier it was relying on: what this line uniquely covers is the reader who, ONCE THE POLL HAS EXHAUSTED, neither navigates in-site nor unloads cleanly. Still comment-only: zero non-comment added lines against main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
moritzhartmeier
left a comment
moritzhartmeier
left a comment
Member
There was a problem hiding this comment.
Most of the new comment checks out against the code (listeners stay armed after the interval is cleared, the four false-exits of capturePostHogEvent, vote properties being a subset of the comment's, and the argument against gating on delivered). Two parts need fixing, see inline.
Comment on lines
+218
to
+220
| // exhausts it flushes on a live page anyway, and this line is then a | ||
| // no-op, which is fine: it costs nothing and it is the only thing standing | ||
| // there once the poll is done. |
Member
There was a problem hiding this comment.
This "no-op" claim is false while the poll is still running. The poll only fires every RETRY_MS (3 s). Say PostHog finishes loading about a second after the reader accepts cookies, and they press Send before the next tick: heldVote.current is still set, so this line sends the vote before the comment. Without it, the comment would go first and the vote would follow on the next tick or at unmount, out of order. That ordering is exactly the job the original comment on line 195 gives this line. It also isn't a no-op if PostHog is still missing: the send fails and the vote is re-held.
The risk is the edit this comment exists to prevent: a reader who trusts it could gate the flush on "poll exhausted", or drop it because the poll "covers" that window, and ordering would quietly break. Suggest saying that before exhaustion it keeps the vote ahead of the comment, not that it's a no-op.
Comment on lines
+206
to
+213
| // - UNMOUNT is reliable. On this SPA the widget unmounts on a client-side | ||
| // route change, which is how most readers leave a docs page, and the | ||
| // page is fully alive when it runs (see the note on the cleanup above). | ||
| // It is a better bet than the flush below, not a worse one - do not | ||
| // remove it on the strength of the next bullet. | ||
| // - PAGEHIDE is the unreliable one: a real unload, where an in-flight | ||
| // request may never leave the browser, and where an accepted event is | ||
| // not a delivered one. |
Member
There was a problem hiding this comment.
This contradicts the PR description. The description says pagehide and the unmount lastChance() "both fire at unload, the least reliable moment", and that the submit() flush "is the only one that happens while the page is alive". This comment says the opposite: unmount "is reliable", the page is "fully alive" when it runs, and it's "a better bet than the flush below". Since the reasoning is the whole point of this PR, please reconcile one of the two before merge so reviewers approve the argument that actually ships.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #422, which merged while this was still in review. Comment only — 43 added lines in
PageFeedback/index.tsx, no executable change (verified: zero non-comment added lines againstmain).Why
submit()flushes a held vote before capturing the comment:Nothing in the file said why that first line has to stay. Two reasonable-looking refactors remove it: gating it on the comment succeeding, or deleting it as redundant with the retry poll.
What the other paths actually give you. The poll stops retrying after
RETRY_LIMIT * RETRY_MS, but clearing the interval does not unregister thepagehidelistener or the unmountlastChance()— the effect does not re-run, so both stay armed. That is worth knowing and it is not the same as the vote being safe: both call the same helper, which returnstrueon accepted rather than received, and both fire at unload, the least reliable moment there is for getting a request out of a browser.So they are a fallback, not cover. This flush is the only one that happens while the page is alive and the reader is still there.
Why a lost vote matters rests on a premise this repo cannot check: vote counts are read from
docs_page_feedback. Note that this is a strict prefix ofdocs_page_feedback_comment, so an insight matching by "contains" or regex rather than exact equality already counts both — in which case the rationale is wrong today, not merely at risk. Neither event name appears anywhere else in the tree. The comment says so, rather than asserting the conclusion.What the comment says
The flush must stay unconditional and inside
submit().The argument against gating on
deliveredneeds no scenario: the gate can only ever lose a vote and can never gain one. Wheredeliveredis true the vote goes either way; where it is false the vote may still have been accepted — three ofcapturePostHogEvent's four false-exits are shared state read one line apart, but the fourth is a throw, and the vote's properties are a subset of the comment's, so a payload that throws on the comment need not have thrown on the vote.Statement order is explicitly called out as not what protects this — it only keeps the pair in sequence.
Review notes
Three review rounds. The first draft credited statement order for the protection, which was backwards — it would have licensed both destructive edits while forbidding a harmless swap. Later rounds tightened two further claims:
capturePostHogEventreturningtruemeans PostHog accepted the event, not that it reached the server (its own docstring is careful about this), and the flush buys at-least-once for a commenter's vote, not exactly-once — not being double-counted rests on thesessionStorageguard, which is per-tab and swallows its own failures.No test pins this; the repo has no test harness at all, and adding one for a comment would be disproportionate. Noting the gap rather than closing it.
🤖 Generated with Claude Code