feat: badge GitHub-synced comments and propagate to share payload - #604
Merged
tomasz-tomczyk merged 6 commits intoMay 29, 2026
Conversation
Owner
|
Thank you! Sorry it's taking me a while to get around to this. I'll review today ❤️ |
Comments synced from a GitHub PR carry their review-comment ID into the shared review JSON, so crit-web (and any other consumer of the share payload) can tell GitHub-synced comments apart from native crit comments. The new field is omitempty: locally-authored comments produce the same wire output as before. Replies carry the same field for the same reason. Verification: [x] go test -run TestBuildSharePayload ./... [x] go test -run TestHandleFileCommentsGitHubID ./... [x] gofmt -l . && go vet ./...
When a comment or reply has a non-zero github_id we paint a small "GitHub" pill in the card header. The badge uses the neutral pill tokens (informational, not a warning) and a shared --crit-icon-github CSS variable so the SVG mask is defined once in theme.css. Class renamed from .comment-card-gh-badge to .github-badge to match the project convention (.outdated-badge, .live-thread-badge). Verification: [x] node --test frontend/__tests__/crit-comment-card.test.js [x] cd e2e && npx playwright test tests/github-synced-comment.spec.ts --project=git-mode
The .github-badge text color and its masked octocat icon paint are both driven by var(--crit-pill-fg). That token existed when this branch was authored but was later removed from theme.css on main (tomasz-tomczyk#612/tomasz-tomczyk#300), so on current main the badge icon paints with an undefined color (transparent) and the GitHub mark disappears. Re-add --crit-pill-fg to all four theme blocks so the badge renders as intended; check-css-vars.sh passes. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
tomasz-tomczyk
force-pushed
the
feat/issue-370-flag-github-synced-comments
branch
from
May 29, 2026 22:01
656c0d3 to
88bc1c6
Compare
Instance 1 (markdown) injects a GitHub-synced comment with a synced reply, plus a synced reply mixed into a native thread, into the review file after finish. The daemon's file watcher merges them (preserving github_id), so the .github-badge from this PR renders in a real review. Gives `make test-diff` a local visual fixture for the tomasz-tomczyk#370 badge alongside native comments. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
`func truncate` is defined in both roundtrip_integration_test.go and share_test.go (the latter added by tomasz-tomczyk#505). They only collide when compiled together under the e2e_github build tag, which CI never builds — so `make e2e-roundtrip` failed with 'truncate redeclared in this block'. Remove the copy from the tagged file; it now resolves to the always-compiled share_test.go helper. Unblocks the crit<->GitHub roundtrip suite, which is the real github_id pull/push persistence coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Owner
|
Thank you! Looks great! I just added it to our local 'seeds' in |
ESLint (the lint-frontend CI job) flags `var ghBadge` under no-var. Switch the badge element to const in both the reply renderer (app.js) and the comment-card header (crit-comment-card.js). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #604 +/- ##
==========================================
+ Coverage 70.63% 70.68% +0.04%
==========================================
Files 54 54
Lines 14243 14248 +5
==========================================
+ Hits 10061 10071 +10
+ Misses 3412 3409 -3
+ Partials 770 768 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Why
GitHub-synced comments now carry a visual badge in the review UI, and the share payload includes
github_idso crit-web can distinguish them from native crit comments. This closes the two local-side items from the issue: (a) surface the sync origin to reviewers at a glance, and (b) pass that signal through the share endpoint. Closes #370Key changes
share.go: addedGitHubID int64toshareComment/shareReplystructs, propagated incommentToShareComment. Two new tests lock the JSON shape for synced and non-synced cases..github-badgepill rendered in both comment headers and reply lists, styled with existing--crit-pill-*tokens. Octocat icon lives as a CSS mask variable (--crit-icon-github) intheme.cssto avoid duplicating the data URI.server_test.go: assertsgithub_idpresence in the GET response.round-completeevent path to avoid mtime-based race conditions.Why no
sourcefieldThe issue suggested a
source: 'github'discriminator alongsidegithub_id. A non-zerogithub_idalready implies GitHub origin unambiguously, so adding a separate enum today would be dead weight. If we later sync from GitLab or Gerrit we can introduce asourcefield at that point without breaking existing consumers (the comment inshare.godocuments this decision).Verification
go test -run TestBuildSharePayload ./...go test -run TestHandleFileCommentsGitHubID ./...gofmt -l . && go vet ./...node --test frontend/__tests__/crit-comment-card.test.jscd e2e && npx playwright test tests/github-synced-comment.spec.ts --project=git-modego test ./...(full regression pass; knownTestInstallCodexPluginConfigDefaultsNewFileTo0600failure on Windows due to 0600 permission semantics is pre-existing and unrelated)Screenshots
No screenshots attached. If you'd like specific scenarios captured (light/dark, mobile viewport, stacked badges on a reply) I can put together a fixture and grab them.
Out of scope
sourceenum makes sense only when we add a second providerNotes
The frontend unit tests (
frontend/__tests__/*.test.js) aren't wired into CI today. They run locally vianode --testand insidemake e2e-live-utils, but that target isn't in the GitHub Actions workflow. Not something to fix in this PR, just flagging it.