feat: Allow multiple lists for Rule Engine #2490#2620
Open
Ahmed-Abdel-karim wants to merge 1 commit intokarakeep-app:mainfrom
Open
feat: Allow multiple lists for Rule Engine #2490#2620Ahmed-Abdel-karim wants to merge 1 commit intokarakeep-app:mainfrom
Ahmed-Abdel-karim wants to merge 1 commit intokarakeep-app:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughConverts single-list rule events to multi-list support: introduces Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Greptile SummaryThis PR upgrades the rule engine so that Confidence Score: 5/5Safe to merge; all remaining findings are P2 style/quality suggestions with no functional or security impact. No P0 or P1 issues found. The two flagged items are a sql.raw anti-pattern (low practical risk given the data types involved) and a redundant no-op DELETE in the migration. Core logic, type safety, ownership checks, and cleanup paths are all correct. packages/trpc/models/lists.ts (sql.raw CASE statement) and packages/db/drizzle/0083_rule_engine_multi_list_support.sql (duplicate DELETE). Important Files Changed
Prompt To Fix All With AIThis is a comment left during a code review.
Path: packages/trpc/models/lists.ts
Line: 537-546
Comment:
**`sql.raw` with string-concatenated CASE statement**
`r.id` is interpolated directly into the CASE expression via `sql.raw`, while `r.event` is escaped only with a single-quote doubling (`replace(/'/g, "''")`). In practice, both fields contain database-generated cuid2 IDs and hardcoded enum strings so injection is not currently possible — but the pattern is fragile. If the data model ever broadens to include free-text, this silently becomes injectable. Drizzle's `sql.raw` is intended for structural SQL tokens, not data values.
A safer alternative loops over the rules individually (each update within the same transaction), or builds fully parameterised per-row updates with `sql` tagged template literals and Drizzle's parameter placeholders, avoiding the hand-rolled CASE block entirely.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/db/drizzle/0083_rule_engine_multi_list_support.sql
Line: 27-33
Comment:
**Duplicate DELETE statement**
This `DELETE FROM ruleEngineRules …` block is identical to the one at lines 1–7 and is a no-op here: the earlier statement already removed every rule where `listId` is `NULL` or `''`, so by the time this executes nothing remains to delete. The redundant statement is harmless but indicates the migration was assembled by concatenating fragments without a final review pass.
How can I resolve this? If you propose a fix, please make it concise.Reviews (6): Last reviewed commit: "feat: Allow multiple lists for Rule Engi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/cli/src/commands/migrate.ts (1)
585-592:⚠️ Potential issue | 🟠 MajorNormalize legacy single-list events before remapping.
This migrator reads rules from another server. If the source still returns the pre-multi-list shape for
addedToList/removedFromList, Line 592 will throw one.listIds.map(...)and skip that rule migration.♻️ Suggested normalization
case "addedToList": - case "removedFromList": - return { ...e, listIds: e.listIds.map(mapList) }; + case "removedFromList": { + const listIds = + "listIds" in e + ? e.listIds + : "listId" in e && e.listId + ? [e.listId] + : []; + return { type: e.type, listIds: listIds.map(mapList) }; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/commands/migrate.ts` around lines 585 - 592, mapEvent currently assumes addedToList/removedFromList events have listIds array and will throw if the legacy single-list shape is present; update mapEvent to normalize legacy shapes before remapping by detecting when e.listIds is missing or not an array and converting the single-list field into an array (e.g., if e.listId or non-array e.listIds exists, produce an array), then call listIds.map(mapList); modify the addedToList/removedFromList branch in mapEvent to perform this normalization so mapList is always called on an array.packages/db/schema.ts (1)
734-746:⚠️ Potential issue | 🟠 MajorAdd cleanup for stale list IDs when lists are deleted.
The foreign key constraint that previously enforced
(userId, listId) → bookmarkListshas been removed to support the JSON array format inlistIds. However, no cleanup logic exists inList.delete()to remove deleted list IDs from rules'listIdsarrays. Rules will retain stale references to deleted lists, causing them to silently fail matching without any error indication, which could confuse users if rules appear configured but never trigger.Add application-level cleanup in
List.delete()to remove deleted list IDs from all user rules'listIdsarrays, or at minimum document this as expected behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/schema.ts` around lines 734 - 746, List.delete() currently removes a list but doesn’t purge its id from rules' JSON listIds, leaving stale references in the ruleEngine.listIds column; update List.delete() to, after deleting the bookmark list, run an update on the ruleEngine rows for that user to remove the deleted listId from the listIds JSON array (either via a DB JSON-array update or by selecting matching rules, filtering out the id in memory, and writing back), targeting the ruleEngine table and its listIds field and ensuring the change is scoped to rl.userId matching the list owner so you don’t affect other users' rules.
🧹 Nitpick comments (1)
packages/trpc/routers/rules.test.ts (1)
341-357: Add a mixed-array ownership case here.This updates the schema usage, but it still only exercises
listIdswith one element. A case like[listId, otherUserListId]would catch regressions where the validator only checks the first ID.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/routers/rules.test.ts` around lines 341 - 357, Add a new assertion in the test "cannot create rule with event on another user's list" that uses a mixed listIds array (e.g., [listId, otherUserListId]) so the validator is exercised for non-first elements; modify the invalidRuleInput.event.listIds to include both a valid own list id and otherUserListId (referencing variables otherUserListId and the current user's list id like listId or a fixture variable), then call api.create(invalidRuleInput) and assert it rejects with the same /List not found/ error to ensure per-id ownership checks are performed.
🤖 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/web/components/dashboard/lists/BookmarkListSelector.tsx`:
- Around line 197-203: The combobox trigger in BookmarkListSelector is a plain
div (the element with role="combobox" and className built via cn) so the
multi-select path is not keyboard-focusable or activatable; make the trigger
keyboard-accessible by replacing the div with a focusable element (e.g., a
button) or adding tabIndex={0} and a keyDown handler that toggles open on
Enter/Space, and ensure the same open state logic and aria-expanded behavior
used by the existing single-select Button is preserved (update the element that
renders the combobox trigger inside BookmarkListSelector to handle onClick plus
onKeyDown and focus styles).
In `@packages/trpc/lib/ruleEngine.ts`:
- Around line 75-84: In doesEventMatchRule, the code currently uses
event.listId.includes(id) causing substring matches; change the check to test
equality by verifying the event's listId is contained in the rule's listIds
array (e.g., use rule.event.listIds.includes(event.listId) or
rule.event.listIds.some(id => id === event.listId)) while keeping the existing
guards (rule.event.type, "listIds" in rule.event, and the
addedToList/removedFromList types).
- Line 272: Remove the leftover debug statement console.log("event", event) from
packages/trpc/lib/ruleEngine.ts; delete that line from the function that handles
incoming events and, if you need non-invasive diagnostics, replace it with a
proper logger call (e.g., processLogger.debug or logger.debug) that avoids
printing sensitive data.
- Line 197: Remove the leftover debug console.log call in ruleEngine.ts by
deleting the console.log("Action", action) statement (inside the rule evaluation
/ action handling code path where the variable action is used); if logging is
required use the project's structured logger at debug level (e.g., replace with
logger.debug(...)) instead of console.log and ensure no other stray console.*
calls remain in the rule engine.
---
Outside diff comments:
In `@apps/cli/src/commands/migrate.ts`:
- Around line 585-592: mapEvent currently assumes addedToList/removedFromList
events have listIds array and will throw if the legacy single-list shape is
present; update mapEvent to normalize legacy shapes before remapping by
detecting when e.listIds is missing or not an array and converting the
single-list field into an array (e.g., if e.listId or non-array e.listIds
exists, produce an array), then call listIds.map(mapList); modify the
addedToList/removedFromList branch in mapEvent to perform this normalization so
mapList is always called on an array.
In `@packages/db/schema.ts`:
- Around line 734-746: List.delete() currently removes a list but doesn’t purge
its id from rules' JSON listIds, leaving stale references in the
ruleEngine.listIds column; update List.delete() to, after deleting the bookmark
list, run an update on the ruleEngine rows for that user to remove the deleted
listId from the listIds JSON array (either via a DB JSON-array update or by
selecting matching rules, filtering out the id in memory, and writing back),
targeting the ruleEngine table and its listIds field and ensuring the change is
scoped to rl.userId matching the list owner so you don’t affect other users'
rules.
---
Nitpick comments:
In `@packages/trpc/routers/rules.test.ts`:
- Around line 341-357: Add a new assertion in the test "cannot create rule with
event on another user's list" that uses a mixed listIds array (e.g., [listId,
otherUserListId]) so the validator is exercised for non-first elements; modify
the invalidRuleInput.event.listIds to include both a valid own list id and
otherUserListId (referencing variables otherUserListId and the current user's
list id like listId or a fixture variable), then call
api.create(invalidRuleInput) and assert it rejects with the same /List not
found/ error to ensure per-id ownership checks are performed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6bb5b629-903b-4733-b2db-4089736792d6
📒 Files selected for processing (15)
apps/cli/src/commands/lists.tsapps/cli/src/commands/migrate.tsapps/web/components/dashboard/lists/BookmarkListSelector.tsxapps/web/components/dashboard/rules/RuleEngineEventSelector.tsxapps/web/components/dashboard/rules/RuleEngineRuleEditor.tsxpackages/db/drizzle/0081_0081_rule_engine_multi_list_support.sql.sqlpackages/db/drizzle/meta/0081_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/schema.tspackages/shared/types/rules.tspackages/shared/utils/listUtils.tspackages/trpc/lib/ruleEngine.tspackages/trpc/models/rules.tspackages/trpc/routers/rules.test.tspackages/trpc/routers/rules.ts
Ahmed-Abdel-karim
force-pushed
the
feat/Allow-multiple-lists-for-Rule-Engine
branch
2 times, most recently
from
c5509b1 to
9b148a8
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/trpc/routers/rules.test.ts (1)
341-357: Add a mixed-ownershiplistIdscase here.This only proves the all-invalid array is rejected. The new regression path is validating just one element or using
some()semantics, which could still let[listId, otherUserListId]through.Test extension
await expect(() => api.create(invalidRuleInput)).rejects.toThrow( /List not found/, ); + + await expect(() => + api.create({ + ...invalidRuleInput, + event: { type: "addedToList", listIds: [listId, otherUserListId] }, + }), + ).rejects.toThrow(/List not found/);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/routers/rules.test.ts` around lines 341 - 357, Add a new assertion inside the test "cannot create rule with event on another user's list" that verifies mixed-ownership listIds are rejected: construct a variant of invalidRuleInput where event.listIds includes both a valid list id (e.g., listId or user-owned list) and otherUserListId (e.g., [listId, otherUserListId]) and call api.create with it, asserting it also rejects with /List not found/; update references to the existing invalidRuleInput and api.create invocation to use this mixed array to ensure the validation checks every element (not just all-invalid).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/db/drizzle/0081_0081_rule_engine_multi_list_support.sql.sql`:
- Around line 18-33: The migration currently only rewrites legacy list events
when json_extract("event",'$.listId') IS NOT NULL, leaving rows with type in
('addedToList','removedFromList') and a missing listId in the old shape; update
the CASE that checks json_extract("event",'$.type') to always rewrite those
types: when type is in ('addedToList','removedFromList') set the event to
json_set(json_remove("event",'$.listId'), '$.listIds',
json_array(json_extract("event",'$.listId')) ) if listId is present, and to
json_set(json_remove("event",'$.listId'), '$.listIds', json_array()) (i.e. an
empty array) when json_extract("event",'$.listId') IS NULL; likewise change the
separate CASE that maps the "listId" column (the CASE around "listId" ->
json_array("listId") / ELSE NULL) to return json_array() instead of NULL for
NULL listId so stored rows always have a listIds array shape.
---
Nitpick comments:
In `@packages/trpc/routers/rules.test.ts`:
- Around line 341-357: Add a new assertion inside the test "cannot create rule
with event on another user's list" that verifies mixed-ownership listIds are
rejected: construct a variant of invalidRuleInput where event.listIds includes
both a valid list id (e.g., listId or user-owned list) and otherUserListId
(e.g., [listId, otherUserListId]) and call api.create with it, asserting it also
rejects with /List not found/; update references to the existing
invalidRuleInput and api.create invocation to use this mixed array to ensure the
validation checks every element (not just all-invalid).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5cf8c33e-26e8-49f8-bc07-9018fc0e5f20
📒 Files selected for processing (15)
apps/cli/src/commands/lists.tsapps/cli/src/commands/migrate.tsapps/web/components/dashboard/lists/BookmarkListSelector.tsxapps/web/components/dashboard/rules/RuleEngineEventSelector.tsxapps/web/components/dashboard/rules/RuleEngineRuleEditor.tsxpackages/db/drizzle/0081_0081_rule_engine_multi_list_support.sql.sqlpackages/db/drizzle/meta/0081_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/schema.tspackages/shared/types/rules.tspackages/shared/utils/listUtils.tspackages/trpc/lib/ruleEngine.tspackages/trpc/models/rules.tspackages/trpc/routers/rules.test.tspackages/trpc/routers/rules.ts
✅ Files skipped from review due to trivial changes (3)
- packages/shared/utils/listUtils.ts
- apps/cli/src/commands/migrate.ts
- packages/db/drizzle/meta/_journal.json
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/trpc/routers/rules.ts
- packages/db/schema.ts
- packages/trpc/lib/ruleEngine.ts
- apps/cli/src/commands/lists.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/trpc/models/lists.ts (2)
478-485: Type annotation is verbose but correct.The explicit
SQLiteTransactiontype with full generic parameters ensures type safety. Consider extracting this to a type alias if used elsewhere in the codebase for cleaner signatures.♻️ Optional: Extract transaction type alias
// Could be defined in a shared types file type DbTransaction = SQLiteTransaction< "sync", RunResult, typeof schema, ExtractTablesWithRelations<typeof schema> >; // Then used as: protected async cleanupRulesAfterListDeletion(tx: DbTransaction) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/models/lists.ts` around lines 478 - 485, The parameter type for cleanupRulesAfterListDeletion is overly verbose; extract the long SQLiteTransaction generic into a reusable type alias (e.g., DbTransaction) in a shared/types or top-of-file location and replace the signature protected async cleanupRulesAfterListDeletion(tx: SQLiteTransaction<...>) with protected async cleanupRulesAfterListDeletion(tx: DbTransaction); ensure the alias uses the exact generics ( "sync", RunResult, typeof schema, ExtractTablesWithRelations<typeof schema> ) so type safety is preserved and update any other functions using the same full transaction type to use the alias too.
548-553: Consider batching updates for better performance.Individual UPDATE statements in a loop may be inefficient if many rules reference the deleted list. For SQLite, this is acceptable for small datasets, but consider a batch approach if performance becomes a concern.
♻️ Alternative: Use raw SQL with CASE for batch update
// If performance becomes an issue with many rules, consider: if (rulesToUpdate.length > 0) { const caseStatements = rulesToUpdate .map((r) => `WHEN id = '${r.id}' THEN '${r.event.replace(/'/g, "''")}'`) .join(" "); await tx.run(sql` UPDATE ${ruleEngineRulesTable} SET event = CASE ${sql.raw(caseStatements)} END WHERE id IN (${sql.join(rulesToUpdate.map(r => r.id), sql`, `)}) `); }Note: This is a micro-optimization and only recommended if profiling shows this as a bottleneck.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/models/lists.ts` around lines 548 - 553, The loop that issues individual UPDATEs for each rule (the for loop iterating over rulesToUpdate calling tx.update(ruleEngineRulesTable).set(...).where(...)) can be batched for better performance; replace the per-item updates with a single batched UPDATE using a CASE expression (or a single UPDATE ... WHERE id IN (...) with appropriate mapping) built from rulesToUpdate, run via tx.run or the query builder, and ensure you escape/parameterize rule.event strings and map ids from rulesToUpdate to avoid SQL injection and preserve correctness.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/db/drizzle/0081_rule_engine_multi_list_support.sql`:
- Line 36: The INSERT from `_backup_ruleEngineActions` into `ruleEngineActions`
will raise PK conflicts because `ruleEngineActions` still contains rows after
`ruleEngineRules` was dropped with PRAGMA foreign_keys=OFF; update the migration
to either DELETE FROM `ruleEngineActions` before running the INSERT from
`_backup_ruleEngineActions` or remove the backup/restore steps entirely if the
data is preserved — locate the statements referencing `ruleEngineRules`,
`ruleEngineActions`, and `_backup_ruleEngineActions` and add a DELETE (or skip
the backup/restore block) to ensure no duplicate keys during the restore.
- Around line 26-30: The migration currently converts NULL listId into an empty
list (CASE ... THEN json_array(json_extract("event", '$.listId')) ELSE
json('[]') END), which will create rules with listIds: [] that fail validation;
change the migration to delete any rules where json_extract("event", '$.listId')
IS NULL (or the JSON path is missing) before performing the transformation so no
rule with a null/missing listId is migrated, e.g., add a DELETE FROM ... WHERE
json_extract("event", '$.listId') IS NULL targeting the same table/rows the
migration transforms (ensure you reference the same table and the "event" column
used by the CASE and the "listId" key) rather than converting them to empty
arrays.
In `@packages/trpc/models/lists.ts`:
- Around line 506-517: The loop over rules currently returns false on JSON.parse
or schema validation failure (see the parsedEvent variable and
zRuleEngineRuleEventSchema.safeParse usage), which aborts cleanup for remaining
rules and yields an inconsistent Promise<void | false> return type; change the
behavior to log the parse/validation error (use the module logger or
console.error) and continue to the next rule instead of returning, and update
the function signature/return path to consistently return Promise<void> (or
throw a single error for total failure if desired) so the function no longer
returns false on individual rule failures.
---
Nitpick comments:
In `@packages/trpc/models/lists.ts`:
- Around line 478-485: The parameter type for cleanupRulesAfterListDeletion is
overly verbose; extract the long SQLiteTransaction generic into a reusable type
alias (e.g., DbTransaction) in a shared/types or top-of-file location and
replace the signature protected async cleanupRulesAfterListDeletion(tx:
SQLiteTransaction<...>) with protected async cleanupRulesAfterListDeletion(tx:
DbTransaction); ensure the alias uses the exact generics ( "sync", RunResult,
typeof schema, ExtractTablesWithRelations<typeof schema> ) so type safety is
preserved and update any other functions using the same full transaction type to
use the alias too.
- Around line 548-553: The loop that issues individual UPDATEs for each rule
(the for loop iterating over rulesToUpdate calling
tx.update(ruleEngineRulesTable).set(...).where(...)) can be batched for better
performance; replace the per-item updates with a single batched UPDATE using a
CASE expression (or a single UPDATE ... WHERE id IN (...) with appropriate
mapping) built from rulesToUpdate, run via tx.run or the query builder, and
ensure you escape/parameterize rule.event strings and map ids from rulesToUpdate
to avoid SQL injection and preserve correctness.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9f84172a-2d7a-4709-981c-52295a236fa5
📒 Files selected for processing (7)
packages/db/drizzle/0081_rule_engine_multi_list_support.sqlpackages/db/drizzle/meta/0081_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/schema.tspackages/trpc/models/lists.tspackages/trpc/models/rules.tspackages/trpc/routers/rules.test.ts
💤 Files with no reviewable changes (1)
- packages/db/schema.ts
✅ Files skipped from review due to trivial changes (1)
- packages/db/drizzle/meta/_journal.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/trpc/routers/rules.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/db/drizzle/0081_rule_engine_multi_list_support.sql`:
- Around line 19-25: The migration is re-inserting action rows from
_backup_ruleEngineActions after deleting invalid list-scoped rules
(ruleEngineRules), which will recreate orphaned actions; modify the restore step
(the INSERT that selects from _backup_ruleEngineActions around the restore lines
and any related SELECT/INSERT logic) to only restore actions whose rule_id
exists in ruleEngineRules (i.e., JOIN or WHERE rule_id IN (SELECT id FROM
ruleEngineRules)), or alternatively DELETE FROM _backup_ruleEngineActions WHERE
rule_id NOT IN (SELECT id FROM ruleEngineRules) before the INSERT so only
surviving rule IDs are reinserted and no orphaned _ruleEngineActions are
restored.
In `@packages/trpc/models/lists.ts`:
- Around line 495-520: The SQL WHERE using json_extract/json_each can error on
malformed JSON; update the query that builds "rules" (uses
ruleEngineRulesTable.event and the json_extract/json_each conditions) to guard
JSON1 calls by checking json_valid(event) before calling json_extract/json_each
(or remove the json_each listIds filtering from SQL and instead select rows that
pass a safe type check and then filter listIds in TypeScript after parsing), so
malformed ruleEngineRules.event rows no longer cause the query to throw and
corrupted events can be handled by the existing JSON.parse and
zRuleEngineRuleEventSchema validation logic.
- Around line 569-582: The transaction callbacks currently use async/await which
breaks atomicity with better-sqlite3; change the callbacks passed to
this.ctx.db.transaction to be synchronous (remove the async keyword and avoid
await inside) so all operations run within the same tx; specifically, update the
block that deletes from bookmarkLists and calls
cleanupRulesAfterListDeletion(tx) (and the similar block at the other
occurrence) so cleanupRulesAfterListDeletion runs synchronously against the
provided tx (or inline its synchronous queries) and all tx.delete/.../.where/...
calls are executed without awaiting to preserve the transaction boundary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b4a5b79b-b51f-406c-9ae6-6aba82836caf
📒 Files selected for processing (2)
packages/db/drizzle/0081_rule_engine_multi_list_support.sqlpackages/trpc/models/lists.ts
1 task
Ahmed-Abdel-karim
force-pushed
the
feat/Allow-multiple-lists-for-Rule-Engine
branch
2 times, most recently
from
f9416b8 to
dd90044
Compare
![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: 088bc7bebe
ℹ️ 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".
| and( | ||
| eq(bookmarkLists.id, this.list.id), | ||
| eq(bookmarkLists.userId, this.ctx.user.id), | ||
| eq(ruleEngineRulesTable.userId, this.ctx.user.id), |
There was a problem hiding this comment.
cleanupRulesAfterListDeletion only scans rules owned by the deleting user (ruleEngineRulesTable.userId = this.ctx.user.id). After this change, collaborators can create list-event rules on shared lists (middleware checks List.fromId, not ownership), so when the owner deletes a shared list, collaborator rules referencing that list are left with dangling event.listIds. Those rules become stale and fail later validations on update because the missing list ID can no longer be resolved.
Useful? React with 👍 / 👎.
| case "addedToList": | ||
| case "removedFromList": | ||
| return { ...e, listId: mapList(e.listId) }; | ||
| return { ...e, listIds: e.listIds.map(mapList) }; |
There was a problem hiding this comment.
remapRuleIds now assumes list events always have listIds, but migrations can read rules from pre-0083 servers that still emit event.listId. In that case e.listIds.map(...) throws, and the per-rule catch path skips the rule, causing list-trigger rules to be dropped during migration. Add a compatibility branch that maps listId when listIds is absent.
Useful? React with 👍 / 👎.
MohamedBassem
requested changes
Apr 16, 2026
Collaborator
MohamedBassem
left a comment
MohamedBassem
left a comment
There was a problem hiding this comment.
This was a pretty big PR and you seemed to covered all the angles for it perfectly. Thanks a lot. Overall, looks good to me, but left some minor comments here and there before we can merge it.
Comment on lines
+38
to
+46
| const zAddedToListRuleEvent = z.object({ | ||
| type: z.literal("addedToList"), | ||
| listIds: z.array(z.string()), | ||
| }); | ||
|
|
||
| const zRemovedFromListRuleEvent = z.object({ | ||
| type: z.literal("removedFromList"), | ||
| listIds: z.array(z.string()), | ||
| }); |
Collaborator
There was a problem hiding this comment.
mind expanding on why we need to create new rule event instead of modifying the existing one? With the database migration it should be safe to just modify the old event? I feel like copying zRuleEngineEventSchema to only include the new events sounds a bit error prone.
Contributor
Author
There was a problem hiding this comment.
Yeah, that makes sense. I think they are two separate concepts that both have the same name (event):
RuleEngineEvent is the concrete event produced by the system, e.g. one bookmark was added to one list, so it carries listId.
RuleEngineRuleEvent is the rule matcher/filter, so e.g. match if the event list is any of these configured lists, so it carries listIds.
So I didn’t want to change all places that produce this event to listIds (expecting listIds and producing listIds) just because there is a change in the rule matcher/filter.
But of course, I understand that this can be error-prone, and I can change/refactor it if you think that’s better.
feat: Allow multiple lists for Rule Engine karakeep-app#2490 feat: Allow multiple lists for Rule Engine karakeep-app#2490
Ahmed-Abdel-karim
force-pushed
the
feat/Allow-multiple-lists-for-Rule-Engine
branch
from
088bc7b to
5f4ed0e
Compare
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
2 participants
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.
Summary
Previously, rule engine events (
addedToList/removedFromList) onlysupported a single list per rule, requiring users to create duplicate
rules for each list. This PR allows multiple lists to be selected for
a single rule. Closes #2490
Changes
Schema
listIdfromruleEngineRulesTableand rely solely onlistIdsstored within theeventpayload(userId, listId) → bookmarkListsthat was enforcing single list ownershipcleanupRulesAfterListDeletionto Remove deleted list IDs fromevent.listIdsand Delete rules when no list IDs remain (i.e. the rule becomes invalid)cleanupRulesAfterListDeletionas part of the list deletion flowMigration
eventJSON to replacelistIdwithlistIdsarrayTypes
zRuleEngineEventSchemainto two separate schemas:zRuleEngineEventSchema— dispatched events, retains singlelistId(what actually happened)zRuleEngineRuleEventSchema— rule filter definition, useslistIdsarray (what the rule matches against)Rule Engine
deepEqualcomparison for list events with a dedicateddoesEventMatchRulefunctionlistIdis included in the rule'slistIdsarrayUI
BookmarkListSelectorto support both single and multi-select modesListNameFromPathas a shared utility replacing inline path formatting that was duplicated across multiple componentsScreenshots
Testing