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, your pull request is larger than the review limit of 150000 diff characters
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 7 minutes and 46 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (98)
📝 WalkthroughWalkthroughCentralizes Convex test module discovery and audit-log component registration, adds backend activity timeline query and types, emits mirrored audit events for link create/delete, and implements frontend LinkedRecordsPanel, AddLinkDialog, FieldDiffDisplay, ActivityTimeline, and an entity icon helper. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as "UI (ActivityTimeline)"
participant Frontend as "Frontend (convex client)"
participant Convex as "Convex getRecordActivity"
participant Audit as "AuditLog component"
participant Users as "Users table"
UI->>Frontend: request getRecordActivity(recordId, recordKind, cursor, limit)
Frontend->>Convex: invoke getRecordActivity(...)
Convex->>Audit: auditLog.queryByResource(resourceType, resourceId...)
Note right of Audit: returns audit entries (possibly multiple resource types)
Convex->>Users: batch lookup actorIds -> user display info
Users-->>Convex: user display data
Audit-->>Convex: audit entries
Convex->>Frontend: merged, deduped, enriched events + continueCursor
Frontend->>UI: render events (actor, description, diff, pagination)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 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.
There was a problem hiding this comment.
Pull request overview
ENG-258 adds CRM “Relations” + “Activity” sidebar panels backed by new Convex activity APIs, and updates Convex tests to avoid import.meta.glob(...) so codegen/deploy workflows aren’t blocked.
Changes:
- Added frontend UI for linked-record management (grouped links, add-link dialog, remove-link confirmation) and an activity timeline with field diffs + pagination.
- Added backend activity types and
getRecordActivityquery; added mirrored audit-log entries for link create/delete so relation changes appear in timelines. - Replaced
import.meta.glob(...)usage in many Convex tests with static module maps + a shared audit-log component registration helper.
Reviewed changes
Copilot reviewed 98 out of 99 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/components/admin/shell/LinkedRecordsPanel.tsx | New Relations panel UI for listing/grouping links + add/remove actions. |
| src/components/admin/shell/AddLinkDialog.tsx | New dialog to search candidate records and create links. |
| src/components/admin/shell/ActivityTimeline.tsx | New Activity panel UI with paging + event rendering. |
| src/components/admin/shell/FieldDiffDisplay.tsx | New compact before/after diff renderer for field updates. |
| src/components/admin/shell/entity-icon.tsx | New Lucide icon resolver for object/entity icons. |
| convex/crm/types.ts | Adds ActivityEvent / ActivityQueryResult types. |
| convex/crm/activityQueries.ts | New getRecordActivity query (pagination, actor enrichment, diff parsing). |
| convex/crm/recordLinks.ts | Logs mirrored link audit events onto both participating entities’ resource streams. |
| convex/test/registerAuditLogComponent.ts | Helper to register the convex-audit-log component in convex-test harnesses. |
| convex/test/moduleMaps.ts | Static module maps used by convex-test in place of import.meta.glob. |
| convex/_generated/api.d.ts | Regenerated Convex API types to include new modules. |
| convex/payments/webhooks/tests/reversalIntegration.test.ts | Switches tests to static module map + new audit log registration helper. |
| convex/payments/transfers/tests/principalReturn.test.ts | Switches tests to static module map + new audit log registration helper. |
| convex/payments/transfers/tests/outboundFlow.integration.test.ts | Switches tests to static module map + new audit log registration helper. |
| convex/payments/transfers/tests/inboundFlow.integration.test.ts | Switches tests to static module map + new audit log registration helper. |
| convex/payments/transfers/tests/handlers.integration.test.ts | Switches tests to static module map + new audit log registration helper. |
| convex/payments/transfers/tests/financialProperties.test.ts | Switches tests to static module map + new audit log registration helper. |
| convex/payments/payout/tests/batchPayout.test.ts | Switches tests to static module map for convex-test modules. |
| convex/payments/payout/tests/adminPayout.test.ts | Switches tests to static module map for convex-test modules. |
| convex/payments/obligations/tests/correctiveObligation.test.ts | Switches tests to static module map + new audit log registration helper. |
| convex/payments/cashLedger/tests/writeOff.test.ts | Switches tests to static module map for convex-test modules. |
| convex/payments/cashLedger/tests/waiver.test.ts | Switches tests to static module map + new audit log registration helper. |
| convex/payments/cashLedger/tests/transferReconciliation.test.ts | Switches tests to static module map + new audit log registration helper. |
| convex/payments/cashLedger/tests/suspenseResolution.test.ts | Switches tests to static module map for convex-test modules. |
| convex/payments/cashLedger/tests/servicingFeeRecognition.test.ts | Switches tests to static module map for convex-test modules. |
| convex/payments/cashLedger/tests/sequenceCounter.test.ts | Switches tests to static module map for convex-test modules. |
| convex/payments/cashLedger/tests/reversalReconciliation.test.ts | Switches tests to static module map for convex-test modules. |
| convex/payments/cashLedger/tests/reversalIntegration.test.ts | Switches tests to static module map + new audit log registration helper. |
| convex/payments/cashLedger/tests/reversalCascade.test.ts | Switches tests to static module map + new audit log registration helper. |
| convex/payments/cashLedger/tests/replayIntegrity.test.ts | Switches tests to static module map for convex-test modules. |
| convex/payments/cashLedger/tests/reconciliationSuite.test.ts | Switches tests to static module map + new audit log registration helper. |
| convex/payments/cashLedger/tests/queries.test.ts | Switches tests to static module map for convex-test modules. |
| convex/payments/cashLedger/tests/postingGroups.test.ts | Switches tests to static module map for convex-test modules. |
| convex/payments/cashLedger/tests/postingGroupIntegration.test.ts | Switches tests to static module map for convex-test modules. |
| convex/payments/cashLedger/tests/postEntry.test.ts | Switches tests to static module map for convex-test modules. |
| convex/payments/cashLedger/tests/paymentReversalIntegration.test.ts | Switches tests to static module map + new audit log registration helper. |
| convex/payments/cashLedger/tests/lenderPayoutPosting.test.ts | Switches tests to static module map for convex-test modules. |
| convex/payments/cashLedger/tests/lenderPayableIntegration.test.ts | Switches tests to static module map for convex-test modules. |
| convex/payments/cashLedger/tests/lenderPayableBalance.test.ts | Switches tests to static module map for convex-test modules. |
| convex/payments/cashLedger/tests/integration.test.ts | Switches tests to static module map for convex-test modules. |
| convex/payments/cashLedger/tests/financialInvariantStress.test.ts | Switches tests to static module map for convex-test modules. |
| convex/payments/cashLedger/tests/financialInvariants.test.ts | Switches tests to static module map for convex-test modules. |
| convex/payments/cashLedger/tests/entryTypes.test.ts | Switches tests to static module map for convex-test modules. |
| convex/payments/cashLedger/tests/e2eLifecycle.test.ts | Switches tests to static module map for convex-test modules. |
| convex/payments/cashLedger/tests/disbursementGate.test.ts | Switches tests to static module map for convex-test modules. |
| convex/payments/cashLedger/tests/dealCashEvents.test.ts | Switches tests to static module map for convex-test modules. |
| convex/payments/cashLedger/tests/corrections.test.ts | Switches tests to static module map for convex-test modules. |
| convex/payments/cashLedger/tests/controlSubaccounts.test.ts | Switches tests to static module map for convex-test modules. |
| convex/payments/cashLedger/tests/constraintsAndBalanceExemption.test.ts | Switches tests to static module map for convex-test modules. |
| convex/payments/cashLedger/tests/chaosTests.test.ts | Switches tests to static module map for convex-test modules. |
| convex/payments/cashLedger/tests/cashReceiptIntegration.test.ts | Switches tests to static module map + new audit log registration helper. |
| convex/payments/cashLedger/tests/cashReceipt.test.ts | Switches tests to static module map + new audit log registration helper. |
| convex/payments/cashLedger/tests/cashApplication.test.ts | Switches tests to static module map for convex-test modules. |
| convex/payments/cashLedger/tests/auditTrail.test.ts | Switches tests to static module map for convex-test modules. |
| convex/payments/tests/generation.test.ts | Switches tests to static module map for convex-test modules. |
| convex/payments/tests/crons.test.ts | Switches tests to static module map + new audit log registration helper. |
| convex/machines/tests/deal.integration.test.ts | Switches tests to static module map + new audit log registration helper. |
| convex/lib/tests/orgScope.transferResolution.test.ts | Switches tests to static module map for convex-test modules. |
| convex/ledger/tests/testUtils.test.ts | Switches tests to static module map for convex-test modules. |
| convex/ledger/tests/sequenceCounter.test.ts | Switches tests to static module map for convex-test modules. |
| convex/ledger/tests/queries.test.ts | Switches tests to static module map for convex-test modules. |
| convex/ledger/tests/postEntry.test.ts | Switches tests to static module map for convex-test modules. |
| convex/ledger/tests/mintAndIssue.test.ts | Switches tests to static module map for convex-test modules. |
| convex/ledger/tests/ledger.test.ts | Switches tests to static module map for convex-test modules. |
| convex/ledger/tests/cursors.test.ts | Switches tests to static module map for convex-test modules. |
| convex/ledger/tests/convenienceMutations.test.ts | Switches tests to static module map for convex-test modules. |
| convex/ledger/tests/bootstrap.test.ts | Switches tests to static module map for convex-test modules. |
| convex/ledger/tests/accounts.test.ts | Switches tests to static module map for convex-test modules. |
| convex/engine/effects/tests/transfer.test.ts | Switches tests to static module map + new audit log registration helper. |
| convex/engine/effects/tests/obligationAccrual.integration.test.ts | Switches tests to static module map for convex-test modules. |
| convex/engine/effects/tests/dealLockingFee.test.ts | Switches tests to static module map for convex-test modules. |
| convex/dispersal/tests/selfHealing.test.ts | Switches tests to static module map + new audit log registration helper. |
| convex/dispersal/tests/reconciliation.test.ts | Switches tests to static module map for convex-test modules. |
| convex/dispersal/tests/integration.test.ts | Switches tests to static module map for convex-test modules. |
| convex/dispersal/tests/disbursementBridge.test.ts | Switches tests to static module map for convex-test modules. |
| convex/dispersal/tests/createDispersalEntries.test.ts | Switches tests to static module map + new audit log registration helper. |
| convex/deals/tests/effects.test.ts | Switches tests to static module map for convex-test modules. |
| convex/deals/tests/dealClosing.test.ts | Switches tests to static module map for convex-test modules. |
| convex/deals/tests/access.test.ts | Switches tests to static module map for convex-test modules. |
| convex/crm/tests/recordLinks.test.ts | Switches tests to static module map + new audit log registration helper. |
| convex/crm/tests/linkTypes.test.ts | Switches tests to static module map + new audit log registration helper. |
| convex/crm/tests/links.test.ts | Replaces skipped-suite empty tests with no-op placeholders to satisfy linting. |
| convex/crm/tests/helpers.ts | Switches CRM test harness to static module map + new audit log registration helper. |
| convex/auth/tests/resourceChecks.test.ts | Switches tests to static module map for convex-test modules. |
| convex/accrual/tests/queryHelpers.test.ts | Switches tests to static module map for convex-test modules. |
| convex/accrual/tests/proration.test.ts | Switches tests to static module map for convex-test modules. |
| convex/accrual/tests/ownershipPeriods.test.ts | Switches tests to static module map for convex-test modules. |
| convex/accrual/tests/accrual.integration.test.ts | Switches tests to static module map for convex-test modules. |
| specs/ENG-258/tasks.md | Task tracking for ENG-258 delivery. |
| specs/ENG-258/chunks/manifest.md | Chunk manifest for ENG-258 planning/execution. |
| specs/ENG-258/chunks/chunk-01-backend-activity/tasks.md | Backend chunk task list. |
| specs/ENG-258/chunks/chunk-01-backend-activity/status.md | Backend chunk completion/status notes. |
| specs/ENG-258/chunks/chunk-01-backend-activity/context.md | Backend chunk context/requirements. |
| specs/ENG-258/chunks/chunk-02-linked-records-panel/tasks.md | Linked-records UI chunk task list. |
| specs/ENG-258/chunks/chunk-02-linked-records-panel/status.md | Linked-records UI chunk completion/status notes. |
| specs/ENG-258/chunks/chunk-02-linked-records-panel/context.md | Linked-records UI chunk context/requirements. |
| specs/ENG-258/chunks/chunk-03-activity-timeline/tasks.md | Activity timeline UI chunk task list. |
| specs/ENG-258/chunks/chunk-03-activity-timeline/status.md | Activity timeline UI chunk completion/status notes. |
| specs/ENG-258/chunks/chunk-03-activity-timeline/context.md | Activity timeline UI chunk context/requirements. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+7
to
+12
| import { convexModules } from "../../../test/moduleMaps"; | ||
| import { getOrCreateCashAccount } from "../../cashLedger/accounts"; | ||
| import { postObligationAccrued } from "../../cashLedger/integrations"; | ||
| import { registerAuditLogComponent } from "../../test/registerAuditLogComponent"; | ||
|
|
||
| const modules = import.meta.glob("/convex/**/*.ts"); | ||
| const modules = convexModules; |
There was a problem hiding this comment.
The registerAuditLogComponent import path is likely incorrect for this file location: ../../test/registerAuditLogComponent resolves under convex/payments/..., but the helper lives at convex/test/registerAuditLogComponent.ts. This will break test compilation.
Update the relative import to point at convex/test/registerAuditLogComponent (consistent with the convexModules import).
![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: 10
🧹 Nitpick comments (11)
convex/test/moduleMaps.ts (1)
501-520: KeepauditTrailModulesderived from the main registry.These entries are duplicated verbatim from
convexModules. If one list changes and the other does not, component registration will fail with a missing-module error that's hard to trace. A smallpickModules(...)helper would keep this to one source of truth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/test/moduleMaps.ts` around lines 501 - 520, auditTrailModules is a verbatim duplicate of convexModules which risks drift; add a small helper (e.g., pickModules(source: ModuleMap, keys: string[] | (k:string)=>boolean): ModuleMap) and use it to derive auditTrailModules from the main convexModules registry (call pickModules(convexModules, k => k.includes("/components/auditTrail/") ) or pass the explicit key list) and replace the hardcoded object with the result of pickModules so there is a single source of truth.convex/test/registerAuditLogComponent.ts (1)
3-43: Avoid hand-maintainingconvex-audit-log's private dist manifest here.This helper now depends on the package's internal
dist/componentfile list, so aconvex-audit-logupdate can break registration without any local code change. If this wrapper has to stay in-repo, I'd add a sync/generation check against the installed package rather than hard-coding the paths. Please verify it against the currently installedconvex-audit-logversion before merging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/test/registerAuditLogComponent.ts` around lines 3 - 43, The current helper hard-codes the convex-audit-log dist/component manifest (the auditLogModules object and the import of auditLogSchema), which can break when the package updates; replace the hard-coded manifest with a generated one by reading the installed package's dist/component directory (e.g., using fs.readdirSync or an npm script at build time) to discover files and build the auditLogModules map dynamically, and add a verification step (a sync check or throw/logging) that compares the runtime/generated keys against the expected files before registering (use the auditLogModules identifier and the auditLogSchema import as the touch points to locate where to plug generation/verification).specs/ENG-258/chunks/chunk-01-backend-activity/tasks.md (1)
7-7: Task T-003 is marked incomplete — verify before merging.The quality gate task (
bun check && bun typecheck && bunx convex codegen) is unchecked while T-001 and T-002 are marked complete. Ensure the quality gate passes before finalizing this chunk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/ENG-258/chunks/chunk-01-backend-activity/tasks.md` at line 7, Task T-003 (the quality gate) is unchecked — run the command sequence "bun check && bun typecheck && bunx convex codegen" locally and fix any lint/type/codegen failures until it completes successfully; update the tasks.md entry for T-003 to mark it complete once the command exits cleanly, and if failures reference specific functions or files from the output, address those errors (type errors, lint violations, or codegen issues) in the corresponding functions/modules before re-running the quality gate.convex/deals/__tests__/access.test.ts (1)
18-19: Stale comment: "Module glob" no longer accurate.The comment on line 18 references "Module glob" but the code now uses a precomputed module map instead of
import.meta.glob. Consider updating the comment to reflect the new approach (e.g., "Module map" or "Shared modules").📝 Suggested comment update
-// ── Module glob ───────────────────────────────────────────────────── +// ── Module map ────────────────────────────────────────────────────── const modules = convexModules;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/deals/__tests__/access.test.ts` around lines 18 - 19, The comment "Module glob" above the const declaration is stale because the code now uses a precomputed module map; update the comment near the const modules = convexModules; line to reflect the new approach (for example "Module map" or "Shared modules") so it accurately describes that modules comes from the precomputed convexModules map.convex/crm/recordLinks.ts (1)
61-75: Pass resolved resource types into the mirror logger.These lines can trigger extra
ctx.db.get()lookups just to recovernativeTableinformation thatcreateLinkalready loaded earlier in the mutation. Letting the caller supplysourceResourceType/targetResourceType(or the loadedobjectDefs) would remove the redundant reads on the create path and keeplogMirrorActivityEvents()focused on emitting audit rows.Also applies to: 94-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/crm/recordLinks.ts` around lines 61 - 75, getEntityAuditResourceType currently re-fetches objectDefs via ctx.db.get to read nativeTable, causing redundant reads; update the API and call sites so the resolved resource type (or the already-loaded objectDef) is passed in from createLink into logMirrorActivityEvents instead of looking it up again. Concretely, change getEntityAuditResourceType to accept an optional sourceResourceType/targetResourceType or an objectDef parameter and use that when provided, then modify createLink and the logMirrorActivityEvents invocation (and the analogous logic around the 94-101 block) to pass the preloaded nativeTable/resource type through so no extra ctx.db.get is performed.src/components/admin/shell/LinkedRecordsPanel.tsx (3)
66-72: Consider memoizing derived data to avoid recalculation on every render.
objectDefMapandtotalLinksare recomputed on every render. While not a performance issue with small datasets, memoizing them would be more idiomatic:💡 Suggested improvement
+ import { useMemo, useState } from "react"; - import { useState } from "react"; - const objectDefMap = new Map( - (objectDefs ?? []).map((objectDef) => [objectDef._id, objectDef]) - ); - const totalLinks = (linkGroups ?? []).reduce( - (total, group) => total + group.links.length, - 0 - ); + const objectDefMap = useMemo( + () => new Map((objectDefs ?? []).map((objectDef) => [objectDef._id, objectDef])), + [objectDefs] + ); + const totalLinks = useMemo( + () => (linkGroups ?? []).reduce((total, group) => total + group.links.length, 0), + [linkGroups] + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/admin/shell/LinkedRecordsPanel.tsx` around lines 66 - 72, The component recomputes objectDefMap and totalLinks on every render; wrap their creation in React.useMemo to memoize results: compute objectDefMap from objectDefs inside a useMemo with [objectDefs] as the dependency array, and compute totalLinks from linkGroups inside a useMemo with [linkGroups] as the dependency array (references: objectDefMap, totalLinks, objectDefs, linkGroups, component LinkedRecordsPanel) so derived data is only recalculated when inputs change.
148-152: Type assertion is unnecessary — ConvexIdtypes are directly comparable.The cast
(ltd.sourceObjectDefId as string) === (objectDefId as string)is not needed. ConvexId<"objectDefs">values can be compared directly with===:♻️ Simplified comparison
- const isSource = - (ltd.sourceObjectDefId as string) === (objectDefId as string); + const isSource = ltd.sourceObjectDefId === objectDefId;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/admin/shell/LinkedRecordsPanel.tsx` around lines 148 - 152, The comparison currently uses unnecessary string casts; update the map over linkTypeDefs in LinkedRecordsPanel to compare ltd.sourceObjectDefId directly to objectDefId (i.e., replace (ltd.sourceObjectDefId as string) === (objectDefId as string) with ltd.sourceObjectDefId === objectDefId) so TypeScript uses the Convex Id<"objectDefs"> equality directly; no other logic changes needed—just remove the type assertions around sourceObjectDefId and objectDefId in that comparison.
211-214:AlertDialogActiononClickcalls an async function without awaiting.Similar to
AddLinkDialog,handleConfirmRemoveis async butonClickdoesn't handle the returned promise. The internal try/catch handles errors, but explicitly voiding the promise communicates intent:💡 Optional improvement
<AlertDialogAction disabled={removing} - onClick={handleConfirmRemove} + onClick={() => void handleConfirmRemove()} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/admin/shell/LinkedRecordsPanel.tsx` around lines 211 - 214, The onClick handler for AlertDialogAction currently calls the async function handleConfirmRemove without handling its returned promise; update the JSX to explicitly void the promise (e.g., onClick={() => void handleConfirmRemove()}) or wrap it in an async arrow and await it (e.g., onClick={async () => await handleConfirmRemove()}) to communicate intent and avoid unhandled promise warnings—follow the same pattern used for AddLinkDialog and ensure the disabled prop (removing) logic remains unchanged.src/components/admin/shell/AddLinkDialog.tsx (1)
210-217: AsynconClickhandler returns a promise that React ignores.
handleSelectRecordisasync, but passing it directly toonClickmeans React receives a Promise that it doesn't await. While this works in practice, unhandled promise rejections in event handlers can be swallowed. The current try/catch inside the function handles errors, so this is not critical, but wrapping in a non-async function is cleaner:💡 Optional improvement
onClick={() => - handleSelectRecord(record._id, record._kind) + void handleSelectRecord(record._id, record._kind) }The
voidoperator explicitly discards the promise, signaling intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/admin/shell/AddLinkDialog.tsx` around lines 210 - 217, In AddLinkDialog's Button where onClick currently receives the async function handleSelectRecord(record._id, record._kind), avoid passing the async function directly so React doesn't get a Promise; wrap the call in a non-async inline handler that invokes the async function with the void operator (e.g., () => void handleSelectRecord(...)) or similar non-async wrapper to explicitly discard the returned promise and preserve existing try/catch behavior in handleSelectRecord and the Button props (disabled={linking}, variant="ghost", key).src/components/admin/shell/FieldDiffDisplay.tsx (1)
78-88: Note:JSON.stringifycomparison may yield false negatives for equivalent objects with different key ordering.
areValuesEqualfalls back toJSON.stringifycomparison, which is order-sensitive. Two semantically equal objects{ a: 1, b: 2 }and{ b: 2, a: 1 }would compare as unequal, potentially showing spurious diffs. This is likely acceptable for audit diffs where the backend serializes consistently, but worth being aware of.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/admin/shell/FieldDiffDisplay.tsx` around lines 78 - 88, The current areValuesEqual function uses JSON.stringify for fallback which is order-sensitive and can mark semantically-equal objects as different; replace that fallback with an order-insensitive deep equality check (e.g., use a stable/normalized stringify that sorts object keys before stringifying, or call a proven deep-equal utility such as lodash/isEqual) so objects like {a:1,b:2} and {b:2,a:1} compare equal; update the function areValuesEqual to attempt Object.is, then perform the normalized deep comparison, and preserve the try/catch to return false on serialization errors.convex/crm/activityQueries.ts (1)
159-166: Fallback to all native types could be inefficient.When
matchingNativeTypesis empty (ID doesn't normalize to any known table), querying all 6 native types may be wasteful. This is a defensive fallback, but consider whether returning an empty result or throwing an error would be more appropriate for an invalidrecordId.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/crm/activityQueries.ts` around lines 159 - 166, The current fallback returns all NATIVE_AUDIT_RESOURCE_TYPES when matchingNativeTypes is empty, which can trigger unnecessary queries; change the behavior to treat an unnormalizable recordId as invalid by returning an empty array (or throwing a validation error) instead of spreading NATIVE_AUDIT_RESOURCE_TYPES. Specifically, modify the code that uses ctx.db.normalizeId and the matchingNativeTypes/ NATIVE_AUDIT_RESOURCE_TYPES branch so that when matchingNativeTypes.length === 0 you return [] (or raise an error) and update any callers to handle an empty result accordingly.
🤖 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/activityQueries.ts`:
- Around line 168-193: loadAuditEntries currently passes the same limit to each
auditLog.queryByResource call which can underfetch after deduplication; update
loadAuditEntries to compensate by either (A) requesting a larger per-resource
buffer (e.g., perResourceLimit = Math.ceil(limit * N) or limit + buffer) when
calling auditLog.queryByResource, or (B) iteratively fetch additional pages from
auditLog.queryByResource for resourceTypes until the deduped map contains at
least the desired count (limit) or all sources are exhausted; reference the
loadAuditEntries function and auditLog.queryByResource to implement the
per-resource buffer or iterative fetching and ensure the final return still
deduplicates and sorts before truncating to the requested limit.
In `@convex/payments/obligations/__tests__/correctiveObligation.test.ts`:
- Line 15: The import for registerAuditLogComponent in
correctiveObligation.test.ts points to "../../test/registerAuditLogComponent"
(resolving to convex/payments/test) instead of the shared helper under
convex/test; update the import to climb one additional directory (use the
relative path that reaches convex/test, e.g.
"../../../test/registerAuditLogComponent") so registerAuditLogComponent resolves
to the shared helper.
In `@convex/payments/transfers/__tests__/financialProperties.test.ts`:
- Line 35: The import for registerAuditLogComponent is using the wrong relative
path and resolves to convex/payments/test instead of the shared helper; update
the import in convex/payments/transfers/__tests__/financialProperties.test.ts to
point to the shared helper by changing the relative path from
"../../test/registerAuditLogComponent" to
"../../../test/registerAuditLogComponent" so the registerAuditLogComponent
symbol imports from convex/test/registerAuditLogComponent.
In `@convex/payments/transfers/__tests__/principalReturn.test.ts`:
- Line 37: The import for registerAuditLogComponent in principalReturn.test.ts
is one "../" too shallow and points to convex/payments/test; update the import
path for registerAuditLogComponent to traverse three levels back to the shared
test helpers (use the ../../../test/registerAuditLogComponent path) so the test
resolves the helper from convex/test instead of convex/payments/test.
In `@convex/payments/webhooks/__tests__/reversalIntegration.test.ts`:
- Line 20: The import for registerAuditLogComponent in
reversalIntegration.test.ts points to the wrong relative location; update the
import so it resolves to the shared test helper module (the
registerAuditLogComponent module in the repository-level test helpers one
directory above the payments package) so all calls to registerAuditLogComponent
in this file succeed; locate the import line that currently references
"../../test/registerAuditLogComponent" and change it to reference the correct
relative path that reaches the top-level test helper module, then run the test
suite to confirm the 10 usages of registerAuditLogComponent work.
In `@specs/ENG-258/chunks/chunk-01-backend-activity/status.md`:
- Line 7: Update the status entry to reflect that the implementation uses
cursor-based pagination rather than offset pagination: change the text "offset
pagination" in specs/ENG-258/chunks/chunk-01-backend-activity/status.md to
"cursor-based pagination" (or mention continueCursor/isDone), referencing the
ActivityQueryResult type in convex/crm/types.ts so the doc matches the
implemented continueCursor/isDone pagination semantics.
In `@specs/ENG-258/chunks/chunk-02-linked-records-panel/context.md`:
- Around line 6-19: The spec shows using TanStack Query integration
(useSuspenseQuery + convexQuery from `@convex-dev/react-query`) but the components
LinkedRecordsPanel.tsx, AddLinkDialog.tsx, and ActivityTimeline.tsx currently
import/use useQuery from 'convex/react'; fix by either updating the spec to
reflect the implemented pattern or refactor the components to the spec: replace
useQuery usages with useSuspenseQuery, import convexQuery and call
convexQuery(api.crm.linkQueries.getLinkedRecords, { ... }) (or the equivalent
API calls used elsewhere) and ensure import lines and query argument shapes
match the spec; update any type/useEffect/hooks that depend on the current
useQuery behavior to match suspense-based semantics.
In `@src/components/admin/shell/ActivityTimeline.tsx`:
- Line 176: The code destructures Icon, accentClassName, and label from
EVENT_STYLES[event.eventType] without guarding against unknown event.eventType
values; add a runtime guard in ActivityTimeline.tsx to handle missing map
entries by checking EVENT_STYLES[event.eventType] and falling back to a default
style object (e.g., a default Icon, accentClassName and label) before
destructuring, or use a conditional that assigns EVENT_STYLES[event.eventType]
|| DEFAULT_EVENT_STYLE to ensure Icon, accentClassName, and label are always
defined.
- Around line 82-103: The merge effect can receive stale pages when
recordId/recordKind change because the first useEffect clears state but
in-flight queries may still resolve; add a stable context check (e.g., a ref
like activeRecordKey or a requestCounter) that you set inside the reset effect
(when setting cursor null / events [] / nextCursor / isDone) and include that
key in the second useEffect’s dependency and validation before calling
setEvents/setNextCursor/setIsDone so you only merge pages whose page.contextKey
=== activeRecordKey; update the code that feeds/returns page (or attach the key
to the response) and use mergeActivityEvents only when the active key matches to
prevent merging stale results into the new record’s timeline.
---
Nitpick comments:
In `@convex/crm/activityQueries.ts`:
- Around line 159-166: The current fallback returns all
NATIVE_AUDIT_RESOURCE_TYPES when matchingNativeTypes is empty, which can trigger
unnecessary queries; change the behavior to treat an unnormalizable recordId as
invalid by returning an empty array (or throwing a validation error) instead of
spreading NATIVE_AUDIT_RESOURCE_TYPES. Specifically, modify the code that uses
ctx.db.normalizeId and the matchingNativeTypes/ NATIVE_AUDIT_RESOURCE_TYPES
branch so that when matchingNativeTypes.length === 0 you return [] (or raise an
error) and update any callers to handle an empty result accordingly.
In `@convex/crm/recordLinks.ts`:
- Around line 61-75: getEntityAuditResourceType currently re-fetches objectDefs
via ctx.db.get to read nativeTable, causing redundant reads; update the API and
call sites so the resolved resource type (or the already-loaded objectDef) is
passed in from createLink into logMirrorActivityEvents instead of looking it up
again. Concretely, change getEntityAuditResourceType to accept an optional
sourceResourceType/targetResourceType or an objectDef parameter and use that
when provided, then modify createLink and the logMirrorActivityEvents invocation
(and the analogous logic around the 94-101 block) to pass the preloaded
nativeTable/resource type through so no extra ctx.db.get is performed.
In `@convex/deals/__tests__/access.test.ts`:
- Around line 18-19: The comment "Module glob" above the const declaration is
stale because the code now uses a precomputed module map; update the comment
near the const modules = convexModules; line to reflect the new approach (for
example "Module map" or "Shared modules") so it accurately describes that
modules comes from the precomputed convexModules map.
In `@convex/test/moduleMaps.ts`:
- Around line 501-520: auditTrailModules is a verbatim duplicate of
convexModules which risks drift; add a small helper (e.g., pickModules(source:
ModuleMap, keys: string[] | (k:string)=>boolean): ModuleMap) and use it to
derive auditTrailModules from the main convexModules registry (call
pickModules(convexModules, k => k.includes("/components/auditTrail/") ) or pass
the explicit key list) and replace the hardcoded object with the result of
pickModules so there is a single source of truth.
In `@convex/test/registerAuditLogComponent.ts`:
- Around line 3-43: The current helper hard-codes the convex-audit-log
dist/component manifest (the auditLogModules object and the import of
auditLogSchema), which can break when the package updates; replace the
hard-coded manifest with a generated one by reading the installed package's
dist/component directory (e.g., using fs.readdirSync or an npm script at build
time) to discover files and build the auditLogModules map dynamically, and add a
verification step (a sync check or throw/logging) that compares the
runtime/generated keys against the expected files before registering (use the
auditLogModules identifier and the auditLogSchema import as the touch points to
locate where to plug generation/verification).
In `@specs/ENG-258/chunks/chunk-01-backend-activity/tasks.md`:
- Line 7: Task T-003 (the quality gate) is unchecked — run the command sequence
"bun check && bun typecheck && bunx convex codegen" locally and fix any
lint/type/codegen failures until it completes successfully; update the tasks.md
entry for T-003 to mark it complete once the command exits cleanly, and if
failures reference specific functions or files from the output, address those
errors (type errors, lint violations, or codegen issues) in the corresponding
functions/modules before re-running the quality gate.
In `@src/components/admin/shell/AddLinkDialog.tsx`:
- Around line 210-217: In AddLinkDialog's Button where onClick currently
receives the async function handleSelectRecord(record._id, record._kind), avoid
passing the async function directly so React doesn't get a Promise; wrap the
call in a non-async inline handler that invokes the async function with the void
operator (e.g., () => void handleSelectRecord(...)) or similar non-async wrapper
to explicitly discard the returned promise and preserve existing try/catch
behavior in handleSelectRecord and the Button props (disabled={linking},
variant="ghost", key).
In `@src/components/admin/shell/FieldDiffDisplay.tsx`:
- Around line 78-88: The current areValuesEqual function uses JSON.stringify for
fallback which is order-sensitive and can mark semantically-equal objects as
different; replace that fallback with an order-insensitive deep equality check
(e.g., use a stable/normalized stringify that sorts object keys before
stringifying, or call a proven deep-equal utility such as lodash/isEqual) so
objects like {a:1,b:2} and {b:2,a:1} compare equal; update the function
areValuesEqual to attempt Object.is, then perform the normalized deep
comparison, and preserve the try/catch to return false on serialization errors.
In `@src/components/admin/shell/LinkedRecordsPanel.tsx`:
- Around line 66-72: The component recomputes objectDefMap and totalLinks on
every render; wrap their creation in React.useMemo to memoize results: compute
objectDefMap from objectDefs inside a useMemo with [objectDefs] as the
dependency array, and compute totalLinks from linkGroups inside a useMemo with
[linkGroups] as the dependency array (references: objectDefMap, totalLinks,
objectDefs, linkGroups, component LinkedRecordsPanel) so derived data is only
recalculated when inputs change.
- Around line 148-152: The comparison currently uses unnecessary string casts;
update the map over linkTypeDefs in LinkedRecordsPanel to compare
ltd.sourceObjectDefId directly to objectDefId (i.e., replace
(ltd.sourceObjectDefId as string) === (objectDefId as string) with
ltd.sourceObjectDefId === objectDefId) so TypeScript uses the Convex
Id<"objectDefs"> equality directly; no other logic changes needed—just remove
the type assertions around sourceObjectDefId and objectDefId in that comparison.
- Around line 211-214: The onClick handler for AlertDialogAction currently calls
the async function handleConfirmRemove without handling its returned promise;
update the JSX to explicitly void the promise (e.g., onClick={() => void
handleConfirmRemove()}) or wrap it in an async arrow and await it (e.g.,
onClick={async () => await handleConfirmRemove()}) to communicate intent and
avoid unhandled promise warnings—follow the same pattern used for AddLinkDialog
and ensure the disabled prop (removing) logic remains unchanged.
🪄 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: 13129806-4edd-4b0f-bef0-79234bc93242
⛔ Files ignored due to path filters (1)
convex/_generated/api.d.tsis excluded by!**/_generated/**
📒 Files selected for processing (98)
convex/accrual/__tests__/accrual.integration.test.tsconvex/accrual/__tests__/ownershipPeriods.test.tsconvex/accrual/__tests__/proration.test.tsconvex/accrual/__tests__/queryHelpers.test.tsconvex/auth/__tests__/resourceChecks.test.tsconvex/crm/__tests__/helpers.tsconvex/crm/__tests__/linkTypes.test.tsconvex/crm/__tests__/links.test.tsconvex/crm/__tests__/recordLinks.test.tsconvex/crm/activityQueries.tsconvex/crm/recordLinks.tsconvex/crm/types.tsconvex/deals/__tests__/access.test.tsconvex/deals/__tests__/dealClosing.test.tsconvex/deals/__tests__/effects.test.tsconvex/dispersal/__tests__/createDispersalEntries.test.tsconvex/dispersal/__tests__/disbursementBridge.test.tsconvex/dispersal/__tests__/integration.test.tsconvex/dispersal/__tests__/reconciliation.test.tsconvex/dispersal/__tests__/selfHealing.test.tsconvex/engine/effects/__tests__/dealLockingFee.test.tsconvex/engine/effects/__tests__/obligationAccrual.integration.test.tsconvex/engine/effects/__tests__/transfer.test.tsconvex/ledger/__tests__/accounts.test.tsconvex/ledger/__tests__/bootstrap.test.tsconvex/ledger/__tests__/convenienceMutations.test.tsconvex/ledger/__tests__/cursors.test.tsconvex/ledger/__tests__/ledger.test.tsconvex/ledger/__tests__/mintAndIssue.test.tsconvex/ledger/__tests__/postEntry.test.tsconvex/ledger/__tests__/queries.test.tsconvex/ledger/__tests__/sequenceCounter.test.tsconvex/ledger/__tests__/testUtils.test.tsconvex/lib/__tests__/orgScope.transferResolution.test.tsconvex/machines/__tests__/deal.integration.test.tsconvex/payments/__tests__/crons.test.tsconvex/payments/__tests__/generation.test.tsconvex/payments/cashLedger/__tests__/auditTrail.test.tsconvex/payments/cashLedger/__tests__/cashApplication.test.tsconvex/payments/cashLedger/__tests__/cashReceipt.test.tsconvex/payments/cashLedger/__tests__/cashReceiptIntegration.test.tsconvex/payments/cashLedger/__tests__/chaosTests.test.tsconvex/payments/cashLedger/__tests__/constraintsAndBalanceExemption.test.tsconvex/payments/cashLedger/__tests__/controlSubaccounts.test.tsconvex/payments/cashLedger/__tests__/corrections.test.tsconvex/payments/cashLedger/__tests__/dealCashEvents.test.tsconvex/payments/cashLedger/__tests__/disbursementGate.test.tsconvex/payments/cashLedger/__tests__/e2eLifecycle.test.tsconvex/payments/cashLedger/__tests__/entryTypes.test.tsconvex/payments/cashLedger/__tests__/financialInvariantStress.test.tsconvex/payments/cashLedger/__tests__/financialInvariants.test.tsconvex/payments/cashLedger/__tests__/integration.test.tsconvex/payments/cashLedger/__tests__/lenderPayableBalance.test.tsconvex/payments/cashLedger/__tests__/lenderPayableIntegration.test.tsconvex/payments/cashLedger/__tests__/lenderPayoutPosting.test.tsconvex/payments/cashLedger/__tests__/paymentReversalIntegration.test.tsconvex/payments/cashLedger/__tests__/postEntry.test.tsconvex/payments/cashLedger/__tests__/postingGroupIntegration.test.tsconvex/payments/cashLedger/__tests__/postingGroups.test.tsconvex/payments/cashLedger/__tests__/queries.test.tsconvex/payments/cashLedger/__tests__/reconciliationSuite.test.tsconvex/payments/cashLedger/__tests__/replayIntegrity.test.tsconvex/payments/cashLedger/__tests__/reversalCascade.test.tsconvex/payments/cashLedger/__tests__/reversalIntegration.test.tsconvex/payments/cashLedger/__tests__/reversalReconciliation.test.tsconvex/payments/cashLedger/__tests__/sequenceCounter.test.tsconvex/payments/cashLedger/__tests__/servicingFeeRecognition.test.tsconvex/payments/cashLedger/__tests__/suspenseResolution.test.tsconvex/payments/cashLedger/__tests__/transferReconciliation.test.tsconvex/payments/cashLedger/__tests__/waiver.test.tsconvex/payments/cashLedger/__tests__/writeOff.test.tsconvex/payments/obligations/__tests__/correctiveObligation.test.tsconvex/payments/payout/__tests__/adminPayout.test.tsconvex/payments/payout/__tests__/batchPayout.test.tsconvex/payments/transfers/__tests__/financialProperties.test.tsconvex/payments/transfers/__tests__/handlers.integration.test.tsconvex/payments/transfers/__tests__/inboundFlow.integration.test.tsconvex/payments/transfers/__tests__/outboundFlow.integration.test.tsconvex/payments/transfers/__tests__/principalReturn.test.tsconvex/payments/webhooks/__tests__/reversalIntegration.test.tsconvex/test/moduleMaps.tsconvex/test/registerAuditLogComponent.tsspecs/ENG-258/chunks/chunk-01-backend-activity/context.mdspecs/ENG-258/chunks/chunk-01-backend-activity/status.mdspecs/ENG-258/chunks/chunk-01-backend-activity/tasks.mdspecs/ENG-258/chunks/chunk-02-linked-records-panel/context.mdspecs/ENG-258/chunks/chunk-02-linked-records-panel/status.mdspecs/ENG-258/chunks/chunk-02-linked-records-panel/tasks.mdspecs/ENG-258/chunks/chunk-03-activity-timeline/context.mdspecs/ENG-258/chunks/chunk-03-activity-timeline/status.mdspecs/ENG-258/chunks/chunk-03-activity-timeline/tasks.mdspecs/ENG-258/chunks/manifest.mdspecs/ENG-258/tasks.mdsrc/components/admin/shell/ActivityTimeline.tsxsrc/components/admin/shell/AddLinkDialog.tsxsrc/components/admin/shell/FieldDiffDisplay.tsxsrc/components/admin/shell/LinkedRecordsPanel.tsxsrc/components/admin/shell/entity-icon.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/admin/shell/ActivityTimeline.tsx (1)
74-113:⚠️ Potential issue | 🟠 MajorKey the local timeline state by record; this ref check never rejects anything.
Line 88 stores the current
recordId/recordKind, and Lines 101-104 compare that ref straight back to the same current props. Because the reset effect runs before the merge effect, this “stale response” guard is effectively a no-op. The old record’seventscan still flash until the reset lands, and this check would not stop an older page if one is ever surfaced here.💡 Simple way to make record changes atomic
-export function ActivityTimeline({ - recordId, - recordKind, -}: ActivityTimelineProps) { +export function ActivityTimeline(props: ActivityTimelineProps) { + return ( + <ActivityTimelineInner + key={`${props.recordKind}:${props.recordId}`} + {...props} + /> + ); +} + +function ActivityTimelineInner({ + recordId, + recordKind, +}: ActivityTimelineProps) {Once the component remounts per record, the
activeRecordRef/reset dance can be removed and the local pagination state starts cleanly every time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/admin/shell/ActivityTimeline.tsx` around lines 74 - 113, The current stale-response guard using activeRecordRef is ineffective; instead key the local timeline state by record so updates for one record cannot overwrite another: remove activeRecordRef and its comparison in the page effect, change the events state to a map keyed by `${recordId}:${recordKind}` and update usages of setEvents and mergeActivityEvents to read/merge/write into eventsMap[key], and keep the reset effect to clear/set the entry for the new key (via setEvents(prev => ({ ...prev, [key]: [] }))). Update the page effect to only update eventsMap[key] and setNextCursor/setIsDone for that key so pagination and merges are atomic per record (refer to activeRecordRef, setEvents, mergeActivityEvents, and the two useEffect blocks for exact locations).
🧹 Nitpick comments (2)
src/components/admin/shell/AddLinkDialog.tsx (2)
159-168: Consider addingaria-labelto the search input for accessibility.The search input has a visual icon and placeholder, but screen readers benefit from an explicit
aria-labeldescribing the input's purpose.♿ Proposed accessibility improvement
<Input + aria-label={`Search ${candidateLabel.toLowerCase()}`} className="pl-9" disabled={isNativeSearch || linking} onChange={(e) => setSearchQuery(e.target.value)} placeholder={`Search ${candidateLabel.toLowerCase()}...`} value={searchQuery} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/admin/shell/AddLinkDialog.tsx` around lines 159 - 168, The search Input lacks an explicit aria-label for screen readers; update the Input component (the one with className="pl-9" and props disabled={isNativeSearch || linking}, onChange={(e) => setSearchQuery(e.target.value)}, placeholder={`Search ${candidateLabel.toLowerCase()}...`} and value={searchQuery}) to include an appropriate aria-label such as aria-label={`Search ${candidateLabel}`} (or a similarly descriptive string) so assistive technologies can identify the field despite the visual icon.
144-145: Minor:isSearchingmay show spinner whilecandidateObjectDefis loading.When
candidateObjectDefis stillundefined(loading), the search query is skipped, makingsearchResults === undefined. If the user has typed something,isSearchingbecomestrue, showing the spinner even though we're waiting for the object definition, not search results.This is acceptable UX (still a loading state), but for more precise feedback:
♻️ Optional: More precise loading state
const isSearching = - debouncedQuery.trim().length > 0 && searchResults === undefined; + debouncedQuery.trim().length > 0 && + candidateObjectDef !== undefined && + candidateObjectDef.isSystem !== true && + searchResults === undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/admin/shell/AddLinkDialog.tsx` around lines 144 - 145, isSearching currently becomes true whenever debouncedQuery has text and searchResults is undefined, which also occurs while candidateObjectDef is still loading; update the condition so the spinner only shows when a query exists, searchResults are undefined, and candidateObjectDef has already loaded (i.e., require candidateObjectDef !== undefined) — modify the isSearching expression (referenced symbol: isSearching) to include candidateObjectDef in the check along with debouncedQuery and searchResults.
🤖 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/activityQueries.ts`:
- Around line 250-254: Normalize and validate the incoming optional limit and
cursor before they are used to build the page window: parse cursor to an integer
(reject non-numeric or negative values), clamp cursor to a reasonable MAX_CURSOR
(e.g. 10_000) and clamp limit to a MIN_LIMIT of 1 and a sensible MAX_LIMIT (e.g.
100) so limit=0, negative, fractional or absurdly large values cannot flow into
slice() or queryByResource() or produce a perpetual continueCursor="0". Apply
these checks immediately after the .input() values are read (before the logic
that computes the page window/continueCursor and before any scaling of backend
fetch size), and return a validation error for malformed inputs or silently cap
them to the defined bounds as appropriate.
---
Duplicate comments:
In `@src/components/admin/shell/ActivityTimeline.tsx`:
- Around line 74-113: The current stale-response guard using activeRecordRef is
ineffective; instead key the local timeline state by record so updates for one
record cannot overwrite another: remove activeRecordRef and its comparison in
the page effect, change the events state to a map keyed by
`${recordId}:${recordKind}` and update usages of setEvents and
mergeActivityEvents to read/merge/write into eventsMap[key], and keep the reset
effect to clear/set the entry for the new key (via setEvents(prev => ({ ...prev,
[key]: [] }))). Update the page effect to only update eventsMap[key] and
setNextCursor/setIsDone for that key so pagination and merges are atomic per
record (refer to activeRecordRef, setEvents, mergeActivityEvents, and the two
useEffect blocks for exact locations).
---
Nitpick comments:
In `@src/components/admin/shell/AddLinkDialog.tsx`:
- Around line 159-168: The search Input lacks an explicit aria-label for screen
readers; update the Input component (the one with className="pl-9" and props
disabled={isNativeSearch || linking}, onChange={(e) =>
setSearchQuery(e.target.value)}, placeholder={`Search
${candidateLabel.toLowerCase()}...`} and value={searchQuery}) to include an
appropriate aria-label such as aria-label={`Search ${candidateLabel}`} (or a
similarly descriptive string) so assistive technologies can identify the field
despite the visual icon.
- Around line 144-145: isSearching currently becomes true whenever
debouncedQuery has text and searchResults is undefined, which also occurs while
candidateObjectDef is still loading; update the condition so the spinner only
shows when a query exists, searchResults are undefined, and candidateObjectDef
has already loaded (i.e., require candidateObjectDef !== undefined) — modify the
isSearching expression (referenced symbol: isSearching) to include
candidateObjectDef in the check along with debouncedQuery and searchResults.
🪄 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: be5d1e27-fed1-4ce5-9bcf-d71669e68191
📒 Files selected for processing (9)
convex/crm/activityQueries.tsconvex/payments/obligations/__tests__/correctiveObligation.test.tsconvex/payments/transfers/__tests__/financialProperties.test.tsconvex/payments/transfers/__tests__/handlers.integration.test.tsconvex/payments/transfers/__tests__/inboundFlow.integration.test.tsconvex/payments/transfers/__tests__/principalReturn.test.tsconvex/payments/webhooks/__tests__/reversalIntegration.test.tssrc/components/admin/shell/ActivityTimeline.tsxsrc/components/admin/shell/AddLinkDialog.tsx
✅ Files skipped from review due to trivial changes (1)
- convex/payments/webhooks/tests/reversalIntegration.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- convex/payments/transfers/tests/handlers.integration.test.ts
- convex/payments/transfers/tests/principalReturn.test.ts
- convex/payments/transfers/tests/financialProperties.test.ts
- convex/payments/transfers/tests/inboundFlow.integration.test.ts
- convex/payments/obligations/tests/correctiveObligation.test.ts
Connorbelez
force-pushed
the
Connorbelez/eng-258-linked-records
branch
from
e2d9d3c to
9d6f152
Compare
This was referenced Apr 9, 2026
Merged
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.
eng-258
responding to feedback
Summary by CodeRabbit
New Features
Documentation
Chores