Skip to content

fix: Tier-3 minor fixes — dead stub removal + telemetry quit flush (#693, #690)#709

Merged
malpern merged 2 commits into
masterfrom
fix/tier3-693-690
Jun 2, 2026
Merged

fix: Tier-3 minor fixes — dead stub removal + telemetry quit flush (#693, #690)#709
malpern merged 2 commits into
masterfrom
fix/tier3-693-690

Conversation

@malpern
Copy link
Copy Markdown
Owner

@malpern malpern commented Jun 2, 2026

Two small Tier-3 cleanups.

#693PackDetailView.copyValidationErrorsToClipboard() was an empty placeholder, never called, with no validation-error state to copy (the real impl is in LiveKeyboardOverlayView). Deleted as dead code.

#690 — KindaVim telemetry flushed on a ~5s timer with no final flush in deinit, losing up to one interval on quit. Now calls the existing MainActor-safe flushNow() from applicationWillTerminate (no-op when disabled).

malpern and others added 2 commits June 2, 2026 09:47
PackDetailView's copyValidationErrorsToClipboard() was an empty placeholder
that's never called and has no validation-error state to copy. The working
implementation lives in the overlay path (LiveKeyboardOverlayView). Delete the
dead stub.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The KindaVim telemetry store flushes on a ~5s timer and skips a final flush in
deinit (MainActor restriction), so up to one interval of aggregate counters was
lost on quit. Call the existing MainActor-safe flushNow() from
applicationWillTerminate. No-op when telemetry is disabled.

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

claude Bot commented Jun 2, 2026

Code Review

Summary: Two small Tier-3 cleanups — a dead stub removal and a telemetry quit-flush fix. Both changes are minimal, correct, and well-scoped.


#693 — Dead stub removal (PackDetailView+Helpers.swift)

Clean removal. copyValidationErrorsToClipboard() was never called, had no state to operate on, and had no implementation. Deleting it is the right call — empty placeholders create false impressions of available functionality and accrue as quiet tech debt.

No concerns here.


#690 — Telemetry quit flush (App.swift, KindaVimTelemetryStore.swift)

Correct fix. A one-line call to an already-tested flushNow() from applicationWillTerminate. Placement before kanataManager?.cleanupSync() is appropriate.

Concurrency is safe. KindaVimTelemetryStore is @MainActor, and Swift's SDK annotations make NSApplicationDelegate callbacks implicitly @MainActor — the synchronous call compiles correctly without a bridging await.

Minor nit — comment references issue number. Per project convention, issue references belong in the PR description rather than inline comments. The WHY ("up to one ~5s flush interval would otherwise be dropped") is the genuinely non-obvious bit worth keeping; the ticket reference (#690) could be dropped:

// Before:
// Flush pending KindaVim telemetry so counters aren't lost on quit (up to one
// ~5s flush interval would otherwise be dropped — see #690). No-op when disabled.

// Suggested:
// Flush pending telemetry — up to one ~5s debounce interval can otherwise be
// lost on quit. No-op when telemetry is disabled.

Not a blocker — nit-level only.

Test coverage: flushNow() itself is already tested; the integration here is a single delegation call, so no new test is needed.

deinit comment update is clean — removing the stale TODO and pointing to the now-implemented fix is good housekeeping.


Summary

Area Status
Correctness ✅ Both changes are correct
Concurrency safety ✅ MainActor isolation is sound
Project conventions ✅ Minor comment-style nit only
Test coverage ✅ Adequate for the scope
Risk ✅ Low — no logic changes, no new code paths

Verdict: Looks good to merge. The comment nit is optional.

@malpern malpern merged commit 29c6323 into master Jun 2, 2026
3 checks passed
@malpern malpern deleted the fix/tier3-693-690 branch June 2, 2026 16:50
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