feat(log): differentiate activation reasons and add row context#13
Merged
Conversation
The activation log's Reason column collapsed roughly ten distinct real-world triggers into four labels: appActivated covered app switches, launcher overlays, and URL poll re-checks alike, while lockEngaged covered the master toggle, rule edits, and the startup restore. The two most diagnostic facts — the source switched away from, and the app or launcher behind the lock — were computed and then discarded. Expand ActivationReason to nine cases and carry four optional context fields (from-source, triggering bundle/app name, rule branch, matched URL host) from the engine into each event. The log now renders "X -> Y", an App column, and a rule-source subtitle. RuleResolver reports which branch produced the target; urlMatched now fires only when a post-lock URL change re-resolves, while the enabling force keeps its own reason and the URL provenance lives in the rule branch. The new SwiftData fields are optional so lightweight migration preserves existing history; new enum raw values need no migration. All nine new catalog keys are translated for every supported language, and LockIMEKit coverage is extended for the new paths. Signed-off-by: Kevin Cui <bh@bugs.cc>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughThis PR enriches the activation logging pipeline with a more granular taxonomy of activation reasons and contextual metadata tracking. New Sequence Diagram(s)sequenceDiagram
participant User
participant AppState
participant LockEngine
participant RuleResolver
participant LockController
participant LogStore
participant UI as ActivationLogPane
User->>AppState: setMasterEnabled(true)
AppState->>AppState: commit(reason: .lockEngaged)
AppState->>LockEngine: apply(config, reason: .lockEngaged)
LockEngine->>RuleResolver: resolve(...)
RuleResolver-->>LockEngine: LockResolution.lock(source, .appRule)
LockEngine->>LockController: setEnabled(true, reason: .lockEngaged)
LockController->>LockController: force(fromSourceName, ruleSource, bundleID)
LockController->>LockEngine: onActivation event
LockEngine->>LogStore: record(ActivationEvent, triggeringAppName)
LogStore->>LogStore: ActivationLogEntry persisted
UI->>LogStore: fetch entries
LogStore-->>UI: entries with context fields
UI-->>User: display "source1 → source2" and rule source
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/LockIMEKit/Logging/LogStore.swift (1)
41-47:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLog the original error, not
localizedDescription.Both catch paths still stringify the Foundation-localized message, so diagnostics can flip to the system language and lose the original error payload. Log the error object (or stable domain/code fields) instead.
Suggested fix
- Self.log.error("Failed to save activation log entry: \(error.localizedDescription, privacy: .public)") + Self.log.error("Failed to save activation log entry: \(String(describing: error), privacy: .public)") ... - Self.log.error("Failed to purge expired log entries: \(error.localizedDescription, privacy: .public)") + Self.log.error("Failed to purge expired log entries: \(String(describing: error), privacy: .public)")As per coding guidelines, "Never display text localized by someone else's bundle. Map errors to semantic categories whose messages are catalog keys ... and log the original error instead. Avoid using Foundation/Sparkle
error.localizedDescriptionwhich resolves against the system language."Also applies to: 59-60
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/LockIMEKit/Logging/LogStore.swift` around lines 41 - 47, In record(_ event: ActivationEvent, triggeringAppName: String? = nil) replace logging of error.localizedDescription with logging the original Error object (or its stable domain/code) so the full, non-localized payload is preserved; locate the catch blocks in LogStore.save paths (including the second catch around lines 59–60) and change Self.log.error("Failed to save activation log entry: ...") to include the error itself (or error as NSError with domain/code) rather than error.localizedDescription, keeping a clear descriptive message and the original error payload.Source: Coding guidelines
🧹 Nitpick comments (1)
Tests/LockIMEKitTests/LogStoreTests.swift (1)
20-45: ⚡ Quick winCover
matchedHostin this round-trip test too.This is the only new persisted field that still goes unexercised here, so a broken
matchedHostmapping inActivationLogEntry.init(_:)would slip through.Suggested test tweak
store.record( ActivationEvent( timestamp: .now, inputSource: "com.apple.keylayout.US", inputSourceName: "U.S.", reason: .revertedSwitch, durationMs: 2.0, fromSourceName: "Pinyin", triggeringBundleID: "com.apple.Safari", ruleSource: .appRule, - matchedHost: nil + matchedHost: "github.com" ), triggeringAppName: "Safari" ) @@ `#expect`(row.triggeringAppName == "Safari") `#expect`(row.ruleSource == .appRule) + `#expect`(row.matchedHost == "github.com") `#expect`(row.reason == .revertedSwitch)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/LockIMEKitTests/LogStoreTests.swift` around lines 20 - 45, Update the recordsContextFields test in LogStoreTests to exercise matchedHost: construct the ActivationEvent with a non-nil matchedHost (e.g. "example.com") when calling store.record and after fetching the ActivationLogEntry (rows.first) add an assertion that row.matchedHost equals that value; this ensures ActivationLogEntry.init(_:) correctly maps matchedHost along with fromSourceName, triggeringBundleID, triggeringAppName, ruleSource, and reason.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/LockIMEKit/LockEngine/LockEngine.swift`:
- Around line 145-154: The switch branch that handles RuleResolver.resolve(...)
currently forces reason: ruleSource == .urlRule ? .urlMatched : reason; change
this so that when the incoming reason is an "apply-driven" reason (e.g.
.startupApplied, .lockEngaged, .configChanged passed by apply(reason:)), you
preserve that reason instead of overwriting it with .urlMatched; otherwise, if
the resolution truly originated from a URL and the incoming reason is not an
apply-driven type, set .urlMatched. Update the controller.setTarget(...) call in
the .lock case to conditionally choose .urlMatched only when ruleSource ==
.urlRule AND reason is not one of the apply-driven reasons, using
RuleResolver.resolve, ruleSource, urlMatch and the existing reason variable to
implement the check.
- Around line 93-96: The apply(_:reason:) flow can emit a forced switch because
reevaluate(reason:) runs while the controller is still in the old enabled state;
modify apply to check the incoming config.isEnabled and if it's false call
controller.setEnabled(false, reason: reason) before assigning self.config and
calling reevaluate(reason:) so the controller is disabled prior to reevaluation;
ensure you still call controller.setEnabled(config.isEnabled, reason: reason)
for the enabling path and keep reevaluate(reason:) after self.config assignment.
---
Outside diff comments:
In `@Sources/LockIMEKit/Logging/LogStore.swift`:
- Around line 41-47: In record(_ event: ActivationEvent, triggeringAppName:
String? = nil) replace logging of error.localizedDescription with logging the
original Error object (or its stable domain/code) so the full, non-localized
payload is preserved; locate the catch blocks in LogStore.save paths (including
the second catch around lines 59–60) and change Self.log.error("Failed to save
activation log entry: ...") to include the error itself (or error as NSError
with domain/code) rather than error.localizedDescription, keeping a clear
descriptive message and the original error payload.
---
Nitpick comments:
In `@Tests/LockIMEKitTests/LogStoreTests.swift`:
- Around line 20-45: Update the recordsContextFields test in LogStoreTests to
exercise matchedHost: construct the ActivationEvent with a non-nil matchedHost
(e.g. "example.com") when calling store.record and after fetching the
ActivationLogEntry (rows.first) add an assertion that row.matchedHost equals
that value; this ensures ActivationLogEntry.init(_:) correctly maps matchedHost
along with fromSourceName, triggeringBundleID, triggeringAppName, ruleSource,
and reason.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e6e4076a-7ac5-46f6-b3ec-5a3db544ee24
📒 Files selected for processing (15)
Sources/LockIME/AppState.swiftSources/LockIME/Localizable.xcstringsSources/LockIME/UI/Settings/ActivationLogPane.swiftSources/LockIMEKit/Enhanced/URLMatcher.swiftSources/LockIMEKit/LockEngine/InputSource.swiftSources/LockIMEKit/LockEngine/LockController.swiftSources/LockIMEKit/LockEngine/LockEngine.swiftSources/LockIMEKit/Logging/ActivationLogEntry.swiftSources/LockIMEKit/Logging/LogStore.swiftSources/LockIMEKit/Rules/RuleResolver.swiftTests/LockIMEKitTests/LockControllerTests.swiftTests/LockIMEKitTests/LockEngineTests.swiftTests/LockIMEKitTests/LogStoreTests.swiftTests/LockIMEKitTests/RuleResolverTests.swiftTests/LockIMEKitTests/URLMatcherTests.swift
…ched Address review on the activation-reason change. apply() now stops enforcing before reevaluating a disabled config, so turning the lock off never forces one last switch when the source has drifted off target. And an apply-driven reason (lockEngaged / configChanged / startupApplied) is preserved when its target resolves via a URL rule on an already-enabled engine, instead of being overwritten with urlMatched — the URL provenance is already carried by ruleSource. Signed-off-by: Kevin Cui <bh@bugs.cc>
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
1 participant
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.
What & why
The activation log's Reason column collapsed roughly ten distinct real-world triggers into four labels:
appActivatedcovered a real app switch, a launcher overlay (Spotlight/Raycast) taking focus, the overlay being dismissed, and the 1.5 s enhanced-mode URL poll re-check — indistinguishably.lockEngagedcovered the master toggle turning on, every config/rule edit, and the startup/restore apply.So a user staring at the log saw a wall of "App activated" / "Lock engaged" with no way to tell what fired the switch, which app it was for, or why that source was chosen.
Changes
ActivationReason4 → 9 cases:appActivated·launcherFocused·launcherDismissed·urlPolled·urlMatched·revertedSwitch·lockEngaged·configChanged·startupApplied. The reason is threaded from each engine entry point (handleLauncherChangesplits on focus/dismiss, the URL poll loop emitsurlPolled,apply(_:reason:)carries the caller's intent fromAppState).ActivationEvent/ActivationLogEntry:fromSourceName,triggeringBundleID+triggeringAppName,ruleSource,matchedHost.RuleResolvernow reports which branch (app rule / global default / URL rule) produced the target.X → Y, a new App column shows the triggering app/launcher, and a dimmed rule-source subtitle (plus the matched host for URL rows) sits under each reason.SupportedLanguagecases, matching existing terminology.Notes
reasonRaw/ruleSourceRaware strings).apply()time is attributed to its own reason (startupApplied/lockEngaged/configChanged) with the URL provenance kept inruleSource;urlMatchedfires only when a post-lock URL change re-resolves the target. Covered by a dedicated test.Testing
make test— 107 passing, 0 failures. New coverage acrossLockController,LockEngine,LogStore,RuleResolver,URLMatcher;LocalizationGuardTestsgreen. An adversarial multi-agent review of the diff surfaced no correctness or i18n bugs.