fix: Address Copilot review comments on #100 and #102 - #104
Merged
Conversation
Two real findings plus one readability change. checkScopedFile's doc comment still described the earlier behavior, where the first scope's failure was reported. The implementation was changed to prefer a scope that exists but is misconfigured over one that is merely absent, so the comment contradicted the code directly above it. TestControlEventBurstTriggersOneRefresh slept a hard-coded 500ms while waiting for further coalesced refreshes, which is disconnected from controlRefreshWindow and would silently stop testing anything if that window grew. Derived from the window instead. Also switched *failures++ to (*failures)++ to match the surrounding style. Copilot reported the original as incrementing the pointer and failing to compile; neither is true, since Go has no pointer arithmetic and ++ applies to the *failures expression, which is why CI passed on three platforms and three Go versions. Verified separately. The change is for clarity only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Ready to approve
The changes are low-risk (doc/test/readability), consistent with surrounding code, and don’t introduce behavioral changes to production logic.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR is a small follow-up that addresses prior review feedback by (1) correcting a stale doc comment in cmd/doctor.go, and (2) making a watch coalescing test resilient to future timing-window changes, plus a minor clarity tweak to a pointer increment expression.
Changes:
- Update
checkScopedFile’s doc comment to match its “prefer misconfigured over absent” failure-selection behavior. - Replace a fixed sleep in
TestControlEventBurstTriggersOneRefreshwith a duration derived fromcontrolRefreshWindow. - Change
*failures++to(*failures)++for readability (no behavior change).
File summaries
| File | Description |
|---|---|
| watch/control_events_test.go | Makes the control-event coalescing test’s wait time track the actual debounce window to avoid silent false passes if the window changes. |
| cmd/doctor.go | Aligns checkScopedFile documentation with current behavior and clarifies the pointer increment syntax. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
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
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.
Follow-up to the Copilot review comments that arrived on #100 and #102 as those merged. Two are real; one is a false positive that I've changed anyway for readability.
1. Stale doc comment on
checkScopedFile(#100) — validThe function comment still said:
That described the first implementation. It was later changed to prefer a scope that exists but is misconfigured over one that is merely absent, so the doc contradicted the code immediately below it. Rewritten to defer to the rule the body documents.
2. Hard-coded sleep in the coalescing test (#102) — valid
TestControlEventBurstTriggersOneRefreshslept a fixed500 * time.Millisecondwaiting for any further coalesced refreshes. That's disconnected fromcontrolRefreshWindow: if the window ever grew past 500ms the test would stop testing anything and silently pass. Now derived from the window:Several multiples, so a slow CI machine doesn't read a late second refresh as a pass.
3.
*failures++(#100) — not a defect, changed for clarityCopilot reported this as incrementing the pointer and not compiling. Neither is the case. Go has no pointer arithmetic, and
++is a statement applied to the whole*failuresexpression — which is why #100 built and passed on three platforms across Go 1.24/1.25/1.26. Verified independently:(*failures)++is clearer and matches the form the surrounding code already used, so I've made the change — but on style grounds, not correctness, and no behavior changes.Type of change
Checklist
go build && ./codemap .CONTRIBUTING.md; this does not add a new language.Verification
go test ./..., plus-count=3on./cmdand./watchto confirm the retimed sleep didn't introduce flake.go vet,staticcheck,gofmtclean;go vetclean cross-compiled for windows/amd64 and linux/amd64.