feat: Add Email Notifications for Limit Warnings (#47)#77
Open
ayash911 wants to merge 1 commit intoyash-pouranik:mainfrom
Open
feat: Add Email Notifications for Limit Warnings (#47)#77ayash911 wants to merge 1 commit intoyash-pouranik:mainfrom
ayash911 wants to merge 1 commit intoyash-pouranik:mainfrom
Conversation
- Implemented database and storage limit monitoring - Supported BYOD databases size thresholds calculation via db.stats() - Integrated dynamic thresholds UI for Managed (Percentages) & External configs (Absolute Sizes) - Implemented email queue job mapping through styled HTML template - Delivered fully scoped Jest unit tests validating threshold logic, cooldown limits, and module architecture
|
@ayash911 is attempting to deploy a commit to the Yash Pouranik's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
📝 WalkthroughWalkthroughImplements a comprehensive resource limit notification system that sends email alerts when storage or database usage crosses configured thresholds. Supports both percentage-based thresholds for managed projects and absolute limits for BYOD projects. Includes backend endpoint for configuration, frontend UI, notification logic with 7-day cooldown enforcement, external database size calculation with caching, and email queue integration. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PublicAPI as Public API<br/>(data/storage)
participant CheckNotify as checkAndNotify<br/>Logic
participant ExtDB as External DB<br/>(if BYOD)
participant EmailQueue as Email Queue
participant Worker as Email Worker
participant EmailSvc as Email Service
Client->>PublicAPI: Upload file / Insert data
PublicAPI->>PublicAPI: Create resource
PublicAPI->>CheckNotify: checkAndNotify({project, resourceType, currentUsage, ownerEmail})
alt External Database
CheckNotify->>ExtDB: db.stats() [with 1-hour cache]
ExtDB-->>CheckNotify: dataSize
end
CheckNotify->>CheckNotify: Calculate % usage or match absolute limit
CheckNotify->>CheckNotify: Detect threshold crossing (80%, 95%, custom)
CheckNotify->>CheckNotify: Check 7-day cooldown for threshold
alt Threshold crossed AND cooldown expired
CheckNotify->>EmailQueue: Enqueue limit-warning job
CheckNotify->>CheckNotify: Update lastLimitNotification timestamp
PublicAPI-->>Client: 200/201 response (fire-and-forget)
EmailQueue->>Worker: Poll job
Worker->>EmailSvc: sendLimitWarningEmail(email, {projectName, resourceType, ...})
EmailSvc-->>Worker: Email sent
Worker-->>EmailQueue: Job complete
else
PublicAPI-->>Client: 200/201 response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
packages/common/src/index.js (1)
64-66: Consolidate duplicateemailServiceimports.Line 66 re-requires
./utils/emailServiceeven though it’s already imported at Line 32. Merge into one import to reduce duplication.♻️ Proposed refactor
-const { sendOtp, sendReleaseEmail, sendAuthOtpEmail } = require('./utils/emailService'); +const { sendOtp, sendReleaseEmail, sendAuthOtpEmail, sendLimitWarningEmail } = require('./utils/emailService'); @@ -const { sendLimitWarningEmail } = require('./utils/emailService');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/common/src/index.js` around lines 64 - 66, There is a duplicated require of './utils/emailService'—remove the redundant import and consolidate into the existing require so `sendLimitWarningEmail` (and any other symbols from emailService) are imported only once; update references to use the single imported identifier (e.g., `sendLimitWarningEmail`) and ensure no other code expects a second require.apps/dashboard-api/src/__tests__/project.controller.test.js (2)
3-3: Unused import:mongooseThe
mongoosemodule is imported but not used anywhere in the test file.🔧 Proposed fix
'use strict'; -const mongoose = require('mongoose'); - // Mock `@urbackend/common` first to mock Project and cache utilities🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard-api/src/__tests__/project.controller.test.js` at line 3, Remove the unused require('mongoose') import at the top of the test file (the const mongoose = require('mongoose'); line) since it is never referenced in this test suite; if the tests actually need mongoose setup/teardown, replace the standalone import with the appropriate test helper call (e.g., a connect/disconnect helper) and reference those helper functions instead of leaving the unused mongoose symbol.
55-91: Consider adding test formarkModifiedconditional logic.The test verifies that
markModifiedis called whenstorage/databaseare provided, but doesn't verify it's NOT called when onlyenabledis passed. This would ensure the conditional at lines 1186-1188 of project.controller.js works correctly.📝 Additional test case
test('should not call markModified when only enabled is changed', async () => { mockReq.body = { email: { enabled: false } }; // no storage/database const mockProjectDoc = { _id: '60c72b2f9b1d8b001c8e4b52', notificationSettings: { email: { enabled: true } }, markModified: jest.fn(), save: mockProjectSave }; mockProjectFindOne.mockResolvedValueOnce(mockProjectDoc); mockProjectSave.mockResolvedValueOnce(mockProjectDoc); await updateNotificationSettings(mockReq, mockRes); expect(mockProjectDoc.markModified).not.toHaveBeenCalled(); expect(mockProjectSave).toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard-api/src/__tests__/project.controller.test.js` around lines 55 - 91, The test suite is missing a negative case for the markModified conditional in updateNotificationSettings; add a new test in apps/dashboard-api/src/__tests__/project.controller.test.js that calls updateNotificationSettings with mockReq.body.email containing only { enabled: <value> } (no storage or database), set up mockProjectFindOne to return a mockProjectDoc with notificationSettings.email present, and assert that mockProjectDoc.markModified was NOT called while mockProjectSave was called and mockRes.json returned the expected success payload; this verifies the conditional logic around markModified in the updateNotificationSettings handler.apps/public-api/src/controllers/data.controller.js (1)
12-12: Unused import:isProjectDbExternalThis symbol is imported but never used in the controller. The code uses
project.resources.db.isExternaldirectly instead.🔧 Proposed fix
const { sanitize, Project, getConnection, getCompiledModel, QueryEngine, validateData, validateUpdateData, Developer, checkAndNotify, calculateExternalDbSize, - isProjectDbExternal, } = require("@urbackend/common");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/public-api/src/controllers/data.controller.js` at line 12, Remove the unused import isProjectDbExternal from the imports in data.controller.js; the controller already uses project.resources.db.isExternal directly, so update the import list to drop isProjectDbExternal (or replace its usage if you intended to use the helper) to satisfy the linter and avoid dead code.apps/public-api/src/__tests__/limitNotification.test.js (1)
25-104: Test duplicates implementation instead of importing from source.Re-implementing
shouldSendNotification,formatBytes, andcheckAndNotifyinline creates maintenance risk—if the source logic changes, tests may still pass while actual behavior differs. The comment mentions ESM issues, but consider extracting pure functions to a testable module or using Jest'smoduleNameMapperto resolve the import chain.For now, if the duplication is intentional due to module loading constraints, add a comment noting which source file/lines these mirror and consider periodic manual verification.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/public-api/src/__tests__/limitNotification.test.js` around lines 25 - 104, Tests duplicate the logic for shouldSendNotification, formatBytes, and checkAndNotify which risks drifting from production; either import the real pure helpers or clearly document the duplication. Modify the code under test to import shouldSendNotification, formatBytes, and checkAndNotify from the source module (export those helpers if necessary from the original implementation) so tests exercise the real logic (update test setup or Jest moduleNameMapper if ESM import issues occur); if importing is impossible right now, add a concise comment above the inline implementations stating they mirror the production functions (include the production module name and line range) and add a TODO to remove the duplication once the helpers are exported.apps/web-dashboard/src/pages/ProjectSettings.jsx (1)
1284-1299: Consider adding input validation for threshold values.The code filters
NaNvalues but doesn't validate:
- Negative thresholds or absolute limits
- Thresholds outside 0-100 range for percentages
- Zero values for absolute limits
Invalid values could cause unexpected notification behavior.
🛡️ Proposed validation
const emailSettings = { enabled, storage: { type: storageType, thresholds: storageType === "percentage" - ? storageThresholds.split(",").map(t => parseInt(t.trim())).filter(t => !isNaN(t)) + ? storageThresholds.split(",").map(t => parseInt(t.trim(), 10)).filter(t => !isNaN(t) && t > 0 && t <= 100) : [], absoluteLimit: storageType === "absolute" && storageAbsolute - ? parseInt(storageAbsolute) * 1024 * 1024 + ? Math.max(1, parseInt(storageAbsolute, 10)) * 1024 * 1024 : null }, database: { type: dbType, thresholds: dbType === "percentage" - ? dbThresholds.split(",").map(t => parseInt(t.trim())).filter(t => !isNaN(t)) + ? dbThresholds.split(",").map(t => parseInt(t.trim(), 10)).filter(t => !isNaN(t) && t > 0 && t <= 100) : [], absoluteLimit: dbType === "absolute" && dbAbsolute - ? parseInt(dbAbsolute) * 1024 * 1024 + ? Math.max(1, parseInt(dbAbsolute, 10)) * 1024 * 1024 : null } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web-dashboard/src/pages/ProjectSettings.jsx` around lines 1284 - 1299, Validate parsed threshold and absolute limit values before assigning to thresholds and absoluteLimit: for storageThresholds and dbThresholds (used when storageType/dbType === "percentage") parse and trim each entry, then filter out non-numeric, negative, or >100 values (and optionally enforce integer and uniqueness) before setting thresholds; for storageAbsolute and dbAbsolute (used when storageType/dbType === "absolute") parse and ensure the result is a positive non-zero number before multiplying by 1024*1024 and assigning to absoluteLimit; update the logic around storageThresholds/storageAbsolute/dbThresholds/dbAbsolute in the object construction so invalid values are discarded and absoluteLimit is null if the parsed value is <= 0 or NaN.packages/common/src/utils/calculateExternalDbSize.js (1)
41-48: Use Model.updateOne() with $set operator to match codebase conventions.The current
project.updateOne()call lacks the standard$setoperator and is inconsistent with all other update patterns in the codebase (e.g.,limitNotification.js,data.controller.js), which useModel.updateOne(filter, { $set: {...} }). While technically valid, adopting the standard pattern improves consistency and clarity.🔧 Proposed fix
+const Project = require('../models/Project'); + // Persist cache (fire-and-forget; don't let failures block the caller) -project.updateOne({ - 'cachedUsageStats.database': { - size: sizeBytes, - lastCalculated: new Date(), - }, -}).catch((err) => { +Project.updateOne( + { _id: project._id }, + { + $set: { + 'cachedUsageStats.database': { + size: sizeBytes, + lastCalculated: new Date(), + }, + }, + } +).catch((err) => { console.error('[calculateExternalDbSize] Failed to update cache:', err.message); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/common/src/utils/calculateExternalDbSize.js` around lines 41 - 48, The update to the project's cached usage uses project.updateOne(...) without the $set operator; change the call to use the standard Model.updateOne(filter, { $set: { 'cachedUsageStats.database': { size: sizeBytes, lastCalculated: new Date() } } }) pattern to match existing conventions (refer to the project.updateOne call and the 'cachedUsageStats.database' payload) and keep the existing .catch logging unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/dashboard-api/src/controllers/project.controller.js`:
- Around line 1164-1192: The updateNotificationSettings handler currently does a
shallow merge of req.body.email into project.notificationSettings.email which
can overwrite sibling fields; replace the shallow merge in
updateNotificationSettings so you perform a deep/recursive merge of
project.notificationSettings.email and the incoming email payload (preserving
unspecified nested fields like storage.type and storage.absoluteLimit), validate
the resulting settings against a schema (e.g., required shapes for email.storage
and email.database, allowed keys and types) before assigning, only call
project.markModified('notificationSettings') when nested objects actually
changed, and retain the deleteProjectById(projectId) and save flow after
successful validation/merge.
In `@apps/public-api/src/controllers/storage.controller.js`:
- Around line 71-79: The current notification uses project.storageUsed to
compute postUploadUsage which can be stale for BYOD external storage; update or
read an authoritative cumulative usage before calling checkAndNotify.
Specifically, in the block around Developer.findById(...).then(...) replace the
direct use of project.storageUsed with either (a) fetch the authoritative
external usage (e.g., call a function like getExternalStorageUsage(project.id)
or the external storage service API) and compute postUploadUsage =
authoritativeUsage + file.size, or (b) persistently increment
project.storageUsed (save the updated project record) so the stored value is
authoritative, then compute postUploadUsage from the saved value and call
checkAndNotify with that updated number; ensure checkAndNotify is called only
after the authoritative value is obtained/saved.
In `@apps/web-dashboard/src/pages/ProjectSettings.jsx`:
- Around line 1471-1478: The displayed storage and database values are in bytes
but shown with an "MB" suffix; update the JSX where
project?.cachedUsageStats?.storage?.size, project?.storageUsed,
project?.cachedUsageStats?.database?.size (and optionally
project?.databaseSizeMB if it's bytes) are rendered to convert bytes to
megabytes by dividing by 1024 * 1024 (and format/round as desired) before
appending "MB" so the numbers reflect MB units correctly.
In `@packages/common/src/models/Project.js`:
- Around line 91-133: The thresholds arrays in notificationSettings (fields
notificationSettings.email.storage.thresholds and
notificationSettings.email.database.thresholds) currently accept arbitrary
numbers but lastLimitNotification only tracks threshold80/threshold95/custom;
add a validator on those thresholds schema entries to only allow 80 and 95 (or
empty) so cooldown keys stay consistent: update the mongoose.Schema definition
for notificationSettings to replace thresholds: { type: [Number], default:
[80,95] } with a validators option (e.g., validate: { validator: arr =>
arr.every(n => n === 80 || n === 95), message: 'thresholds must be 80 or 95' })
for both storage and database, and keep the default as [80,95]; this ensures
notificationSettings.thresholds and lastLimitNotification (storage/database ->
threshold80/threshold95/custom) remain aligned.
In `@packages/common/src/utils/emailTemplates/limitWarning.js`:
- Around line 15-105: The template function limitWarningTemplate inserts
unescaped user-controlled values (projectName, currentUsage, limit, percentage
and any dynamic pieces used in usageBlock, actionItems, footerNote) directly
into the HTML; create or reuse an HTML-escaping helper (e.g., escapeHtml) and
apply it to every interpolated value and any strings composed from them before
building usageBlock, actionItems, alertHeadline and the final returned HTML so
raw markup cannot be injected.
In `@packages/common/src/utils/limitNotification.js`:
- Around line 86-102: The current flow can send an email via emailQueue.add and
then fail to persist the cooldown timestamp with Project.updateOne, causing
duplicate notifications; update the implementation in limitNotification.js to
persist the cooldown first (set the updatePath:
`lastLimitNotification.${resourceType}.${alertKey}` using Project.updateOne) and
only after a successful update call emailQueue.add('limit-warning', {...}), or
alternatively perform both operations inside a transaction/atomic operation and
rollback the enqueue on update failure—ensure you reference and handle failures
from both emailQueue.add and Project.updateOne so the cooldown timestamp and
sent-state remain consistent.
---
Nitpick comments:
In `@apps/dashboard-api/src/__tests__/project.controller.test.js`:
- Line 3: Remove the unused require('mongoose') import at the top of the test
file (the const mongoose = require('mongoose'); line) since it is never
referenced in this test suite; if the tests actually need mongoose
setup/teardown, replace the standalone import with the appropriate test helper
call (e.g., a connect/disconnect helper) and reference those helper functions
instead of leaving the unused mongoose symbol.
- Around line 55-91: The test suite is missing a negative case for the
markModified conditional in updateNotificationSettings; add a new test in
apps/dashboard-api/src/__tests__/project.controller.test.js that calls
updateNotificationSettings with mockReq.body.email containing only { enabled:
<value> } (no storage or database), set up mockProjectFindOne to return a
mockProjectDoc with notificationSettings.email present, and assert that
mockProjectDoc.markModified was NOT called while mockProjectSave was called and
mockRes.json returned the expected success payload; this verifies the
conditional logic around markModified in the updateNotificationSettings handler.
In `@apps/public-api/src/__tests__/limitNotification.test.js`:
- Around line 25-104: Tests duplicate the logic for shouldSendNotification,
formatBytes, and checkAndNotify which risks drifting from production; either
import the real pure helpers or clearly document the duplication. Modify the
code under test to import shouldSendNotification, formatBytes, and
checkAndNotify from the source module (export those helpers if necessary from
the original implementation) so tests exercise the real logic (update test setup
or Jest moduleNameMapper if ESM import issues occur); if importing is impossible
right now, add a concise comment above the inline implementations stating they
mirror the production functions (include the production module name and line
range) and add a TODO to remove the duplication once the helpers are exported.
In `@apps/public-api/src/controllers/data.controller.js`:
- Line 12: Remove the unused import isProjectDbExternal from the imports in
data.controller.js; the controller already uses project.resources.db.isExternal
directly, so update the import list to drop isProjectDbExternal (or replace its
usage if you intended to use the helper) to satisfy the linter and avoid dead
code.
In `@apps/web-dashboard/src/pages/ProjectSettings.jsx`:
- Around line 1284-1299: Validate parsed threshold and absolute limit values
before assigning to thresholds and absoluteLimit: for storageThresholds and
dbThresholds (used when storageType/dbType === "percentage") parse and trim each
entry, then filter out non-numeric, negative, or >100 values (and optionally
enforce integer and uniqueness) before setting thresholds; for storageAbsolute
and dbAbsolute (used when storageType/dbType === "absolute") parse and ensure
the result is a positive non-zero number before multiplying by 1024*1024 and
assigning to absoluteLimit; update the logic around
storageThresholds/storageAbsolute/dbThresholds/dbAbsolute in the object
construction so invalid values are discarded and absoluteLimit is null if the
parsed value is <= 0 or NaN.
In `@packages/common/src/index.js`:
- Around line 64-66: There is a duplicated require of
'./utils/emailService'—remove the redundant import and consolidate into the
existing require so `sendLimitWarningEmail` (and any other symbols from
emailService) are imported only once; update references to use the single
imported identifier (e.g., `sendLimitWarningEmail`) and ensure no other code
expects a second require.
In `@packages/common/src/utils/calculateExternalDbSize.js`:
- Around line 41-48: The update to the project's cached usage uses
project.updateOne(...) without the $set operator; change the call to use the
standard Model.updateOne(filter, { $set: { 'cachedUsageStats.database': { size:
sizeBytes, lastCalculated: new Date() } } }) pattern to match existing
conventions (refer to the project.updateOne call and the
'cachedUsageStats.database' payload) and keep the existing .catch logging
unchanged.
🪄 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: aefb3e87-4c91-437f-be12-c5758bb0319e
⛔ Files ignored due to path filters (2)
apps/web-dashboard/dist/assets/index-BLM4F-ar.jsis excluded by!**/dist/**apps/web-dashboard/dist/index.htmlis excluded by!**/dist/**
📒 Files selected for processing (14)
apps/dashboard-api/src/__tests__/project.controller.test.jsapps/dashboard-api/src/controllers/project.controller.jsapps/dashboard-api/src/routes/projects.jsapps/public-api/src/__tests__/limitNotification.test.jsapps/public-api/src/controllers/data.controller.jsapps/public-api/src/controllers/storage.controller.jsapps/web-dashboard/src/pages/ProjectSettings.jsxpackages/common/src/index.jspackages/common/src/models/Project.jspackages/common/src/queues/emailQueue.jspackages/common/src/utils/calculateExternalDbSize.jspackages/common/src/utils/emailService.jspackages/common/src/utils/emailTemplates/limitWarning.jspackages/common/src/utils/limitNotification.js
Comment on lines
+1164
to
+1192
| module.exports.updateNotificationSettings = async (req, res) => { | ||
| try { | ||
| const { projectId } = req.params; | ||
| const { email } = req.body; // { enabled: true, storage: {...}, database: {...} } | ||
|
|
||
| if (!email) { | ||
| return res.status(400).json({ error: "Missing 'email' settings in body" }); | ||
| } | ||
|
|
||
| const project = await Project.findOne({ _id: projectId, owner: req.user._id }); | ||
| if (!project) return res.status(404).json({ error: "Project not found or access denied." }); | ||
|
|
||
| project.notificationSettings = { | ||
| ...project.notificationSettings, | ||
| email: { | ||
| ...project.notificationSettings?.email, | ||
| ...email, | ||
| } | ||
| }; | ||
|
|
||
| if (email.storage || email.database) { | ||
| project.markModified('notificationSettings'); | ||
| } | ||
|
|
||
| await project.save(); | ||
|
|
||
| await deleteProjectById(projectId); | ||
| res.json({ success: true, settings: project.notificationSettings }); | ||
| } catch (err) { |
Contributor
There was a problem hiding this comment.
Validate and deep-merge notification settings payload.
Line 1176-1182 performs a shallow merge. Partial updates like email.storage = { thresholds: [...] } can overwrite sibling storage fields (type, absoluteLimit) unintentionally, and there’s no schema validation to prevent invalid payload shapes.
🧩 Proposed fix
module.exports.updateNotificationSettings = async (req, res) => {
try {
const { projectId } = req.params;
- const { email } = req.body; // { enabled: true, storage: {...}, database: {...} }
+ const { email } = req.body;
if (!email) {
return res.status(400).json({ error: "Missing 'email' settings in body" });
}
const project = await Project.findOne({ _id: projectId, owner: req.user._id });
if (!project) return res.status(404).json({ error: "Project not found or access denied." });
+ const currentEmail = project.notificationSettings?.email || {};
project.notificationSettings = {
...project.notificationSettings,
email: {
- ...project.notificationSettings?.email,
+ ...currentEmail,
...email,
+ storage: email.storage
+ ? { ...(currentEmail.storage || {}), ...email.storage }
+ : currentEmail.storage,
+ database: email.database
+ ? { ...(currentEmail.database || {}), ...email.database }
+ : currentEmail.database,
}
};
- if (email.storage || email.database) {
- project.markModified('notificationSettings');
- }
+ project.markModified('notificationSettings');
await project.save();
await deleteProjectById(projectId);
res.json({ success: true, settings: project.notificationSettings });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/dashboard-api/src/controllers/project.controller.js` around lines 1164 -
1192, The updateNotificationSettings handler currently does a shallow merge of
req.body.email into project.notificationSettings.email which can overwrite
sibling fields; replace the shallow merge in updateNotificationSettings so you
perform a deep/recursive merge of project.notificationSettings.email and the
incoming email payload (preserving unspecified nested fields like storage.type
and storage.absoluteLimit), validate the resulting settings against a schema
(e.g., required shapes for email.storage and email.database, allowed keys and
types) before assigning, only call project.markModified('notificationSettings')
when nested objects actually changed, and retain the
deleteProjectById(projectId) and save flow after successful validation/merge.
Comment on lines
+71
to
+79
| Developer.findById(project.owner).select('email').then((owner) => { | ||
| if (owner?.email) { | ||
| const postUploadUsage = (project.storageUsed || 0) + file.size; | ||
| checkAndNotify({ | ||
| project, | ||
| resourceType: 'storage', | ||
| currentUsage: postUploadUsage, | ||
| ownerEmail: owner.email, | ||
| }).catch((e) => console.error('[storage] notification error:', e.message)); |
Contributor
There was a problem hiding this comment.
BYOD storage alerts can use stale/non-cumulative usage.
At Line 73, usage is derived from project.storageUsed, but this path does not ensure external storage usage is cumulatively tracked before notifying. That can miss threshold crossings for BYOD alerts.
Use a usage source that is authoritative for external storage (or persist cumulative storageUsed for external uploads/deletes before calling checkAndNotify).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/public-api/src/controllers/storage.controller.js` around lines 71 - 79,
The current notification uses project.storageUsed to compute postUploadUsage
which can be stale for BYOD external storage; update or read an authoritative
cumulative usage before calling checkAndNotify. Specifically, in the block
around Developer.findById(...).then(...) replace the direct use of
project.storageUsed with either (a) fetch the authoritative external usage
(e.g., call a function like getExternalStorageUsage(project.id) or the external
storage service API) and compute postUploadUsage = authoritativeUsage +
file.size, or (b) persistently increment project.storageUsed (save the updated
project record) so the stored value is authoritative, then compute
postUploadUsage from the saved value and call checkAndNotify with that updated
number; ensure checkAndNotify is called only after the authoritative value is
obtained/saved.
Comment on lines
+1471
to
+1478
| <div style={{ fontSize: "1.1rem", fontWeight: 600 }}>{project?.cachedUsageStats?.storage?.size || project?.storageUsed || 0} MB</div> | ||
| {project?.resources?.storage?.isExternal && project?.cachedUsageStats?.storage?.lastCalculated && ( | ||
| <div style={{ fontSize: "0.75rem", color: "var(--color-text-muted)", marginTop: "4px" }}>As of {new Date(project.cachedUsageStats.storage.lastCalculated).toLocaleString()}</div> | ||
| )} | ||
| </div> | ||
| <div style={{ padding: "10px", background: "rgba(0,0,0,0.2)", borderRadius: "6px" }}> | ||
| <div style={{ fontSize: "0.85rem", color: "var(--color-text-muted)" }}>Database Used</div> | ||
| <div style={{ fontSize: "1.1rem", fontWeight: 600 }}>{project?.cachedUsageStats?.database?.size || project?.databaseSizeMB || 0} MB</div> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how storageUsed and databaseSizeMB are stored in the Project schema
ast-grep --pattern 'storageUsed: {
$$$
}'
rg -n "storageUsed|databaseSizeMB" --type=js -g '!**/test/**' -g '!**/__tests__/**' -A2 -B2 packages/common/src/models/Repository: yash-pouranik/urBackend
Length of output: 429
🏁 Script executed:
# Check databaseSizeMB schema definition
rg -n "databaseSizeMB" --type=js -A2 -B2 packages/common/src/models/Repository: yash-pouranik/urBackend
Length of output: 49
🏁 Script executed:
# Check cachedUsageStats schema
rg -n "cachedUsageStats" --type=js -A5 -B2 packages/common/src/models/Repository: yash-pouranik/urBackend
Length of output: 713
🏁 Script executed:
# Check how storageUsed is calculated/set in the backend
rg -n "storageUsed\s*=" --type=js -A3 -B1 | head -50Repository: yash-pouranik/urBackend
Length of output: 976
🏁 Script executed:
# Check if there's any conversion logic for displaying these values
rg -n "storageUsed|databaseSizeMB" apps/web-dashboard/src/ --type=js -B2 -A2 | head -100Repository: yash-pouranik/urBackend
Length of output: 1909
🏁 Script executed:
# Check full cachedUsageStats schema including storage
rg -n "cachedUsageStats" packages/common/src/models/Project.js -A15 -B2Repository: yash-pouranik/urBackend
Length of output: 806
🏁 Script executed:
# Find where databaseSizeMB is set/calculated
rg -n "databaseSizeMB" --type=js -B3 -A3 | head -80Repository: yash-pouranik/urBackend
Length of output: 40724
Convert byte values to MB before displaying cached usage stats.
Both cachedUsageStats.storage.size and cachedUsageStats.database.size are stored as bytes in the schema, but the code displays them directly with an "MB" suffix (lines 1471, 1478). Similarly, storageUsed is stored in bytes but shown as MB. Divide these values by 1024 * 1024 to convert bytes to MB for accurate display.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web-dashboard/src/pages/ProjectSettings.jsx` around lines 1471 - 1478,
The displayed storage and database values are in bytes but shown with an "MB"
suffix; update the JSX where project?.cachedUsageStats?.storage?.size,
project?.storageUsed, project?.cachedUsageStats?.database?.size (and optionally
project?.databaseSizeMB if it's bytes) are rendered to convert bytes to
megabytes by dividing by 1024 * 1024 (and format/round as desired) before
appending "MB" so the numbers reflect MB units correctly.
Comment on lines
+91
to
+133
| notificationSettings: { | ||
| type: new mongoose.Schema({ | ||
| email: { | ||
| enabled: { type: Boolean, default: true }, | ||
| storage: { | ||
| type: { type: String, enum: ['percentage', 'absolute'], default: 'percentage' }, | ||
| thresholds: { type: [Number], default: [80, 95] }, | ||
| absoluteLimit: { type: Number, default: null }, // bytes — for BYOD | ||
| }, | ||
| database: { | ||
| type: { type: String, enum: ['percentage', 'absolute'], default: 'percentage' }, | ||
| thresholds: { type: [Number], default: [80, 95] }, | ||
| absoluteLimit: { type: Number, default: null }, // bytes — for BYOD | ||
| }, | ||
| }, | ||
| }, { _id: false }), | ||
| default: () => ({ | ||
| email: { | ||
| enabled: true, | ||
| storage: { type: 'percentage', thresholds: [80, 95], absoluteLimit: null }, | ||
| database: { type: 'percentage', thresholds: [80, 95], absoluteLimit: null }, | ||
| }, | ||
| }), | ||
| }, | ||
|
|
||
| // Tracks when each threshold was last alerted (7-day cooldown) | ||
| lastLimitNotification: { | ||
| type: new mongoose.Schema({ | ||
| storage: { | ||
| threshold80: { type: Date, default: null }, | ||
| threshold95: { type: Date, default: null }, | ||
| custom: { type: Date, default: null }, | ||
| }, | ||
| database: { | ||
| threshold80: { type: Date, default: null }, | ||
| threshold95: { type: Date, default: null }, | ||
| custom: { type: Date, default: null }, | ||
| }, | ||
| }, { _id: false }), | ||
| default: () => ({ | ||
| storage: { threshold80: null, threshold95: null, custom: null }, | ||
| database: { threshold80: null, threshold95: null, custom: null }, | ||
| }), |
Contributor
There was a problem hiding this comment.
Constrain thresholds to cooldown-supported keys.
Line 97 and Line 102 allow arbitrary threshold values, but cooldown slots are fixed to threshold80/threshold95/custom (Line 120-127). This can produce inconsistent cooldown tracking for non-80/95 values.
🛠️ Proposed fix
storage: {
type: { type: String, enum: ['percentage', 'absolute'], default: 'percentage' },
- thresholds: { type: [Number], default: [80, 95] },
- absoluteLimit: { type: Number, default: null }, // bytes — for BYOD
+ thresholds: {
+ type: [{ type: Number, enum: [80, 95] }],
+ default: [80, 95],
+ },
+ absoluteLimit: { type: Number, min: 0, default: null }, // bytes — for BYOD
},
database: {
type: { type: String, enum: ['percentage', 'absolute'], default: 'percentage' },
- thresholds: { type: [Number], default: [80, 95] },
- absoluteLimit: { type: Number, default: null }, // bytes — for BYOD
+ thresholds: {
+ type: [{ type: Number, enum: [80, 95] }],
+ default: [80, 95],
+ },
+ absoluteLimit: { type: Number, min: 0, default: null }, // bytes — for BYOD
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/common/src/models/Project.js` around lines 91 - 133, The thresholds
arrays in notificationSettings (fields
notificationSettings.email.storage.thresholds and
notificationSettings.email.database.thresholds) currently accept arbitrary
numbers but lastLimitNotification only tracks threshold80/threshold95/custom;
add a validator on those thresholds schema entries to only allow 80 and 95 (or
empty) so cooldown keys stay consistent: update the mongoose.Schema definition
for notificationSettings to replace thresholds: { type: [Number], default:
[80,95] } with a validators option (e.g., validate: { validator: arr =>
arr.every(n => n === 80 || n === 95), message: 'thresholds must be 80 or 95' })
for both storage and database, and keep the default as [80,95]; this ensures
notificationSettings.thresholds and lastLimitNotification (storage/database ->
threshold80/threshold95/custom) remain aligned.
Comment on lines
+15
to
+105
| const limitWarningTemplate = ({ | ||
| projectName, | ||
| resourceType, | ||
| currentUsage, | ||
| limit, | ||
| percentage, | ||
| isBYOD, | ||
| }) => { | ||
| const resourceLabel = resourceType === 'storage' ? 'Storage' : 'Database'; | ||
| const resourceIcon = resourceType === 'storage' ? '🗄️' : '🛢️'; | ||
|
|
||
| const alertHeadline = isBYOD | ||
| ? `Your custom ${resourceLabel.toLowerCase()} alert threshold has been reached.` | ||
| : `You've used <strong>${percentage}%</strong> of your ${resourceLabel.toLowerCase()} limit.`; | ||
|
|
||
| const usageBlock = isBYOD | ||
| ? `<div class="stat"><span class="stat-label">Current usage</span><span class="stat-value">${currentUsage}</span></div>` | ||
| : ` | ||
| <div class="stat"><span class="stat-label">Current usage</span><span class="stat-value">${currentUsage}</span></div> | ||
| <div class="stat"><span class="stat-label">Plan limit</span><span class="stat-value">${limit}</span></div> | ||
| <div class="stat"><span class="stat-label">Usage</span><span class="stat-value">${percentage}%</span></div>`; | ||
|
|
||
| const actionItems = resourceType === 'storage' | ||
| ? `<li>Delete unused files from your project</li> | ||
| ${isBYOD ? '<li>Expand your external storage bucket capacity</li>' : '<li>Upgrade your plan for higher limits</li>'} | ||
| <li>Review large file uploads and remove duplicates</li>` | ||
| : `<li>Remove stale or test documents from your collections</li> | ||
| ${isBYOD ? '<li>Scale up your external MongoDB cluster</li>' : '<li>Upgrade your plan for higher limits</li>'} | ||
| <li>Optimise your data schema to reduce document size</li>`; | ||
|
|
||
| const footerNote = isBYOD | ||
| ? 'This is a custom alert you configured in your project settings.' | ||
| : 'You will receive at most one alert per threshold every 7 days.'; | ||
|
|
||
| return `<!DOCTYPE html> | ||
| <html> | ||
| <head> | ||
| <meta charset="utf-8" /> | ||
| <style> | ||
| body{font-family:-apple-system,BlinkMacSystemFont,'Segoe UI',Roboto,Helvetica,Arial,sans-serif;background:#ffffff;color:#111111;margin:0;padding:0} | ||
| .container{max-width:600px;margin:0 auto;padding:40px 20px} | ||
| .logo{margin-bottom:32px;font-weight:800;font-size:24px;letter-spacing:-.03em;color:#111} | ||
| .badge{display:inline-block;padding:4px 10px;background:#f59e0b;color:#fff;border-radius:6px;font-size:13px;font-weight:600;margin-bottom:24px} | ||
| h1{font-size:26px;font-weight:700;line-height:1.2;margin-bottom:16px;letter-spacing:-.02em} | ||
| .alert-box{background:#fffbeb;border-left:4px solid #f59e0b;border-radius:4px;padding:16px 20px;margin:24px 0;font-size:15px;line-height:1.6;color:#333} | ||
| .stats{background:#f8f9fa;border-radius:8px;padding:16px 20px;margin:20px 0} | ||
| .stat{display:flex;justify-content:space-between;padding:6px 0;border-bottom:1px solid #efefef;font-size:14px} | ||
| .stat:last-child{border-bottom:none} | ||
| .stat-label{color:#666} | ||
| .stat-value{font-weight:600;color:#111} | ||
| .actions{font-size:15px;line-height:1.6;color:#444;margin-bottom:24px} | ||
| .actions ul{padding-left:20px;margin:12px 0} | ||
| .actions li{margin-bottom:8px} | ||
| .cta{display:inline-block;background:#111;color:#fff;padding:12px 24px;border-radius:8px;font-weight:600;text-decoration:none;font-size:15px} | ||
| .footer{margin-top:64px;padding-top:32px;border-top:1px solid #eee;font-size:13px;color:#888} | ||
| .footer p{margin:4px 0} | ||
| </style> | ||
| </head> | ||
| <body> | ||
| <div class="container"> | ||
| <div class="logo">urBackend</div> | ||
| <div class="badge">${resourceIcon} ${resourceLabel} Alert</div> | ||
| <h1>Resource Limit Warning</h1> | ||
|
|
||
| <div class="alert-box"> | ||
| <strong>Project:</strong> ${projectName}<br/> | ||
| ${alertHeadline} | ||
| </div> | ||
|
|
||
| <div class="stats"> | ||
| ${usageBlock} | ||
| </div> | ||
|
|
||
| <div class="actions"> | ||
| <strong>What you can do:</strong> | ||
| <ul> | ||
| ${actionItems} | ||
| <li>Adjust alert preferences in your project settings</li> | ||
| </ul> | ||
| </div> | ||
|
|
||
| <a href="https://urbackend.bitbros.in/dashboard" class="cta">Go to Dashboard</a> | ||
|
|
||
| <div class="footer"> | ||
| <p>${footerNote}</p> | ||
| <p>© ${new Date().getFullYear()} urBackend Inc. • Developer platform.</p> | ||
| </div> | ||
| </div> | ||
| </body> | ||
| </html>`; | ||
| }; |
Contributor
There was a problem hiding this comment.
Escape interpolated values before injecting into HTML.
projectName and related dynamic fields are inserted directly into markup (e.g., Line 80), which allows HTML injection in email content.
🔒 Proposed fix
const limitWarningTemplate = ({
@@
}) => {
+ const escapeHtml = (unsafe = '') => String(unsafe)
+ .replace(/&/g, '&')
+ .replace(/</g, '<')
+ .replace(/>/g, '>')
+ .replace(/"/g, '"')
+ .replace(/'/g, ''');
+
+ const safeProjectName = escapeHtml(projectName);
+ const safeCurrentUsage = escapeHtml(currentUsage);
+ const safeLimit = escapeHtml(limit);
+ const safePercentage = percentage == null ? '' : escapeHtml(percentage);
@@
- : `You've used <strong>${percentage}%</strong> of your ${resourceLabel.toLowerCase()} limit.`;
+ : `You've used <strong>${safePercentage}%</strong> of your ${resourceLabel.toLowerCase()} limit.`;
@@
- ? `<div class="stat"><span class="stat-label">Current usage</span><span class="stat-value">${currentUsage}</span></div>`
+ ? `<div class="stat"><span class="stat-label">Current usage</span><span class="stat-value">${safeCurrentUsage}</span></div>`
@@
- <div class="stat"><span class="stat-label">Current usage</span><span class="stat-value">${currentUsage}</span></div>
- <div class="stat"><span class="stat-label">Plan limit</span><span class="stat-value">${limit}</span></div>
- <div class="stat"><span class="stat-label">Usage</span><span class="stat-value">${percentage}%</span></div>`;
+ <div class="stat"><span class="stat-label">Current usage</span><span class="stat-value">${safeCurrentUsage}</span></div>
+ <div class="stat"><span class="stat-label">Plan limit</span><span class="stat-value">${safeLimit}</span></div>
+ <div class="stat"><span class="stat-label">Usage</span><span class="stat-value">${safePercentage}%</span></div>`;
@@
- <strong>Project:</strong> ${projectName}<br/>
+ <strong>Project:</strong> ${safeProjectName}<br/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const limitWarningTemplate = ({ | |
| projectName, | |
| resourceType, | |
| currentUsage, | |
| limit, | |
| percentage, | |
| isBYOD, | |
| }) => { | |
| const resourceLabel = resourceType === 'storage' ? 'Storage' : 'Database'; | |
| const resourceIcon = resourceType === 'storage' ? '🗄️' : '🛢️'; | |
| const alertHeadline = isBYOD | |
| ? `Your custom ${resourceLabel.toLowerCase()} alert threshold has been reached.` | |
| : `You've used <strong>${percentage}%</strong> of your ${resourceLabel.toLowerCase()} limit.`; | |
| const usageBlock = isBYOD | |
| ? `<div class="stat"><span class="stat-label">Current usage</span><span class="stat-value">${currentUsage}</span></div>` | |
| : ` | |
| <div class="stat"><span class="stat-label">Current usage</span><span class="stat-value">${currentUsage}</span></div> | |
| <div class="stat"><span class="stat-label">Plan limit</span><span class="stat-value">${limit}</span></div> | |
| <div class="stat"><span class="stat-label">Usage</span><span class="stat-value">${percentage}%</span></div>`; | |
| const actionItems = resourceType === 'storage' | |
| ? `<li>Delete unused files from your project</li> | |
| ${isBYOD ? '<li>Expand your external storage bucket capacity</li>' : '<li>Upgrade your plan for higher limits</li>'} | |
| <li>Review large file uploads and remove duplicates</li>` | |
| : `<li>Remove stale or test documents from your collections</li> | |
| ${isBYOD ? '<li>Scale up your external MongoDB cluster</li>' : '<li>Upgrade your plan for higher limits</li>'} | |
| <li>Optimise your data schema to reduce document size</li>`; | |
| const footerNote = isBYOD | |
| ? 'This is a custom alert you configured in your project settings.' | |
| : 'You will receive at most one alert per threshold every 7 days.'; | |
| return `<!DOCTYPE html> | |
| <html> | |
| <head> | |
| <meta charset="utf-8" /> | |
| <style> | |
| body{font-family:-apple-system,BlinkMacSystemFont,'Segoe UI',Roboto,Helvetica,Arial,sans-serif;background:#ffffff;color:#111111;margin:0;padding:0} | |
| .container{max-width:600px;margin:0 auto;padding:40px 20px} | |
| .logo{margin-bottom:32px;font-weight:800;font-size:24px;letter-spacing:-.03em;color:#111} | |
| .badge{display:inline-block;padding:4px 10px;background:#f59e0b;color:#fff;border-radius:6px;font-size:13px;font-weight:600;margin-bottom:24px} | |
| h1{font-size:26px;font-weight:700;line-height:1.2;margin-bottom:16px;letter-spacing:-.02em} | |
| .alert-box{background:#fffbeb;border-left:4px solid #f59e0b;border-radius:4px;padding:16px 20px;margin:24px 0;font-size:15px;line-height:1.6;color:#333} | |
| .stats{background:#f8f9fa;border-radius:8px;padding:16px 20px;margin:20px 0} | |
| .stat{display:flex;justify-content:space-between;padding:6px 0;border-bottom:1px solid #efefef;font-size:14px} | |
| .stat:last-child{border-bottom:none} | |
| .stat-label{color:#666} | |
| .stat-value{font-weight:600;color:#111} | |
| .actions{font-size:15px;line-height:1.6;color:#444;margin-bottom:24px} | |
| .actions ul{padding-left:20px;margin:12px 0} | |
| .actions li{margin-bottom:8px} | |
| .cta{display:inline-block;background:#111;color:#fff;padding:12px 24px;border-radius:8px;font-weight:600;text-decoration:none;font-size:15px} | |
| .footer{margin-top:64px;padding-top:32px;border-top:1px solid #eee;font-size:13px;color:#888} | |
| .footer p{margin:4px 0} | |
| </style> | |
| </head> | |
| <body> | |
| <div class="container"> | |
| <div class="logo">urBackend</div> | |
| <div class="badge">${resourceIcon} ${resourceLabel} Alert</div> | |
| <h1>Resource Limit Warning</h1> | |
| <div class="alert-box"> | |
| <strong>Project:</strong> ${projectName}<br/> | |
| ${alertHeadline} | |
| </div> | |
| <div class="stats"> | |
| ${usageBlock} | |
| </div> | |
| <div class="actions"> | |
| <strong>What you can do:</strong> | |
| <ul> | |
| ${actionItems} | |
| <li>Adjust alert preferences in your project settings</li> | |
| </ul> | |
| </div> | |
| <a href="https://urbackend.bitbros.in/dashboard" class="cta">Go to Dashboard</a> | |
| <div class="footer"> | |
| <p>${footerNote}</p> | |
| <p>© ${new Date().getFullYear()} urBackend Inc. • Developer platform.</p> | |
| </div> | |
| </div> | |
| </body> | |
| </html>`; | |
| }; | |
| const limitWarningTemplate = ({ | |
| projectName, | |
| resourceType, | |
| currentUsage, | |
| limit, | |
| percentage, | |
| isBYOD, | |
| }) => { | |
| const escapeHtml = (unsafe = '') => String(unsafe) | |
| .replace(/&/g, '&') | |
| .replace(/</g, '<') | |
| .replace(/>/g, '>') | |
| .replace(/"/g, '"') | |
| .replace(/'/g, '''); | |
| const safeProjectName = escapeHtml(projectName); | |
| const safeCurrentUsage = escapeHtml(currentUsage); | |
| const safeLimit = escapeHtml(limit); | |
| const safePercentage = percentage == null ? '' : escapeHtml(percentage); | |
| const resourceLabel = resourceType === 'storage' ? 'Storage' : 'Database'; | |
| const resourceIcon = resourceType === 'storage' ? '🗄️' : '🛢️'; | |
| const alertHeadline = isBYOD | |
| ? `Your custom ${resourceLabel.toLowerCase()} alert threshold has been reached.` | |
| : `You've used <strong>${safePercentage}%</strong> of your ${resourceLabel.toLowerCase()} limit.`; | |
| const usageBlock = isBYOD | |
| ? `<div class="stat"><span class="stat-label">Current usage</span><span class="stat-value">${safeCurrentUsage}</span></div>` | |
| : ` | |
| <div class="stat"><span class="stat-label">Current usage</span><span class="stat-value">${safeCurrentUsage}</span></div> | |
| <div class="stat"><span class="stat-label">Plan limit</span><span class="stat-value">${safeLimit}</span></div> | |
| <div class="stat"><span class="stat-label">Usage</span><span class="stat-value">${safePercentage}%</span></div>`; | |
| const actionItems = resourceType === 'storage' | |
| ? `<li>Delete unused files from your project</li> | |
| ${isBYOD ? '<li>Expand your external storage bucket capacity</li>' : '<li>Upgrade your plan for higher limits</li>'} | |
| <li>Review large file uploads and remove duplicates</li>` | |
| : `<li>Remove stale or test documents from your collections</li> | |
| ${isBYOD ? '<li>Scale up your external MongoDB cluster</li>' : '<li>Upgrade your plan for higher limits</li>'} | |
| <li>Optimise your data schema to reduce document size</li>`; | |
| const footerNote = isBYOD | |
| ? 'This is a custom alert you configured in your project settings.' | |
| : 'You will receive at most one alert per threshold every 7 days.'; | |
| return `<!DOCTYPE html> | |
| <html> | |
| <head> | |
| <meta charset="utf-8" /> | |
| <style> | |
| body{font-family:-apple-system,BlinkMacSystemFont,'Segoe UI',Roboto,Helvetica,Arial,sans-serif;background:`#ffffff`;color:`#111111`;margin:0;padding:0} | |
| .container{max-width:600px;margin:0 auto;padding:40px 20px} | |
| .logo{margin-bottom:32px;font-weight:800;font-size:24px;letter-spacing:-.03em;color:`#111`} | |
| .badge{display:inline-block;padding:4px 10px;background:`#f59e0b`;color:`#fff`;border-radius:6px;font-size:13px;font-weight:600;margin-bottom:24px} | |
| h1{font-size:26px;font-weight:700;line-height:1.2;margin-bottom:16px;letter-spacing:-.02em} | |
| .alert-box{background:`#fffbeb`;border-left:4px solid `#f59e0b`;border-radius:4px;padding:16px 20px;margin:24px 0;font-size:15px;line-height:1.6;color:`#333`} | |
| .stats{background:`#f8f9fa`;border-radius:8px;padding:16px 20px;margin:20px 0} | |
| .stat{display:flex;justify-content:space-between;padding:6px 0;border-bottom:1px solid `#efefef`;font-size:14px} | |
| .stat:last-child{border-bottom:none} | |
| .stat-label{color:`#666`} | |
| .stat-value{font-weight:600;color:`#111`} | |
| .actions{font-size:15px;line-height:1.6;color:`#444`;margin-bottom:24px} | |
| .actions ul{padding-left:20px;margin:12px 0} | |
| .actions li{margin-bottom:8px} | |
| .cta{display:inline-block;background:`#111`;color:`#fff`;padding:12px 24px;border-radius:8px;font-weight:600;text-decoration:none;font-size:15px} | |
| .footer{margin-top:64px;padding-top:32px;border-top:1px solid `#eee`;font-size:13px;color:`#888`} | |
| .footer p{margin:4px 0} | |
| </style> | |
| </head> | |
| <body> | |
| <div class="container"> | |
| <div class="logo">urBackend</div> | |
| <div class="badge">${resourceIcon} ${resourceLabel} Alert</div> | |
| <h1>Resource Limit Warning</h1> | |
| <div class="alert-box"> | |
| <strong>Project:</strong> ${safeProjectName}<br/> | |
| ${alertHeadline} | |
| </div> | |
| <div class="stats"> | |
| ${usageBlock} | |
| </div> | |
| <div class="actions"> | |
| <strong>What you can do:</strong> | |
| <ul> | |
| ${actionItems} | |
| <li>Adjust alert preferences in your project settings</li> | |
| </ul> | |
| </div> | |
| <a href="https://urbackend.bitbros.in/dashboard" class="cta">Go to Dashboard</a> | |
| <div class="footer"> | |
| <p>${footerNote}</p> | |
| <p>© ${new Date().getFullYear()} urBackend Inc. • Developer platform.</p> | |
| </div> | |
| </div> | |
| </body> | |
| </html>`; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/common/src/utils/emailTemplates/limitWarning.js` around lines 15 -
105, The template function limitWarningTemplate inserts unescaped
user-controlled values (projectName, currentUsage, limit, percentage and any
dynamic pieces used in usageBlock, actionItems, footerNote) directly into the
HTML; create or reuse an HTML-escaping helper (e.g., escapeHtml) and apply it to
every interpolated value and any strings composed from them before building
usageBlock, actionItems, alertHeadline and the final returned HTML so raw markup
cannot be injected.
Comment on lines
+86
to
+102
| // Enqueue the email | ||
| await emailQueue.add('limit-warning', { | ||
| ownerEmail, | ||
| projectName: project.name, | ||
| resourceType, | ||
| currentUsage: formatBytes(currentUsage), | ||
| limit: limitBytes != null ? formatBytes(limitBytes) : formatBytes(resourceSettings.absoluteLimit), | ||
| percentage: percentage != null ? Math.round(percentage) : null, | ||
| isBYOD: !!isExternal, | ||
| }); | ||
|
|
||
| // Persist cooldown timestamp | ||
| const updatePath = `lastLimitNotification.${resourceType}.${alertKey}`; | ||
| await Project.updateOne( | ||
| { _id: project._id }, | ||
| { $set: { [updatePath]: new Date() } } | ||
| ); |
Contributor
There was a problem hiding this comment.
Potential race condition between queue add and timestamp persist.
If the emailQueue.add succeeds but Project.updateOne fails, the email is sent but the cooldown timestamp isn't persisted. On the next request, another email could be sent (until updateOne eventually succeeds). This could result in duplicate emails within the cooldown window.
Consider reversing the order or using a transaction, though for notification use cases this may be acceptable as the window is small and failures are rare.
🔧 Alternative: persist timestamp first
+ // Persist cooldown timestamp first to prevent duplicate sends on partial failure
+ const updatePath = `lastLimitNotification.${resourceType}.${alertKey}`;
+ await Project.updateOne(
+ { _id: project._id },
+ { $set: { [updatePath]: new Date() } }
+ );
+
// Enqueue the email
await emailQueue.add('limit-warning', {
ownerEmail,
projectName: project.name,
resourceType,
currentUsage: formatBytes(currentUsage),
limit: limitBytes != null ? formatBytes(limitBytes) : formatBytes(resourceSettings.absoluteLimit),
percentage: percentage != null ? Math.round(percentage) : null,
isBYOD: !!isExternal,
});
- // Persist cooldown timestamp
- const updatePath = `lastLimitNotification.${resourceType}.${alertKey}`;
- await Project.updateOne(
- { _id: project._id },
- { $set: { [updatePath]: new Date() } }
- );
console.log(`[limitNotification] ✅ ${resourceType} alert dispatched for project "${project.name}" (key: ${alertKey})`);This way, if emailQueue.add fails, the timestamp is already set (preventing retries) but no email was sent. This errs on the side of missing an email rather than sending duplicates—typically preferred for alert fatigue reasons.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Enqueue the email | |
| await emailQueue.add('limit-warning', { | |
| ownerEmail, | |
| projectName: project.name, | |
| resourceType, | |
| currentUsage: formatBytes(currentUsage), | |
| limit: limitBytes != null ? formatBytes(limitBytes) : formatBytes(resourceSettings.absoluteLimit), | |
| percentage: percentage != null ? Math.round(percentage) : null, | |
| isBYOD: !!isExternal, | |
| }); | |
| // Persist cooldown timestamp | |
| const updatePath = `lastLimitNotification.${resourceType}.${alertKey}`; | |
| await Project.updateOne( | |
| { _id: project._id }, | |
| { $set: { [updatePath]: new Date() } } | |
| ); | |
| // Persist cooldown timestamp first to prevent duplicate sends on partial failure | |
| const updatePath = `lastLimitNotification.${resourceType}.${alertKey}`; | |
| await Project.updateOne( | |
| { _id: project._id }, | |
| { $set: { [updatePath]: new Date() } } | |
| ); | |
| // Enqueue the email | |
| await emailQueue.add('limit-warning', { | |
| ownerEmail, | |
| projectName: project.name, | |
| resourceType, | |
| currentUsage: formatBytes(currentUsage), | |
| limit: limitBytes != null ? formatBytes(limitBytes) : formatBytes(resourceSettings.absoluteLimit), | |
| percentage: percentage != null ? Math.round(percentage) : null, | |
| isBYOD: !!isExternal, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/common/src/utils/limitNotification.js` around lines 86 - 102, The
current flow can send an email via emailQueue.add and then fail to persist the
cooldown timestamp with Project.updateOne, causing duplicate notifications;
update the implementation in limitNotification.js to persist the cooldown first
(set the updatePath: `lastLimitNotification.${resourceType}.${alertKey}` using
Project.updateOne) and only after a successful update call
emailQueue.add('limit-warning', {...}), or alternatively perform both operations
inside a transaction/atomic operation and rollback the enqueue on update
failure—ensure you reference and handle failures from both emailQueue.add and
Project.updateOne so the cooldown timestamp and sent-state remain consistent.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements a resource limit notification system that alerts users via email when storage or database usage reaches specific thresholds. The changes include updates to the Project model, new backend controllers for managing notification settings, and a frontend configuration form. Review feedback highlights critical bugs in the dashboard regarding unit conversions and incorrect property names, as well as performance concerns and potential runtime errors in the backend logic.
| <div style={{ display: "grid", gap: "10px", gridTemplateColumns: "1fr 1fr" }}> | ||
| <div style={{ padding: "10px", background: "rgba(0,0,0,0.2)", borderRadius: "6px" }}> | ||
| <div style={{ fontSize: "0.85rem", color: "var(--color-text-muted)" }}>Storage Used</div> | ||
| <div style={{ fontSize: "1.1rem", fontWeight: 600 }}>{project?.cachedUsageStats?.storage?.size || project?.storageUsed || 0} MB</div> |
Contributor
There was a problem hiding this comment.
The usage statistics are displayed in MB, but the values project.cachedUsageStats.storage.size and project.storageUsed are stored in bytes in the database. This will result in showing very large, incorrect numbers to the user. You should divide these values by (1024 * 1024) and format them before displaying.
Suggested change
| <div style={{ fontSize: "1.1rem", fontWeight: 600 }}>{project?.cachedUsageStats?.storage?.size || project?.storageUsed || 0} MB</div> | |
| <div style={{ fontSize: "1.1rem", fontWeight: 600 }}>{((project?.cachedUsageStats?.storage?.size || project?.storageUsed || 0) / (1024 * 1024)).toFixed(1)} MB</div> |
| </div> | ||
| <div style={{ padding: "10px", background: "rgba(0,0,0,0.2)", borderRadius: "6px" }}> | ||
| <div style={{ fontSize: "0.85rem", color: "var(--color-text-muted)" }}>Database Used</div> | ||
| <div style={{ fontSize: "1.1rem", fontWeight: 600 }}>{project?.cachedUsageStats?.database?.size || project?.databaseSizeMB || 0} MB</div> |
Contributor
There was a problem hiding this comment.
Similar to the storage usage, the database usage is displayed in MB but the underlying values are in bytes. Additionally, project.databaseSizeMB does not exist on the Project model; the correct property name is databaseUsed. Please apply the unit conversion and fix the property name.
Suggested change
| <div style={{ fontSize: "1.1rem", fontWeight: 600 }}>{project?.cachedUsageStats?.database?.size || project?.databaseSizeMB || 0} MB</div> | |
| <div style={{ fontSize: "1.1rem", fontWeight: 600 }}>{((project?.cachedUsageStats?.database?.size || project?.databaseUsed || 0) / (1024 * 1024)).toFixed(1)} MB</div> |
Comment on lines
+41
to
+48
| project.updateOne({ | ||
| 'cachedUsageStats.database': { | ||
| size: sizeBytes, | ||
| lastCalculated: new Date(), | ||
| }, | ||
| }).catch((err) => { | ||
| console.error('[calculateExternalDbSize] Failed to update cache:', err.message); | ||
| }); |
Contributor
There was a problem hiding this comment.
Using project.updateOne() assumes that the project argument is a Mongoose Document instance. However, in many parts of the application (especially when coming from cache), the project object is a plain JavaScript object. In such cases, project.updateOne will be undefined and throw an error. It is safer to use the Model directly: Project.updateOne({ _id: project._id }, ...) (ensure Project is imported from the models).
Comment on lines
+63
to
+75
| Developer.findById(project.owner).select('email').then(async (owner) => { | ||
| if (owner?.email) { | ||
| const currentUsage = project.resources.db.isExternal | ||
| ? await calculateExternalDbSize(project) | ||
| : (project.databaseUsed || 0) + docSize; | ||
| checkAndNotify({ | ||
| project, | ||
| resourceType: 'database', | ||
| currentUsage, | ||
| ownerEmail: owner.email, | ||
| }).catch((e) => console.error('[data] notification error:', e.message)); | ||
| } | ||
| }).catch(() => {}); // swallow lookup errors |
Contributor
There was a problem hiding this comment.
Performing a Developer.findById query on every single data insertion to retrieve the owner's email adds significant overhead to the Public API, especially for high-traffic projects. Since the owner's email rarely changes, consider caching this lookup or including the owner's email in the cached project object to avoid redundant database round-trips.
| storage: { | ||
| type: storageType, | ||
| thresholds: storageType === "percentage" | ||
| ? storageThresholds.split(",").map(t => parseInt(t.trim())).filter(t => !isNaN(t)) |
Contributor
There was a problem hiding this comment.
The thresholds entered by the user are not validated to be within a reasonable range (e.g., 0 to 100). While the backend might handle these values, it's better to provide immediate feedback or filter out invalid percentages in the frontend to prevent confusing alerts.
thresholds: storageType === "percentage"
? storageThresholds.split(",").map(t => parseInt(t.trim())).filter(t => !isNaN(t) && t >= 0 && t <= 100)
: [],| database: { | ||
| type: dbType, | ||
| thresholds: dbType === "percentage" | ||
| ? dbThresholds.split(",").map(t => parseInt(t.trim())).filter(t => !isNaN(t)) |
Contributor
There was a problem hiding this comment.
The thresholds entered by the user are not validated to be within a reasonable range (e.g., 0 to 100). While the backend might handle these values, it's better to provide immediate feedback or filter out invalid percentages in the frontend to prevent confusing alerts.
thresholds: dbType === "percentage"
? dbThresholds.split(",").map(t => parseInt(t.trim())).filter(t => !isNaN(t) && t >= 0 && t <= 100)
: [],
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request Description
Fixes #47
Implemented a fully fleshed out proactive alert system that accurately scans usages dynamically while avoiding disrupting critical application functionality (Fire-and-forget checks). Supports standard cloud resources alongside fully-configured custom external BYOD pipelines.
What's changed?
calculateExternalDbSize.jscaching utility usingdb.stats().ProjectMonorepo Schema safely carrying thresholds and cooldown rules.limit-warningqueue events using uniquely templatedlimitWarning.js.NotificationSettingsFormandCurrent UsageMB blocks natively.Validation
Summary by CodeRabbit
New Features
Tests