Skip to content

fix(notifications): validate URLs and show errors#196

Merged
s0up4200 merged 2 commits into
developfrom
codex/fix-pushover-url-validation
Feb 8, 2026
Merged

fix(notifications): validate URLs and show errors#196
s0up4200 merged 2 commits into
developfrom
codex/fix-pushover-url-validation

Conversation

@s0up4200

@s0up4200 s0up4200 commented Feb 8, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Validate notification URLs on create/update; return "error/details" for bad URLs
  • Frontend parses API error payloads and shows details in toasts (incl. Test)
  • Fix Pushover example URL to match Shoutrrr format
  • Add regression test for notification URL validation

Why

Testing

  • pnpm -C web lint (fails on develop; unrelated existing lint errors)
  • pnpm -C web build
  • Other: go test ./...

Screenshots (if UI)

  • n/a

Checklist

  • Diff is scoped to the issue (no unrelated changes)
  • No new lockfiles (pnpm only)
  • No generated/dist files committed
  • AI-assisted changes reviewed and edited by a human

Summary by CodeRabbit

  • New Features

    • Added upfront URL validation for notification channels and test-sends; test sends now return a clear success message.
  • Bug Fixes

    • Improved and more descriptive error messages in the notification UI (including trimmed whitespace handling).
    • Validation now reports missing ntfy host/topic explicitly.
    • Reduced accidental exposure of credentials in error logs/responses.
  • Tests

    • Added tests covering notification URL parsing and ntfy host/topic edge cases.

@coderabbitai

coderabbitai Bot commented Feb 8, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

Adds upfront notification URL validation (no network calls), tightens ntfy URL error messages to avoid echoing credentials, wires validation into server handlers and test flows, standardizes frontend API error parsing, and updates mutation error callbacks to propagate descriptive toast messages.

Changes

Cohort / File(s) Summary
Notification validation core
internal/notifications/notifications.go, internal/notifications/ntfy.go, internal/notifications/validate_test.go
Adds exported ValidateNotificationURL(rawURL string) error to validate single URLs without network calls; tightens ntfy parsing errors (require host, don't echo URL); adds unit tests for pushover and ntfy edge cases.
Server handlers
internal/server/notification_handlers.go
Trim and validate input URLs on create/update/test endpoints using ValidateNotificationURL; return 400 with error "details" on validation failures; improve test notification error responses and success message.
Frontend API
web/src/api/notifications.ts
Adds internal error helpers (ApiErrorPayload, parseErrorMessage, assertOk) and replaces ad-hoc response checks with assertOk; updates pushover example format comment.
Frontend settings UI
web/src/components/settings/NotificationSettings.tsx
Updates mutation onError callbacks to accept err: unknown and surface descriptive toast messages (create/update/delete/test channels and rules).

Sequence Diagram(s)

(no sequence diagrams generated)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Poem

🐰 I sniff the bytes and tidy each URL line,
I hide the secrets, keep credentials fine.
I hop from backend checks to frontend toasts,
A little rabbit guard for notification hosts. 🎈

🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title directly addresses the main change—URL validation and error reporting for notifications—which is accurately reflected throughout the changeset.
Description check ✅ Passed Description covers all key aspects: validation logic, error handling in the UI, Pushover URL format fix, testing, and issue linkage—aligning well with the template requirements.
Linked Issues check ✅ Passed Changes address issue #193 by fixing Pushover URL format confusion, validating notification URLs on create/update, and providing clear error feedback to users.
Out of Scope Changes check ✅ Passed All changes are directly scoped to URL validation and error handling for notifications; no unrelated or extraneous modifications detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ 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 codex/fix-pushover-url-validation

No actionable comments were generated in the recent review. 🎉

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.5.0)

Error: unknown linters: 'modernize', run 'golangci-lint help linters' to see the list of supported linters
The command is terminated due to an error: unknown linters: 'modernize', run 'golangci-lint help linters' to see the list of supported linters


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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@internal/notifications/ntfy.go`:
- Around line 69-73: The parseNtfyURL function currently only checks topic from
parsed.Path; add a host validation immediately after the topic check (inspect
parsed.Host or parsed.Hostname) and return an error (e.g., "ntfy URL must
include a host") if it's empty so malformed URLs like "ntfy:///topic" are
rejected early before building apiURL; keep the error message generic to avoid
echoing credentials and update any callers to handle the new error path.

In `@internal/server/notification_handlers.go`:
- Around line 43-48: Trim the incoming URL once and reuse the trimmed value for
both validation and persistence: call strings.TrimSpace on input.URL, assign the
result back (or to a local trimmedURL) and then pass that trimmed value to
notifications.ValidateNotificationURL and to whatever code stores input.URL, so
you don't validate/store the untrimmed string; apply the same change to the
other occurrence around the block referencing input.URL (lines ~74-79).

Comment thread internal/notifications/ntfy.go
Comment thread internal/server/notification_handlers.go Outdated
@s0up4200 s0up4200 merged commit fc9c822 into develop Feb 8, 2026
11 checks passed
@s0up4200 s0up4200 deleted the codex/fix-pushover-url-validation branch February 8, 2026 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Pushover notifications don't work

1 participant