feat(mcp): add approval attention controls and localized notifications#311
feat(mcp): add approval attention controls and localized notifications#311Stiwar0098 wants to merge 4 commits into
Conversation
- Add mcpApprovalAttention utility: window snapshot/restore, focus request, system notification with localized title/body, and alert sound - Add mcpApprovalAlwaysOnTop and mcpApprovalNotifySound settings in McpSafetySection with i18n labels (en, es, de, fr, it, ja, ru, zh) - Trigger attention flow from AiApprovalGate on pending approvals - Add isLanguageApplied() helper to avoid unnecessary changeLanguage() calls - Add Tauri capabilities: notification:default, is-visible, is-minimized, request-user-attention, is-always-on-top, set-always-on-top - Add Rust config structs for new approval settings - Add unit and component tests for attention flow and opt-out paths - Update .gitignore to exclude .atl/ Closes TabularisDB#307
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Resolved Issues
Files Reviewed (2 files)
Previous Review Summaries (2 snapshots, latest commit 8601478)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 8601478)Status: No Issues Found | Recommendation: Merge All previously identified issues remain resolved, and no new issues were introduced in the latest commits. Resolved Issues
Files Reviewed (3 files)
Previous review (commit 941629b)Status: No Issues Found | Recommendation: Merge All previously identified issues have been resolved in the latest commits. Resolved Issues
Files Reviewed (3 files)
Reviewed by kimi-k2.6-20260420 · 201,821 tokens |
- SettingsProvider: remove isLanguageApplied/queueLanguageApplication from mount effect dependency array to prevent re-running loadSettings on language change (WARNING per kilo-review) - mcpApprovalAttention: capture fresh snapshot when approval ID changes instead of reusing stale state from a partially-failed restore - Add test coverage for fresh snapshot capture on different approval ID
|
hey @Stiwar0098 thanks for your work! didnt had time to review it yet but it is still on my todo! will do it tomorrow ASAP |
There was a problem hiding this comment.
Review — MCP approval attention controls
Nice work @Stiwar0098 — clean watcher→event→gate architecture and solid config tests. Tested locally on Linux; the core flow works when a language is set. Findings:
🔴 Blocking — no approval shows without a configured language. Fresh config (language: null, the default on first launch) → modal and notification never appear, the MCP call just times out. Backend emit is fine — it's the isLanguageSettled gate (L47, L79). i18next has fallbackLng: 'en', so decouple from the gate and translate with the fallback instead of suppressing.
🟡 Alert sound — couldn't validate. On my machine the sound never played (the notification did). I'm fairly sure that's my own OS notification-sound settings, so I can't confirm whether the code is fine or whether it's hitting the Web Audio autoplay policy (programmatic open → no user gesture → AudioContext stays suspended, L252-266). @debba — could you validate the sound on a setup where notification sounds are known to work?
🟡 React rule #2 — sync setState in useEffect (L245-246, L253-254).
ℹ️ Window not raising on Linux is not a bug — the WM blocks focus-stealing; the code calls the right APIs.
Left two inline suggestions on the gate decoupling.
|
On the sound point, I tested the approval flow on Windows and the notification sound does play correctly on my side. That suggests the implementation is working at least on Windows, and the earlier no-sound result may have been environment-specific on Linux. |
|
On the setState note, this is now addressed in 8601478 as well. The language effect no longer does synchronous flag resets inside useEffect; isLanguageReady and isLanguageSettled are derived from the current i18n state plus the tracked result of the async language application for the active language, which preserves the same behavior without the extra post-render setState pattern. |
The in-page Web Audio alert tone never plays on Linux: WebKitGTK's autoplay policy keeps the programmatically-started AudioContext suspended (no user gesture), so the synthesized beep is silent there. Route the alert through the OS notification's XDG theme sound (message-new-instant) on Linux instead, and skip the in-page tone to avoid a double alert. macOS/Windows keep using the Web Audio tone, which their webviews allow without a user gesture.
|
Followed up on the alert sound, @Stiwar0098. Confirmed it's not a bug in your code — on Linux the synthesized Web Audio tone can't play because WebKitGTK's autoplay policy keeps the programmatically-started To avoid a review ping-pong round I pushed a fix straight to the branch ( Could you give it a quick check on Windows to confirm the tone still plays there as before? 🙏 |
NewtTheWolf
left a comment
NewtTheWolf
left a comment
There was a problem hiding this comment.
Re-reviewed 941629b..ed9c12f — both blocking points resolved, nicely done @Stiwar0098.
✅ Language gate — isLanguageSettled fully decoupled from the effect, deps, and render gate. Verified end-to-end on a fresh language: null config (staged a pending approval): modal and notification now appear with English fallback text, where before nothing showed. The inverted test locks it in.
✅ React rule #2 — isLanguageReady/isLanguageSettled are derived during render; the i18n effect only sets state post-await with a language guard.
🔊 Sound — addressed in the follow-up commit I pushed (Linux now uses the OS notification sound; see comment above). Works end-to-end on KDE.
Branch tests green, typecheck + lint clean (the exhaustive-deps warning on the loadSettings mount effect is the intentional run-once pattern). LGTM 🚀
|
Sorry for the delay on this. I rechecked the approval flow on Windows, and the tone still plays as before after the Linux-specific follow-up, so Windows behavior looks unchanged on my side. |
2 participants
Summary
Adds attention-grabbing controls for MCP approval gates so users never miss a pending approval, with fully localized notification text.
Closes #307
What's new
Attention Flow (src/utils/mcpApprovalAttention.ts)
equestUserAttention(Informational)
Settings (McpSafetySection)
Trigger (AiApprovalGate)
Infrastructure
otification:default, is-visible, is-minimized,
equest-user-attention, is-always-on-top, set-always-on-top
Test plan