Skip to content

Review-followup sweep: 8 bug fixes, 5 improvements, 6 suggestions (+1… - #2

Merged
rokogan merged 1 commit into
masterfrom
claude/review-followups-from-1.6.2
May 24, 2026
Merged

Review-followup sweep: 8 bug fixes, 5 improvements, 6 suggestions (+1…#2
rokogan merged 1 commit into
masterfrom
claude/review-followups-from-1.6.2

Conversation

@rokogan

@rokogan rokogan commented May 24, 2026

Copy link
Copy Markdown
Contributor

…2 tests)

Off the v1.6.2 base, before the v1.6.3..v1.6.12 upstream sweep — push to a branch for the human to integrate. Some of these may overlap with the upstream work; cherry-pick what's still useful.

Bug fixes (8):

  • B1 UnitConverter.TryParse now routes through LocaleNumber.TryParse so European-decimal selections like "1,5 kg" stop misparsing as 15 kg.
  • B2 README hsl example changed from hsl(120deg, ...) (regex never matched) to hsl(120, ...).
  • B3 README privacy section now lists ipinfo.io and api.qrserver.com, the two cloud actions previously omitted from the cloud-actions paragraph.
  • B4 ConvertTimezoneAction fallback parse uses InvariantCulture to match the detector's parse; previously "1/2/2024" diverged between detector verdict and action behavior on non-US locales.
  • B5 MathEvaluator doc-comment now notes the period-decimal requirement for European-locale users.
  • B6 Clipboard.SetText sites wrapped in TrySetClipboardText with toast on failure; paste-mode now aborts the paste if the write failed (would otherwise paste stale clipboard contents).
  • B7 Translation 429 detection via HttpStatusCode.TooManyRequests instead of message-string matching (matches the dictionary path style).
  • B8 Removed Task.Run from AutoStart_Changed so Save runs single-threaded on UI; updated SettingsManager.Save lock comment to reflect the new invariant (lock only protects the file write, not Current).

Improvements (5):

  • I1 Memoized the fixed (non-search) action-ID set in a Lazy so Load-time PruneStaleActionIds no longer instantiates a fresh ActionRegistry.
  • I2 Hover-preview cap 64KB -> 4KB. The preview is truncated to 120 chars anyway; full Execute on click still does the heavy parse.
  • I3 README atomic-writes wording: "crash-safe (temp + replace), not fsync'd" instead of the previous bare "atomic writes" claim.
  • I4 ExcludedAppsBox text-change handler no longer rebuilds the list per keystroke; the assignment moved into a FlushPendingTextEdits helper called from the debounce tick / Close / Save_Click.
  • I5 ActionRegistry.SplitWords now splits XMLHttpRequest correctly as XML | Http | Request (was "XMLHttp" | "Request"). 3 new tests.

Suggestions implemented (6):

  • S1 ToolbarWindow.xaml.cs (997 lines) split into 4 partial classes: the shell (323 lines), .Buttons (333), .Preview (168), .Actions (262).
  • S3 Esc dismissal now via a global WH_KEYBOARD_LL hook on its own STA thread (Core/KeyboardHook.cs); removed the GetAsyncKeyState polling from both ToolbarWindow and ResultPopup.
  • S4 New opt-in setting RestoreClipboardAfterAction (default off): after a copy-action puts result text on the clipboard, restore the previous contents ~3s later. Matches PastePlainTextAction's snapshot pattern.
  • S5 README risky-extension and UNC bullets expanded with the threat each prompt actually prevents.
  • S6 Added reject-path tests: Application1/HelloWorld12 not Base64, a+b/foo*bar/x-y-z not Math, locale-decimal cases for UnitConverter.
  • S7 Process.Start("explorer.exe", ...) migrated to ProcessStartInfo + ArgumentList so the path is quoted by the runtime instead of interpolated into a command-line string.

Tests: 214 -> 226. Build + tests green in Debug and Release.

…2 tests)

Off the v1.6.2 base, before the v1.6.3..v1.6.12 upstream sweep — push to
a branch for the human to integrate. Some of these may overlap with the
upstream work; cherry-pick what's still useful.

Bug fixes (8):
- B1 UnitConverter.TryParse now routes through LocaleNumber.TryParse
  so European-decimal selections like "1,5 kg" stop misparsing as 15 kg.
