Skip to content

refactor: bundled cleanup — wrappers, mustGetwd, browser.go, error surfacing#428

Merged
tomasz-tomczyk merged 1 commit intomainfrom
refactor-cleanup-bundle
May 2, 2026
Merged

refactor: bundled cleanup — wrappers, mustGetwd, browser.go, error surfacing#428
tomasz-tomczyk merged 1 commit intomainfrom
refactor-cleanup-bundle

Conversation

@tomasz-tomczyk
Copy link
Copy Markdown
Owner

Summary

Bundled refactor PR addressing four small cleanup opportunities surfaced by an audit (separate from #425, which covers the comment-CLI / review-file extraction).

  • Delete 7 unscoped wrapper functions in github.go that were one-line delegates to *Scoped variants with inheritedScope{}. Removed: appendComment, appendReviewComment, appendFileComment, addCommentToCritJSON, addReviewCommentToCritJSON, addFileCommentToCritJSON, bulkAddCommentsToCritJSON. No production callers; tests updated to call the *Scoped versions directly. Aligns with .claude/rules/go.md which forbids same-signature wrappers.
  • Add mustGetwd() helper and replace 7 ignored-error os.Getwd() sites in main.go (×6) and share.go (×1). Previously these silently kept "" as cwd on failure, leading to bizarre downstream paths in the unlikely deleted-cwd scenario.
  • Extract browser.go from main.go. browser_test.go already existed in the package — pure code move, no test changes. Resolves the smell of a test file with no corresponding source file.
  • Distinguish ENOENT from parse/IO errors in two corruption-tolerant loaders:
    • share.go loadExistingShareCfg now returns (CritJSON, bool, error). ENOENT → (zero, false, nil); parse/IO → wrapped error so the caller can surface a diagnostic. Single production caller updated.
    • focus_cli.go loadCritJSONForOutputDir logs parse/IO errors loudly to stderr instead of swallowing them as missing-file. Signature unchanged because the underlying loader synthesizes a fresh struct on ENOENT.
  • Inline pluralReply at its single call site; delete the helper and its test.

Review

  • Code review: passed (fresh go-expert review — 0 blockers, 0 warnings, 3 acceptable notes)
  • Parity audit: N/A (no review-page changes)

Test plan

  • `go build ./...` clean
  • `go vet ./...` clean
  • `gofmt -l .` clean
  • `go test ./...` passes (35s)
  • Pre-commit hooks (golangci-lint, ESLint, Stylelint, CSS-var check) all pass on commit

🤖 Generated with Claude Code

…rfacing

- Delete 7 unscoped wrapper functions in github.go (per .claude/rules/go.md)
  that were one-line delegates to *Scoped variants with inheritedScope{}.
  Update test callers to call Scoped versions directly.
- Add mustGetwd() helper; replace 7 ignored-error os.Getwd() sites in
  main.go and share.go that silently kept "" as cwd on failure.
- Extract browser.go from main.go (browser_test.go already existed in the
  package — pure code move, no test changes).
- Distinguish ENOENT from parse/IO errors:
  - share.go loadExistingShareCfg now returns (CritJSON, bool, error);
    callers surface parse errors with diagnostics instead of treating
    corrupt files as missing.
  - focus_cli.go loadCritJSONForOutputDir logs parse/IO errors loudly
    to stderr rather than swallowing them.
- Inline pluralReply at its single call site; delete the helper and its
  test.

Builds clean, vet clean, gofmt clean, tests pass (go test ./... — ok 35s).
@tomasz-tomczyk tomasz-tomczyk merged commit 57ba566 into main May 2, 2026
7 checks passed
@codecov
Copy link
Copy Markdown

codecov Bot commented May 2, 2026

Codecov Report

❌ Patch coverage is 59.37500% with 39 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.26%. Comparing base (68ade03) to head (3f89e82).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
browser.go 71.18% 17 Missing ⚠️
main.go 37.50% 14 Missing and 1 partial ⚠️
focus_cli.go 0.00% 4 Missing ⚠️
share.go 66.66% 3 Missing ⚠️

❌ Your patch status has failed because the patch coverage (59.37%) 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     #428      +/-   ##
==========================================
- Coverage   67.38%   67.26%   -0.12%     
==========================================
  Files          26       27       +1     
  Lines        9747     9748       +1     
==========================================
- Hits         6568     6557      -11     
- Misses       2663     2675      +12     
  Partials      516      516              
Flag Coverage Δ
e2e 34.11% <7.29%> (+0.03%) ⬆️
unit 63.51% <59.37%> (-0.12%) ⬇️

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 deleted the refactor-cleanup-bundle branch May 2, 2026 11:22
tomasz-tomczyk added a commit that referenced this pull request May 2, 2026
- review_file.go: document gh-CLI repo-scoping assumption that keeps
  cross-repo branch-name collisions from being a real-world hazard.
- main.go: extend runPull comment to note that a corrupt cwd file also
  produces empty cj.Branch and triggers branch-based redirect.
- .golangci.yml: update stale gocyclo exclusion to reference the
  *Scoped function name (post-#428 wrapper deletion).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
tomasz-tomczyk added a commit that referenced this pull request May 2, 2026
* refactor: split github.go into github / comment_cli / review_file

github.go had grown to ~1774 lines mixing three unrelated concerns:
gh-CLI / PR push-pull, the comment-authoring CLI, and review-file I/O.
PR #424's bulk-routing fallback made the entanglement obvious.

Move whole functions, no behavior changes, no signature changes:

- review_file.go: resolveReviewPath, resolveReviewPathFromDaemon,
  pickReviewPath, resolveReviewPathFromSessions, writeCritJSON,
  loadCritJSON, saveCritJSON, clearCritJSON,
  findReviewFileByCommentID, reviewFileContainsComment,
  cjContainsCommentID

- comment_cli.go: appendComment[Scoped], readAnchorFromDisk,
  appendReply, addCommentToCritJSON[Scoped], addReplyToCritJSON,
  BulkCommentEntry (incl. UnmarshalJSON), processBulkEntry and
  helpers, parseLineSpec, bulkAddCommentsToCritJSON[Scoped],
  resolveBulkTarget, addReviewCommentToCritJSON[Scoped],
  addFileCommentToCritJSON[Scoped], appendReviewComment[Scoped],
  appendFileComment[Scoped]

- github.go: keeps only the gh-CLI / PR sync surface
  (detectPR, fetchPRComments, mergeGHComments*, createGHReview,
  push/pull plumbing, truncateStr).

Tests stay in github_test.go for this commit — they all live in
package main and continue to compile and pass against the moved
functions. Splitting the 2700-line test file along the same lines
is mechanical follow-up work and not required for the refactor to
be useful.

go build ./... and go test ./... both pass.

* fix: route crit pull --pr to review file matching the PR's branch

When a user runs `crit pull <pr>` from a worktree whose cwd-resolved
review file is for a different branch than the named PR, comments
were silently merged into the wrong review file — same class of
cwd-vs-intent mismatch that PR #424 fixed for `crit comment`.

The trigger is narrow: only kicks in when the user passed an
explicit PR number, didn't override --output, and the existing
review file's "branch" field disagrees with the PR's headRefName.
Under those conditions we scan ~/.crit/reviews/ for exactly one
review file matching the PR's branch and route to it, with a stderr
note. If zero or multiple match, we fall back to the cwd-resolved
path (today's behavior).

Adds findReviewFileByBranch helper to review_file.go alongside
findReviewFileByCommentID and a redirectReviewPathForPR shim
in main.go that wires the fetch+match together.

* fix: route crit push --pr to review file matching the PR's branch

Mirrors the previous commit for `crit pull`. When a user runs
`crit push <pr>` from a worktree whose cwd-resolved review file is
for a different branch than the named PR, we'd silently post the
wrong comments to that PR — strictly worse than the pull case
because pushing creates side effects on GitHub that are visible to
reviewers.

Trigger conditions are identical to the pull fix: explicit PR
number, no --output override, existing review file's branch
disagrees with the PR's headRefName, and exactly one alt review
file matches the PR's branch. Reuses redirectReviewPathForPR.

* fix: log Note when multiple review files match PR branch in redirect

When a multi-worktree user runs crit pull/push with --pr and several
review files share the PR's head branch, findReviewFileByBranch returned
a generic error and redirectReviewPathForPR swallowed it — leaving the
user with the cwd file and no signal of why.

Distinguish "no match" (silent fallback) from "ambiguous" (stderr Note)
via sentinel errors. The Note tells the user to pass --output to
disambiguate.

* fix: reroute crit pull/push --pr when cwd review file missing

Previously the gate in runPull/runPush required cj.Branch != "" before
attempting to redirect to a branch-matching review file. When the cwd
review file didn't exist, cj.Branch was empty and the redirect never
fired — the user got a fresh wrong-location file silently.

For runPull: drop the cj.Branch != "" gate; pass empty cwdBranch through
to redirectReviewPathForPR, which now skips its cwd-vs-PR-branch
early-out when cwdBranch is empty (no false-positive risk without cwd
state).

For runPush: tolerate ENOENT on the initial review file read so an
explicit --pr from a clean checkout can find the right file by branch
via the redirect. Only fail with "no review file found" if the redirect
also misses.

* test: cover findReviewFileByBranch + redirectReviewPathForPR

Table-driven tests for both helpers using the existing fetchPRByNumberFn
test seam (via withFetchPRByNumber) and a temp HOME for isolated
~/.crit/reviews/ fixtures.

Covers all redirect branches: cwd-matches-PR, unique-alt, no-alt,
ambiguous-alt (asserts the new stderr Note), fetch error, empty
HeadRefName, and empty cwdBranch (Fix 1: redirect must fire even when
the cwd review file is missing). Also covers the four
findReviewFileByBranch outcomes (single, none, ambiguous, exclude).

* docs: address second-pass review notes

- review_file.go: document gh-CLI repo-scoping assumption that keeps
  cross-repo branch-name collisions from being a real-world hazard.
- main.go: extend runPull comment to note that a corrupt cwd file also
  produces empty cj.Branch and triggers branch-based redirect.
- .golangci.yml: update stale gocyclo exclusion to reference the
  *Scoped function name (post-#428 wrapper deletion).

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

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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