Merged
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @Connorbelez, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR introduces five new Convex CRM modules implementing comprehensive view definition and child entity management: filterOperatorValidation for operator validation, viewDefs for view CRUD operations, viewFields for field properties management, viewFilters for filter management, and viewKanbanGroups for Kanban group operations. Supporting specification and task documentation is included. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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 |
Owner
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![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: 1
🧹 Nitpick comments (2)
convex/crm/viewDefs.ts (1)
245-256: Type annotation forpatchis too narrow.The
patchobject is typed asRecord<string, string | number>, butboundFieldIdis typed asId<"fieldDefs">. While this works at runtime (Convex IDs are strings), the type annotation should be more accurate for TypeScript correctness.🔧 Proposed fix
- const patch: Record<string, string | number> = { + const patch: { + updatedAt: number; + name?: string; + boundFieldId?: typeof args.boundFieldId; + } = { updatedAt: Date.now(), }; if (args.name !== undefined) { patch.name = args.name; } if (args.boundFieldId !== undefined) { patch.boundFieldId = args.boundFieldId; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/crm/viewDefs.ts` around lines 245 - 256, The patch object is currently typed as Record<string, string | number>, which is too narrow because args.boundFieldId has type Id<"fieldDefs">; update the type for patch (the variable named patch used before calling ctx.db.patch) to allow Convex IDs (string) and numbers — e.g., widen it to Record<string, string | number | Id<"fieldDefs">> or use a more permissive type like Record<string, unknown> or a specific union including Id<"fieldDefs"> — then ensure ctx.db.patch(args.viewDefId, patch) still receives the updated typed object; adjust any related local references (args.boundFieldId, args.name) to match the new patch type.convex/crm/viewFields.ts (1)
124-129: Silently ignoring missing field IDs may cause unintended ordering gaps.When
fieldIdscontains IDs that don't have correspondingviewFieldsrecords, they are silently skipped. Additionally, iffieldIdsis a subset of actualviewFields, the omitted fields retain their olddisplayOrdervalues, potentially causing duplicate or inconsistent ordering.Consider either:
- Validating that all provided
fieldIdshave correspondingviewFieldsrecords- Requiring
fieldIdsto include allviewFieldsfor the view- Documenting that this is intentional partial-reorder behavior
🔧 Option 1: Validate all fieldIds exist in viewFields
// Build a lookup from fieldDefId to viewField const fieldToViewField = new Map( viewFields.map((vf) => [vf.fieldDefId, vf]) ); + // Validate all provided fieldIds exist in viewFields + for (const fieldId of args.fieldIds) { + if (!fieldToViewField.has(fieldId)) { + throw new ConvexError( + `Field ${fieldId} is not part of this view's fields` + ); + } + } + // Update displayOrder for each fieldId in the provided order for (let i = 0; i < args.fieldIds.length; i++) { const vf = fieldToViewField.get(args.fieldIds[i]); - if (vf) { - await ctx.db.patch(vf._id, { displayOrder: i }); - } + await ctx.db.patch(vf!._id, { displayOrder: i }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/crm/viewFields.ts` around lines 124 - 129, The code currently skips missing IDs when updating displayOrder (loop over args.fieldIds using fieldToViewField and ctx.db.patch), causing inconsistent ordering; update the handler to validate that every id in args.fieldIds exists in fieldToViewField before patching: compute missingIds = args.fieldIds.filter(id => !fieldToViewField.has(id)) and if missingIds.length > 0 throw a clear error (or return a validation failure) listing missingIds so callers must supply only valid viewField IDs (alternatively enforce that args.fieldIds contains the full set by comparing against the set of viewFields and error if sizes differ).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@convex/crm/viewKanbanGroups.ts`:
- Around line 53-56: The current loop only patches args.groupIds which allows
omitted groups to keep old displayOrder and cause collisions; modify the reorder
so it fetches all existing group IDs for the same board (e.g., query the groups
table using the boardId available in args or ctx), build a canonical ordered
list by taking args.groupIds in order then appending any remaining existing
group IDs (preserving their current relative order), and then iterate that full
list to call ctx.db.patch(...) to set displayOrder sequentially for every group;
update code around args.groupIds and the ctx.db.patch calls to use this full
ordered list.
---
Nitpick comments:
In `@convex/crm/viewDefs.ts`:
- Around line 245-256: The patch object is currently typed as Record<string,
string | number>, which is too narrow because args.boundFieldId has type
Id<"fieldDefs">; update the type for patch (the variable named patch used before
calling ctx.db.patch) to allow Convex IDs (string) and numbers — e.g., widen it
to Record<string, string | number | Id<"fieldDefs">> or use a more permissive
type like Record<string, unknown> or a specific union including Id<"fieldDefs">
— then ensure ctx.db.patch(args.viewDefId, patch) still receives the updated
typed object; adjust any related local references (args.boundFieldId, args.name)
to match the new patch type.
In `@convex/crm/viewFields.ts`:
- Around line 124-129: The code currently skips missing IDs when updating
displayOrder (loop over args.fieldIds using fieldToViewField and ctx.db.patch),
causing inconsistent ordering; update the handler to validate that every id in
args.fieldIds exists in fieldToViewField before patching: compute missingIds =
args.fieldIds.filter(id => !fieldToViewField.has(id)) and if missingIds.length >
0 throw a clear error (or return a validation failure) listing missingIds so
callers must supply only valid viewField IDs (alternatively enforce that
args.fieldIds contains the full set by comparing against the set of viewFields
and error if sizes differ).
🪄 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: 8ec04639-4967-4bff-b0c8-4ecd5de5e336
📒 Files selected for processing (12)
convex/crm/filterOperatorValidation.tsconvex/crm/viewDefs.tsconvex/crm/viewFields.tsconvex/crm/viewFilters.tsconvex/crm/viewKanbanGroups.tsspecs/ENG-252/chunks/chunk-01-viewdefs-core/context.mdspecs/ENG-252/chunks/chunk-01-viewdefs-core/tasks.mdspecs/ENG-252/chunks/chunk-02-child-entities/context.mdspecs/ENG-252/chunks/chunk-02-child-entities/tasks.mdspecs/ENG-252/chunks/manifest.mdspecs/ENG-252/tasks.mdspecs/ENG-252/type-design-review.md
There was a problem hiding this comment.
Pull request overview
Adds CRM “view” infrastructure to Convex (viewDefs + viewFields + viewFilters + kanban groups), plus accompanying ENG-252 spec docs and task/chunk planning artifacts.
Changes:
- Introduces
viewDefsCRUD (create/update/delete/duplicate/list/get) with capability checks and kanban group auto-creation. - Adds child-entity operations for
viewFields,viewFilters, andviewKanbanGroups. - Adds
filterOperatorValidation.tsand extensive ENG-252 specification/task documentation.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
convex/crm/filterOperatorValidation.ts |
FieldType → valid filter-operator mapping helpers. |
convex/crm/viewDefs.ts |
View definition CRUD, capability validation, cascade delete, duplication, and kanban group bootstrapping. |
convex/crm/viewFields.ts |
View column visibility/width updates and reordering. |
convex/crm/viewFilters.ts |
Filter CRUD with operator-vs-fieldType validation. |
convex/crm/viewKanbanGroups.ts |
Kanban group reorder/toggle/list operations. |
specs/ENG-252/type-design-review.md |
Type/invariant review notes and recommended follow-ups. |
specs/ENG-252/tasks.md |
Master task list + quality gate notes. |
specs/ENG-252/chunks/manifest.md |
Chunk breakdown and execution order. |
specs/ENG-252/chunks/chunk-01-viewdefs-core/{context.md,tasks.md} |
Chunk-01 implementation context + checklist. |
specs/ENG-252/chunks/chunk-02-child-entities/{context.md,tasks.md} |
Chunk-02 implementation context + checklist. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ilent failures - Extract validateFieldCapability + createKanbanGroupsFromField helpers (DRY) - Replace silent ?? [] fallbacks with explicit throws in getValidOperators and kanban group creation - Add completeness checks to reorderViewFields and reorderKanbanGroups - Add orgId defense-in-depth filter to listViews query - Add cross-object field validation in setViewFieldVisibility - Add isActive check on fieldDef in addViewFilter - Add name validation (trim, empty, max length) to createView/updateView/duplicateView - Export KANBAN_NO_VALUE_SENTINEL constant, fix patch types to Record<string, unknown> - Remove unnecessary type cast in isValidOperatorForFieldType - Add positive width validation in setViewFieldWidth - Add null guard on post-patch db.get in updateView - Update chunk task checkboxes to reflect completed status Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Mar 30, 2026
Merged
Merged
Connorbelez
added a commit
that referenced
this pull request
Apr 20, 2026
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added comprehensive view management system enabling users to create, update, delete, and duplicate CRM views * Added view field configuration with visibility toggles, custom ordering, and column width adjustments * Added filtering capabilities for views with operator validation * Added Kanban board group management including reordering and collapse state controls <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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
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 by CodeRabbit