fix(settings): make tracking toggle take effect immediately (issue #20) - #23
fix(settings): make tracking toggle take effect immediately (issue #20)#23linletian wants to merge 8 commits into
Conversation
Toggling the tracking switch in Settings → Services now removes the instance's slot from AppState._slotViewDataList, so the menu bar stops rendering the disabled instance. The pruning runs on every state-mutation path (setInstanceTracking / setInstances / mergeCycleResult) via a shared private helper, so a toggle that flips between cycle start and merge cannot be re-introduced by a late supplier success. The new setInstanceTracking(uuid:enabled:) method is the single authoritative update path; SettingsViewModel will use it to make the toggle take effect immediately in the next commit. Tests: 9 new cases in AppStateTrackingToggleTests covering each mutation path and the toggle-off → on → refresh lifecycle.
Thin async wrapper that calls AppState.setInstanceTracking and syncs the @published snapshot, mirroring the existing updateInstance pattern. Used by SettingsViewModel in the next commit to make the tracking toggle take effect immediately.
The toggle in Settings → Services now propagates to the runtime AppState without waiting for Save Changes. The local draft is still updated, so hasUnsavedChanges keeps showing the toggle as a pending commit until the user clicks Save (or Discard). discardChanges() is now async and rolls the runtime tracking flag back per-UUID (instead of a full setInstances rebuild that would re-flash the menu bar for every unrelated instance). The SettingsWindow 'Don't Save' path awaits the rollback before closing the window to keep the user-visible state consistent. The two new test files (AppStateTrackingToggleTests, this one) are added to the APIUsageStatusTests target via project.pbxproj so the suite actually runs them. xcodegen is not installed on this machine; the project.pbxproj updates mirror the existing test-file registration pattern. Tests: 6 new cases in SettingsViewModelTrackingToggleTests covering immediate propagation, hasUnsavedChanges semantics, discard rollback, revert-to-original behavior, and durable save() persistence.
#20) Editing an existing instance now lets the user clear every metric and save — the user is intentionally pausing tracking for that entry. New-instance behavior is unchanged: at least one metric is still required so we never persist fully unconfigured entries. A warning banner ("Disabling all metrics will pause usage tracking") surfaces only in the edit-with-zero-metrics state so the user has a clear signal about the side effect on the menu bar slot. The validation rules are extracted into a static `evaluateFormState(...)` helper. The instance computed properties (isFormFilled, isAllMetricsDisabled) delegate to it, so the runtime and unit-test paths share one implementation. The helper is the test entry point — SwiftUI's @State defaults can't be observed from XCTest, so the unit tests exercise the pure function directly. Tests: 8 new cases in InstanceEditorViewValidationTests covering new/edit/populated/empty combinations, shortName rules, banner visibility, and the MiniMax-no-models short-circuit.
Add 'Tracking toggle semantics (issue #20)' subsection covering the AppState mutation contract (setInstanceTracking / setInstances / mergeCycleResult / pruneDisabledSlots), the toggle-path flow from SettingsView down to the menu-bar re-render, the interaction with RefreshService's already-correct enabled filter, and an explicit note that the existing ColorState.disabled branches in MenuBarIconRenderer are still dead code (deliberately left for a future cleanup PR — see the companion .omo/plans/fix-issue20-services-tracking-toggle.md local record per AGENTS.md §7).
Review — PR #23 (issue #20 tracking toggle)Implementation is clean: Blocking1. Warning banner text contradicts the actual menu-bar behavior —
The slot is removed only when Reproducer: Services → edit instance → uncheck every metric → Save. The menu bar keeps the slot; only the numbers go away. Fix options (any one):
Non-blocking (suggested before merge if scope allows)2. The rollback loop looks up the instance in the current 3. Stale doc comment —
4. Self-contradictory comment in The comment says "empty instances list" then "single no-provider instance" then cites the 5. Banner fires on form open for already-paused instances —
What works
|
…review) PR #23 review surfaced two related issues with the warning banner that fires when the user clears every metric in the edit form. **1. Banner text contradicted actual behavior.** The old copy said 'The menu bar slot will disappear', but `AppState` only prunes a slot when `trackingEnabled == false`. Saving with zero metrics while tracking stays on leaves the shortName visible — `mapInstanceToSlotData` produces an empty-snapshot slot and `MenuBarIconRenderer.expandToMetricSlots` appends it unconditionally (the `.disabled` guard is dead code). Reworded the banner to match reality: shortName stays, no data renders until a metric is re-enabled. **2. Banner fired on form open for already-paused instances.** `isAllMetricsDisabled` is a function of `(isEditing, selectedMetrics.isEmpty)`; opening an instance that was previously saved with zero metrics made the banner pop immediately with forward-looking wording for a state the user didn't just enter. Tracked a new `@State hasUserTouchedMetrics` that the form's load hook resets and each metric-toggle mutates. The banner now only surfaces when the user is the one doing the disabling. Added a static helper `shouldShowAllMetricsBanner(...)` so the gating logic is testable from XCTest (SwiftUI @State defaults can't be observed from XCTest, same pattern as `evaluateFormState`). Three new cases in InstanceEditorViewValidationTests cover the open-vs-touch distinction and the new/edit/populated matrix.
review) PR #23 review surfaced a discard-path inconsistency: the previous `deleteInstance` only mutated the local draft, so a sequence like "toggle A off → delete A → Discard" left `appState._instances` with A still in it (tracking off) while the local draft had A back (tracking on). The slot stayed pruned past the Discard. Added a per-UUID removal path on `AppState`: - `AppState.removeInstance(uuid:)` evicts the UUID from both `_instances` and `_slotViewDataList` (mirrors `setInstanceTracking`'s single-UUID, immediate-propagation semantics) - `AppStateProxy.removeInstance(uuid:)` wraps it - `SettingsViewModel.deleteInstance` now awaits the proxy call after the local-draft mutation, matching the toggle path `discardChanges` now also calls `appState.setInstances` after the per-UUID toggle rollback. The per-UUID loop is still the fast path for toggle-only drafts (no slot re-flash for unrelated instances), but a draft that added or removed an instance needs the full rewrite so the runtime catches up. `setInstances` already runs `pruneDisabledSlots` so the final state matches the restored tracking flags. Tests: 2 new cases in AppStateTrackingToggleTests (unknown-UUID no-op, happy-path eviction from both _instances and _slotViewDataList) + 2 new cases in SettingsViewModelTrackingToggleTests (deleteInstance propagates to AppState, toggle+delete+discard stays consistent).
#23 review) PR #23 review surfaced two dead-code / stale-doc items. **Stale doc comment** — `Instance.swift` line 16 claimed `SettingsViewModel.setInstanceEnabled writes to this setter`. `setInstanceEnabled` is now `async` and was the *only* caller of that setter; with it gone, the comment was misleading. Replaced with an explanation of why the bridge itself stays (v1 JSON schema compat). **Dead deprecated entry** — `SettingsViewModel.setInstanceEnabled` was the only place project-wide that wrote through `Instance.enabled`. With the only caller gone, the entire method is dead. Removed the `@available(\*, deprecated, ...)` declaration. **Self-contradictory test comment** — `testSavePersistsToggleState` claimed the test avoided the supplier by using an empty instances list / a single no-provider instance / the `enabledInstances` short-circuit. None of those describe what the test actually does: the "test" provider has no registered `Supplier` in `SupplierRegistry`, so `RefreshService.performRefresh` short-circuits via the registry lookup (see `RefreshService.swift:454-466`). Rewrote the comment to match the real mechanism so future readers don't go chasing the wrong code path.
|
Thanks for the thorough review — all five issues were accurate. Fixes are pushed in three follow-up commits (branch
Test coverage added in the three commits: 7 new cases (3 in Release build verified: Happy to re-cut any of the commits or push a follow-up if you'd prefer (B) on issue #1 — but as noted I think (A) is the right call given the "暂停跟踪 vs 暂停通知" feature is in the planned future work and the two designs want different shape from |
Re-review — PR #23 (fixes addressing the previous review)All five items from the previous review are addressed cleanly:
One new finding from the fixes worth addressing before merge: Suggestion (not blocking) —
|
|
Thanks for the re-review — both findings checked out against the code. Addressed in three commits on
A third commit, Test status: every suite touched by these commits is green — One environmental caveat on the full-suite number: |
Re-review (round 3) — PR #23 final passBoth findings from the previous review are addressed cleanly and the code is in a state I'd be comfortable approving. ✅ 0edf009 — dead per-UUID loop removed
The stale-reference sweep is also good:
✅ 198bfd5 — slot-buffer contract pinnedBoth tests now seed a real slot via
The new assertions use the same No new findingsI checked the remaining surface area of the PR for any issues the two refactors might have introduced:
Final stateThe PR is 12 commits, 1225 additions / 20 deletions, with clean test coverage at every layer (parser/cycle slot, view-model rollback, form validation, slot-buffer invariants). The five items from the first review and the two items from the second are all resolved with no regressions introduced. Ready to merge from my side. Thanks for the iteration — the final shape of |
1 participant
Summary
修复 GitHub issue #20 — settings → Services 标签下，关闭某 instance 的 "tracking toggle" 后，状态栏仍显示该 instance 的 slot；同时修复 "进入实例取消所有 metrics 后无法保存" 的次生缺陷。
Root causes
SettingsViewModel.setInstanceTrackingEnabledinstances[index].trackingEnabled，不通知 AppStateAppState.setInstances/mergeCycleResult_slotViewDataList只按"已删除"UUID 过滤，不按"已禁用"过滤MenuBarController.renderIconlatestSlotData渲染（依赖上游修复）InstanceEditorView.isFormFilled!selectedMetrics.isEmpty，编辑现有 instance 时无法保存空 metricsAppState/AppStateProxysetInstanceTracking(uuid:enabled:)单实例 toggle 入口SettingsViewModel.discardChanges正向佐证：
RefreshService.performRefreshline 335 的enabled过滤、LimitRolloverDetector与NotificationManager的instance.enabled守卫早已正确。本 fix 之前的"实际刷新好像无效"现象不是刷新逻辑错误，而是状态栏 slot 残留让用户误以为还在刷新。Changes (5 commits)
fix(appstate): prune slots for disabled instances— 抽pruneDisabledSlots()私有 helper，setInstances/mergeCycleResult末尾过滤；新增setInstanceTracking(uuid:enabled:)。fix(proxy): expose setInstanceTracking to view models—AppStateProxyasync 包装，调用appState.setInstanceTracking+syncFromState。fix(settings): make tracking toggle take effect immediately—SettingsViewModel.setInstanceTrackingEnabled改 async 即时通知；discardChanges改 async 逐 UUID 回滚；SettingsView.onToggleTracking与SettingsWindow"Don't Save" 路径同步改 Task 包装。fix(editor): allow saving an existing instance with zero metrics—InstanceEditorView.isFormFilled改为允许编辑现有 instance 时空 metrics（新建仍要求至少 1 个）；抽static evaluateFormState(...)纯函数作为测试入口；新增 "Disabling all metrics will pause usage tracking" 警告 banner。docs(issue20): sync tracking toggle semantics in ARCHITECTURE §2.6— 文档同步。Design decisions
trackingEnabled权威值，所有 set 路径统一清理 disabled slot，单一真相InstanceEditorView允许空 metrics（仅编辑现有 instance），新建仍要求至少 1 个ColorState.disabled死代码（MenuBarIconRenderer.expandToMetricSlots内的colorState != .disabled过滤）— 留待未来独立清理 PRTests
新增 3 个 test 文件，23 个 case：
AppStateTrackingToggleTestsSettingsViewModelTrackingToggleTestsInstanceEditorViewValidationTests总计 394 tests, 0 failures（baseline 371 + 新增 23）。
Manual verification checklist
产品侧验证（请 reviewer 跑）：
instances.json（重载后保留）Mechanical adjustments (vs plan)
InstanceEditorView.isFormFilled/isAllMetricsDisabled抽为static evaluateFormState(...)纯函数（instance computed property 委托它）— SwiftUI view 构造时@State默认值无法在 XCTest 触发.onAppear加载，static helper 让测试可直接调。project.pbxproj（每个新 test 文件 4 处登记），本机未装xcodegen。brew install xcodegen长时间无响应。如未来切回 xcodegen 流程，git restore APIUsageStatus.xcodeproj/project.pbxproj && xcodegen generate重新生成即可。Out of scope
ColorState.disabled死代码清理（独立 PR）Instance.enabled计算桥废弃（破坏性变更）References
.omo/plans/fix-issue20-services-tracking-toggle.md（local-only per .gitignore）docs/ARCHITECTURE.md§2.6 "Tracking toggle semantics（issue Bug：settings Services 界面关闭跟踪实例无效 #20）"