Modernize Desktop-Notifier

[amotl]

About

• Desktop-Notifier no longer supports sync calls.

References

• Desktop-Notifier No Longer Supports Sync Calls #706
• Modernize Desktop-Notifier #711

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

The change adds layered configuration resolution for job fields and updates desktop notifications to use asynchronous dispatch, stricter message handling, and revised sound defaults. Related tests now mock and verify the asynchronous notification method.

Changes

Configuration Resolution

Layer / File(s)	Summary
Layered option lookup and optional transformation mqttwarn/core.py, tests/test_core_infra.py	Makes xform inputs optional, adds ordered configuration lookup across service-target, service, target, and bare option keys, and updates the related xform assertion.
Job field resolution mqttwarn/core.py	Uses layered lookup for title, image, and message construction with updated defaults and format handling.

Desktop Notifications

Layer / File(s)	Summary
Async notification dispatch mqttwarn/services/desktopnotify.py	Adds asynchronous notification sending, handles JSON and non-JSON messages, rejects missing messages, and defaults dictionary-configured sound playback to disabled.
Notification test contract tests/services/test_desktopnotify.py	Updates fixtures and assertions for asynchronous sends, failure handling, JSON notifications, and sound-enabled calls.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant plugin
  participant send_notification
  participant DesktopNotifier
  plugin->>send_notification: asyncio.run(message, title, sound)
  send_notification->>DesktopNotifier: await send(message, title, sound)

Loading

Poem

I’m a rabbit with configs in my den,
Layered keys choose the right one again.
Async bells softly hop through the night,
JSON and sound now behave just right.
Tests thump their paws: “The calls are all tight!”

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title matches the main theme of modernizing the desktop notifier and reflects the sync-to-async change.
Description check	✅ Passed	The description is directly related to the change, noting that Desktop-Notifier no longer supports sync calls.

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 5

🧹 Nitpick comments (2)

mqttwarn/services/desktopnotify.py (1)

67-68: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy lift

Resolve the known blocking behavior before merging.

asyncio.run() blocks the synchronous plugin until send_notification() completes. If the TODO is accurate, mqttwarn/core.py’s 10-second plugin timeout can report failure while the underlying thread remains alive, consuming worker capacity. Confirm backend behavior and use a long-lived event loop or background dispatch if sending does not return promptly.

tests/services/test_desktopnotify.py (1)

79-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add regression coverage for the new payload shapes.

Cover a raw {"message": ...} dictionary and scalar/list JSON values. These cases exercise the new branches and would catch the current .get() crash and missing-key behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b575ff8e-37f0-4ee7-b472-476dbb6ee6ab

📥 Commits

Reviewing files that changed from the base of the PR and between f930775 and 0b4dfc2.

📒 Files selected for processing (3)

• mqttwarn/core.py
• mqttwarn/services/desktopnotify.py
• tests/services/test_desktopnotify.py

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Handle dictionary payloads explicitly.

For a dictionary without "message", .get() returns None, so the notification becomes "None" at Line 64. The bare except also masks unrelated errors; use an explicit isinstance/mapping check and define a fallback for missing keys.

🧰 Tools

🪛 Ruff (0.15.20)

[error] 56-56: Do not use bare except

(E722)

Source: Linters/SAST tools

[coderabbitai]

🧹 Nitpick comments (3)

mqttwarn/services/desktopnotify.py (3)

21-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Update the message type contract for dictionary support.

is_json() and ProcessorItem.message still advertise only str/bytes, although this branch intentionally accepts dictionaries. Define a shared message type and narrow with isinstance instead of cast.

Also applies to: 48-55

---

43-46: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Use the notifier’s Sound/None contract.

desktop-notifier v5+ deprecated boolean sound values; its current API expects Sound | None. The mocked tests will not catch this integration issue. Prefer DEFAULT_SOUND if playSound else None, or verify the pinned version intentionally supports booleans. (raw.githubusercontent.com)

Also applies to: 66-68

Source: MCP tools

---

66-68: 🩺 Stability & Availability | 🔵 Trivial

Verify that notification dispatch returns promptly.

core.process_job runs this synchronous plugin under a timeout, while this TODO states that asyncio.run() may wait until the notification closes. If reproducible with the pinned notifier version, jobs can time out while the plugin worker continues running. Confirm the coroutine completes after scheduling and add a regression test; I can help redesign the loop ownership if needed. The upstream API documents send() as scheduling a notification and returning its identifier. (desktop-notifier.readthedocs.io)

Source: MCP tools

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 60ba1bb5-7ba8-434e-9f3b-0516e04bf8fb

📥 Commits

Reviewing files that changed from the base of the PR and between 0b4dfc2 and 405f938.

📒 Files selected for processing (3)

• mqttwarn/core.py
• mqttwarn/services/desktopnotify.py
• tests/services/test_desktopnotify.py

🚧 Files skipped from review as they are similar to previous changes (2)

• mqttwarn/core.py
• tests/services/test_desktopnotify.py

[coderabbitai]

Actionable comments posted: 3

♻️ Duplicate comments (1)

mqttwarn/core.py (1)

581-583: 🎯 Functional Correctness | 🟡 Minor

The previous xform sentinel issue still applies.

When no title is configured, origin_title is passed as function, so xform enters transformation and may log failed-invocation stack traces. Keep the missing option as None and pass origin_title only as orig_value.

🧹 Nitpick comments (1)

mqttwarn/services/desktopnotify.py (1)

21-28: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Avoid decoding JSON twice.

is_json() fully parses the payload, then plugin() parses it again. Return the parsed value from one helper or parse once inside plugin().

Also applies to: 51-52

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: cf9bd91b-355e-468f-968f-39a6df16c7d1

📥 Commits

Reviewing files that changed from the base of the PR and between 405f938 and e70165c.

📒 Files selected for processing (4)

• mqttwarn/core.py
• mqttwarn/services/desktopnotify.py
• tests/services/test_desktopnotify.py
• tests/test_core_infra.py

🚧 Files skipped from review as they are similar to previous changes (1)

• tests/services/test_desktopnotify.py