- B2 README hsl example changed from hsl(120deg, ...) (regex never matched)
  to hsl(120, ...).
- B3 README privacy section now lists ipinfo.io and api.qrserver.com,
  the two cloud actions previously omitted from the cloud-actions paragraph.
- B4 ConvertTimezoneAction fallback parse uses InvariantCulture to match
  the detector's parse; previously "1/2/2024" diverged between detector
  verdict and action behavior on non-US locales.
- B5 MathEvaluator doc-comment now notes the period-decimal requirement
  for European-locale users.
- B6 Clipboard.SetText sites wrapped in TrySetClipboardText with toast on
  failure; paste-mode now aborts the paste if the write failed (would
  otherwise paste stale clipboard contents).
- B7 Translation 429 detection via HttpStatusCode.TooManyRequests
  instead of message-string matching (matches the dictionary path style).
- B8 Removed Task.Run from AutoStart_Changed so Save runs single-threaded
  on UI; updated SettingsManager.Save lock comment to reflect the new
  invariant (lock only protects the file write, not Current).

Improvements (5):
- I1 Memoized the fixed (non-search) action-ID set in a Lazy so Load-time
  PruneStaleActionIds no longer instantiates a fresh ActionRegistry.
- I2 Hover-preview cap 64KB -> 4KB. The preview is truncated to 120 chars
  anyway; full Execute on click still does the heavy parse.
- I3 README atomic-writes wording: "crash-safe (temp + replace), not
  fsync'd" instead of the previous bare "atomic writes" claim.
- I4 ExcludedAppsBox text-change handler no longer rebuilds the list per
  keystroke; the assignment moved into a FlushPendingTextEdits helper
  called from the debounce tick / Close / Save_Click.
- I5 ActionRegistry.SplitWords now splits XMLHttpRequest correctly as
  XML | Http | Request (was "XMLHttp" | "Request"). 3 new tests.

Suggestions implemented (6):
- S1 ToolbarWindow.xaml.cs (997 lines) split into 4 partial classes:
  the shell (323 lines), .Buttons (333), .Preview (168), .Actions (262).
- S3 Esc dismissal now via a global WH_KEYBOARD_LL hook on its own STA
  thread (Core/KeyboardHook.cs); removed the GetAsyncKeyState polling
  from both ToolbarWindow and ResultPopup.
- S4 New opt-in setting RestoreClipboardAfterAction (default off): after
  a copy-action puts result text on the clipboard, restore the previous
  contents ~3s later. Matches PastePlainTextAction's snapshot pattern.
- S5 README risky-extension and UNC bullets expanded with the threat
  each prompt actually prevents.
- S6 Added reject-path tests: Application1/HelloWorld12 not Base64,
  a+b/foo*bar/x-y-z not Math, locale-decimal cases for UnitConverter.
- S7 Process.Start("explorer.exe", ...) migrated to ProcessStartInfo +
  ArgumentList so the path is quoted by the runtime instead of
  interpolated into a command-line string.

Tests: 214 -> 226. Build + tests green in Debug and Release.
@rokogan
rokogan merged commit fd7e6cc into master May 24, 2026
1 check passed
rokogan added a commit that referenced this pull request Jun 29, 2026
From an adversarial review of the v2.0.0 diff:
- TextCapture: only restore the clipboard if its sequence number still
  equals the one right after our copy — if a third party wrote in the
  meantime, they own it now; don't clobber them. [review #1]
- ResultPopup: broaden the online-lookup consent wording so it's accurate
  for custom "fetch" recipe actions (which send to their own host), not
  just the three built-in services. [review #6]
- SelectionTracker: claim the Ctrl+C debounce slot before the 100ms await
  so a rapid second Ctrl+C is dropped instead of double-firing. [review #2]
- MouseHook/KeyboardHook: null _hookThread in Uninstall so a later
  Install() isn't no-op'd by the non-null guard. [review #7]
- SettingsManager: prune stale IDs from AppHiddenActions too, dropping
  now-empty per-app entries. [review #13]

Release build clean; 289 tests pass.
@rokogan
rokogan deleted the claude/review-followups-from-1.6.2 branch June 29, 2026 02:35
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