mirror the change screen's thread semantics - #45
Merged
Conversation
get_change_comments reconstructed threads per Gerrit's server-side CommentThreads (change-wide id pooling, chronological tails). The change screen computes threads differently (polygerrit comment-util): range patching across replies is array-order-dependent, absent numbers sort before present ones, and a reply whose parent has not sorted yet forks into its own thread. On real reviews the two disagree: a ranged root with rangeless out-of-order replies renders on screen as a thread ending in the reviewer's unresolved re-raise while the server-side model calls it resolved -- an agent told the user nothing was pending while their screen showed 19 unresolved threads. The contract is now: any thread the change screen shows as unresolved is unresolved. Thread building replicates polygerrit stable-3.13 bug-for-bug: order-dependent sanitiseRanges, undefined-first numeric comparisons, patchset-level path pinned first, drafts after published, fork-on-unseen-parent. The inherited sort range stays internal; rendering keeps each comment's real anchor. Verified against the reported review: 148 threads, 19 unresolved, digit-identical to the change screen. Consequence: the tool can now disagree with has:unresolved search results in exactly the cases where the UI does. Fixes: #44
Output splits into an unresolved and a resolved section — a file with both kinds of threads appears in each — so what needs action reads first. Inside a section, files order as the change screen names them (patchset level, commit message, then paths); threads within a file follow the time of their latest comment; comments within a thread follow history. Comment ids break ties only on equal timestamps. Thread grouping and resolved state stay the change screen's replica; only the presentation order is the tool's own.
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
1 participant
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.
Fixes #44.
The tool now replicates polygerrit stable-3.13 comment threading bug-for-bug, because the contract is: any thread the change screen shows as unresolved is unresolved. Replicated quirks: order-dependent
sanitiseRanges(a reply preceding its parent in the payload array inherits no range),compareNumber's absent-sorts-first ordering,/PATCHSET_LEVELpinned before all paths, drafts after published, andcreateCommentThreads' fork-on-unseen-parent. The inherited sort range stays internal — rendering keeps each comment's real anchor.Validated against the reported review live: 148 threads, 19 unresolved — digit-identical to the change screen's chip, where the previous (server-semantics) reconstruction reported 64/0. The reporter's exact failing call (
status=unresolved) now returns those 19 threads.Documented consequence: the tool can disagree with
has:unresolvedsearch results in exactly the cases where the UI itself does. Fork behavior pinned by a new golden (ranged root + out-of-order rangeless reply); the rename golden updated to the screen's behavior (reply forks when its file sorts before the root's).