Skip to content

feat: send notifications using directus API#808

Open
alexey-yarmosh wants to merge 4 commits intomasterfrom
manage-notifications
Open

feat: send notifications using directus API#808
alexey-yarmosh wants to merge 4 commits intomasterfrom
manage-notifications

Conversation

@alexey-yarmosh
Copy link
Copy Markdown
Member

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 13, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 6cf75908-cde7-4199-9aa6-4608ec9e5619

📥 Commits

Reviewing files that changed from the base of the PR and between 0bab7f3 and 6ed4398.

📒 Files selected for processing (1)
  • test/tests/unit/override/adopted-probes.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • test/tests/unit/override/adopted-probes.test.ts

Walkthrough

This PR introduces two main changes: it updates the authentication method in the adoption token API from a custom 'X-Api-Key' header to a Bearer token in the Authorization header, and it refactors the notification system in the adopted-probes module to send notifications via HTTP POST requests to a Directus API endpoint instead of direct database inserts. The sendNotification method signature is expanded to include a notification type parameter. Test files are updated to validate the new HTTP-based notification delivery mechanism.

Possibly related PRs

  • globalping#661: Modifies notification logic in the same adopted-probes file to update notification message text content.
  • globalping#791: Updates the adoption-token.ts file to change authentication method and adoption payload structure.
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: migrating notification handling to use the Directus API instead of direct database inserts.
Description check ✅ Passed The description provides a reference to the related GitHub issue, which is relevant to the changeset involving notification system updates.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch manage-notifications
📝 Coding Plan
  • Generate coding plan for human review comments

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

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/lib/override/adopted-probes.ts`:
- Around line 772-783: The POST to Directus in the notification path (called
from updateDProbe) can fail transiently and currently drops the notification;
modify the flow to use a durable outbox/retry: before calling got.post, persist
the notification payload to a new or existing "outbox" store (e.g., a
notifications_outbox table or Directus-native items) and then either (a) call
got.post and mark the outbox row as sent on success, or (b) enqueue a background
retry job that reads pending outbox rows and retries sends with backoff; update
updateDProbe (and the function containing the got.post call) to only rely on the
outbox write for durability and to mark/send asynchronously, and add a
background worker/cron that scans and retries unsent outbox entries until
acknowledged so transient Directus outages won’t permanently drop notifications.
- Around line 763-766: The sendNotification() function currently performs a
direct read against NOTIFICATIONS_TABLE (directus_notifications) via the
existing = await this.sql(...) query which causes failures in dev/test because
migrations don't create that table; remove that direct-table deduplication by
deleting or bypassing the query in sendNotification() and rely on the Directus
API endpoint to handle deduplication (i.e., call the API unconditionally or call
its dedupe endpoint), or alternatively restore provisioning of
directus_notifications in your dev/test migrations until the direct read is
removed; update references to NOTIFICATIONS_TABLE and the SQL block in
sendNotification() accordingly so the code no longer assumes the local table
exists.

In `@test/tests/unit/override/adopted-probes.test.ts`:
- Around line 102-125: The suite-level stub gotPostStub created from
sandbox.stub(got, 'post') is only reset with sandbox.reset() in beforeEach,
which clears call history but does not restore the original got.post, so
patching persists across suites; add an after() teardown that calls
sandbox.restore() (or alternatively create and restore gotPostStub per test) to
restore the original got.post and other sandboxed replacements—refer to the
existing gotPostStub and sandbox.reset usage and implement the restore in an
after() hook for this test file.
🪄 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: 31d36456-9b20-42c2-90dd-253289f9dd84

📥 Commits

Reviewing files that changed from the base of the PR and between 7b3b883 and 0bab7f3.

📒 Files selected for processing (6)
  • migrations/dashboard/create-tables.js.sql
  • src/adoption/adoption-token.ts
  • src/lib/override/adopted-probes.ts
  • test/e2e/cases/adopted-probes.test.ts
  • test/tests/integration/adoption-token.test.ts
  • test/tests/unit/override/adopted-probes.test.ts
💤 Files with no reviewable changes (2)
  • test/tests/integration/adoption-token.test.ts
  • migrations/dashboard/create-tables.js.sql

@MartinKolarik MartinKolarik force-pushed the master branch 3 times, most recently from a03a68e to 0460718 Compare April 4, 2026 11:43
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.

1 participant