feat: credits notification#841
Conversation
WalkthroughThis PR implements a low-credits notification feature. The database schema is extended with a Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@migrations/dashboard/create-tables.js.sql`:
- Around line 51-54: The low_credits_default_threshold column in
directus_settings allows negative values; update the CREATE TABLE for
directus_settings to enforce non-negative values by adding a CHECK constraint
like CHECK (low_credits_default_threshold >= 0) (or change the column to INT
UNSIGNED NULL) so the column low_credits_default_threshold cannot be negative
while still allowing NULL if desired.
In `@src/lib/credits.ts`:
- Around line 115-120: In refreshDefaultThreshold, the cached
this.defaultThreshold is never cleared when
directus_settings.low_credits_default_threshold is NULL or the row is missing;
update the method (refreshDefaultThreshold) so that if typeof
row?.low_credits_default_threshold !== 'number' you explicitly set
this.defaultThreshold to null (or the intended disabled sentinel) instead of
leaving the previous value, ensuring runtime disabling works when the DB value
is cleared.
- Around line 98-101: The threshold check treats 0 as falsy and skips
notifications; update the conditional in the credits logic so it explicitly
checks for undefined/null rather than truthiness: use the computed threshold
(variable threshold) and change the guard from "if (threshold && previousAmount
> threshold && remainingAmount <= threshold)" to an explicit existence check
(e.g., threshold !== undefined && threshold !== null) so that threshold === 0
will still trigger the notifications path that calls got.post to
`${directusUrl}/notifications` when previousAmount and remainingAmount cross the
threshold.
In `@src/lib/server.ts`:
- Around line 98-101: The Promise.all currently fails startup if
credits.syncPreferences() rejects; change the second element so only
credits.syncPreferences is locally handled: keep using logIfTooLong but wrap the
credits.syncPreferences() invocation in a try/catch or append .catch(...) to log
the error (e.g., processLogger.warn/processLogger.error) and swallow it so it
resolves, while leaving logIfTooLong(auth.syncTokens(), 'auth.syncTokens')
unchanged; reference the credits.syncPreferences and logIfTooLong call sites so
the startup continues even if the credits sync transiently fails.
In `@test/tests/integration/credits-notification.test.ts`:
- Around line 64-80: The test uses a mutable flag (posted) and a fixed sleep
(setTimeout(50)) which is timing-flaky; replace that pattern by capturing the
nock scope returned from
nock('https://dash-directus.globalping.io').post('/notifications', ...) and
after calling requestAgent.post('/v1/measurements') poll the scope with a
bounded wait until scope.isDone() (or call scope.done() in a try/catch with a
short retry loop) to deterministically assert the notification was sent, and for
the negative case assert scope.isDone() === false after the same bounded grace
period; update the assertions around the mutable posted flag and setTimeout(50)
accordingly.
🪄 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: 22a13b1a-14bb-4c30-a4ba-651e35508530
📒 Files selected for processing (8)
migrations/dashboard/create-tables.js.sqlseeds/dashboard/development/index.jsseeds/dashboard/test/index.jssrc/lib/credits.tssrc/lib/server.tstest/tests/integration/credits-notification.test.tstest/tests/unit/credits.test.tstest/tests/unit/override/adopted-probes.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/credits.ts (1)
51-52: 💤 Low valueConsider wrapping JSON.parse in try-catch for resilience.
If a single row contains malformed
notification_preferences, the entire sync fails and no users are updated. Wrapping the parse in a try-catch and skipping invalid rows would make the sync more robust.♻️ Suggested improvement
this.userIdToPreference = new Map(rows.flatMap<[string, false | number]>(({ id, notification_preferences }) => { - const prefs = JSON.parse(notification_preferences) as NotificationPreferences; + let prefs: NotificationPreferences; + try { + prefs = JSON.parse(notification_preferences) as NotificationPreferences; + } catch { + logger.error(`Invalid notification_preferences JSON for user ${id}`); + return []; + } const lowCredits = prefs.low_credits;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/credits.ts` around lines 51 - 52, The current rows.flatMap callback used to populate userIdToPreference parses notification_preferences with JSON.parse and can throw, causing the whole sync to fail; wrap the JSON.parse call inside a try-catch within the flatMap callback (or a small helper used by it), and on catch skip that row (return [] from the flatMap) and optionally log the offending id and parse error; ensure the result still conforms to the expected [string, false | number] tuples for userIdToPreference and keep the types using NotificationPreferences when parse succeeds.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/lib/credits.ts`:
- Around line 51-52: The current rows.flatMap callback used to populate
userIdToPreference parses notification_preferences with JSON.parse and can
throw, causing the whole sync to fail; wrap the JSON.parse call inside a
try-catch within the flatMap callback (or a small helper used by it), and on
catch skip that row (return [] from the flatMap) and optionally log the
offending id and parse error; ensure the result still conforms to the expected
[string, false | number] tuples for userIdToPreference and keep the types using
NotificationPreferences when parse succeeds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1dcc05ee-b2e4-462f-a942-4433070b78a5
📒 Files selected for processing (2)
migrations/dashboard/create-tables.js.sqlsrc/lib/credits.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- migrations/dashboard/create-tables.js.sql
MartinKolarik
left a comment
There was a problem hiding this comment.
I don't think the API should be involved in this at all, especially as we might need to refactor the credits handling soon to introduce caching. Directus can do a simple SELECT * FROM users join credits where credits < threshold every few minutes, and we can add a flag on the user, e.g., creditsBelowThreshold, which decides whether a new notification is sent (not if already true). The flag resets on credit additions if the credits go above the threshold.
35b42dd to
5e327ba
Compare
Fixes jsdelivr/globalping-dash#198
Explicit threshold approach is used as the simplest to understand and use.