Skip to content

fix: import GitHub thread resolved state on crit pull#462

Merged
tomasz-tomczyk merged 1 commit intomainfrom
fix-453-import-resolved-state
May 5, 2026
Merged

fix: import GitHub thread resolved state on crit pull#462
tomasz-tomczyk merged 1 commit intomainfrom
fix-453-import-resolved-state

Conversation

@tomasz-tomczyk
Copy link
Copy Markdown
Owner

Summary

  • crit pull now reads thread-level isResolved via GraphQL (reviewThreads) and joins it onto imported comments by databaseID. Previously, resolves done on github.com (or via the resolveReviewThread mutation) silently leaked past the local Comment.Resolved flag.
  • Asymmetric merge: GitHub-resolved → local resolved (always); GitHub-unresolved + local resolved → keep local. Mirrors the existing Body-on-pull precedent and avoids clobbering resolutions made via crit / crit-web.
  • GraphQL fetch is best-effort: a failure logs a warning and proceeds with the existing REST-only merge.
  • Unskips TestRoundtrip_GitHubThreadResolvedOnWeb in the e2e harness.

Review

  • Code review: passed (1 inherited pre-existing dedup-by-body limitation noted, not introduced by this PR)
  • Parity audit: N/A (server-side GH sync, no review-page surface)

Test plan

  • New unit tests cover thread-resolved propagation on new + deduped comments, asymmetric clobber-protection, nil-map no-op, and reply-ID fallback.
  • go test -race -count=1 ./... green.
  • Roundtrip e2e against crit-md/crit-roundtrip-sandbox passes.

Closes #453

🤖 Generated with Claude Code

REST /pulls/{n}/comments returns no thread-level isResolved bit, so when
a reviewer clicked Resolve on github.com (or invoked resolveReviewThread
via GraphQL), `crit pull` left local Comment.Resolved at false. The local
review file silently disagreed with the PR.

Fix: add a GraphQL fetch over reviewThreads { isResolved comments {
databaseId } } and join isResolved onto each imported comment by
databaseID. The bit is shared across the whole thread (root + replies),
so only the root's Resolved / ResolvedRound is updated; replies inherit
thread state implicitly via the parent. ResolvedRound is stamped from
cj.ReviewRound, mirroring the local `crit comment --resolve` semantics.

Conflict semantics (asymmetric, follows Body precedent):
  - remote resolved + local unresolved -> set Resolved=true
  - remote unresolved + local resolved -> KEEP local (no clobber)
  - both same -> no-op

The asymmetry matters because crit doesn't push resolved state back to
GitHub, and a local user may resolve a thread via crit / crit-web
independently. A symmetric merge would silently undo that.

The GraphQL fetch is best-effort: a failure prints a warning and
proceeds with the existing REST-only merge, so token-scope or transient
GraphQL outages don't block a comment pull.

The previously-skipped roundtrip scenario
TestRoundtrip_GitHubThreadResolvedOnWeb is unskipped and passes against
crit-md/crit-roundtrip-sandbox.

Closes #453
@tomasz-tomczyk tomasz-tomczyk merged commit 5175895 into main May 5, 2026
6 checks passed
@tomasz-tomczyk tomasz-tomczyk deleted the fix-453-import-resolved-state branch May 5, 2026 12:58
@codecov
Copy link
Copy Markdown

codecov Bot commented May 5, 2026

Codecov Report

❌ Patch coverage is 32.35294% with 69 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.75%. Comparing base (67e1709) to head (35ccf29).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
github.go 34.02% 63 Missing and 1 partial ⚠️
main.go 0.00% 5 Missing ⚠️

❌ Your patch status has failed because the patch coverage (32.35%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #462      +/-   ##
==========================================
- Coverage   69.10%   68.75%   -0.36%     
==========================================
  Files          36       36              
  Lines       10546    10634      +88     
==========================================
+ Hits         7288     7311      +23     
- Misses       2697     2762      +65     
  Partials      561      561              
Flag Coverage Δ
e2e 32.73% <0.00%> (-0.28%) ⬇️
unit 66.24% <32.35%> (-0.34%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

crit pull does not import GitHub review-thread resolved state

1 participant