Updated project files#164
Conversation
Changes made
|
@Malcolm-Wander Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
📝 WalkthroughWalkthroughNotifications now includes Sequelize/Postgres setup, service startup database authentication, a ChangesNotification preferences storage
Sequence Diagram(s)sequenceDiagram
participant NotificationsService
participant Sequelize
participant PostgresDatabase
participant HttpServer
NotificationsService->>Sequelize: connectDb()
Sequelize->>PostgresDatabase: authenticate()
PostgresDatabase-->>Sequelize: connection result
Sequelize-->>NotificationsService: success or error
NotificationsService->>HttpServer: start after connection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. 🔧 OpenGrep (1.23.0)apps/backend/notifications/src/index.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.12][ERROR]: unable to find a config; path apps/backend/notifications/package.json┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.13][ERROR]: unable to find a config; path Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@apps/backend/notifications/src/db.ts`:
- Line 6: The databaseUrl fallback in db.ts currently masks missing
configuration by silently connecting to a local Postgres instance with embedded
credentials. Update the DATABASE_URL handling in the db.ts initialization so the
fallback is only allowed for local development/test, and make it fail fast in
deployed/non-local environments when DATABASE_URL is absent. Use the existing
databaseUrl constant and the surrounding db connection setup as the place to
enforce this, and document that DATABASE_URL is now required outside local dev.
- Around line 12-13: Validate the Sequelize pool settings in db.ts before
building the config: read DATABASE_POOL_MIN and DATABASE_POOL_MAX, reject blank
strings, NaN, negative values, and any case where the parsed min is greater than
max, then fail fast at startup with a clear error message. Update the config
construction around the pool settings so Sequelize only receives validated
numeric values, and keep the check close to the existing pool config symbols
`min` and `max` for easy location.
In `@apps/backend/notifications/src/notificationPreferences.ts`:
- Around line 23-39: The upsertUserPreferences function is overwriting omitted
channels with defaults instead of preserving existing settings. Update this
function to load the current NotificationPreferences row for the given userId,
merge the incoming Partial<ChannelPreferences> with the existing values, and
then upsert the combined result so partial updates only change specified fields.
Keep the final return shape based on the saved record and use the existing
upsertUserPreferences and NotificationPreferences symbols to make the change
easy to locate.
🪄 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 Plus
Run ID: d30c7564-b2dd-4891-89d2-f82f5c7f28ff
📒 Files selected for processing (6)
apps/backend/notifications/package.jsonapps/backend/notifications/src/db.tsapps/backend/notifications/src/index.tsapps/backend/notifications/src/models/NotificationPreferences.tsapps/backend/notifications/src/notificationPreferences.tsdatabase/migrations/004_notification_preferences.sql
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@apps/backend/notifications/package.json`:
- Around line 28-34: The devDependencies block in package.json has a duplicate
`@types/ws` entry with conflicting versions, so remove one of the entries and keep
a single consistent `@types/ws` declaration. Make sure the remaining dependency
version is the one intended for the notifications package, and verify the
manifest is valid with no repeated keys.
🪄 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 Plus
Run ID: 5db8ac10-e763-45f6-ad00-f6c07023a204
📒 Files selected for processing (2)
apps/backend/notifications/package.jsonapps/backend/notifications/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/backend/notifications/src/index.ts
| "devDependencies": { | ||
| "@types/jsonwebtoken": "^9.0.10", | ||
| "@types/node": "^22.0.0", | ||
| "@types/validator": "^13.15.10", | ||
| "@types/ws": "^8.18.1", | ||
| "@types/web-push": "^3.6.4", | ||
| "@types/ws": "^8.5.0", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove the duplicate @types/ws entry.
package.json declares @types/ws twice with different versions. That makes dependency resolution ambiguous, and many tools will either reject the manifest or silently keep only the last value.
Suggested fix
"devDependencies": {
"`@types/jsonwebtoken`": "^9.0.10",
"`@types/node`": "^22.0.0",
"`@types/validator`": "^13.15.10",
"`@types/ws`": "^8.18.1",
"`@types/web-push`": "^3.6.4",
- "`@types/ws`": "^8.5.0",
"tsx": "^4.19.0",
"typescript": "^5.7.0",
"vitest": "^2.1.9"📝 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.
| "devDependencies": { | |
| "@types/jsonwebtoken": "^9.0.10", | |
| "@types/node": "^22.0.0", | |
| "@types/validator": "^13.15.10", | |
| "@types/ws": "^8.18.1", | |
| "@types/web-push": "^3.6.4", | |
| "@types/ws": "^8.5.0", | |
| "devDependencies": { | |
| "`@types/jsonwebtoken`": "^9.0.10", | |
| "`@types/node`": "^22.0.0", | |
| "`@types/validator`": "^13.15.10", | |
| "`@types/ws`": "^8.18.1", | |
| "`@types/web-push`": "^3.6.4", |
🧰 Tools
🪛 Biome (2.5.0)
[error] 32-32: The key @types/ws was already declared.
(lint/suspicious/noDuplicateObjectKeys)
🤖 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 `@apps/backend/notifications/package.json` around lines 28 - 34, The
devDependencies block in package.json has a duplicate `@types/ws` entry with
conflicting versions, so remove one of the entries and keep a single consistent
`@types/ws` declaration. Make sure the remaining dependency version is the one
intended for the notifications package, and verify the manifest is valid with no
repeated keys.
Source: Linters/SAST tools
|
Resolve the conflicts |
2 participants
Changes made
Closed #135
Summary
Type of change
Test plan
pnpm typecheckpnpm testChecklist
Summary by CodeRabbit
New Features
notification_preferencestable to persist per-user preference defaults.Bug Fixes