refactor(e-signature): return signing results#29
Conversation
![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.
📝 WalkthroughWalkthroughThe PR converts the ChangesError Handling Modernization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 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 docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="libraries/e-signature/package.json">
<violation number="1" location="libraries/e-signature/package.json:42">
P2: Adding `neverthrow` introduces a third-party runtime dependency, which conflicts with the repository dependency policy (runtime deps should be limited to `@f-o-t/*` and `zod`).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| "@f-o-t/digital-certificate": "^2.3.0", | ||
| "@f-o-t/pdf": "^0.5.2", | ||
| "@f-o-t/qrcode": "^1.0.2", | ||
| "neverthrow": "^8.2.0", |
There was a problem hiding this comment.
P2: Adding neverthrow introduces a third-party runtime dependency, which conflicts with the repository dependency policy (runtime deps should be limited to @f-o-t/* and zod).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At libraries/e-signature/package.json, line 42:
<comment>Adding `neverthrow` introduces a third-party runtime dependency, which conflicts with the repository dependency policy (runtime deps should be limited to `@f-o-t/*` and `zod`).</comment>
<file context>
@@ -29,27 +29,25 @@
"@f-o-t/digital-certificate": "^2.3.0",
"@f-o-t/pdf": "^0.5.2",
"@f-o-t/qrcode": "^1.0.2",
+ "neverthrow": "^8.2.0",
"zod": "^4.3.6"
},
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
libraries/e-signature/__tests__/plugins/react.test.tsx (1)
221-234:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThis test no longer exercises the non-Error wrapping branch.
With the new mock setup,
mockSignErroris always wrapped innew Error(...)viaerrAsync(new Error(mockSignError)), so the awaited result'serroris already anError. The hook'serr instanceof Error ? err : new Error("Unknown signing error")therefore short-circuits to the original error, and the test passes only because the chosen message happens to match. The non-Error wrap branch is now uncovered.To actually exercise that branch, have the mock emit a non-
Errorrejection — e.g.,errAsync("string-failure" as unknown as Error)— and assert the hook normalizes it toError("Unknown signing error").🧪 Suggested test adjustment
it("wraps non-Error throws in a plain Error", async () => { - mockSignError = "Unknown signing error"; - - const { result } = renderHook(() => useSignPdf()); - - await act(async () => { - await result.current - .sign({ pdf: makePdf(), p12: makeP12(), password: "pw" }) - .catch(() => {}); - }); - - expect(result.current.error).toBeInstanceOf(Error); - expect(result.current.error?.message).toBe("Unknown signing error"); + // Override mock to reject with a non-Error value + mock.module("../../src/sign-pdf.ts", () => ({ + signPdf: () => errAsync("string-failure" as unknown as Error), + })); + + const { result } = renderHook(() => useSignPdf()); + + await act(async () => { + await result.current + .sign({ pdf: makePdf(), p12: makeP12(), password: "pw" }) + .catch(() => {}); + }); + + expect(result.current.error).toBeInstanceOf(Error); + expect(result.current.error?.message).toBe("Unknown signing error"); });🤖 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 `@libraries/e-signature/__tests__/plugins/react.test.tsx` around lines 221 - 234, The test currently never exercises the non-Error wrapping branch because the mock (mockSignError / errAsync) always rejects with an Error; change the mock to reject with a non-Error value (for example make the mock return errAsync("string-failure" as unknown as Error) or otherwise throw a non-Error), then call useSignPdf().sign(...) as before and assert that result.current.error is an instance of Error with message "Unknown signing error" to verify the hook normalizes non-Error rejections into a plain Error; adjust the mock creation referenced by mockSignError/errAsync and keep the assertions on result.current.error and its message.libraries/e-signature/src/sign-pdf.ts (2)
386-401:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
toPdfSignErrordiscards the original error context.Mapping every failure to
new PdfSignError(message)drops the originalcause, stack trace, and any structured details (e.g., aZodError.issuesfrompdfSignOptionsSchema.parse, or underlying@f-o-t/crypto/@f-o-t/pdferrors). With the newResultAsyncAPI this is the only error consumers ever receive, so debugging signing failures in production becomes considerably harder.Extend
PdfSignErrorto accept acauseand forward it fromtoPdfSignError(theError({ cause })form is supported on all targets a browser bundle currently ships to).🛠️ Proposed fix
function toPdfSignError(error: unknown): PdfSignError { if (error instanceof PdfSignError) return error; const message = error instanceof Error ? error.message : String(error); - return new PdfSignError(message); + return new PdfSignError(message, { cause: error }); }export class PdfSignError extends Error { - constructor(message: string) { - super(message); + constructor(message: string, options?: { cause?: unknown }) { + super(message, options); this.name = "PdfSignError"; } }🤖 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 `@libraries/e-signature/src/sign-pdf.ts` around lines 386 - 401, Update PdfSignError to accept an optional cause (e.g., constructor(message: string, cause?: unknown) and call super(message, { cause })) and then change toPdfSignError to pass the original error as the cause when wrapping non-PdfSignError values (use Error({ cause }) form). Ensure toPdfSignError still returns the original instance if error instanceof PdfSignError, otherwise construct new PdfSignError(message, error) so underlying stacks and structured data (e.g., ZodError issues or upstream `@f-o-t` errors) are preserved for consumers of ResultAsync.
46-65:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale JSDoc —
@returnsand@exampleno longer match the new API.The function now returns
ResultAsync<Uint8Array, PdfSignError>, so@returns The signed PDF as a Uint8Arrayis incorrect and the example would assign aResult, not the bytes. Update the docs so consumers know to.isOk()/.value(or.match) on the result.📝 Proposed JSDoc fix
- * `@param` pdf - The PDF document as a Uint8Array or ReadableStream<Uint8Array> - * `@param` options - Signing options - * `@returns` The signed PDF as a Uint8Array - * - * `@example` - * ```ts - * const signedPdf = await signPdf(pdfBytes, { - * certificate: { p12, password: "secret" }, - * reason: "Document approval", - * location: "Corporate Office", - * policy: "pades-icp-brasil", - * }); - * ``` + * `@param` pdf - The PDF document as a Uint8Array or ReadableStream<Uint8Array> + * `@param` options - Signing options + * `@returns` A `ResultAsync<Uint8Array, PdfSignError>` resolving to the signed PDF + * on success or a `PdfSignError` on failure. + * + * `@example` + * ```ts + * const result = await signPdf(pdfBytes, { + * certificate: { p12, password: "secret" }, + * reason: "Document approval", + * location: "Corporate Office", + * policy: "pades-icp-brasil", + * }); + * if (result.isErr()) throw result.error; + * const signedPdf = result.value; + * ```🤖 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 `@libraries/e-signature/src/sign-pdf.ts` around lines 46 - 65, The JSDoc for signPdf is stale: update the `@returns` to state the function returns a ResultAsync<Uint8Array, PdfSignError> (resolving to the signed PDF on success or a PdfSignError on failure) and replace the current `@example` so it shows awaiting the ResultAsync (e.g., const result = await signPdf(...); if (result.isErr()) throw result.error; const signedPdf = result.value;) or using .match; reference the signPdf function, ResultAsync type and PdfSignError in the doc to make usage clear.
🤖 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.
Outside diff comments:
In `@libraries/e-signature/__tests__/plugins/react.test.tsx`:
- Around line 221-234: The test currently never exercises the non-Error wrapping
branch because the mock (mockSignError / errAsync) always rejects with an Error;
change the mock to reject with a non-Error value (for example make the mock
return errAsync("string-failure" as unknown as Error) or otherwise throw a
non-Error), then call useSignPdf().sign(...) as before and assert that
result.current.error is an instance of Error with message "Unknown signing
error" to verify the hook normalizes non-Error rejections into a plain Error;
adjust the mock creation referenced by mockSignError/errAsync and keep the
assertions on result.current.error and its message.
In `@libraries/e-signature/src/sign-pdf.ts`:
- Around line 386-401: Update PdfSignError to accept an optional cause (e.g.,
constructor(message: string, cause?: unknown) and call super(message, { cause
})) and then change toPdfSignError to pass the original error as the cause when
wrapping non-PdfSignError values (use Error({ cause }) form). Ensure
toPdfSignError still returns the original instance if error instanceof
PdfSignError, otherwise construct new PdfSignError(message, error) so underlying
stacks and structured data (e.g., ZodError issues or upstream `@f-o-t` errors) are
preserved for consumers of ResultAsync.
- Around line 46-65: The JSDoc for signPdf is stale: update the `@returns` to
state the function returns a ResultAsync<Uint8Array, PdfSignError> (resolving to
the signed PDF on success or a PdfSignError on failure) and replace the current
`@example` so it shows awaiting the ResultAsync (e.g., const result = await
signPdf(...); if (result.isErr()) throw result.error; const signedPdf =
result.value;) or using .match; reference the signPdf function, ResultAsync type
and PdfSignError in the doc to make usage clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3065f15d-1fdf-4d08-9d26-71e5e4b1de6a
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
libraries/e-signature/__tests__/performance.test.tslibraries/e-signature/__tests__/plugins/react.test.tsxlibraries/e-signature/__tests__/sign-pdf.test.tslibraries/e-signature/bunup.config.tslibraries/e-signature/fot.config.tslibraries/e-signature/package.jsonlibraries/e-signature/src/batch.tslibraries/e-signature/src/plugins/react/index.tslibraries/e-signature/src/sign-pdf.ts
💤 Files with no reviewable changes (1)
- libraries/e-signature/fot.config.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (6)
libraries/e-signature/src/plugins/react/index.ts (1)
113-123: LGTM!The Result handling is correct: the type guard from
isErr()narrowssignedResulttoOk, and the previous public throw‑on‑failure contract ofsign()(documented on lines 56–60) is preserved.libraries/e-signature/bunup.config.ts (1)
1-28: LGTM!Externals align with the runtime
dependenciesinpackage.json, anddts.inferTypes: truewill correctly emit references toneverthrow.ResultAsyncas a bare external import in the generated.d.ts(consistent with the newsignPdfreturn type).libraries/e-signature/package.json (1)
31-54: LGTM!
neverthrowis correctly declared as a runtime dependency to back the newResultAsyncAPI, and the bunup-based scripts align with the newbunup.config.ts. Pinning bunup to an exact version is a reasonable choice for build-tool reproducibility.libraries/e-signature/src/batch.ts (1)
58-66: LGTM!Throwing on
isErr()keeps the per-filetry/catch→file_errorevent flow intact, andsignedResult.valueyields the bytes unchanged forfile_complete. Public event shape is unchanged.libraries/e-signature/__tests__/performance.test.ts (1)
42-49: LGTM!
signOkis a clean, minimal adapter that preserves the previous "throws on failure" semantics these benchmarks rely on. Mirrors the helper added insign-pdf.test.ts.libraries/e-signature/__tests__/sign-pdf.test.ts (1)
48-64: LGTM!Both helpers correctly bridge the
ResultAsyncAPI to the existing assertion style:signOkrethrows onisErr()so legacy throw-based tests keep working, andsignErrfails loudly if signing unexpectedly succeeds. Naming and shape are consistent with the helper duplicated inperformance.test.ts.
1 participant
Summary
Validation
Note: this PR is intended to pair with the digital-certificate Result API refactor.
Summary by cubic
Refactors
signPdfto return aneverthrowResultAsyncso callers handle expected signing errors without exceptions, and migrates the library build tobunupfor a simpler toolchain.signPdfnow returnsResultAsync<Uint8Array, PdfSignError>; callers must check.isOk()or unwrap the result.useSignPdfto unwrap results and surface standardized errors; tests assert success/error paths.bunup(bunup.config.tsadded,fot.config.tsremoved); scripts now usebunup,bun test, andtsc. Addedneverthrowandbunupdependencies, and aligned with the@f-o-t/digital-certificateResult API.Written for commit 1157be9. Summary will update on new commits.
Summary by CodeRabbit
Breaking Changes
Improvements