feat(documents): add Documenso signing flow#424
Open
Connorbelez wants to merge 8 commits intographite-base/424from
Open
feat(documents): add Documenso signing flow#424Connorbelez wants to merge 8 commits intographite-base/424from
Connorbelez wants to merge 8 commits intographite-base/424from
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
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
This was referenced Apr 20, 2026
Open
Open
Open
Open
Owner
Author
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
This was referenced Apr 20, 2026
Open
Open
![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
Adds a Documenso-backed signing flow for signable deal-package documents, including normalized envelope/recipient persistence, embedded signing sessions, provider sync, and new broker-facing portal routes.
Changes:
- Introduces
signatureEnvelopes/signatureRecipientstables + signing-status extensions on generated documents, along with a Documenso provider adapter. - Updates deal package generation and read surfaces to create/sign/sync signable documents and expose recipient-aware portal/admin UI actions.
- Adds broker portal routes and updates tests/spec artifacts for the ENG-288 rollout (plus a small obligations-cron monitoring adjustment).
Reviewed changes
Copilot reviewed 51 out of 52 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/lender/deal-detail-page.test.tsx | Updates lender deal UI test fixtures/assertions for signable + archived signing surfaces. |
| src/test/convex/documents/dealPackages.test.ts | Adds extensive signing-flow tests (Documenso mock, envelope/recipient persistence, sync, archival, idempotency). |
| src/test/broker/deal-detail-page.test.tsx | Adds broker deal detail page test for archived artifacts rendering. |
| src/test/admin/deal-dedicated-details.test.tsx | Updates admin deal details test for signable + archived artifacts UI and refresh action. |
| src/routes/broker/index.tsx | Adds broker workspace landing route. |
| src/routes/broker.deals.tsx | Adds broker deals route outlet. |
| src/routes/broker.deals.$dealId.tsx | Adds broker deal detail route. |
| src/routeTree.gen.ts | Regenerates route tree to include broker routes. |
| src/components/portal/deals/PortalDealDetailPage.tsx | New shared portal deal detail UI with signable docs, recipient chips, embedded signing, refresh, and archive sections. |
| src/components/lender/deals/LenderDealDetailPage.tsx | Replaces lender-specific page with shared PortalDealDetailPage wrapper. |
| src/components/broker/deals/BrokerDealDetailPage.tsx | Adds broker wrapper around PortalDealDetailPage. |
| src/components/admin/shell/dedicated-detail-panels.tsx | Replaces admin signable placeholder with envelope/recipient details, refresh action, and archived artifacts list. |
| specs/ENG-288/tasks.md | Adds ENG-288 task tracking doc. |
| specs/ENG-288/summary.md | Adds ENG-288 scope/constraints summary. |
| specs/ENG-288/status.md | Adds ENG-288 execution status + blockers. |
| specs/ENG-288/execution-checklist.md | Adds ENG-288 checklist and validation status. |
| specs/ENG-288/chunks/manifest.md | Adds chunk manifest. |
| specs/ENG-288/chunks/chunk-04-tests-and-validation/tasks.md | Adds chunk task list for tests/validation. |
| specs/ENG-288/chunks/chunk-04-tests-and-validation/status.md | Adds chunk status/validation summary. |
| specs/ENG-288/chunks/chunk-04-tests-and-validation/context.md | Adds chunk context doc. |
| specs/ENG-288/chunks/chunk-03-reads-and-ui/tasks.md | Adds chunk tasks for reads/UI. |
| specs/ENG-288/chunks/chunk-03-reads-and-ui/status.md | Adds chunk status/validation for reads/UI. |
| specs/ENG-288/chunks/chunk-03-reads-and-ui/context.md | Adds chunk context for reads/UI. |
| specs/ENG-288/chunks/chunk-02-package-signing-flow/tasks.md | Adds chunk tasks for signing flow. |
| specs/ENG-288/chunks/chunk-02-package-signing-flow/status.md | Adds chunk status/validation for signing flow. |
| specs/ENG-288/chunks/chunk-02-package-signing-flow/context.md | Adds chunk context for signing flow. |
| specs/ENG-288/chunks/chunk-01-schema-provider/tasks.md | Adds chunk tasks for schema/provider. |
| specs/ENG-288/chunks/chunk-01-schema-provider/status.md | Adds chunk status/validation for schema/provider. |
| specs/ENG-288/chunks/chunk-01-schema-provider/context.md | Adds chunk context for schema/provider. |
| specs/ENG-288/audit.md | Adds spec-audit output and requirement ledger. |
| convex/test/moduleMaps.ts | Registers new signature modules in Convex test module map. |
| convex/schema.ts | Adds signature envelope/recipient tables; extends generatedDocuments signing/archive fields; adds broker dealAccess roles. |
| convex/payments/obligations/monitoring.ts | Adjusts same-day metric patching to keep max counts. |
| convex/payments/obligations/crons.ts | Ensures monitoring row is recorded on no-backlog days (wave 0). |
| convex/payments/tests/crons.test.ts | Updates cron tests to drain scheduled work consistently and align monitoring expectations. |
| convex/engine/effects/dealClosingEffects.ts | Wires archive effect to real signable-archive action (no longer a stub). |
| convex/engine/effects/dealAccess.ts | Extends deal access effect to grant broker roles and improves logging/idempotency. |
| convex/documents/signature/webhooks.ts | Adds authenticated sync action to refresh envelope state from provider and persist normalized state. |
| convex/documents/signature/sessions.ts | Adds authenticated embedded signing session issuance gated by canonical recipient match. |
| convex/documents/signature/provider.ts | Adds provider interface + status mapping helpers. |
| convex/documents/signature/documenso.ts | Implements Documenso adapter (config/env resolution, create/distribute, sync, artifact download). |
| convex/documents/dealPackages.ts | Implements signable generation, envelope/recipient persistence, signing/archived surfaces, status summarization, and archiving action. |
| convex/documents/contracts.ts | Adds validators/types for signature provider/status enums and new instance statuses. |
| convex/documentEngine/generation.ts | Extends Documenso recipient config to include platformRole. |
| convex/deals/queries.ts | Ensures portal deal detail query passes viewer context into document package surface for recipient eligibility. |
| convex/deals/mutations.ts | Exports DealAccessRole and extends accepted roles with broker roles. |
| convex/deals/tests/access.test.ts | Updates deal access tests for broker roles and new effect behavior. |
| convex/crm/detailContextQueries.ts | Passes viewer context into package surface for admin/CRM deal detail context. |
| convex/crm/tests/detailContextQueries.test.ts | Minor import ordering adjustment after viewer-user changes. |
| convex/auth/resourceChecks.ts | Removes implicit broker-via-mortgage access; uses explicit dealAccess records. |
| convex/auth/tests/resourceChecks.test.ts | Updates access tests to reflect broker requires explicit dealAccess. |
| convex/_generated/api.d.ts | Regenerates API types to include new signature modules (and one added admin origination module). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
convex/engine/effects/dealAccess.ts (1)
100-123:⚠️ Potential issue | 🟠 MajorDon’t skip broker grants when lawyer data is absent.
The new broker grant path is unreachable when
lawyerIdis missing orlawyerTypeis unset, sobroker_of_record/assigned_brokeraccess can be skipped even though those grants do not depend on lawyer access.Suggested fix
- const lawyerId = deal.lawyerId; - if (!lawyerId) { - return; - } - - if (!deal.lawyerType) { - console.warn( - `[createDealAccess] Deal ${args.entityId} has lawyerId but no lawyerType — skipping access grant` - ); - return; - } - - const grantsByUserId = new Map<string, DealAccessRole>([ - [lawyerId, deal.lawyerType], - ]); + const grantsByUserId = new Map<string, DealAccessRole>(); + if (deal.lawyerId) { + if (deal.lawyerType) { + grantsByUserId.set(deal.lawyerId, deal.lawyerType); + } else { + console.warn( + `[createDealAccess] Deal ${args.entityId} has lawyerId but no lawyerType — skipping lawyer access grant` + ); + } + } const brokerGrants = await resolveBrokerDealAccessGrants(ctx, { dealId: args.entityId, mortgageId: deal.mortgageId, }); for (const grant of brokerGrants) { if (!grantsByUserId.has(grant.userId)) { grantsByUserId.set(grant.userId, grant.role); } } + + if (grantsByUserId.size === 0) { + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/engine/effects/dealAccess.ts` around lines 100 - 123, The code currently returns early when lawyerId or deal.lawyerType is missing which prevents calling resolveBrokerDealAccessGrants and applying broker grants; change createDealAccess logic so resolveBrokerDealAccessGrants is always called and its results merged into grantsByUserId even if lawyerId or lawyerType is absent: initialize grantsByUserId as an empty Map, if lawyerId and deal.lawyerType exist then set(grant) for the lawyer, then await resolveBrokerDealAccessGrants(ctx, { dealId: args.entityId, mortgageId: deal.mortgageId }) and merge brokerGrants into grantsByUserId (using has before set as existing code does), and remove the early returns that skip broker processing.
🧹 Nitpick comments (8)
convex/deals/queries.ts (1)
214-243: Optional: dedupe viewer lookup when viewer is also the lender/seller.When
ctx.viewer.authId === deal.buyerIdor=== deal.sellerId, thisPromise.allissues a redundantusersindex lookup. Harmless (query, not mutation), but a trivial optimization for the common "lender viewing their own deal" path:♻️ Optional refactor
- const [mortgage, property, lenderUser, sellerUser, viewerUser] = - await Promise.all([ + const [mortgage, property, lenderUser, sellerUser] = await Promise.all([ ctx.db.get(deal.mortgageId), ctx.db .get(deal.mortgageId) .then((mortgageRow) => mortgageRow ? ctx.db.get(mortgageRow.propertyId) : null ), ctx.db .query("users") .withIndex("authId", (query) => query.eq("authId", deal.buyerId)) .unique(), ctx.db .query("users") .withIndex("authId", (query) => query.eq("authId", deal.sellerId)) .unique(), - ctx.db - .query("users") - .withIndex("authId", (query) => query.eq("authId", ctx.viewer.authId)) - .unique(), ]); + const viewerUser = + ctx.viewer.authId === deal.buyerId + ? lenderUser + : ctx.viewer.authId === deal.sellerId + ? sellerUser + : await ctx.db + .query("users") + .withIndex("authId", (q) => q.eq("authId", ctx.viewer.authId)) + .unique();Also note the pre-existing double
ctx.db.get(deal.mortgageId)on lines 216/218 (mortgage is fetched twice); worth folding into the same cleanup if you touch this block again, but it's not introduced here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/deals/queries.ts` around lines 214 - 243, This block performs redundant DB calls: ctx.db.get(deal.mortgageId) is invoked twice and the users index query for viewerUser duplicates lenderUser/sellerUser when ctx.viewer.authId equals deal.buyerId or deal.sellerId. Fix by fetching mortgage once into the mortgage variable and derive property from that single mortgage result, and avoid a separate users query for viewerUser when ctx.viewer.authId matches deal.buyerId or deal.sellerId by reusing lenderUser or sellerUser respectively (fall back to the indexed query only if no match). Ensure the rest of the code (including the call to readDealDocumentPackageSurface using viewerUser?._id and ctx.viewer.isFairLendAdmin) still receives the correct user object.src/components/admin/shell/dedicated-detail-panels.tsx (1)
1496-1514: Recipient chip React key can collide when multiple recipients share aplatformRole.If a signable envelope has, say, two
borrower_primary/borrower_coborrowerentries (or any two recipients with the sameplatformRole), the key${document.instanceId}-${recipient.platformRole}collides and React will warn/misbehave on re-renders. Prefer a stable per-recipient identifier that the newsignatureRecipientstable already provides.♻️ Proposed fix
- key={`${document.instanceId}-${recipient.platformRole}`} + key={`${document.instanceId}-${recipient.providerRecipientId ?? recipient.userId ?? recipient.email ?? recipient.platformRole}`}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/admin/shell/dedicated-detail-panels.tsx` around lines 1496 - 1514, The recipient chip key can collide because it uses `${document.instanceId}-${recipient.platformRole}`; update the map in document.signing.recipients to use a stable per-recipient identifier from the new signatureRecipients table (e.g., recipient.id or recipient.signatureRecipientId) instead of platformRole so keys are unique across recipients (for example, replace the key expression with `${document.instanceId}-${recipient.id}` or similar, falling back to a deterministic composite only if that unique id is missing).convex/documents/signature/webhooks.ts (1)
15-20: Filename vs. contents mismatch — no webhook handler here yet.
webhooks.tscurrently only exports a user-triggered sync action (syncSignableDocumentEnvelope), not an HTTP webhook endpoint. If an actual Documenso webhook receiver is landing in a later chunk, that's fine; otherwise consider moving this intosessions.tsor a newsync.tsso the file name reflects its contents and future webhook code has a clean home.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/documents/signature/webhooks.ts` around lines 15 - 20, The file named "webhooks" only contains the user-triggered authed action syncSignableDocumentEnvelope (handler function syncSignableDocumentEnvelope) rather than an HTTP webhook receiver; either move syncSignableDocumentEnvelope into a more appropriate module (e.g., sessions or a new sync module) or rename the file to reflect its purpose, and update exports/imports accordingly so the symbol syncSignableDocumentEnvelope lives in a file whose name matches its responsibility (or keep webhooks.ts only if you will add the actual webhook handler later).convex/documents/signature/documenso.ts (2)
140-144:DOCUMENSO_TIMEOUT_MSis parsed twice.Minor, but worth extracting to a single parse to avoid re-running
Number.parseIntand to make the> 0guard clearer.♻️ Proposed refactor
- const timeoutMs = - process.env.DOCUMENSO_TIMEOUT_MS && - Number.parseInt(process.env.DOCUMENSO_TIMEOUT_MS, 10) > 0 - ? Number.parseInt(process.env.DOCUMENSO_TIMEOUT_MS, 10) - : DEFAULT_TIMEOUT_MS; + const parsedTimeout = process.env.DOCUMENSO_TIMEOUT_MS + ? Number.parseInt(process.env.DOCUMENSO_TIMEOUT_MS, 10) + : Number.NaN; + const timeoutMs = + Number.isFinite(parsedTimeout) && parsedTimeout > 0 + ? parsedTimeout + : DEFAULT_TIMEOUT_MS;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/documents/signature/documenso.ts` around lines 140 - 144, The timeoutMs computation parses process.env.DOCUMENSO_TIMEOUT_MS twice; parse it once into a named variable (e.g., parsedTimeout = Number.parseInt(process.env.DOCUMENSO_TIMEOUT_MS, 10)), then set timeoutMs = parsedTimeout > 0 ? parsedTimeout : DEFAULT_TIMEOUT_MS. Update the reference to DOCUMENSO_TIMEOUT_MS and the timeoutMs assignment accordingly so the > 0 guard is applied to the single parsed value.
263-275: Recipient matching may be ambiguous for multi-signer templates.
matchProviderRecipientkeys onemail + providerRole + signingOrder. If two recipients share the same email (e.g., the same lawyer appearing as both approver and signer, or a test fixture reusing an email across roles) and Documenso reorders recipients in its response, the wrong provider recipient id can be associated with the wrong platform role. Matching byexternalId(if Documenso supports setting one per recipient on create) or by position in the request list would be more robust.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/documents/signature/documenso.ts` around lines 263 - 275, matchProviderRecipient currently identifies provider recipients by email + providerRole + signingOrder which can be ambiguous when the same email appears across roles or Documenso reorders its response; update matchProviderRecipient (and any caller that builds SignatureProviderRecipientInput) to prefer matching by a stable identifier: if DocumensoRecipientResponse.externalId (or a similar per-recipient external id) exists, match input.externalId === recipient.externalId; otherwise, fall back to positional matching using the original providerRecipients array index vs input.signingOrder (or an explicit providerIndex field) instead of relying on email+role alone; adjust types SignatureProviderRecipientInput and DocumensoRecipientResponse usage accordingly so callers set externalId when creating recipients and the lookup uses it first.src/test/convex/documents/dealPackages.test.ts (1)
93-198: LGTM on the mock Documenso fetch plumbing.The mock covers envelope create/fetch/distribute/recipient, signed-PDF and completion-certificate download (both dedicated item and
?version=certificatefallback), with configurable failure toggles matching the provider's code paths inconvex/documents/signature/documenso.ts. The ordering of branches correctly discriminates on pathname +versionquery param, and the 404 fallback for missing certificates mirrorstryDownloadOptionalArtifact's behavior.One small note: re-calling
installMockDocumensoFetchmid-test replaces the global stub, so earlier mocks'.mock.callswon't record subsequent calls. The archive-idempotency test (lines 1330–1518) handles this correctly by asserting on the latest mock's call counts, but it's worth a comment on the helper so future test authors don't trip on it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/convex/documents/dealPackages.test.ts` around lines 93 - 198, The mock fetch helper (installMockDocumensoFetch / the block that creates fetchMock and calls vi.stubGlobal("fetch", fetchMock)) replaces the global fetch when invoked, which means calling it mid-test will swap out the stub and previous mock call history is not preserved; add a concise comment above the helper explaining this behavior and instructing test authors to assert call counts only against the latest installed mock (or preserve references if they need cumulative history).src/components/portal/deals/PortalDealDetailPage.tsx (1)
632-647:onClosefires quiet sync even when dialog never actually opened.Because the
<dialog>is rendered for every active signable document, any programmaticclose()(including ones not preceded byshowModal()— e.g., if browser autocloses on route change) will triggersyncSignableDocumentEnvelope. That's usually harmless but can generate unnecessary provider traffic per signable row on unmount. Consider gating the sync on a "was opened" flag set insidelaunchEmbeddedSigning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/portal/deals/PortalDealDetailPage.tsx` around lines 632 - 647, The onClose handler for the dialog (identified by signingDialogId and using signingFrameId) calls syncEnvelope for every rendered signable dialog even if it was never opened; to fix, add a "wasOpened" flag (e.g., a React state or a Map keyed by document.instanceId) that launchEmbeddedSigning sets to true when it calls showModal/open, then modify the dialog onClose to only call syncEnvelope and clear/reset the flag if wasOpened is true; ensure the flag is reset after syncing and that the iframe src is still cleared unconditionally.convex/schema.ts (1)
692-729: New signature tables look consistent with the provider/webhook flow.Indexes align with the access patterns you use in
convex/documents/signature/webhooks.tsandsessions.ts:by_deal,by_generated_document,by_provider_envelopefor reconciliation, andby_envelope/by_envelope_provider_recipient/by_user_statusfor recipient lookups.Two small things to consider:
- Convex doesn't enforce uniqueness, so
(providerCode, providerEnvelopeId)and(envelopeId, providerRecipientId)are only unique by convention. Ensure the insert paths indocumenso.ts/webhook sync do awithIndex(...).unique()-style check before inserting to avoid duplicate rows if a retry or webhook race fires twice.signatureRecipients.platformRoleis a free-formv.string()— the rest of the codebase (e.g. templates seeded in tests) uses canonical tokens likeborrower_primary,lawyer_primary,lender_primary. If you ever want to constrain this at the schema level, a validator mirroring the seeding vocabulary would prevent typo-driven access-control bugs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/schema.ts` around lines 692 - 729, The signature tables are fine but you must (1) guard against duplicate inserts by checking the relevant index uniqueness before writing: in your insert paths (references: functions in documenso.ts and convex/documents/signature/webhooks.ts that create rows in signatureEnvelopes and signatureRecipients) perform a lookup using the matching index (e.g., index "by_provider_envelope" for signatureEnvelopes and "by_envelope_provider_recipient" for signatureRecipients) and enforce uniqueness (the Convex pattern: query the index and only insert if no existing row or fail/update) to avoid retry/webhook races; and (2) optionally constrain signatureRecipients.platformRole by replacing v.string() with a validator listing the canonical tokens used in seeds (e.g., an enum validator matching borrower_primary, lawyer_primary, lender_primary) to prevent typos.
🤖 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/documents/dealPackages.ts`:
- Around line 2242-2268: The external provider.createEnvelope call is
non-idempotent and can create duplicate envelopes if the DB write fails; update
the flow to use a stable idempotency key (generatedDocumentId) and check for an
existing provider envelope before creating a new one: first query for any
existing providerEnvelopeId tied to generatedDocumentId (or call the provider
API to find an envelope by metadata) and if found reuse it; if not found, call
provider.createEnvelope passing generatedDocumentId as an idempotency/metadata
field so the provider can dedupe, then persist the providerEnvelopeId/lastError
by calling
internal.documents.dealPackages.createSignatureEnvelopeWithRecipientsInternal
(ensure that createSignatureEnvelopeWithRecipientsInternal records
providerEnvelopeId even when the provider reports an existing envelope); also
handle provider errors by storing lastError and treating subsequent retries
idempotently by re-checking the existing envelope via the same key.
- Around line 1487-1518: The loop over instances building targets throws for old
failed retry rows that were archived by archiveRetryInstanceIfNeeded; update the
loop in the block that creates targets (variables: instances, instance, targets)
to skip any instance that has been archived by the retry path by adding a guard
like if (instance.archived === true || instance.archivedAt) continue; before
validating generatedDocumentId and querying signatureEnvelopes (the
ctx.db.query("signatureEnvelopes")...unique() call), so archived incomplete
attempts are ignored and do not block archiving the successful package.
- Line 1178: The exported function patchGeneratedDocumentSigningStateInternal
(and the other newly exported internals mentioned) currently use raw
internalMutation/internalQuery/internalAction; replace those with the project’s
fluent-convex builder pattern and make visibility explicit by ending the
exported builder chain with .internal() (e.g., convert internalMutation(...) to
the repository’s fluent builder call and append .internal()); apply the same
change to all other new exported internals so each export uses the fluent
builder and an explicit .internal() visibility marker.
- Around line 2195-2197: The code currently returns early when
sourceBlueprintSnapshot.templateId is falsy, silently skipping creation;
instead, record a failed signable instance so finalization sees the missing
document. Replace the early return in the block referencing
sourceBlueprintSnapshot.templateId with logic that creates or appends a failure
record (e.g., push a signable instance with status 'failed', include blueprint
id/metadata and an error message like "missing templateId") or call the existing
failure helper (e.g., markSignableAsFailed/createFailedSignableInstance) so the
rest of the pipeline and package finalization will report the malformed
private_templated_signable rather than treating it as present.
- Around line 963-977: The failed-status classification (failedRows) includes
"signature_draft", "signature_declined", and "signature_voided" but the retry
eligibility filter only checks "generation_failed" and
"signature_pending_recipient_resolution", causing declined/voided/draft packages
to never be retried; update the retry filter to include the same failed statuses
as failedRows (add "signature_draft", "signature_declined", and
"signature_voided") so retry logic and failedRows are in sync, and apply the
same change to the analogous retry filter referenced around the other occurrence
(the block at 1852-1857).
- Around line 1194-1203: The patch call to ctx.db.patch on generatedDocumentId
is currently assigning optional args (completionCertificateStorageId,
documensoEnvelopeId, finalPdfStorageId, signingCompletedAt, signingStatus,
updatedAt) directly, which can unset existing fields when args are undefined;
change the patch to build the patch object only with keys whose corresponding
values are !== undefined (e.g., conditionally add
completionCertificateStorageId, finalPdfStorageId, signingCompletedAt,
signingStatus, documensoEnvelopeId fallback logic) before calling ctx.db.patch
for generatedDocumentId, and apply the same conditional-patch construction
pattern to the recipient patch (the recipient patch call) and the second
generatedDocument patch (the other ctx.db.patch for generatedDocument) so
optional timestamps like openedAt, signedAt, declinedAt are only written when
provided.
In `@convex/documents/signature/documenso.ts`:
- Around line 134-144: The current fallback sets appBaseUrl to
apiBaseUrl.replace(DOCUMENSO_API_SUFFIX_RE, "") which can leave API path
segments intact; change the logic in the appBaseUrl computation so when
DOCUMENSO_APP_BASE_URL is unset you defensively strip any "/api" path instead of
relying solely on DOCUMENSO_API_SUFFIX_RE (e.g., run a secondary regex to remove
"/api" and anything after it), and if that secondary strip yields a string that
still contains "/api" (or if apiBaseUrl clearly doesn't match expected API host
form) throw or log an explicit error rather than using the unsafe value; adjust
normalizeSigningUrl usage accordingly so it always receives a clean user-facing
base URL and reference variables/functions appBaseUrl, apiBaseUrl,
DOCUMENSO_API_SUFFIX_RE, and normalizeSigningUrl when making the change.
- Around line 419-474: The createAndOptionallyDistributeEnvelope flow currently
catches any /envelope/distribute error and returns status: "draft", which maps
to "signature_draft" and is not picked up by buildPackageWorkItems for retries;
change createAndOptionallyDistributeEnvelope to set a distinct retryable state
(e.g., "needs_distribution" or "signature_pending_distribution") and preserve
the created envelopeId and distributionError so subsequent retries call
/envelope/distribute rather than creating a new envelope; update
buildPackageWorkItems to include that new status in its retryable set and ensure
retryPackageGeneration (and createSignableGeneratedInstance) does not create a
fresh envelope when an existing envelopeId is present; add a unit/integration
test exercising failDistribute: true that asserts envelopeId is preserved and
that a later retry triggers /envelope/distribute instead of a new
/envelope/create.
In `@convex/engine/effects/dealAccess.ts`:
- Around line 125-138: The log in createDealAccess currently records user
identifiers via grantedRecords built from grantsByUserId and grantDealAccess;
change it to avoid emitting userId by recording only role and a non-identifying
count or accessId (e.g., store `${role}:${accessId}` or `${role}`) and update
the console.info call to log roles and totals (e.g., "Granted roles: ..., count:
N for deal=...") so no email/auth IDs are included; keep grantDealAccess usage
and grantedRecords variable but remove or replace userId when composing log
entries.
In `@specs/ENG-288/audit.md`:
- Line 37: The spec line claiming "OUT_OF_SCOPE" for final signed artifact
archival is inconsistent with the code wiring (archiveSignedDocuments ->
archiveCompletedSignableDocumentsInternal) and UI tests asserting archived final
PDF/certificate links; update the requirement text to reflect that archival of
signed artifacts is now in-scope (or alternatively remove/undo the wiring), and
explicitly state what remains for Phase 9: consuming downloadCompletedArtifacts
and persisting final artifacts into platform storage; reference the functions
archiveSignedDocuments and archiveCompletedSignableDocumentsInternal and the
consumer downloadCompletedArtifacts when making the clarification.
In `@src/components/portal/deals/PortalDealDetailPage.tsx`:
- Around line 249-274: The launchEmbeddedSigning function currently sets
frame.src and opens dialog only when DOM lookups succeed, which can silently
fail; change it to detect missing nodes and surface a toast error when either
the iframe (signingFrameId) or dialog (signingDialogId) is null, avoid opening a
dialog if the iframe is missing or avoid setting frame.src if dialog is missing,
and ensure you clear or don't mutate DOM state on failure; additionally replace
the document.getElementById lookups with React refs keyed by instance (useRef
per instance or a ref map) so the iframe/dialog references are deterministic
across re-renders (update launchEmbeddedSigning and any JSX that renders the
iframe/dialog to accept/use those refs).
- Around line 276-288: The header badges in PortalDealDetailPage render raw enum
strings; update the Badge label sources to pass detail.deal.status,
detail.documentPackage?.status, and detail.mortgage.status through the existing
formatEnumLabel helper (e.g., Badge children =>
formatEnumLabel(detail.deal.status),
formatEnumLabel(detail.documentPackage?.status ?? "No package yet") or only
format when a status exists, and formatEnumLabel(detail.mortgage.status)) so
snake_case values like "partial_failure" and "signature_sent" display as
human-friendly labels consistent with the other badges on the page.
In `@src/routes/broker.deals`.$dealId.tsx:
- Around line 4-11: Add a beforeLoad auth gate to the createFileRoute call for
the BrokerDealDetailRoute: implement a beforeLoad that reads the current
session/user (same mechanism used in src/routes/broker/index.tsx), verifies the
user is authenticated and has the broker role, and if not throws redirect({ to:
'/login', search: { redirect: location.href } }); update the Route definition
(the createFileRoute call that currently exports Route) to include this
beforeLoad so the page never renders unauthenticated content; leave
BrokerDealDetailRoute and Route.useParams unchanged and ensure you import/throw
redirect exactly as specified.
In `@src/routes/broker/index.tsx`:
- Around line 9-11: The Route currently created with
createFileRoute("/broker/")({ component: BrokerWorkspaceIndexRoute }) lacks a
beforeLoad auth gate; add a beforeLoad function to this route that checks
authentication and, if unauthenticated, throws redirect({ to: '/login', search:
{ redirect: location.href } }); also ensure you perform broker-role gating at
the island/component boundary (BrokerWorkspaceIndexRoute or its child island) so
only users with the broker role can view broker content. Use the Route and
BrokerWorkspaceIndexRoute symbols to locate where to add the beforeLoad and
where to enforce role checks.
---
Outside diff comments:
In `@convex/engine/effects/dealAccess.ts`:
- Around line 100-123: The code currently returns early when lawyerId or
deal.lawyerType is missing which prevents calling resolveBrokerDealAccessGrants
and applying broker grants; change createDealAccess logic so
resolveBrokerDealAccessGrants is always called and its results merged into
grantsByUserId even if lawyerId or lawyerType is absent: initialize
grantsByUserId as an empty Map, if lawyerId and deal.lawyerType exist then
set(grant) for the lawyer, then await resolveBrokerDealAccessGrants(ctx, {
dealId: args.entityId, mortgageId: deal.mortgageId }) and merge brokerGrants
into grantsByUserId (using has before set as existing code does), and remove the
early returns that skip broker processing.
---
Nitpick comments:
In `@convex/deals/queries.ts`:
- Around line 214-243: This block performs redundant DB calls:
ctx.db.get(deal.mortgageId) is invoked twice and the users index query for
viewerUser duplicates lenderUser/sellerUser when ctx.viewer.authId equals
deal.buyerId or deal.sellerId. Fix by fetching mortgage once into the mortgage
variable and derive property from that single mortgage result, and avoid a
separate users query for viewerUser when ctx.viewer.authId matches deal.buyerId
or deal.sellerId by reusing lenderUser or sellerUser respectively (fall back to
the indexed query only if no match). Ensure the rest of the code (including the
call to readDealDocumentPackageSurface using viewerUser?._id and
ctx.viewer.isFairLendAdmin) still receives the correct user object.
In `@convex/documents/signature/documenso.ts`:
- Around line 140-144: The timeoutMs computation parses
process.env.DOCUMENSO_TIMEOUT_MS twice; parse it once into a named variable
(e.g., parsedTimeout = Number.parseInt(process.env.DOCUMENSO_TIMEOUT_MS, 10)),
then set timeoutMs = parsedTimeout > 0 ? parsedTimeout : DEFAULT_TIMEOUT_MS.
Update the reference to DOCUMENSO_TIMEOUT_MS and the timeoutMs assignment
accordingly so the > 0 guard is applied to the single parsed value.
- Around line 263-275: matchProviderRecipient currently identifies provider
recipients by email + providerRole + signingOrder which can be ambiguous when
the same email appears across roles or Documenso reorders its response; update
matchProviderRecipient (and any caller that builds
SignatureProviderRecipientInput) to prefer matching by a stable identifier: if
DocumensoRecipientResponse.externalId (or a similar per-recipient external id)
exists, match input.externalId === recipient.externalId; otherwise, fall back to
positional matching using the original providerRecipients array index vs
input.signingOrder (or an explicit providerIndex field) instead of relying on
email+role alone; adjust types SignatureProviderRecipientInput and
DocumensoRecipientResponse usage accordingly so callers set externalId when
creating recipients and the lookup uses it first.
In `@convex/documents/signature/webhooks.ts`:
- Around line 15-20: The file named "webhooks" only contains the user-triggered
authed action syncSignableDocumentEnvelope (handler function
syncSignableDocumentEnvelope) rather than an HTTP webhook receiver; either move
syncSignableDocumentEnvelope into a more appropriate module (e.g., sessions or a
new sync module) or rename the file to reflect its purpose, and update
exports/imports accordingly so the symbol syncSignableDocumentEnvelope lives in
a file whose name matches its responsibility (or keep webhooks.ts only if you
will add the actual webhook handler later).
In `@convex/schema.ts`:
- Around line 692-729: The signature tables are fine but you must (1) guard
against duplicate inserts by checking the relevant index uniqueness before
writing: in your insert paths (references: functions in documenso.ts and
convex/documents/signature/webhooks.ts that create rows in signatureEnvelopes
and signatureRecipients) perform a lookup using the matching index (e.g., index
"by_provider_envelope" for signatureEnvelopes and
"by_envelope_provider_recipient" for signatureRecipients) and enforce uniqueness
(the Convex pattern: query the index and only insert if no existing row or
fail/update) to avoid retry/webhook races; and (2) optionally constrain
signatureRecipients.platformRole by replacing v.string() with a validator
listing the canonical tokens used in seeds (e.g., an enum validator matching
borrower_primary, lawyer_primary, lender_primary) to prevent typos.
In `@src/components/admin/shell/dedicated-detail-panels.tsx`:
- Around line 1496-1514: The recipient chip key can collide because it uses
`${document.instanceId}-${recipient.platformRole}`; update the map in
document.signing.recipients to use a stable per-recipient identifier from the
new signatureRecipients table (e.g., recipient.id or
recipient.signatureRecipientId) instead of platformRole so keys are unique
across recipients (for example, replace the key expression with
`${document.instanceId}-${recipient.id}` or similar, falling back to a
deterministic composite only if that unique id is missing).
In `@src/components/portal/deals/PortalDealDetailPage.tsx`:
- Around line 632-647: The onClose handler for the dialog (identified by
signingDialogId and using signingFrameId) calls syncEnvelope for every rendered
signable dialog even if it was never opened; to fix, add a "wasOpened" flag
(e.g., a React state or a Map keyed by document.instanceId) that
launchEmbeddedSigning sets to true when it calls showModal/open, then modify the
dialog onClose to only call syncEnvelope and clear/reset the flag if wasOpened
is true; ensure the flag is reset after syncing and that the iframe src is still
cleared unconditionally.
In `@src/test/convex/documents/dealPackages.test.ts`:
- Around line 93-198: The mock fetch helper (installMockDocumensoFetch / the
block that creates fetchMock and calls vi.stubGlobal("fetch", fetchMock))
replaces the global fetch when invoked, which means calling it mid-test will
swap out the stub and previous mock call history is not preserved; add a concise
comment above the helper explaining this behavior and instructing test authors
to assert call counts only against the latest installed mock (or preserve
references if they need cumulative history).
🪄 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: 407a70df-2602-4fc8-b2c2-930a43502d07
⛔ Files ignored due to path filters (1)
convex/_generated/api.d.tsis excluded by!**/_generated/**
📒 Files selected for processing (51)
convex/auth/__tests__/resourceChecks.test.tsconvex/auth/resourceChecks.tsconvex/crm/__tests__/detailContextQueries.test.tsconvex/crm/detailContextQueries.tsconvex/deals/__tests__/access.test.tsconvex/deals/mutations.tsconvex/deals/queries.tsconvex/documentEngine/generation.tsconvex/documents/contracts.tsconvex/documents/dealPackages.tsconvex/documents/signature/documenso.tsconvex/documents/signature/provider.tsconvex/documents/signature/sessions.tsconvex/documents/signature/webhooks.tsconvex/engine/effects/dealAccess.tsconvex/engine/effects/dealClosingEffects.tsconvex/payments/__tests__/crons.test.tsconvex/payments/obligations/crons.tsconvex/payments/obligations/monitoring.tsconvex/schema.tsconvex/test/moduleMaps.tsspecs/ENG-288/audit.mdspecs/ENG-288/chunks/chunk-01-schema-provider/context.mdspecs/ENG-288/chunks/chunk-01-schema-provider/status.mdspecs/ENG-288/chunks/chunk-01-schema-provider/tasks.mdspecs/ENG-288/chunks/chunk-02-package-signing-flow/context.mdspecs/ENG-288/chunks/chunk-02-package-signing-flow/status.mdspecs/ENG-288/chunks/chunk-02-package-signing-flow/tasks.mdspecs/ENG-288/chunks/chunk-03-reads-and-ui/context.mdspecs/ENG-288/chunks/chunk-03-reads-and-ui/status.mdspecs/ENG-288/chunks/chunk-03-reads-and-ui/tasks.mdspecs/ENG-288/chunks/chunk-04-tests-and-validation/context.mdspecs/ENG-288/chunks/chunk-04-tests-and-validation/status.mdspecs/ENG-288/chunks/chunk-04-tests-and-validation/tasks.mdspecs/ENG-288/chunks/manifest.mdspecs/ENG-288/execution-checklist.mdspecs/ENG-288/status.mdspecs/ENG-288/summary.mdspecs/ENG-288/tasks.mdsrc/components/admin/shell/dedicated-detail-panels.tsxsrc/components/broker/deals/BrokerDealDetailPage.tsxsrc/components/lender/deals/LenderDealDetailPage.tsxsrc/components/portal/deals/PortalDealDetailPage.tsxsrc/routeTree.gen.tssrc/routes/broker.deals.$dealId.tsxsrc/routes/broker.deals.tsxsrc/routes/broker/index.tsxsrc/test/admin/deal-dedicated-details.test.tsxsrc/test/broker/deal-detail-page.test.tsxsrc/test/convex/documents/dealPackages.test.tssrc/test/lender/deal-detail-page.test.tsx
Add signable package persistence, envelope/session handling, webhook support, and lender/admin signing UI for ENG-288. Also align the Documenso API env fallback with the local env files and fix the payments obligation monitoring tests flagged during the spec audit.
Connorbelez
force-pushed
the
create-mortgage-cleanup
branch
from
28507c4 to
9dbbb66
Compare
This was referenced Apr 20, 2026
Open
Open
Open
Connorbelez
changed the base branch from
create-mortgage-cleanup
to
graphite-base/424
This was referenced Apr 21, 2026
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.
feat(documents): add Documenso signing flow
Add signable package persistence, envelope/session handling, webhook support, and lender/admin signing UI for ENG-288.
Also align the Documenso API env fallback with the local env files and fix the payments obligation monitoring tests flagged during the spec audit.
eng-289
review
Summary by CodeRabbit
Release Notes
New Features
UI Improvements