Persist signal entity links for People reconciliation - #839
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Warning Review limit reached
More reviews will be available in 14 minutes and 12 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR implements signal-to-person entity linking: signals extract and merge person mentions from input text, the system persists these links to a database table, resolves ambiguous names when people are classified or synced, and displays resolved links in the signal detail UI. The feature spans schema, business logic, workflows, APIs, and UI components. ChangesSignal entity linking
🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe31f7c807
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+104
to
+109
| const persisted = await step.run("persist entity links", () => | ||
| replaceSignalEntityLinks(db, { | ||
| candidates, | ||
| clerkOrgId, | ||
| signalId, | ||
| }) |
There was a problem hiding this comment.
When a classified signal remains visibilityScope: "user", shouldIndexSignalEntities still lets the workflow reach this new persist step because it only excludes needs_review; this writes rows into the org-scoped lightfast_org_signal_entity_links table. Those rows are later scanned by org-level reconciliation (reconcileSignalEntityLinksForPeople filters only by clerkOrgId), so private/user-scoped signal mentions become part of the shared org person-link graph. Guard this persist step to team-visible signals or store/enforce link visibility before writing.
Useful? React with 👍 / 👎.
![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: 3
🧹 Nitpick comments (2)
apps/app/src/app/(internal)/api/internal/mcp/signals/get/route.ts (1)
44-59: ⚖️ Poor tradeoffConsider parallel fetching for signal and entity links.
Same opportunity as in the ORPC router: the signal and entity links queries execute sequentially. Fetching them concurrently would reduce latency:
await assertHostedMcpOrgAccess(db, { orgId: parsed.data.actor.orgId, userId: parsed.data.actor.userId, }); - const signal = await getVisibleSignalByPublicId(db, { - publicId: parsed.data.id, - clerkOrgId: parsed.data.actor.orgId, - createdByUserId: parsed.data.actor.userId, - }); - - if (!signal) { - return jsonError("not_found", "Signal not found.", 404); - } - - const entityLinks = await listSignalEntityLinksForSignal(db, { - clerkOrgId: parsed.data.actor.orgId, - signalId: signal.publicId, - }); + const [signal, entityLinks] = await Promise.all([ + getVisibleSignalByPublicId(db, { + publicId: parsed.data.id, + clerkOrgId: parsed.data.actor.orgId, + createdByUserId: parsed.data.actor.userId, + }), + listSignalEntityLinksForSignal(db, { + clerkOrgId: parsed.data.actor.orgId, + signalId: parsed.data.id, + }), + ]); + + if (!signal) { + return jsonError("not_found", "Signal not found.", 404); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/app/src/app/`(internal)/api/internal/mcp/signals/get/route.ts around lines 44 - 59, The signal and entityLinks queries are being executed sequentially which increases latency; update the handler to fetch the signal and listSignalEntityLinksForSignal concurrently (use Promise.all or parallel awaits) by initiating both promises (the call that yields `signal` and the call to `listSignalEntityLinksForSignal(db, { clerkOrgId: parsed.data.actor.orgId, signalId: signalIdOrPublicId })`) before awaiting results, then wait for both to resolve and pass the resolved `signal` and `entityLinks` into `getSignalOutput.parse` and the Response; ensure you preserve the existing variables (`signal`, `entityLinks`, `db`, `parsed.data.actor.orgId`) and any required typing/validation after both promises settle.api/app/src/orpc/router/signals.ts (1)
57-67: ⚖️ Poor tradeoffConsider parallel fetching for signal and entity links.
The signal and entity links queries execute sequentially, adding their latencies together. Since both are independent DB reads scoped to the same org, you could fetch them concurrently:
- const signal = await getVisibleSignalByPublicId(db, { - publicId: getInput.id, - clerkOrgId: context.auth.identity.orgId, - createdByUserId: context.auth.identity.userId, - }); - - if (!signal) { - throw new ORPCError("NOT_FOUND", { - message: "Signal not found.", - }); - } - - const entityLinks = await listSignalEntityLinksForSignal(db, { - clerkOrgId: context.auth.identity.orgId, - signalId: signal.publicId, - }); + const signal = await getVisibleSignalByPublicId(db, { + publicId: getInput.id, + clerkOrgId: context.auth.identity.orgId, + createdByUserId: context.auth.identity.userId, + }); + + if (!signal) { + throw new ORPCError("NOT_FOUND", { + message: "Signal not found.", + }); + } + + const entityLinks = await listSignalEntityLinksForSignal(db, { + clerkOrgId: context.auth.identity.orgId, + signalId: signal.publicId, + });Wait, that's the same code. Let me fix:
- const signal = await getVisibleSignalByPublicId(db, { - publicId: getInput.id, - clerkOrgId: context.auth.identity.orgId, - createdByUserId: context.auth.identity.userId, - }); - - if (!signal) { - throw new ORPCError("NOT_FOUND", { - message: "Signal not found.", - }); - } - - const entityLinks = await listSignalEntityLinksForSignal(db, { - clerkOrgId: context.auth.identity.orgId, - signalId: signal.publicId, - }); + const [signal, entityLinks] = await Promise.all([ + getVisibleSignalByPublicId(db, { + publicId: getInput.id, + clerkOrgId: context.auth.identity.orgId, + createdByUserId: context.auth.identity.userId, + }), + listSignalEntityLinksForSignal(db, { + clerkOrgId: context.auth.identity.orgId, + signalId: getInput.id, + }), + ]); + + if (!signal) { + throw new ORPCError("NOT_FOUND", { + message: "Signal not found.", + }); + }This saves one round-trip's worth of latency.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/app/src/orpc/router/signals.ts` around lines 57 - 67, The signal fetch (the code that produces the local variable signal) and the call to listSignalEntityLinksForSignal are independent and should be executed in parallel to avoid serialized DB latency; refactor so both promises are started before awaiting (e.g., assign the signal fetch promise and the listSignalEntityLinksForSignal(...) promise to variables, await them with Promise.all, then use the resolved values to build the return object), keeping references to the existing identifiers signal and entityLinks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/app/src/app/`(app)/(pending-not-allowed)/[slug]/(workspace)/signals/_components/signal-detail-sheet.tsx:
- Around line 29-34: SignalDetailSheet currently sets slug = "" which causes
SignalDetailContent to build broken hrefs like `/${slug}/people?person=${id}`
when slug is empty; make slug required instead of defaulting to "" by removing
the default value and updating the prop type (ensure callers pass a non-null
slug) so SignalDetailContent and any link construction always receive a valid
workspace slug (update SignalDetailSheet prop signature and any call sites to
provide the slug), or alternatively add a guard in SignalDetailContent to
suppress or fallback on rendering entity links when slug === "" (prefer option
1: require slug).
In `@db/app/src/utils/signal-entity-links.ts`:
- Around line 246-269: The loop over unresolvedLinks calls
resolveSignalEntityLinkRecord for each link, causing hundreds of serial DB
queries; instead, gather all unique identity keys and display names from
unresolvedLinks, run a single query against the People table (using inArray) to
fetch matching people, build a lookup map keyed by the identity key/display
name, then iterate unresolvedLinks and resolve each link from that map
(replacing calls to resolveSignalEntityLinkRecord), keeping the existing update
logic that sets resolvedAt/resolvedPersonId and uses getRowsAffected on the
update result.
---
Nitpick comments:
In `@api/app/src/orpc/router/signals.ts`:
- Around line 57-67: The signal fetch (the code that produces the local variable
signal) and the call to listSignalEntityLinksForSignal are independent and
should be executed in parallel to avoid serialized DB latency; refactor so both
promises are started before awaiting (e.g., assign the signal fetch promise and
the listSignalEntityLinksForSignal(...) promise to variables, await them with
Promise.all, then use the resolved values to build the return object), keeping
references to the existing identifiers signal and entityLinks.
In `@apps/app/src/app/`(internal)/api/internal/mcp/signals/get/route.ts:
- Around line 44-59: The signal and entityLinks queries are being executed
sequentially which increases latency; update the handler to fetch the signal and
listSignalEntityLinksForSignal concurrently (use Promise.all or parallel awaits)
by initiating both promises (the call that yields `signal` and the call to
`listSignalEntityLinksForSignal(db, { clerkOrgId: parsed.data.actor.orgId,
signalId: signalIdOrPublicId })`) before awaiting results, then wait for both to
resolve and pass the resolved `signal` and `entityLinks` into
`getSignalOutput.parse` and the Response; ensure you preserve the existing
variables (`signal`, `entityLinks`, `db`, `parsed.data.actor.orgId`) and any
required typing/validation after both promises settle.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ca71d8eb-e1bf-4286-9501-1b6002d5a31b
⛔ Files ignored due to path filters (3)
db/app/src/migrations/0025_whole_mac_gargan.sqlis excluded by!db/**/migrations/**db/app/src/migrations/meta/0025_snapshot.jsonis excluded by!db/**/migrations/**db/app/src/migrations/meta/_journal.jsonis excluded by!db/**/migrations/**
📒 Files selected for processing (34)
ai/src/__tests__/signal-entity-linker/extract.test.tsai/src/signal-entity-linker/extract.tsapi/app/src/__tests__/entity-index-workflow.test.tsapi/app/src/__tests__/people-workflow.test.tsapi/app/src/__tests__/signal-orpc.test.tsapi/app/src/__tests__/team-member-people-sync.test.tsapi/app/src/__tests__/workspace-signals-router.test.tsapi/app/src/inngest/workflow/classify-people.tsapi/app/src/inngest/workflow/index-signal-entities.tsapi/app/src/orpc/router/signals.tsapi/app/src/router/(pending-not-allowed)/workspace-signals.tsapi/app/src/services/team-members/people-sync.tsapps/app/src/__tests__/app/(app)/(pending-not-allowed)/[slug]/signals-client.test.tsxapps/app/src/__tests__/app/api/internal/mcp-signals-route.test.tsapps/app/src/app/(app)/(pending-not-allowed)/[slug]/(workspace)/signals/_components/signal-detail-content.test.tsxapps/app/src/app/(app)/(pending-not-allowed)/[slug]/(workspace)/signals/_components/signal-detail-content.tsxapps/app/src/app/(app)/(pending-not-allowed)/[slug]/(workspace)/signals/_components/signal-detail-sheet.tsxapps/app/src/app/(app)/(pending-not-allowed)/[slug]/(workspace)/signals/_components/signals-client.tsxapps/app/src/app/(internal)/api/internal/mcp/signals/get/route.tsapps/mcp/src/__tests__/app-signal-intake.test.tsapps/mcp/src/__tests__/audit.test.tsapps/mcp/src/__tests__/e2e-oauth-mcp.test.tsapps/mcp/src/__tests__/tools.test.tsapps/mcp/src/tools/execute.tsdb/app/src/__tests__/schema-conventions.test.tsdb/app/src/__tests__/signal-entity-links.test.tsdb/app/src/index.tsdb/app/src/schema/index.tsdb/app/src/schema/tables/index.tsdb/app/src/schema/tables/org-signal-entity-links.tsdb/app/src/utils/signal-entity-links.tspackages/api-contract/src/__tests__/signals.test.tspackages/api-contract/src/index.tspackages/api-contract/src/schemas/signals.ts
…nks-persistence # Conflicts: # db/app/src/migrations/meta/0025_snapshot.json # db/app/src/migrations/meta/_journal.json
This branch had an error being deployed
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
1 participant
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
entityLinksthrough signal get APIs/MCP and render linked people on Signal detail.Test Plan
Summary by CodeRabbit