fix: notification is not working

[Kumneger0]

Summary by CodeRabbit

• New Features

  • Notifications now include the application icon, improving visual identification of app-initiated alerts.

• Improvements

  • Notification presentation enhanced for a more polished and consistent appearance during system notifications.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Reworks notification code to attach an application icon to desktop notifications. Adds functions that read the embedded logo from assets, write it once to a temp file, cache its path, and switches Notify() to call beeep.Notify(...) with the icon path.

Changes

Cohort / File(s)	Summary
Notification Icon Support internal/notification/notify.go	Added getAppIconPath() which uses getAppLogo() to read embedded logo.png, writes it to a temp cligram-icon.png (cached via once.Do), and returns its path. getAppLogo() now reads from assets.Assets.ReadFile. Notify() now calls beeep.Notify(...) with the icon path instead of beeep.Alert(...). Imports os and filepath added; synchronization via once preserved.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Areas needing attention:
  • Temp file creation and lifecycle (possible orphaned files, permissions)
  • Correctness of assets.Assets.ReadFile usage and presence of logo.png in embedded assets
  • Error handling when asset read or temp write fails and behavior of beeep.Notify() across platforms
  • Thread-safety and correctness of caching via once.Do

Poem

🐰
I nibble bytes of logo light,
I write a tiny icon bright,
Once only made, it hops in place,
Notifications wear my face,
Hooray — a desktop carrot chase! 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly addresses the main change: fixing a broken notification feature by implementing icon path handling and switching from Alert to Notify.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch sysNotification

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

internal/notification/notify.go (2)

17-26: LGTM with a minor suggestion.

The implementation correctly reads the embedded logo and handles errors appropriately. However, returning *[]byte instead of []byte is unnecessary since slices are already reference types in Go.

🔎 Optional refactor to return slice directly

-func getAppLogo() *[]byte {
-	var appLogo *[]byte
+func getAppLogo() []byte {
 	logo, err := assets.Assets.ReadFile("logo.png")
 	if err != nil {
 		slog.Error(err.Error())
 		return nil
 	}
-	appLogo = &logo
-	return appLogo
+	return logo
 }

---

39-39: Consider cleanup strategy for the temporary icon file.

The temporary icon file is created but never cleaned up. While this is a minor issue since it's created only once per application run, consider adding cleanup logic (e.g., removing the file on application exit) to prevent stale files from accumulating over time.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0bbc7d0 and 1fbf532.

📒 Files selected for processing (1)

• internal/notification/notify.go (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

internal/notification/notify.go (1)

assets/assets.go (1)

• Assets (8-8)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: test (1.25.x, macos-latest)

🔇 Additional comments (1)

internal/notification/notify.go (1)

48-56: No changes needed—icon error handling is already in place.

Empty icon paths are possible when logo loading or file writing fails, but the code already logs errors at each stage. beeep.Notify() will return an error for an empty/invalid icon path, which is caught and logged on line 54. The implementation handles this gracefully.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

internal/notification/notify.go (1)

17-26: Simplify by removing unnecessary pointer indirection.

In Go, slices are already reference types, so returning *[]byte adds unnecessary indirection. Consider returning []byte directly and updating the caller accordingly.

🔎 Proposed simplification

-func getAppLogo() *[]byte {
-	var appLogo *[]byte
+func getAppLogo() []byte {
 	logo, err := assets.Assets.ReadFile("logo.png")
 	if err != nil {
 		slog.Error(err.Error())
 		return nil
 	}
-	appLogo = &logo
-	return appLogo
+	return logo
 }

Then update the caller at line 38:

-		if err := os.WriteFile(path, *logoPNG, 0o644); err != nil {
+		if err := os.WriteFile(path, logoPNG, 0o644); err != nil {

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1fbf532 and 633ba3f.

📒 Files selected for processing (1)

• internal/notification/notify.go

🧰 Additional context used

🧬 Code graph analysis (1)

internal/notification/notify.go (1)

assets/assets.go (1)

• Assets (8-8)

🔇 Additional comments (1)

internal/notification/notify.go (1)

46-54: Verify behavior when icon path is empty.

After fixing getAppIconPath() to return an empty string on failure, test that beeep.Notify() handles the empty string gracefully. The beeep library accepts icon as a string with a path to png file or png []byte data, but the library documentation does not explicitly specify how it handles empty icon path parameters. Verify the notification displays without an icon rather than failing.