Conversation
📝 WalkthroughWalkthroughThis PR introduces comprehensive test infrastructure and integration test suites for an EAV-based CRM system. It adds a shared test harness factory with identity fixtures and seeding helpers, plus five test suites covering metadata compilation, record CRUD, system adapters, view engine querying, end-to-end workflows, and scaffolded link tests, alongside four sequential specification chunks documenting test requirements. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Reviewer's GuideAdds a comprehensive CRM EAV test harness and multiple high‑level test suites covering view engine behavior, system adapters over native tables, and shared CRM testing utilities/specs, to validate end‑to‑end behavior of the CRM layer and its integration with native data sources. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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
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.
There was a problem hiding this comment.
Pull request overview
This PR introduces a Convex/Vitest integration test harness for the CRM EAV layer and adds a suite of high-level integration tests (metadata compiler, record CRUD, view engine, native system adapters), along with ENG-260 spec/task documentation to describe the intended coverage and architecture touchpoints.
Changes:
- Added shared CRM
convex-testharness utilities (identities + seeding helpers) to support consistent integration testing. - Added new CRM integration test suites for capabilities derivation, record CRUD behavior, view querying/moves, and native-record adapter behavior (plus a skipped links scaffold).
- Added ENG-260 spec docs (chunk contexts + task lists + manifest) to document the test plan and architecture expectations.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| specs/ENG-260/tasks.md | Master task list describing intended test coverage and validation steps. |
| specs/ENG-260/chunks/manifest.md | Chunk breakdown and execution order for ENG-260 work. |
| specs/ENG-260/chunks/chunk-01-helpers-metadata/tasks.md | Task spec for harness + metadata compiler tests. |
| specs/ENG-260/chunks/chunk-01-helpers-metadata/context.md | Context for harness setup, fluent middleware expectations, and audit-log registration. |
| specs/ENG-260/chunks/chunk-02-record-crud/tasks.md | Task spec for record CRUD testing scenarios. |
| specs/ENG-260/chunks/chunk-02-record-crud/context.md | Context on value routing, validation rules, labelValue, and audit actions. |
| specs/ENG-260/chunks/chunk-03-view-engine/tasks.md | Task spec for view engine test scenarios. |
| specs/ENG-260/chunks/chunk-03-view-engine/context.md | Context on view APIs, filters schema/operators, integrity/repair behavior, and kanban moves. |
| specs/ENG-260/chunks/chunk-04-adapters-links-walkthrough/tasks.md | Task spec for system adapter tests, links scaffolding, and walkthrough test. |
| specs/ENG-260/chunks/chunk-04-adapters-links-walkthrough/context.md | Context for system adapter architecture + bootstrap + walkthrough flow. |
| convex/crm/tests/helpers.ts | New shared convex-test harness (audit-log registration) + identities + seed helpers. |
| convex/crm/tests/metadataCompiler.test.ts | Tests for deriveCapabilities and capability lifecycle via mutations. |
| convex/crm/tests/records.test.ts | Record CRUD integration tests across types, validation, soft delete, and org scoping. |
| convex/crm/tests/viewEngine.test.ts | View engine integration tests for table/kanban/calendar + filters + integrity + kanban moves. |
| convex/crm/tests/systemAdapters.test.ts | System adapter tests for resolveColumnPath plus native querying via system bootstrap. |
| convex/crm/tests/walkthrough.test.ts | End-to-end walkthrough test across object/fields/views/records/search/perf check. |
| convex/crm/tests/links.test.ts | Skipped scaffold for future ENG-257 link backend tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
convex/crm/__tests__/walkthrough.test.ts (1)
169-192: Re-query the kanban view after the status change.Step 8 currently proves the row payload changed, but not that the board regrouped. If the kanban aggregation/cache stops reflecting record updates, this walkthrough still passes. Re-run
queryViewRecordsforkanbanViewIdand assert the counts moved fromNewtoContacted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/crm/__tests__/walkthrough.test.ts` around lines 169 - 192, After updating the record via asAdmin(t).mutation(api.crm.records.updateRecord), re-query the kanban view by calling asAdmin(t).query(api.crm.viewQueries.queryViewRecords, { viewDefId: kanbanViewId, limit: 100 }) and assert the kanban buckets reflect the move (e.g., find the bucket for status "New" and "contacted" in the returned rows or grouping metadata and verify the count for "New" decreased and "contacted" increased compared to the pre-update values); use the same approach you used for tableViewId (tableResult/updatedTable) but target kanbanViewId and compare counts to ensure the board regrouped after the status change.convex/crm/__tests__/systemAdapters.test.ts (2)
334-447: Assert the_kinddiscriminator as part of the contract.This parity check only compares keys. If EAV records were accidentally emitted with
_kind: "native"(or vice versa), the test would still pass because_kindis present on both objects. Please assert the expected discriminator values alongside the key-set comparison.💡 Minimal hardening
const eavRecord = eavResult.records[0]; const nativeRecord = nativeResult.records[0]; const eavKeys = Object.keys(eavRecord).sort(); const nativeKeys = Object.keys(nativeRecord).sort(); + expect(eavRecord._kind).toBe("record"); + expect(nativeRecord._kind).toBe("native"); expect(eavKeys).toEqual(nativeKeys);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/crm/__tests__/systemAdapters.test.ts` around lines 334 - 447, The test currently only compares keys; add explicit assertions for the discriminator values by checking eavRecord._kind equals the expected EAV discriminator (e.g., "eav") and nativeRecord._kind equals the expected native discriminator (e.g., "native") before or after the existing key comparisons; locate the assertions around where eavRecord and nativeRecord are defined in the "EAV and native records have identical UnifiedRecord keys" test (references: eavResult, nativeResult, eavRecord, nativeRecord, and api.crm.recordQueries.queryRecords) and add two expect(...) checks that assert the exact _kind string values.
114-320: Add smoke coverage for the otherqueryNativeTablebranches.This suite only exercises the
mortgagespath throughqueryRecords. The remaining switch arms (borrowers,lenders,brokers,deals,obligations) and the unknown-table failure path stay untested, so a typo in one branch can still ship while this suite stays green. A smallit.each(...)smoke suite, or one directqueryNativeTabletest for the default/error branch, would close that gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/crm/__tests__/systemAdapters.test.ts` around lines 114 - 320, The tests only cover the "mortgages" branch of queryNativeTable; add a small smoke test that bootstraps system objects (internal.crm.systemAdapters.bootstrap.bootstrapSystemObjects), then iterates (it.each) over the remaining native table names ("borrowers","lenders","brokers","deals","obligations") to locate each objectDef by name and call api.crm.recordQueries.queryRecords (via asAdmin(t)) with that objectDefId, asserting the call returns without error and yields records of _kind "native" (or at least a records array); also add one test that constructs/queries with a bogus/unknown objectDef (or directly calls queryNativeTable with an invalid table name) and asserts the default/error branch is exercised (throws or returns the expected error) to cover the unknown-table path.convex/crm/__tests__/viewEngine.test.ts (1)
78-85: Consider reducing type assertion verbosity.The test file uses numerous inline type assertions throughout (e.g.,
result as { columns: Array<{...}>, rows: Array<{...}> }). While these assertions work correctly, they're verbose and repetitive. Consider one of these approaches:
- Define shared result types at the top of the file for common response shapes (e.g.,
TableViewResult,KanbanViewResult,CalendarViewResult).- Improve API return type inference by ensuring the production query functions have explicit, exported return types that TypeScript can infer in tests.
- Use type helper utilities to extract and reuse common patterns.
Example refactor:
// At top of file type TableViewResult = { columns: Array<{ name: string; displayOrder: number; isVisible: boolean }>; rows: Array<{ fields: Record<string, unknown> }>; cursor: string | null; totalCount: number; }; // In test const result = await asAdmin(t).query(...) as TableViewResult;This would reduce duplication and improve maintainability.
Also applies to: 120-123, 159-163, 178-182, 224-231, 264-269, 299-305, 361-367, 405-411, 453-456, 497-500, 536-539
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/crm/__tests__/viewEngine.test.ts` around lines 78 - 85, The test uses repetitive inline type assertions (e.g., the local cast around result and destructured columns/rows); define shared response types at the top of convex/crm/__tests__/viewEngine.test.ts (for example TableViewResult, KanbanViewResult, CalendarViewResult matching the shapes used in tests) and replace the inline assertions like "result as { columns: Array<...>; rows: Array<...> }" with those named types, and/or export/consume the real query return types from the production code so tests can use them (update test usages of result, columns, rows and any asAdmin(...).query(...) casts to use the new shared types to remove duplication and verbosity).
🤖 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/__tests__/walkthrough.test.ts`:
- Around line 194-212: The test currently swallows all failures from the
asAdmin(t).query(api.crm.recordQueries.searchRecords, ...) call; replace the
bare catch with either an explicit feature check (e.g., call a helper like
isSearchIndexSupported()/feature flag before running the search assertion) or
inspect the thrown error and only skip when it matches the known "search index
not supported" sentinel (check error.message/code) while rethrowing other errors
so auth, schema, and logic failures surface; update the try/catch around the
search block (the asAdmin(t).query call and subsequent expect(...) checks) to
implement this targeted handling.
---
Nitpick comments:
In `@convex/crm/__tests__/systemAdapters.test.ts`:
- Around line 334-447: The test currently only compares keys; add explicit
assertions for the discriminator values by checking eavRecord._kind equals the
expected EAV discriminator (e.g., "eav") and nativeRecord._kind equals the
expected native discriminator (e.g., "native") before or after the existing key
comparisons; locate the assertions around where eavRecord and nativeRecord are
defined in the "EAV and native records have identical UnifiedRecord keys" test
(references: eavResult, nativeResult, eavRecord, nativeRecord, and
api.crm.recordQueries.queryRecords) and add two expect(...) checks that assert
the exact _kind string values.
- Around line 114-320: The tests only cover the "mortgages" branch of
queryNativeTable; add a small smoke test that bootstraps system objects
(internal.crm.systemAdapters.bootstrap.bootstrapSystemObjects), then iterates
(it.each) over the remaining native table names
("borrowers","lenders","brokers","deals","obligations") to locate each objectDef
by name and call api.crm.recordQueries.queryRecords (via asAdmin(t)) with that
objectDefId, asserting the call returns without error and yields records of
_kind "native" (or at least a records array); also add one test that
constructs/queries with a bogus/unknown objectDef (or directly calls
queryNativeTable with an invalid table name) and asserts the default/error
branch is exercised (throws or returns the expected error) to cover the
unknown-table path.
In `@convex/crm/__tests__/viewEngine.test.ts`:
- Around line 78-85: The test uses repetitive inline type assertions (e.g., the
local cast around result and destructured columns/rows); define shared response
types at the top of convex/crm/__tests__/viewEngine.test.ts (for example
TableViewResult, KanbanViewResult, CalendarViewResult matching the shapes used
in tests) and replace the inline assertions like "result as { columns:
Array<...>; rows: Array<...> }" with those named types, and/or export/consume
the real query return types from the production code so tests can use them
(update test usages of result, columns, rows and any asAdmin(...).query(...)
casts to use the new shared types to remove duplication and verbosity).
In `@convex/crm/__tests__/walkthrough.test.ts`:
- Around line 169-192: After updating the record via
asAdmin(t).mutation(api.crm.records.updateRecord), re-query the kanban view by
calling asAdmin(t).query(api.crm.viewQueries.queryViewRecords, { viewDefId:
kanbanViewId, limit: 100 }) and assert the kanban buckets reflect the move
(e.g., find the bucket for status "New" and "contacted" in the returned rows or
grouping metadata and verify the count for "New" decreased and "contacted"
increased compared to the pre-update values); use the same approach you used for
tableViewId (tableResult/updatedTable) but target kanbanViewId and compare
counts to ensure the board regrouped after the status change.
🪄 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: d44bfe8d-0337-4ed0-8188-1facf6686e7a
📒 Files selected for processing (17)
convex/crm/__tests__/helpers.tsconvex/crm/__tests__/links.test.tsconvex/crm/__tests__/metadataCompiler.test.tsconvex/crm/__tests__/records.test.tsconvex/crm/__tests__/systemAdapters.test.tsconvex/crm/__tests__/viewEngine.test.tsconvex/crm/__tests__/walkthrough.test.tsspecs/ENG-260/chunks/chunk-01-helpers-metadata/context.mdspecs/ENG-260/chunks/chunk-01-helpers-metadata/tasks.mdspecs/ENG-260/chunks/chunk-02-record-crud/context.mdspecs/ENG-260/chunks/chunk-02-record-crud/tasks.mdspecs/ENG-260/chunks/chunk-03-view-engine/context.mdspecs/ENG-260/chunks/chunk-03-view-engine/tasks.mdspecs/ENG-260/chunks/chunk-04-adapters-links-walkthrough/context.mdspecs/ENG-260/chunks/chunk-04-adapters-links-walkthrough/tasks.mdspecs/ENG-260/chunks/manifest.mdspecs/ENG-260/tasks.md
… FieldSeedConfig Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…kthrough test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…nbanRecord Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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 `@convex/crm/__tests__/viewEngine.test.ts`:
- Around line 643-674: The test "gte/lte filter: inclusive numeric range" only
inserts a gte filter (in the t.run insert into viewFilters using operator "gte")
but never exercises lte; either add a second insert that uses operator "lte"
(e.g., insert an lte filter for the same fieldDefId with value
JSON.stringify(200_000) and assert totalCount equals expected single record or
appropriate combined-range count when querying via asAdmin(...
api.crm.viewQueries.queryViewRecords ...)) or rename the test to "gte filter:
inclusive numeric range" to match the current behavior; update assertions
accordingly and keep references to seedLeadFixture, seedRecord, the viewFilters
insert, and the asAdmin query when making the change.
- Around line 884-891: The test is asserting the wrong audit action: when
calling moveKanbanRecord the system emits "crm.record.fieldUpdated" but the test
searches for "crm.record.updated"; update the assertion in the test (around
auditEntries/queryByResource and the updateEntry find) to look for action ===
"crm.record.fieldUpdated" (and keep the same existence/assertion logic) so the
audit lookup matches the actual event emitted by moveKanbanRecord.
In `@convex/crm/__tests__/walkthrough.test.ts`:
- Around line 210-218: The catch block that checks SEARCH_UNSUPPORTED_RE
currently calls return which aborts the test and skips the subsequent step-10
perf assertions; remove the early return in that catch (the block that tests
`error instanceof Error && SEARCH_UNSUPPORTED_RE.test(error.message)`) so it
only logs the warning via console.warn and allows the test to continue executing
the remaining steps (including the step-10 perf block).
🪄 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: e0ca9fc2-78ad-4504-ab98-28a924584067
📒 Files selected for processing (5)
convex/crm/__tests__/helpers.tsconvex/crm/__tests__/records.test.tsconvex/crm/__tests__/systemAdapters.test.tsconvex/crm/__tests__/viewEngine.test.tsconvex/crm/__tests__/walkthrough.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- convex/crm/tests/systemAdapters.test.ts
| it("gte/lte filter: inclusive numeric range", async () => { | ||
| const fixture = await seedLeadFixture(t); | ||
|
|
||
| await seedRecord(t, fixture.objectDefId, { | ||
| company_name: "Exact", | ||
| deal_value: 200_000, | ||
| }); | ||
| await seedRecord(t, fixture.objectDefId, { | ||
| company_name: "Below", | ||
| deal_value: 100_000, | ||
| }); | ||
| await seedRecord(t, fixture.objectDefId, { | ||
| company_name: "Above", | ||
| deal_value: 300_000, | ||
| }); | ||
|
|
||
| await t.run(async (ctx) => { | ||
| await ctx.db.insert("viewFilters", { | ||
| viewDefId: fixture.defaultViewId, | ||
| fieldDefId: fixture.fieldDefs.deal_value, | ||
| operator: "gte", | ||
| value: JSON.stringify(200_000), | ||
| }); | ||
| }); | ||
|
|
||
| const result = await asAdmin(t).query( | ||
| api.crm.viewQueries.queryViewRecords, | ||
| { viewDefId: fixture.defaultViewId, limit: 25 } | ||
| ); | ||
| const { totalCount } = result as { totalCount: number }; | ||
| expect(totalCount).toBe(2); // Exact + Above | ||
| }); |
There was a problem hiding this comment.
gte/lte test currently exercises only gte.
The case name claims both operators, but only a gte filter is inserted/asserted. Either add an lte assertion path or rename this case to avoid false coverage signals.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@convex/crm/__tests__/viewEngine.test.ts` around lines 643 - 674, The test
"gte/lte filter: inclusive numeric range" only inserts a gte filter (in the
t.run insert into viewFilters using operator "gte") but never exercises lte;
either add a second insert that uses operator "lte" (e.g., insert an lte filter
for the same fieldDefId with value JSON.stringify(200_000) and assert totalCount
equals expected single record or appropriate combined-range count when querying
via asAdmin(... api.crm.viewQueries.queryViewRecords ...)) or rename the test to
"gte filter: inclusive numeric range" to match the current behavior; update
assertions accordingly and keep references to seedLeadFixture, seedRecord, the
viewFilters insert, and the asAdmin query when making the change.
| // Audit: verify crm.record.updated event was emitted | ||
| const auditEntries = await t.query( | ||
| components.auditLog.lib.queryByResource, | ||
| { resourceType: "records", resourceId: recordId } | ||
| ); | ||
| const updateEntry = auditEntries.find( | ||
| (e: { action: string }) => e.action === "crm.record.updated" | ||
| ); |
There was a problem hiding this comment.
Audit assertion uses the wrong action name for moveKanbanRecord.
moveKanbanRecord emits crm.record.fieldUpdated, but this test searches for crm.record.updated, so it can fail even when behavior is correct.
Suggested patch
const updateEntry = auditEntries.find(
- (e: { action: string }) => e.action === "crm.record.updated"
+ (e: { action: string }) => e.action === "crm.record.fieldUpdated"
);
expect(updateEntry).toBeDefined();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@convex/crm/__tests__/viewEngine.test.ts` around lines 884 - 891, The test is
asserting the wrong audit action: when calling moveKanbanRecord the system emits
"crm.record.fieldUpdated" but the test searches for "crm.record.updated"; update
the assertion in the test (around auditEntries/queryByResource and the
updateEntry find) to look for action === "crm.record.fieldUpdated" (and keep the
same existence/assertion logic) so the audit lookup matches the actual event
emitted by moveKanbanRecord.
| } catch (error) { | ||
| // Only treat explicit "search not supported" errors as a reason to skip. | ||
| if (error instanceof Error && SEARCH_UNSUPPORTED_RE.test(error.message)) { | ||
| // convex-test may not support search indexes — skip gracefully | ||
| console.warn( | ||
| "Search index not supported in convex-test — skipping search assertion" | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Avoid returning early from the unsupported-search catch path.
At Line 217, return exits the test and skips the step-10 perf block entirely. Prefer warning and continuing so the rest of the walkthrough still runs when search indexes are unavailable.
Suggested patch
} catch (error) {
// Only treat explicit "search not supported" errors as a reason to skip.
if (error instanceof Error && SEARCH_UNSUPPORTED_RE.test(error.message)) {
// convex-test may not support search indexes — skip gracefully
console.warn(
"Search index not supported in convex-test — skipping search assertion"
);
- return;
+ // continue test; only skip the search assertion itself
}
// Unexpected error — surface it instead of silently skipping.
throw error;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@convex/crm/__tests__/walkthrough.test.ts` around lines 210 - 218, The catch
block that checks SEARCH_UNSUPPORTED_RE currently calls return which aborts the
test and skips the subsequent step-10 perf assertions; remove the early return
in that catch (the block that tests `error instanceof Error &&
SEARCH_UNSUPPORTED_RE.test(error.message)`) so it only logs the warning via
console.warn and allows the test to continue executing the remaining steps
(including the step-10 perf block).
eng-260 eng-260 ## Summary by Sourcery Add comprehensive CRM EAV test harness and high-level specification docs, plus new integration tests for the view engine and native system adapters. Documentation: - Add ENG-260 spec and chunk task/context documentation describing CRM test harness, record/view/system adapter behaviors, and planned link tests. Tests: - Introduce shared CRM convex-test harness with identities and seed helpers for objects, fields, and records. - Add metadata compiler tests covering capability derivation and lifecycle on field create/update/deactivate. - Add record CRUD tests validating value routing, validation, soft delete behavior, and audit logging. - Add view engine tests for table, kanban, and calendar views, including filters, pagination, schema, integrity, and Kanban moves. - Add system adapter tests for native table querying, column resolution, and UnifiedRecord parity between EAV and native records. - Add an end-to-end CRM walkthrough test covering object/field setup, views, records, grouping, and search, plus scaffolding for future link tests. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Added comprehensive test suites for CRM record management, including create, update, delete, and query operations across multiple field types. * Added view engine tests validating table, kanban, and calendar view functionality with filtering and sorting. * Added system adapter tests for native table integration and unified record handling. * Added end-to-end integration test covering the complete CRM workflow. * **Documentation** * Added implementation guides and test context documentation for CRM features across four sequential specification chunks. <!-- end of auto-generated comment: release notes by coderabbit.ai -->

eng-260
eng-260
Summary by Sourcery
Add comprehensive CRM EAV test harness and high-level specification docs, plus new integration tests for the view engine and native system adapters.
Documentation:
Tests:
Summary by CodeRabbit
Tests
Documentation