Conversation
📝 WalkthroughWalkthroughThe PR migrates NPC-targeting in notification rules from numeric IDs to string-based names across the entire system, including database migration, API DTOs, service logic, matching algorithms, form validation, and frontend components. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef7d71a16c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| WHERE nr."filters" IS NOT NULL | ||
| AND (nr."filters"::jsonb) ? 'npcId' | ||
| AND (nr."filters" ->> 'npcId')::int = t."npcId"; |
There was a problem hiding this comment.
npcId filters before removing ID matching
This update only converts rows where (filters->>'npcId')::int has a matching Timer.npcId, so any rule pointing to an NPC ID with no current timer row keeps legacy npcId in JSON. After this commit, timer matching only reads npcName/npcNames, so those unmigrated rules become effectively unfiltered and can schedule notifications for unrelated NPCs in the same guild/world.
Useful? React with 👍 / 👎.
| || jsonb_build_object('npcNames', ( | ||
| SELECT jsonb_agg(DISTINCT t."npcName") |
There was a problem hiding this comment.
npcNames: null fallback from broadening rule scope
In the array migration path, jsonb_agg returns NULL when none of the stored IDs resolve to a timer row, and this writes {"npcNames": null} while removing npcIds. The new matcher treats that as no NPC filter (filters.npcNames?.length is falsy), so affected rules match all timer updates instead of none/specific targets.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/api/src/notifications/dto/update-notification-rule.dto.ts (1)
56-67: LGTM with optional suggestion.The DTO correctly updates NPC targeting from numeric IDs to string names with appropriate validators.
Optional: Consider adding
@MaxLength(255, { each: true })tonpcNamesto enforce consistent length limits on array elements, matching the singlenpcNamefield constraint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/notifications/dto/update-notification-rule.dto.ts` around lines 56 - 67, Add per-element length validation to the npcNames array to match npcName by adding a MaxLength decorator with each: true; specifically update the DTO where npcNames is defined (symbol: npcNames) to include `@MaxLength`(255, { each: true }) alongside `@IsString`({ each: true }) so every string in the array is limited to 255 characters like the npcName field.
🤖 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/api/prisma/migrations/20260406023332_migrate_notification_filters_npc_id_to_name/migration.sql`:
- Around line 1-45: The migration currently leaves orphaned npcId/npcIds that
cause rules to match everything; update the migration to (1) make the
npcIds->npcNames aggregation safe by COALESCEing jsonb_agg to an empty array (so
npcNames becomes [] instead of null) or explicitly remove the npcNames key when
no names resolved, (2) add a final cleanup UPDATE that removes any leftover
'npcId' and 'npcIds' keys from "NotificationRule"."filters" (so the app only
sees npcName/npcNames), and (3) add a pre-migration SELECT that identifies/logs
NotificationRule rows where npcId is not found in Timer or where all npcIds fail
to resolve (so those rules can be reviewed); reference the "NotificationRule"
table, "filters" JSON keys 'npcId'/'npcIds' and new keys 'npcName'/'npcNames',
the "Timer" lookup subquery, and the matchesTimerRule behavior when making these
edits.
In `@apps/api/src/notifications/notification-matching.service.ts`:
- Around line 31-42: In matchesTimerRule, parse filters first via
this.parseFilters(filtersValue) instead of returning false immediately when
npcName is null; only require npcName when filters.npcName or filters.npcNames
are present (i.e., if neither NPC filter exists, allow matching even with
npcName null). Normalize comparisons by canonicalizing values (trim and
toLowerCase) for filters.npcName, every entry of filters.npcNames, and the
incoming npcName before comparing with !== or includes so comparisons are case-
and whitespace-insensitive; update the checks around filters.npcName and
filters.npcNames in matchesTimerRule accordingly and reuse parseFilters result.
---
Nitpick comments:
In `@apps/api/src/notifications/dto/update-notification-rule.dto.ts`:
- Around line 56-67: Add per-element length validation to the npcNames array to
match npcName by adding a MaxLength decorator with each: true; specifically
update the DTO where npcNames is defined (symbol: npcNames) to include
`@MaxLength`(255, { each: true }) alongside `@IsString`({ each: true }) so every
string in the array is limited to 255 characters like the npcName field.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 329120ad-46c3-4b7f-8859-e85ea37b6c93
📒 Files selected for processing (15)
apps/api/prisma/migrations/20260406023332_migrate_notification_filters_npc_id_to_name/migration.sqlapps/api/src/notifications/dto/create-notification-rule.dto.tsapps/api/src/notifications/dto/has-at-least-one-npc.validator.tsapps/api/src/notifications/dto/update-notification-rule.dto.tsapps/api/src/notifications/notification-content.service.tsapps/api/src/notifications/notification-job.service.tsapps/api/src/notifications/notification-matching.service.tsapps/api/src/notifications/notification-rule.service.tsapps/api/src/notifications/notifications-events.handler.tsapps/web/src/features/guild-notifications/hooks/use-notification-rule-form.tsapps/web/src/features/guild-notifications/notification-rule-form-page.tsxapps/web/src/features/guild-notifications/utils/notification-rule-form.schema.tsapps/web/src/features/guild-notifications/utils/notification-settings.utils.tsapps/web/src/hooks/api/guilds/use-guild-notifications.tspackages/types/src/common/notifications.types.ts
| -- Migrate NotificationRule filters from npcId/npcIds to npcName/npcNames | ||
| -- using NPC names resolved from the Timer table. | ||
|
|
||
| -- Step 1: Migrate rules with filters->npcId (single NPC) | ||
| -- Find the NPC name from Timer.npc->>'name' matching the npcId. | ||
| UPDATE "NotificationRule" nr | ||
| SET "filters" = ( | ||
| nr."filters"::jsonb | ||
| - 'npcId' | ||
| || jsonb_build_object('npcName', t."npcName") | ||
| )::jsonb | ||
| FROM ( | ||
| SELECT DISTINCT ON (sub."npcId") | ||
| sub."npcId", | ||
| sub."npc" ->> 'name' AS "npcName" | ||
| FROM "Timer" sub | ||
| WHERE sub."npc" ->> 'name' IS NOT NULL | ||
| ORDER BY sub."npcId", sub."updatedAt" DESC | ||
| ) t | ||
| WHERE nr."filters" IS NOT NULL | ||
| AND (nr."filters"::jsonb) ? 'npcId' | ||
| AND (nr."filters" ->> 'npcId')::int = t."npcId"; | ||
|
|
||
| -- Step 2: Migrate rules with filters->npcIds (array of NPC IDs) | ||
| -- For each rule, resolve all NPC IDs to names and build npcNames array. | ||
| UPDATE "NotificationRule" nr | ||
| SET "filters" = ( | ||
| nr."filters"::jsonb | ||
| - 'npcIds' | ||
| || jsonb_build_object('npcNames', ( | ||
| SELECT jsonb_agg(DISTINCT t."npcName") | ||
| FROM jsonb_array_elements_text(nr."filters" -> 'npcIds') AS elem | ||
| JOIN ( | ||
| SELECT DISTINCT ON (sub."npcId") | ||
| sub."npcId", | ||
| sub."npc" ->> 'name' AS "npcName" | ||
| FROM "Timer" sub | ||
| WHERE sub."npc" ->> 'name' IS NOT NULL | ||
| ORDER BY sub."npcId", sub."updatedAt" DESC | ||
| ) t ON t."npcId" = elem::int | ||
| )) | ||
| )::jsonb | ||
| WHERE nr."filters" IS NOT NULL | ||
| AND (nr."filters"::jsonb) ? 'npcIds' | ||
| AND jsonb_array_length(nr."filters" -> 'npcIds') > 0; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are NotificationRule records with npcId/npcIds that won't resolve against Timer
# First, let's find the schema and understand the data model
echo "=== Checking for Timer and NotificationRule models ==="
rg -n "model Timer" -A 20 --type-add 'prisma:*.prisma' --type prisma
echo ""
echo "=== Checking for NotificationRule model ==="
rg -n "model NotificationRule" -A 30 --type-add 'prisma:*.prisma' --type prisma
echo ""
echo "=== Checking how filters are currently read in matching service ==="
rg -nC5 "filters\.npcId|filters\.npcName" --type tsRepository: lootlog/monorepo
Length of output: 11162
🏁 Script executed:
#!/bin/bash
echo "=== Checking for foreign key constraints or relationships ==="
rg -n "npcId|npcName" apps/api/prisma/schema.prisma | head -30
echo ""
echo "=== Searching for other migrations related to npcId/npcName ==="
fd -e sql apps/api/prisma/migrations | xargs grep -l "npcId\|npcName" 2>/dev/null || echo "No other migration files found"
echo ""
echo "=== Checking for validation/logging in notification services ==="
rg -n "npcId|orphan|unresolved" apps/api/src/notifications/ --type ts -i
echo ""
echo "=== Checking the full notification-matching.service.ts to understand matching logic ==="
cat -n apps/api/src/notifications/notification-matching.service.ts | head -60Repository: lootlog/monorepo
Length of output: 5850
🏁 Script executed:
#!/bin/bash
echo "=== Reading the actual migration file ==="
cat -n apps/api/prisma/migrations/20260406023332_migrate_notification_filters_npc_id_to_name/migration.sql
echo ""
echo "=== Checking for any rollback or recovery migrations ==="
find apps/api/prisma/migrations -name "*.sql" -type f | xargs grep -l "npcId\|npcName" 2>/dev/null | sort
echo ""
echo "=== Searching for migration validation or cleanup scripts ==="
find apps -name "*.ts" -o -name "*.js" | xargs grep -l "migrate_notification_filters_npc_id_to_name\|NotificationRule.*filters" 2>/dev/null | head -20Repository: lootlog/monorepo
Length of output: 2962
Migration will silently orphan rules and cause unintended matching behavior.
The migration has two critical data integrity issues:
-
Step 1 (single
npcId): Rules whosenpcIddoesn't exist in theTimertable won't be updated; they retain{ npcId: ... }in filters. SincematchesTimerRuleonly readsnpcNameandnpcNamesfields and returnstruewhen both are absent, these orphaned rules will match all timers instead of none. -
Step 2 (array
npcIds): NPC IDs not found inTimerare silently dropped from the resultingnpcNamesarray. If all IDs fail to resolve,jsonb_aggreturnsNULL, resulting innpcNames: null, which also matches all timers.
Additionally, the migration does not include a cleanup step to remove the unmigrated npcId/npcIds keys, leaving stale data in filters that the new code ignores.
Add a cleanup step at the end of the migration:
Cleanup SQL
-- Remove unmigrated npcId/npcIds keys to prevent silent rule changes
UPDATE "NotificationRule"
SET "filters" = "filters"::jsonb - 'npcId' - 'npcIds'
WHERE "filters" IS NOT NULL
AND ((("filters"::jsonb) ? 'npcId') OR (("filters"::jsonb) ? 'npcIds'));Consider adding a pre-migration query to log/identify rules that won't be migrated, so they can be manually reviewed or handled by application logic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/api/prisma/migrations/20260406023332_migrate_notification_filters_npc_id_to_name/migration.sql`
around lines 1 - 45, The migration currently leaves orphaned npcId/npcIds that
cause rules to match everything; update the migration to (1) make the
npcIds->npcNames aggregation safe by COALESCEing jsonb_agg to an empty array (so
npcNames becomes [] instead of null) or explicitly remove the npcNames key when
no names resolved, (2) add a final cleanup UPDATE that removes any leftover
'npcId' and 'npcIds' keys from "NotificationRule"."filters" (so the app only
sees npcName/npcNames), and (3) add a pre-migration SELECT that identifies/logs
NotificationRule rows where npcId is not found in Timer or where all npcIds fail
to resolve (so those rules can be reviewed); reference the "NotificationRule"
table, "filters" JSON keys 'npcId'/'npcIds' and new keys 'npcName'/'npcNames',
the "Timer" lookup subquery, and the matchesTimerRule behavior when making these
edits.
| matchesTimerRule(filtersValue: Prisma.JsonValue, npcName: string | null) { | ||
| if (!npcName) { | ||
| return false; | ||
| } | ||
|
|
||
| const filters = this.parseFilters(filtersValue); | ||
|
|
||
| if (filters.npcId && filters.npcId !== npcId) { | ||
| if (filters.npcName && filters.npcName !== npcName) { | ||
| return false; | ||
| } | ||
|
|
||
| if (filters.npcIds?.length && !filters.npcIds.includes(npcId)) { | ||
| if (filters.npcNames?.length && !filters.npcNames.includes(npcName)) { |
There was a problem hiding this comment.
Match timer NPCs against canonical names, and only when the rule filters by NPC.
The guard on Lines 31-34 makes matchesTimerRule({}, null) return false, so world-only/all-NPC timer rules stop matching whenever the caller cannot supply npc.name. On top of that, the raw !== / includes() checks are case- and whitespace-sensitive, which will silently miss logically identical names. Parse filters first, only require a name when NPC filters exist, and compare normalized values.
💡 Suggested fix
matchesTimerRule(filtersValue: Prisma.JsonValue, npcName: string | null) {
- if (!npcName) {
- return false;
- }
-
const filters = this.parseFilters(filtersValue);
+ const hasNpcFilter =
+ typeof filters.npcName === "string" ||
+ (filters.npcNames?.length ?? 0) > 0;
+ const normalizeNpcName = (value: string) =>
+ value.trim().toLowerCase().replace(/\s+/g, " ");
+
+ if (!npcName) {
+ return !hasNpcFilter;
+ }
+
+ const normalizedNpcName = normalizeNpcName(npcName);
- if (filters.npcName && filters.npcName !== npcName) {
+ if (
+ typeof filters.npcName === "string" &&
+ normalizeNpcName(filters.npcName) !== normalizedNpcName
+ ) {
return false;
}
- if (filters.npcNames?.length && !filters.npcNames.includes(npcName)) {
+ if (
+ filters.npcNames?.length &&
+ !filters.npcNames.some(
+ (filterNpcName) =>
+ normalizeNpcName(filterNpcName) === normalizedNpcName,
+ )
+ ) {
return false;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/notifications/notification-matching.service.ts` around lines 31
- 42, In matchesTimerRule, parse filters first via
this.parseFilters(filtersValue) instead of returning false immediately when
npcName is null; only require npcName when filters.npcName or filters.npcNames
are present (i.e., if neither NPC filter exists, allow matching even with
npcName null). Normalize comparisons by canonicalizing values (trim and
toLowerCase) for filters.npcName, every entry of filters.npcNames, and the
incoming npcName before comparing with !== or includes so comparisons are case-
and whitespace-insensitive; update the checks around filters.npcName and
filters.npcNames in matchesTimerRule accordingly and reuse parseFilters result.
1 participant
Summary by CodeRabbit