Skip to content

fix: preserve replies on fingerprint-matched comments + audit cleanup#367

Merged
tomasz-tomczyk merged 1 commit intomainfrom
audit/go-fixes
Apr 26, 2026
Merged

fix: preserve replies on fingerprint-matched comments + audit cleanup#367
tomasz-tomczyk merged 1 commit intomainfrom
audit/go-fixes

Conversation

@tomasz-tomczyk
Copy link
Copy Markdown
Owner

Summary

Release audit follow-up addressing findings from v0.10.0..HEAD review.

Bug fix (P0):

  • Web-authored replies were silently dropped when their parent comment was matched by fingerprint instead of external_id

Refactors:

  • Replace variadic replyUpdates anti-pattern with fixed map argument
  • Refactor mergeWebComments to iterate by index (no mid-range mutation)
  • Log non-NotExist errors in loadCliArgsFromReviewFile (no silent swallow)
  • Use replyCount in runFetch print message
  • Extract shareReviewFiles helper (dedup runShareNew + handleShare)

Tests:

  • Rename TestFetchNewWebComments_*TestFetchWebComments_* after function rename
  • Consolidate duplicated test groups into table-driven subtests
  • New regression: TestFetchWebComments_FingerprintMatchPreservesReplies
  • New coverage: TestMergeWebComments_AppliesReplyUpdates

Review

  • Code review: passed (go-expert agent — verdict: ship)
  • go vet ./... clean
  • go test -race ./... clean

Test plan

  • Local make e2e-share (skipped during audit — requires running crit-web)

🤖 Generated with Claude Code

- Fix data-loss bug where web-authored replies were silently dropped
  when their parent comment was matched by fingerprint (not external_id)
- Replace variadic replyUpdates anti-pattern with fixed map argument
- Refactor mergeWebComments to iterate by index (no mid-range mutation)
- Log non-NotExist errors in loadCliArgsFromReviewFile (no silent swallow)
- Use replyCount in runFetch print message
- Rename TestFetchNewWebComments_* → TestFetchWebComments_*
- Consolidate duplicated test groups into table-driven subtests
- Add TestFetchWebComments_FingerprintMatchPreservesReplies regression
- Add TestMergeWebComments_AppliesReplyUpdates coverage
- Extract shareReviewFiles helper (dedup runShareNew + handleShare)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 26, 2026

Codecov Report

❌ Patch coverage is 68.00000% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.19%. Comparing base (d676a68) to head (d2a2f03).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
main.go 0.00% 10 Missing ⚠️
share.go 83.78% 4 Missing and 2 partials ⚠️

❌ Your patch status has failed because the patch coverage (68.00%) 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     #367      +/-   ##
==========================================
+ Coverage   66.88%   67.19%   +0.31%     
==========================================
  Files          18       18              
  Lines        7807     7823      +16     
==========================================
+ Hits         5222     5257      +35     
+ Misses       2193     2176      -17     
+ Partials      392      390       -2     
Flag Coverage Δ
e2e 34.87% <14.00%> (-0.15%) ⬇️
unit 63.03% <68.00%> (+0.31%) ⬆️

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.

@tomasz-tomczyk tomasz-tomczyk merged commit 15309c5 into main Apr 26, 2026
5 of 6 checks passed
@tomasz-tomczyk tomasz-tomczyk deleted the audit/go-fixes branch April 26, 2026 19:51
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.

1 participant