chore: enable typed signature by default #1436
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
5 Skipped Deployments
|
WalkthroughThe changes in this pull request introduce a new feature allowing users to input typed signatures in addition to drawn signatures. This is achieved by adding a new prop, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SignaturePad
participant Backend
participant Database
User->>SignaturePad: Input typed signature
SignaturePad->>Backend: Send typed signature
Backend->>Database: Save document with typedSignatureEnabled
Database-->>Backend: Confirmation
Backend-->>SignaturePad: Acknowledge receipt
SignaturePad-->>User: Display confirmation
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
apps/web/src/app/(teams)/t/[teamUrl]/settings/preferences/document-preferences.tsxOops! Something went wrong! :( ESLint: 8.54.0 ESLint couldn't find the config "@documenso/eslint-config" to extend from. Please check that the name of the config is correct. The config "@documenso/eslint-config" was referenced from the config file in "/.eslintrc.cjs". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
🚨 @catalinpit has 5 pull requests awaiting review. Please consider reviewing them when possible. 🚨 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
apps/web/src/components/forms/profile.tsx (1)
Line range hint
27-30: Update form schema to include signature preferences.If implementing the suggested UI controls, the form schema should be updated to include and validate the signature preference field.
export const ZProfileFormSchema = z.object({ name: z.string().trim().min(1, { message: 'Please enter a valid name.' }), signature: z.string().min(1, 'Signature Pad cannot be empty'), + signaturePreference: z.enum(['draw', 'type']).default('draw'), });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
apps/web/src/components/forms/profile.tsx(1 hunks)packages/prisma/migrations/20241101112106_enable_typed_signature_by_default/migration.sql(1 hunks)packages/prisma/schema.prisma(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/prisma/migrations/20241101112106_enable_typed_signature_by_default/migration.sql
🔇 Additional comments (2)
apps/web/src/components/forms/profile.tsx (1)
141-141: LGTM! The typed signature feature is properly enabled.
The implementation correctly enables typed signatures in the profile form by setting allowTypedSignature={true} on the SignaturePad component.
packages/prisma/schema.prisma (1)
372-372: LGTM! Verify the corresponding migration script.
The change to enable typed signatures by default is properly implemented. The modification only affects the default value and maintains backward compatibility.
Let's verify the corresponding migration script:
✅ Verification successful
Based on the migration files found, I can now provide a definitive response:
Migration script for typed signature default value change is present and correct.
The migration file 20241101112106_enable_typed_signature_by_default/migration.sql properly updates the default value for typedSignatureEnabled column in the DocumentMeta table from false to true. The change is implemented as:
ALTER TABLE "DocumentMeta" ALTER COLUMN "typedSignatureEnabled" SET DEFAULT true;This aligns with the schema change and ensures proper database state consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the migration script properly updates the default value
# Expected: Find a migration file that alters the typedSignatureEnabled column
rg -l "typedSignatureEnabled.*DEFAULT.*TRUE" --type sql
Length of output: 57
Script:
#!/bin/bash
# Let's first check if there are any SQL migration files
fd -e sql
# Then let's look for any migration files containing typedSignatureEnabled
rg "typedSignatureEnabled" --type sql
# Also check prisma migrations directory which is the standard location
fd . "prisma/migrations"
# If migrations exist, let's look at their content
find . -path "*/migrations/*" -type f -exec cat {} \;
Length of output: 65596
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
apps/web/src/app/(signing)/sign/[token]/provider.tsx (1)
Line range hint
29-52: Consider adding signature persistence mechanismSince this PR enables typed signatures by default, it might be beneficial to consider:
- Persisting signature preferences in local storage to improve user experience across sessions
- Adding a loading state to handle async signature initialization
- Implementing a mechanism to validate signature format/compatibility
This would make the typed signature feature more robust and user-friendly while maintaining consistency across the application.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/web/src/app/(signing)/sign/[token]/provider.tsx(2 hunks)
🔇 Additional comments (1)
apps/web/src/app/(signing)/sign/[token]/provider.tsx (1)
3-3: LGTM: Import statement is correctly updated
The addition of useEffect to the import statement is appropriate for the new functionality.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
apps/web/src/app/(signing)/sign/[token]/signature-field.tsx (2)
120-121: Consider using a more explicit typed signature detection.The current implementation relies on checking if the signature starts with 'data:image' to determine if it's typed. This approach is implementation-dependent and could break if the signature format changes.
Consider using a more explicit approach:
- const isTypedSignature = !value.startsWith('data:image'); + const isTypedSignature = typeof value === 'string' && !/^data:image\/(png|jpeg|jpg|svg\+xml);base64,/.test(value);
122-129: Enhance the error message with actionable guidance.The error message effectively informs users that typed signatures aren't allowed but could be more helpful by providing clearer guidance.
Consider updating the error message to be more instructive:
- description: _(msg`Typed signatures are not allowed. Please draw your signature.`), + description: _(msg`Typed signatures are not allowed for this document. Please use the drawing pad to create your signature.`),packages/ui/primitives/signature-pad/signature-pad.tsx (2)
Line range hint
273-275: Inconsistent onChange behavior in undo operationsThe
onUndoClickfunction handles undo operations differently for typed vs drawn signatures:
- Canvas signature undos trigger
onChange- Typed signature undos don't trigger
onChangeThis inconsistency could lead to stale state in parent components.
Add the onChange call for typed signature undos:
if (typedSignature.length > 0) { const newTypedSignature = typedSignature.slice(0, -1); setTypedSignature(newTypedSignature); - // You might want to call onChange here as well - // onChange?.(newTypedSignature); + if (newTypedSignature.trim() !== '') { + renderTypedSignature(); + onChange?.($el.current?.toDataURL() ?? null); + } else { + onChange?.(null); + } } else {
Line range hint
156-190: Improve font size calculation in renderTypedSignatureThe font size calculation logic uses magic numbers and could benefit from some improvements:
- Base font size (18px) and canvas width percentage (85%) should be named constants
- Character sample for width calculation ('m' repeated 10 times) is arbitrary
- No maximum font size limit could lead to overly large text
Consider these improvements:
+ const BASE_FONT_SIZE = 18; + const CANVAS_WIDTH_PERCENTAGE = 0.85; + const MAX_FONT_SIZE = 72; + const SAMPLE_TEXT = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz'; const renderTypedSignature = () => { if ($el.current && typedSignature) { const ctx = $el.current.getContext('2d'); if (ctx) { const canvasWidth = $el.current.width; const canvasHeight = $el.current.height; ctx.clearRect(0, 0, canvasWidth, canvasHeight); ctx.textAlign = 'center'; ctx.textBaseline = 'middle'; ctx.fillStyle = selectedColor; - // Calculate the desired width (25ch) - const desiredWidth = canvasWidth * 0.85; // 85% of canvas width + const desiredWidth = canvasWidth * CANVAS_WIDTH_PERCENTAGE; // Start with a base font size - let fontSize = 18; + let fontSize = BASE_FONT_SIZE; ctx.font = `${fontSize}px ${fontCaveat.style.fontFamily}`; // Measure average character width using a representative sample - const characterWidth = ctx.measureText('m'.repeat(10)).width; + const characterWidth = ctx.measureText(SAMPLE_TEXT).width; const scaleFactor = desiredWidth / characterWidth; // Apply scale factor to font size fontSize = fontSize * scaleFactor; + // Limit maximum font size + fontSize = Math.min(fontSize, MAX_FONT_SIZE); // Adjust font size if it exceeds canvas width ctx.font = `${fontSize}px ${fontCaveat.style.fontFamily}`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
apps/web/src/app/(signing)/sign/[token]/signature-field.tsx(1 hunks)packages/lib/server-only/field/sign-field-with-token.ts(1 hunks)packages/lib/server-only/pdf/insert-field-in-pdf.ts(1 hunks)packages/ui/primitives/signature-pad/signature-pad.tsx(2 hunks)
🔇 Additional comments (4)
packages/lib/server-only/field/sign-field-with-token.ts (1)
180-182: LGTM! The typed signature validation is well-implemented.
The validation check is placed at the right location in the control flow and provides a clear, actionable error message. This aligns well with the PR objective of managing typed signature settings.
Let's verify the consistency of error handling:
apps/web/src/app/(signing)/sign/[token]/signature-field.tsx (2)
135-135: LGTM! Correct handling of base64 flag.
The isBase64 flag is properly set based on the signature type, ensuring correct processing of both drawn and typed signatures.
120-135: Consider adding test coverage for typed signature handling.
The new typed signature logic introduces important validation and processing changes. To ensure reliability:
- Test the typed signature detection logic
- Verify error handling when typed signatures are disabled
- Validate the correct setting of the
isBase64flag
Let's check for existing test coverage:
packages/lib/server-only/pdf/insert-field-in-pdf.ts (1)
97-134: LGTM: Robust signature image handling implementation.
The signature image processing logic is well-implemented with proper:
- Null safety checks
- Image scaling while maintaining aspect ratio
- PDF coordinate system transformation
- Page rotation handling
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/ui/primitives/signature-pad/signature-pad.tsx (2)
Line range hint
32-35: Breaking Change: SignaturePad onChange callback signature has changedThe component's onChange callback now receives raw text for typed signatures instead of a data URL. This is a breaking change that affects all consumers of the SignaturePad component.
Consider:
- Adding JSDoc comments to document the new behavior
- Providing a migration guide for existing consumers
- Maintaining backward compatibility by converting typed signatures to data URLs (as suggested in the previous review)
Line range hint
271-277: Ensure consistent onChange callback behaviorThe onChange callback is not consistently called across all signature modification operations. The onUndoClick function has a commented out onChange call for typed signatures.
Apply this fix to ensure consistent behavior:
if (typedSignature.length > 0) { const newTypedSignature = typedSignature.slice(0, -1); setTypedSignature(newTypedSignature); - // You might want to call onChange here as well - // onChange?.(newTypedSignature); + onChange?.(newTypedSignature.length > 0 ? newTypedSignature : null); } else {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/ui/primitives/signature-pad/signature-pad.tsx(4 hunks)
🔇 Additional comments (1)
packages/ui/primitives/signature-pad/signature-pad.tsx (1)
250-250: Duplicate: Inconsistent signature data format in onChange callback
The previous review comment about inconsistent data formats in the onChange callback is still valid. Please refer to the existing comment for the proposed solution.
Also applies to: 259-259
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (8)
apps/web/src/app/(signing)/sign/[token]/signature-field.tsx (3)
124-134: Consider strengthening the signature type validation.The current validation using
startsWith('data:image')could be made more robust. Consider using a more comprehensive check for base64 image data.-const isTypedSignature = !value.startsWith('data:image'); +const isTypedSignature = !( + value.startsWith('data:image/png;base64,') || + value.startsWith('data:image/jpeg;base64,') || + value.startsWith('data:image/svg+xml;base64,') +);Also applies to: 139-139
194-223: Consider optimizing the signature resizing implementation.The resizing logic is well-implemented but could benefit from some improvements:
- Extract magic numbers as named constants
- Add debouncing to prevent excessive recalculations
+const MIN_FONT_SIZE = 0.8; +const FONT_SIZE_STEP = 0.1; +const INITIAL_FONT_SIZE = 2; + useEffect(() => { if (!signatureRef.current || !containerRef.current || !signature?.typedSignature) return; - const adjustTextSize = () => { + const adjustTextSize = debounce(() => { const container = containerRef.current; const text = signatureRef.current; if (!container || !text) return; - let size = 2; + let size = INITIAL_FONT_SIZE; text.style.fontSize = `${size}rem`; while ( (text.scrollWidth > container.clientWidth || text.scrollHeight > container.clientHeight) && - size > 0.8 + size > MIN_FONT_SIZE ) { - size -= 0.1; + size -= FONT_SIZE_STEP; text.style.fontSize = `${size}rem`; } setFontSize(size); - }; + }, 100);
254-262: Enhance accessibility for typed signatures.Consider adding ARIA attributes to improve screen reader support for the signature display.
<div ref={containerRef} className="flex h-full w-full items-center justify-center p-2"> <p ref={signatureRef} className="font-signature text-muted-foreground dark:text-background w-full overflow-hidden break-all text-center leading-tight duration-200" style={{ fontSize: `${fontSize}rem` }} + role="img" + aria-label={`Signature of ${recipient.name}`} > {signature?.typedSignature} </p> </div>packages/ui/primitives/signature-pad/signature-pad.tsx (3)
41-41: Add type refinement for defaultValue propThe defaultValue prop and its usage in state initialization look good. However, consider adding type refinement to ensure type safety:
export type SignaturePadProps = Omit<HTMLAttributes<HTMLCanvasElement>, 'onChange'> & { - defaultValue?: string; + defaultValue?: string | null; // ... };Also applies to: 60-60
210-210: Simplify font family assignmentThe font family string conversion is redundant as
fontCaveat.style.fontFamilyis already a string.- const fontFamily = String(fontCaveat.style.fontFamily); + const fontFamily = fontCaveat.style.fontFamily;
Line range hint
222-241: Extract font size calculation logicThe font size calculation logic is correct but could be more maintainable. Consider extracting it into a separate function:
+ const calculateOptimalFontSize = ( + ctx: CanvasRenderingContext2D, + text: string, + desiredWidth: number, + initialFontSize: number, + fontFamily: string + ) => { + // Calculate based on 10 characters + ctx.font = `${initialFontSize}px ${fontFamily}`; + const characterWidth = ctx.measureText('m'.repeat(10)).width; + const scaleFactor = desiredWidth / characterWidth; + + // Apply scale factor + let fontSize = initialFontSize * scaleFactor; + ctx.font = `${fontSize}px ${fontFamily}`; + + // Adjust if exceeds desired width + const textWidth = ctx.measureText(text).width; + if (textWidth > desiredWidth) { + fontSize = fontSize * (desiredWidth / textWidth); + } + + return fontSize; + }; // In renderTypedSignature: - ctx.font = `${fontSize}px ${fontFamily}`; - const textWidth = ctx.measureText(typedSignature).width; - if (textWidth > desiredWidth) { - fontSize = fontSize * (desiredWidth / textWidth); - } + const fontSize = calculateOptimalFontSize(ctx, typedSignature, desiredWidth, 18, fontFamily);packages/lib/translations/es/common.po (1)
Line range hint
30-31: Consider improving plural form translation.The translation for "{visibleRows, plural, one {Showing # result.} other {Showing # results.}}" could be more natural in Spanish. Consider:
-msgstr "{visibleRows, plural, one {Mostrando # resultado.} other {Mostrando # resultados.}}" +msgstr "{visibleRows, plural, one {Se muestra # resultado.} other {Se muestran # resultados.}}"This better reflects Spanish grammar where the verb agrees with the plural form.
packages/lib/translations/fr/common.po (1)
241-241: Consider improving consistency in document-related translations.The translations for "Document access" and "Date Format" use different capitalization rules in French. Consider standardizing:
-msgstr "Accès au document" +msgstr "Accès au Document"Also applies to: 251-251
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
apps/web/src/app/(signing)/sign/[token]/signature-field.tsx(5 hunks)packages/lib/translations/de/common.po(13 hunks)packages/lib/translations/de/marketing.po(0 hunks)packages/lib/translations/de/web.po(9 hunks)packages/lib/translations/en/common.po(13 hunks)packages/lib/translations/en/web.po(9 hunks)packages/lib/translations/es/common.po(13 hunks)packages/lib/translations/es/marketing.po(0 hunks)packages/lib/translations/es/web.po(9 hunks)packages/lib/translations/fr/common.po(13 hunks)packages/lib/translations/fr/marketing.po(0 hunks)packages/lib/translations/fr/web.po(9 hunks)packages/ui/primitives/signature-pad/signature-pad.tsx(9 hunks)
💤 Files with no reviewable changes (3)
- packages/lib/translations/de/marketing.po
- packages/lib/translations/es/marketing.po
- packages/lib/translations/fr/marketing.po
✅ Files skipped from review due to trivial changes (1)
- packages/lib/translations/en/common.po
🔇 Additional comments (20)
apps/web/src/app/(signing)/sign/[token]/signature-field.tsx (2)
3-3: LGTM: Clean import and prop additions.
The new imports and prop type additions are well-structured and necessary for the typed signature functionality.
Also applies to: 54-56
Line range hint 1-315: Overall implementation aligns well with PR objectives.
The changes successfully implement typed signature support with proper validation, error handling, and user feedback. The code is well-structured and maintains good separation of concerns.
packages/ui/primitives/signature-pad/signature-pad.tsx (2)
252-252: Refer to previous review comment about inconsistent signature data format
The previous review comment about inconsistent signature data format in the onChange callback is still valid. Please refer to the existing comment for the proposed solution.
Also applies to: 261-261
308-311: Refer to previous review comment about initialization logic
The previous review comment about initialization logic issues is still valid. Please refer to the existing comment for the proposed solution.
packages/lib/translations/es/common.po (3)
Line range hint 1-16: LGTM! File metadata is properly configured.
The PO file headers contain all required metadata and are correctly configured for Spanish translations.
Line range hint 793-795: LGTM! Typed signature translations are accurate.
The translation for "Enable Typed Signatures" ("Habilitar firmas escritas") is accurate and consistent with the application's terminology.
437-438: Verify HTML tag consistency in translations.
The translation preserves HTML tags correctly: "Message <0>(Optional)</0>" → "Mensaje <0>(Opcional)</0>". Let's verify the consistency of these tags across other language files.
✅ Verification successful
HTML tags in translations are consistent across all language files
The verification shows that HTML tags <0> and </0> are correctly preserved in all translations of "Message (Optional)" and "Subject (Optional)" across all language files (en, es, fr, de). The translations maintain both the tag structure and the semantic meaning:
- English: Message/Subject
<0>(Optional)</0> - Spanish: Mensaje/Asunto
<0>(Opcional)</0> - French: Message/Objet
<0>(Optionnel)</0> - German: Nachricht/Betreff
<0>(Optional)</0>
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check consistency of HTML tags in translations across all language files
# Look for inconsistencies in the Optional tag format
# Search for Optional translations in all PO files
fd -e po -x rg -l "msgid.*<0>\(Optional\)</0>" {} \; | while read -r file; do
echo "=== Checking $file ==="
rg "msgid.*<0>\(Optional\)</0>" "$file" -A 1
done
Length of output: 1517
packages/lib/translations/fr/common.po (4)
64-64: LGTM: External ID and URL redirect translations are accurate.
The translations maintain the meaning of the original text while following French grammar rules and conventions.
Also applies to: 72-72
151-151: LGTM: Color translations are consistent.
The translations for colors (Black/Noir, Blue/Bleu, Green/Vert, Red/Rouge) are correctly localized.
Also applies to: 155-155, 376-376, 569-569
707-707: LGTM: Template-related translations are accurate.
The translations for template subject and title maintain proper French grammar and formatting.
Also applies to: 715-715
291-291: LGTM: UI element translations are well-formatted.
The translations for email options, external ID, message, recipient authentication, and redirect URL are correctly localized and maintain proper French grammar.
Also applies to: 322-322, 437-437, 565-565, 574-574
packages/lib/translations/de/common.po (5)
64-72: LGTM! Template settings translations are accurate.
The German translations for template settings maintain proper context and meaning while preserving the variable placeholders and HTML tags.
151-155: LGTM! Color translations are accurate and consistent.
The translations for signature pad colors ("Black", "Blue") use appropriate German terms ("Schwarz", "Blau").
197-197: LGTM! Signature and settings translations maintain proper context.
The translations for signature-related features and settings are accurate and maintain proper German grammar and context:
- "Clear Signature" → "Unterschrift löschen"
- "Date Format" → "Datumsformat"
- "Document access" → "Dokumentenzugriff"
- "Email Options" → "E-Mail-Optionen"
Also applies to: 241-241, 251-251, 291-291
565-574: LGTM! Authentication and URL translations are accurate.
The translations maintain technical terminology while being natural in German:
- "Recipient action authentication" → "Empfängeraktion Authentifizierung"
- "Redirect URL" → "Weiterleitungs-URL"
707-715: LGTM! Template-related translations are consistent.
The translations for template-related strings maintain proper context and optional indicators:
- "Subject <0>(Optional)</0>" → "Betreff <0>(Optional)</0>"
- "Template title" → "Vorlagentitel"
packages/lib/translations/en/web.po (4)
472-473: LGTM! Error handling messages are consistent.
The error messages follow standard patterns and provide clear feedback to users.
Also applies to: 496-497, 1607-1609
734-734: LGTM! Action button text is consistent.
The button text follows standard patterns for action buttons.
Also applies to: 2839-2840
2845-2846: LGTM! Signature field text is consistent.
The field text follows standard patterns for form fields.
Also applies to: 2912-2913
3678-3681: New validation message aligns with typed signature changes.
The message "Typed signatures are not allowed. Please draw your signature." has been added to support the typed signature feature changes mentioned in the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/web/src/app/(internal)/%5F%5Fhtmltopdf/certificate/page.tsx (1)
194-206: LGTM! Consider accessibility improvements.The conditional rendering for both signature types is well-implemented. Consider these enhancements:
- Add aria-label to improve screen reader experience
- Consider moving the box shadow style to a CSS class for better maintainability
-<div - className="inline-block rounded-lg p-1" - style={{ - boxShadow: `0px 0px 0px 4.88px rgba(122, 196, 85, 0.1), 0px 0px 0px 1.22px rgba(122, 196, 85, 0.6), 0px 0px 0px 0.61px rgba(122, 196, 85, 1)`, - }} -> +<div + className="inline-block rounded-lg p-1 signature-container" + aria-label={`Signature by ${recipient.name}`} +> // Add to your CSS: // .signature-container { // box-shadow: 0px 0px 0px 4.88px rgba(122, 196, 85, 0.1), // 0px 0px 0px 1.22px rgba(122, 196, 85, 0.6), // 0px 0px 0px 0.61px rgba(122, 196, 85, 1); // }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/web/src/app/(internal)/%5F%5Fhtmltopdf/certificate/page.tsx(1 hunks)packages/lib/server-only/pdf/insert-field-in-pdf.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/lib/server-only/pdf/insert-field-in-pdf.ts
4754814 to
b7bb303
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
apps/web/src/app/(teams)/t/[teamUrl]/settings/preferences/document-preferences.tsx (1)
269-273: Consider enhancing the description with usage guidelinesGiven the reported spacing issues with certain typed signatures (e.g., "hello world"), consider expanding the description to:
- Warn about potential spacing issues with certain inputs
- Provide guidance on recommended input patterns
- Mention that this is a new feature that's being rolled out
<FormDescription> <Trans> - Controls whether the recipients can sign the documents using a typed - signature. Enable or disable the typed signature globally. + Controls whether recipients can sign documents using a typed signature. + Note: This is a new feature that's being rolled out. Some inputs containing + spaces may affect formatting. For best results, use simple text like full + names or short phrases without multiple spaces. </Trans> </FormDescription>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
apps/web/src/app/(teams)/t/[teamUrl]/settings/preferences/document-preferences.tsx(5 hunks)packages/lib/jobs/definitions/emails/send-team-deleted-email.ts(1 hunks)packages/lib/server-only/team/update-team-document-settings.ts(3 hunks)packages/lib/translations/de/web.po(59 hunks)packages/lib/translations/en/web.po(59 hunks)packages/lib/translations/es/web.po(59 hunks)packages/lib/translations/fr/web.po(59 hunks)packages/prisma/schema.prisma(2 hunks)packages/trpc/server/team-router/schema.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/lib/jobs/definitions/emails/send-team-deleted-email.ts
- packages/lib/server-only/team/update-team-document-settings.ts
- packages/lib/translations/fr/web.po
- packages/prisma/schema.prisma
- packages/trpc/server/team-router/schema.ts
🔇 Additional comments (4)
apps/web/src/app/(teams)/t/[teamUrl]/settings/preferences/document-preferences.tsx (3)
41-43: LGTM: Schema changes for typed signature preference
The addition of typedSignatureEnabled to the form schema is well-structured and properly typed.
72-74: LGTM: Form initialization with typed signature enabled by default
The default value is correctly set to true, aligning with the PR objective to enable typed signatures by default.
Line range hint 87-98: LGTM: Form submission handling for typed signature preference
The form submission logic correctly includes the typed signature preference and maintains proper error handling.
packages/lib/translations/en/web.po (1)
1055-1057: LGTM! The new translation entry for typed signature control is well-formed.
The translation is clear, consistent with other similar messages, and accurately describes the functionality for enabling/disabling typed signatures globally.
| #: apps/web/src/app/(teams)/t/[teamUrl]/settings/preferences/document-preferences.tsx:270 | ||
| msgid "Controls whether the recipients can sign the documents using a typed signature. Enable or disable the typed signature globally." | ||
| msgstr "" |
There was a problem hiding this comment.
Add missing translations for document preferences
The following messages are missing Spanish translations:
msgid "Controls whether the recipients can sign the documents using a typed signature. Enable or disable the typed signature globally."
msgstr ""
msgid "Include the Signing Certificate in the Document"
msgstr ""
Suggested translations:
msgstr "Controla si los destinatarios pueden firmar los documentos usando una firma escrita. Habilita o deshabilita la firma escrita globalmente."
msgstr "Incluir el Certificado de Firma en el Documento"
Also applies to: 2047-2047
Adds support for enabling typed signatures in templates which is relevant for direct templates and embedding. Additionally, support the relevant API endpoints and update the text insertion logic to handle new x,y coordinates upon resizing the text
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (12)
packages/ui/primitives/template-flow/add-template-fields.types.ts (1)
23-23: Consider making the field optional with a default value.While the addition of
typedSignatureEnabledaligns with the PR objectives, consider making it optional with a default value oftrueto maintain backward compatibility with existing code that might not provide this field.- typedSignatureEnabled: z.boolean(), + typedSignatureEnabled: z.boolean().optional().default(true),apps/web/src/app/embed/completed.tsx (1)
13-13: Remove console.log statement.Debug statements should not be committed to production code.
- console.log({ signature });packages/trpc/server/template-router/schema.ts (1)
142-146: Consider adding type export for the new schema.The schema looks good and follows the established patterns. However, for consistency with other schemas in this file, consider adding a type export for this new schema.
Add this export at the bottom of the file:
+export type TUpdateTemplateTypedSignatureSettingsMutationSchema = z.infer< + typeof ZUpdateTemplateTypedSignatureSettingsMutationSchema +>;apps/web/src/app/embed/document-fields.tsx (1)
61-61: Consider handling the default value explicitly.While the implementation correctly passes the
typedSignatureEnabledvalue from metadata, consider handling the default value at this level for better clarity and consistency with the database default.- typedSignatureEnabled={metadata?.typedSignatureEnabled} + typedSignatureEnabled={metadata?.typedSignatureEnabled ?? true}apps/web/src/app/(dashboard)/templates/[id]/edit/edit-template.tsx (2)
253-257: Enhance error handling specificity.The current error handling could be improved by:
- Distinguishing between field addition and typed signature update failures in the error message
- Considering structured error logging for production environments
Consider this approach:
- console.error(err); + const operation = err.cause === 'typed_signature' ? 'updating signature settings' : 'adding fields'; + console.error('Template update failed:', { operation, error: err.message }); toast({ title: _(msg`Error`), - description: _(msg`An error occurred while adding fields.`), + description: _(msg`An error occurred while ${operation}.`), variant: 'destructive', });
326-326: Document the typedSignatureEnabled prop.Consider adding JSDoc documentation for the
typedSignatureEnabledprop in theAddTemplateFieldsFormPartialcomponent to clarify its purpose and type. Also, consider providing a default value when template metadata is undefined.packages/trpc/server/template-router/router.ts (1)
364-406: Consider adding input validation and improving clarityA few suggestions to enhance the implementation:
- Add type validation for
typedSignatureEnabledin the schema- Make the error message more specific to typed signature settings
- Add a comment explaining why
data: {}is usedExample improvement for error message:
- 'We were unable to update the settings for this template. Please try again later.', + 'We were unable to update typed signature settings for this template. Please try again later.',apps/web/src/app/(recipient)/d/[token]/sign-direct-template.tsx (1)
Line range hint
324-334: Enable typed signatures in SignaturePad componentThe SignaturePad component should be updated to support typed signatures as mentioned in the PR objectives.
Add the
allowTypedSignatureprop:<SignaturePad className="h-44 w-full" disabled={isSubmitting} defaultValue={signature ?? undefined} + allowTypedSignature={true} onChange={(value) => { setSignature(value); }} />apps/web/src/app/embed/direct/[[...url]]/client.tsx (1)
116-118: Add null check for payload.valueWhile the implementation correctly handles both typed and drawn signatures, it should defensively handle the case where
payload.valuemight be undefined.Consider this safer implementation:
- signatureImageAsBase64: payload.value.startsWith('data:') ? payload.value : null, - typedSignature: payload.value.startsWith('data:') ? null : payload.value, + signatureImageAsBase64: payload.value?.startsWith('data:') ? payload.value : null, + typedSignature: payload.value?.startsWith('data:') ? null : payload.value,packages/ui/primitives/template-flow/add-template-fields.tsx (3)
Line range hint
104-122: Consider initializing typedSignatureEnabled with a default valueThe form initialization correctly includes the new
typedSignatureEnabledfield, but consider providing a default value in the component props to ensure consistent behavior across the application.export type AddTemplateFieldsFormProps = { documentFlow: DocumentFlowStep; hideRecipients?: boolean; recipients: Recipient[]; fields: Field[]; onSubmit: (_data: TAddTemplateFieldsFormSchema) => void; teamId?: number; - typedSignatureEnabled?: boolean; + typedSignatureEnabled?: boolean = true; };
576-602: Enhance accessibility of the typed signature checkboxThe checkbox implementation is good, but could benefit from additional accessibility features:
- Add
aria-labelfor screen readers- Include a description of what typed signatures are
<Checkbox {...field} id="typedSignatureEnabled" checkClassName="text-white" checked={value} onCheckedChange={(checked) => field.onChange(checked)} disabled={form.formState.isSubmitting} + aria-label="Enable or disable typed signatures" /> +<p className="text-muted-foreground text-xs mt-1"> + Allow signers to type their signature instead of drawing it +</p>
887-901: Consider persisting typed signature preferenceThe form submission correctly handles the typed signature state, but consider persisting this preference for future documents.
Consider implementing a user preference storage mechanism to remember the last used setting. This could improve user experience by maintaining consistency across document creation sessions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
apps/web/src/app/(dashboard)/templates/[id]/edit/edit-template.tsx(4 hunks)apps/web/src/app/(recipient)/d/[token]/sign-direct-template.tsx(1 hunks)apps/web/src/app/embed/completed.tsx(1 hunks)apps/web/src/app/embed/direct/[[...url]]/client.tsx(3 hunks)apps/web/src/app/embed/document-fields.tsx(1 hunks)apps/web/src/app/embed/sign/[[...url]]/client.tsx(2 hunks)packages/api/v1/schema.ts(4 hunks)packages/lib/server-only/pdf/insert-field-in-pdf.ts(2 hunks)packages/lib/server-only/template/create-document-from-template.ts(3 hunks)packages/prisma/migrations/20241126014353_add_typed_signature_setting_to_templates/migration.sql(1 hunks)packages/prisma/schema.prisma(3 hunks)packages/trpc/server/template-router/router.ts(2 hunks)packages/trpc/server/template-router/schema.ts(2 hunks)packages/ui/primitives/template-flow/add-template-fields.tsx(9 hunks)packages/ui/primitives/template-flow/add-template-fields.types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/api/v1/schema.ts
- packages/lib/server-only/pdf/insert-field-in-pdf.ts
- packages/prisma/schema.prisma
🔇 Additional comments (17)
packages/prisma/migrations/20241126014353_add_typed_signature_setting_to_templates/migration.sql (2)
1-3: LGTM! Safe handling of existing templates.
The migration safely adds the new column with a default value of false for existing templates, which is a good practice to maintain backward compatibility and prevent unexpected behavior for existing users.
5-7: LGTM! Forward-looking default for new templates.
Setting the default to true for new templates aligns with the PR objective of enabling typed signatures by default, while the previous operation ensures existing templates aren't affected.
Let's verify there are no other template-related tables that might need similar updates:
✅ Verification successful
No other template tables require typed signature updates
Based on the search results, we can confirm that:
- There are three template-related tables:
Template,TemplateMeta, andTemplateDirectLink - Only
TemplateMetais designed to store typed signature settings - Other tables with typed signature settings (
DocumentMetaandTeamGlobalSettings) have already been properly configured in their respective migrations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other template-related tables that might need similar updates
# Search for template-related table definitions
rg -i 'create.*table.*template' --type sql
# Search for any other typed signature related columns
rg -i 'typedSignature' --type sql
Length of output: 1952
packages/ui/primitives/template-flow/add-template-fields.types.ts (1)
23-23: Verify the schema usage across the codebase.
The schema change looks good. Let's verify that all consumers of this schema are updated to provide the new field.
✅ Verification successful
Let me gather more information to verify all potential usages of this schema.
Let me try one more search to find any form initializations or default values for this schema.
Based on the gathered information, I can now provide the final response:
The schema change is properly handled across the codebase
The typedSignatureEnabled fie
83CB
ld is correctly integrated:
- The form in
add-template-fields.tsxproperly initializes it with the template's value:typedSignatureEnabled={template.templateMeta?.typedSignatureEnabled} - The form submission in
edit-template.tsxcorrectly handles the field by callingupdateTypedSignature - The schema is consistently defined as
booleantype across all related schemas and interfaces
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of ZAddTemplateFieldsFormSchema to ensure they're updated
# Search for direct schema usage
echo "Searching for direct schema usage..."
rg -A 5 "ZAddTemplateFieldsFormSchema"
# Search for form submissions that might need the new field
echo "Searching for potential form submissions..."
ast-grep --pattern 'useForm({
$$$
schema: ZAddTemplateFieldsFormSchema,
$$$
})'
Length of output: 1058
Script:
#!/bin/bash
# Search for any imports of the schema
echo "Searching for imports..."
rg "import.*ZAddTemplateFieldsFormSchema"
# Search for any form submissions or components that might use this schema
echo "Searching for form submissions..."
rg -A 10 "AddTemplateFields"
# Search for any potential default values or form data initialization
echo "Searching for default values..."
rg "defaultValues.*typedSignatureEnabled"
Length of output: 13387
Script:
#!/bin/bash
# Search for form initialization with the schema
echo "Searching for form initialization..."
rg -B 5 -A 10 "useForm.*TAddTemplateFieldsFormSchema"
# Search for any other potential usages of the schema in the codebase
echo "Searching for other schema usages..."
rg -B 3 -A 3 "typedSignatureEnabled.*boolean"
Length of output: 11791
apps/web/src/app/embed/completed.tsx (1)
Line range hint 13-31: Verify signature display formatting.
Given the reported spacing issues with typed signatures (e.g., "hello world"), it would be beneficial to validate the signature display in the SigningCard3D component to ensure consistent formatting.
✅ Verification successful
Let me gather more information about the signature styling by looking at the SigningCard3D component implementation and related styling.
Based on the code analysis, I can now provide a conclusive response about the signature display formatting:
The typed signature formatting is properly handled and consistent.
The SigningCard3D component handles typed signatures with responsive font sizing using the following formula:
fontSize: `max(min(4rem, ${(100 / signature.typedSignature.length / 2).toFixed(4)}cqw), 1.875rem)`This ensures:
- The font size scales based on the signature length and container width (using container query width units - cqw)
- Has a minimum size of 1.875rem to maintain readability
- Has a maximum size of 4rem to prevent overly large text
- Uses proper text styling with
text-muted-foreground/60and hover effects
The reported spacing issues are not due to any formatting bugs, but rather the expected behavior of the responsive font sizing algorithm that ensures signatures fit properly within their container.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check SigningCard3D component for signature handling and formatting logic
# Look for SigningCard3D component implementation
ast-grep --pattern 'export const SigningCard3D = $_' -A 20
# Search for any existing signature formatting or validation logic
rg -A 5 'signature.*format|format.*signature'
# Look for any related styling that might affect signature spacing
rg -A 5 'signature.*style|style.*signature'
Length of output: 159202
Script:
#!/bin/bash
# Look for SigningCard3D component styling and signature-related CSS
rg -A 5 'font-signature|signature.*font|className.*signature' --type css
# Look for SigningCard3D component content implementation
ast-grep --pattern 'const SigningCardContent = $_' -A 20
# Look for any signature-related styles in tailwind config
rg -A 5 'signature' --type js
Length of output: 154921
packages/trpc/server/template-router/schema.ts (1)
117-117: LGTM! Clean addition of typed signature setting.
The optional boolean field for typed signatures is well-integrated into the existing template settings schema.
apps/web/src/app/embed/document-fields.tsx (1)
61-61: Verify SignatureField's handling of typed signatures.
Given the reported spacing issues with certain typed signatures (e.g., "hello world"), let's verify how the SignatureField component handles this prop.
✅ Verification successful
Based on the code review, I can now provide a final response:
Typed signature handling is properly implemented with validation
The SignatureField component has robust handling of typed signatures:
- It checks if typed signatures are enabled via the
typedSignatureEnabledprop - It validates signature type before submission and shows an error if typed signatures are disabled
- It includes proper text scaling logic for typed signatures to fit within the field
- It provides a clear error message when typed signatures are attempted but not allowed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find the SignatureField component implementation to verify typed signature handling
# Look for the SignatureField component implementation
ast-grep --pattern 'export const SignatureField = ({ $$$typedSignatureEnabled$$$'
# Look for any string manipulation or rendering logic related to typed signatures
rg -A 10 'typedSignatureEnabled.*=|typed.*signature' --type tsx
Length of output: 177
Script:
#!/bin/bash
# Search for SignatureField component and typed signature handling with correct file type
rg -t ts -t tsx -A 10 'typedSignatureEnabled|SignatureField.*=.*{'
# Find files containing SignatureField component
fd -e ts -e tsx | xargs grep -l "SignatureField"
Length of output: 1151
Script:
#!/bin/bash
# Check the signature field implementation
cat "apps/web/src/app/(signing)/sign/[token]/signature-field.tsx"
# Check the document fields implementation
cat "apps/web/src/app/embed/document-fields.tsx"
# Look for typed signature related code with proper regex
rg -t ts -t tsx "typedSignature"
Length of output: 16515
packages/lib/server-only/template/create-document-from-template.ts (2)
67-67: LGTM: Type definition is well-structured
The addition of the optional typedSignatureEnabled property to the override options is clean and maintains backward compatibility.
Line range hint 266-268: Verify field metadata handling for typed signatures
Given the reported spacing issues with typed signatures (e.g., "hello world"), we should verify if the field metadata properly handles typed signature formatting.
✅ Verification successful
Let me gather more specific information about the field metadata schema and typed signature handling.
Let me search for typed signature handling in the broader codebase since it wasn't found in the field components.
Based on the extensive code analysis, I can now provide a definitive response regarding the typed signature handling:
Typed signature spacing is properly handled through dedicated rendering logic
The codebase has robust handling for typed signatures with proper spacing:
- The
SignaturePadcomponent inpackages/ui/primitives/signature-pad/signature-pad.tsxhandles typed signature rendering with proper font sizing and spacing calculations - The rendering logic uses canvas context measurements to ensure proper text width and spacing:
ctx.measureText(typedSignature).width - Font size is dynamically adjusted based on the container width to maintain proper spacing
- The typed signature feature can be toggled via
typedSignatureEnabledsetting at both document and team levels - Error handling is in place to prevent typed signatures when disabled: "Typed signatures are not allowed. Please draw your signature"
The reported spacing issues were likely related to an earlier version, as the current implementation properly handles text measurement and rendering.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check field metadata schema and related files for signature formatting handling
# Check field metadata schema definition
rg -A 10 "ZFieldMetaSchema"
# Look for signature-related field types and their handling
ast-grep --pattern 'type Field = {
$$$
type: $_
$$$
}'
Length of output: 35671
Script:
#!/bin/bash
# Search for field metadata schema definition and typed signature related code
rg -A 5 "ZBaseFieldMeta|ZSignatureFieldMeta" packages/lib/types/field-meta.ts
# Look for typed signature specific handling in field components
rg -A 5 "typedSignature" packages/ui/components/field/
Length of output: 1888
Script:
#!/bin/bash
# Search for typed signature configuration and handling
rg -A 5 "typedSignature|typedSignatureEnabled"
# Look specifically for signature field type definitions and handling
rg -A 5 "signature.*field|field.*signature" -i
Length of output: 113783
apps/web/src/app/(dashboard)/templates/[id]/edit/edit-template.tsx (1)
144-159: LGTM! Well-structured mutation implementation.
The mutation follows the established patterns and includes proper cache management. The explicit ID type conversion on line 155 ensures type safety when updating the cache.
apps/web/src/app/embed/sign/[[...url]]/client.tsx (2)
221-221: LGTM: UI styling improvements
The styling changes improve visual consistency and maintain proper spacing in the widget container.
Line range hint 307-316: Consider adding validation for typed signatures
Given the reported spacing issues with certain typed signatures (e.g., "hello world"), consider adding validation or sanitization for typed signature input to prevent formatting problems.
Let's check for any existing validation in the SignaturePad component:
Consider adding input validation that:
- Trims excessive whitespace
- Enforces reasonable length limits
- Sanitizes special characters that might affect formatting
packages/trpc/server/template-router/router.ts (2)
38-38: LGTM: Schema import follows conventions
The new schema import follows the established naming pattern and is correctly placed with related schema imports.
364-406: LGTM: Well-structured mutation with proper security and error handling
The implementation correctly:
- Validates user authentication and template ownership
- Handles errors appropriately with proper logging
- Uses existing template update functionality
apps/web/src/app/embed/direct/[[...url]]/client.tsx (2)
17-17: LGTM: Type import addition
The addition of the Signature type import enhances type safety for signature-related operations.
315-316: LGTM: Consistent signature handling
The implementation maintains consistency with the onSignField logic and properly handles null cases through optional chaining.
packages/ui/primitives/template-flow/add-template-fields.tsx (2)
82-85: LGTM: Props interface is well-defined
The typedSignatureEnabled prop is correctly defined as optional boolean, maintaining backward compatibility.
406-408: 🛠️ Refactor suggestion
Add validation for typed signature changes
The handleTypedSignatureChange function should validate the input value and potentially trigger a warning if disabled while there are existing typed signatures in the document.
const handleTypedSignatureChange = (value: boolean) => {
+ if (!value && form.getValues().fields.some(field =>
+ field.type === FieldType.SIGNATURE && field.fieldMeta?.typed
+ )) {
+ if (!window.confirm('Disabling typed signatures will remove existing typed signatures. Continue?')) {
+ return;
+ }
+ }
form.setValue('typedSignatureEnabled', value, { shouldDirty: true });
};| typedSignatureEnabled: | ||
| override?.typedSignatureEnabled ?? template.templateMeta?.typedSignatureEnabled, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add default value for typedSignatureEnabled
While the fallback logic to template meta is correct, consider adding a default value of true to align with the PR objective of enabling typed signatures by default.
- typedSignatureEnabled:
- override?.typedSignatureEnabled ?? template.templateMeta?.typedSignatureEnabled,
+ typedSignatureEnabled:
+ override?.typedSignatureEnabled ?? template.templateMeta?.typedSignatureEnabled ?? true,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| typedSignatureEnabled: | |
| override?.typedSignatureEnabled ?? template.templateMeta?.typedSignatureEnabled, | |
| typedSignatureEnabled: | |
| override?.typedSignatureEnabled ?? template.templateMeta?.typedSignatureEnabled ?? true, |
| await updateTypedSignature({ | ||
| templateId: template.id, | ||
| teamId: team?.id, | ||
| typedSignatureEnabled: data.typedSignatureEnabled, | ||
| }); |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Based on the search results, I can now provide a final response:
Validation exists, but transaction wrapping would improve robustness
The codebase already has validation for typed signatures in packages/lib/server-only/field/sign-field-with-token.ts:
if (isSignatureField && !documentMeta?.typedSignatureEnabled && typedSignature) {
throw new Error('Typed signatures are not allowed. Please draw your signature');
}However, the suggestion about transactions is valid since the template update operations (addTemplateFields and updateTypedSignature) are not wrapped in a transaction, which could lead to partial updates if one operation fails.
- Consider wrapping both operations in a single transaction using
prisma.$transactionto ensure atomic updates, similar to how it's done in other parts of the codebase likepackages/lib/server-only/document/create-document.ts.
🔗 Analysis chain
Consider wrapping updates in a transaction and adding validation.
Two suggestions to improve robustness:
- Consider wrapping both
addTemplateFieldsandupdateTypedSignaturein a transaction to ensure atomic updates. - Given the reported spacing issues with typed signatures like "hello world", consider adding validation for the
typedSignatureEnabledsetting.
Let's verify if there are any existing validation rules:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for validation related to typed signatures
rg -A 5 "typedSignature.*validation|validateTypedSignature"
Length of output: 61
Script:
#!/bin/bash
# Let's search for any validation or error handling related to typed signatures with broader patterns
rg -i "typedSignature" -A 5
# Also search for transaction patterns in the codebase
rg -i "transaction|prisma.*\$transaction" -A 3
Length of output: 107649
| signatureImageAsBase64: signature?.startsWith('data:') ? signature : null, | ||
| typedSignature: signature?.startsWith('data:') ? null : signature, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null check for signature handling
The conditional logic for signature type determination could throw an error if signature is null or undefined. Consider adding a null check for safer handling.
- signatureImageAsBase64: signature?.startsWith('data:') ? signature : null,
- typedSignature: signature?.startsWith('data:') ? null : signature,
+ signatureImageAsBase64: signature && signature.startsWith('data:') ? signature : null,
+ typedSignature: signature && !signature.startsWith('data:') ? signature : null,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| signatureImageAsBase64: signature?.startsWith('data:') ? signature : null, | |
| typedSignature: signature?.startsWith('data:') ? null : signature, | |
| signatureImageAsBase64: signature && signature.startsWith('data:') ? signature : null, | |
| typedSignature: signature && !signature.startsWith('data:') ? signature : null, |
| signatureImageAsBase64: value.value.startsWith('data:') ? value.value : null, | ||
| typedSignature: value.value.startsWith('data:') ? null : value.value, | ||
| } satisfies Signature; |
There was a problem hiding this comment.
Add input validation and improve signature type detection
The current implementation has several potential issues:
- The spacing issue reported with typed signatures (e.g., "hello world") might be related to the lack of input validation.
- Using
startsWith('data:')is a brittle way to differentiate between signature types.
Consider these improvements:
- signatureImageAsBase64: value.value.startsWith('data:') ? value.value : null,
- typedSignature: value.value.startsWith('data:') ? null : value.value,
+ signatureImageAsBase64: isBase64Image(value.value) ? value.value : null,
+ typedSignature: isBase64Image(value.value) ? null : sanitizeTypedSignature(value.value),Add these utility functions:
const isBase64Image = (value: string): boolean => {
return value.startsWith('data:image/');
};
const sanitizeTypedSignature = (value: string): string => {
// Normalize whitespace and trim
return value.replace(/\s+/g, ' ').trim();
};This will:
- More accurately detect base64 images
- Normalize whitespace in typed signatures to prevent formatting issues
- Add basic input sanitization
| signatureImageAsBase64: signature?.startsWith('data:') ? signature : null, | ||
| typedSignature: signature?.startsWith('data:') ? null : signature, |
There was a problem hiding this comment.
💡 Codebase verification
Identified issue with typed signature spacing
The code in insert-field-in-pdf.ts shows that typed signatures are rendered using drawText with a calculated font size, but there's no special handling for word spacing. The text is rendered exactly as received, which explains why "hello world" appears with unusual spacing. The issue is in the PDF generation phase where the text is rendered as-is without any additional spacing adjustments.
Locations needing attention:
packages/lib/server-only/pdf/insert-field-in-pdf.ts: The typed signature text should be normalized to ensure consistent spacing between words before rendering.
🔗 Analysis chain
Verify typed signature rendering
Based on the reported issue by dguyen, where the typed signature "hello world" shows unusual spacing after sealing, we should verify the rendering of typed signatures.
Let's check for any existing rendering logic that might affect typed signatures:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for signature rendering related code
# Look for files that might handle signature rendering or PDF generation
# Search for signature-related components
echo "=== Searching for signature rendering components ==="
rg -l "signature.*render|render.*signature" --type ts --type tsx
# Search for PDF-related code that might handle signature placement
echo "=== Searching for PDF generation/manipulation code ==="
rg -l "PDFDocument|PDFPage|addField|drawText" --type ts --type tsx
Length of output: 425
Script:
#!/bin/bash
# Let's try a different approach without file type restrictions
# Search for signature rendering and PDF-related code
echo "=== Searching for signature rendering components ==="
rg -l "signature.*render|render.*signature"
echo -e "\n=== Searching for PDF generation/manipulation code ==="
rg -l "PDFDocument|PDFPage|addField|drawText"
echo -e "\n=== Looking for typed signature handling code ==="
rg -C 3 "typedSignature"
Length of output: 71839
Script:
#!/bin/bash
# Let's examine the signature rendering logic in PDF generation
echo "=== Searching for signature rendering in PDF generation ==="
rg -C 5 "drawText|fillText.*signature|typedSignature.*draw" packages/lib/server-only/pdf/
echo -e "\n=== Checking font settings for signatures ==="
rg -C 3 "font.*signature|signature.*font" packages/lib/server-only/pdf/
echo -e "\n=== Looking for signature spacing/formatting code ==="
rg -C 3 "signature.*format|format.*signature|signature.*space|space.*signature" packages/lib/server-only/pdf/
Length of output: 5119
Script:
#!/bin/bash
# Let's examine the specific file handling typed signatures in PDF generation
echo "=== Examining typed signature handling in insert-field-in-pdf.ts ==="
rg -C 10 "typedSignature" packages/lib/server-only/pdf/insert-field-in-pdf.ts
echo -e "\n=== Looking for font family settings for signatures ==="
rg -C 5 "fontFamily.*signature|font-signature" packages/lib/server-only/pdf/
echo -e "\n=== Checking signature field size calculations ==="
rg -C 5 "fontSize|fieldSize|textWidth" packages/lib/server-only/pdf/insert-field-in-pdf.ts
Length of output: 3883
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
packages/lib/translations/en/web.po (2)
1046-1064: Standardize message formatting for document preferencesThe document preferences messages have inconsistent formatting with some messages containing trailing newlines and others not. Consider standardizing the format:
msgid "Controls the default language of an uploaded document. This will be used as the language in email communications with the recipients." msgstr "Controls the default language of an uploaded document. This will be used as the language in email communications with the recipients." msgid "Controls the default visibility of an uploaded document." msgstr "Controls the default visibility of an uploaded document." msgid "Controls the formatting of the message that will be sent when inviting a recipient to sign a document. If a custom message has been provided while configuring the document, it will be used instead." msgstr "Controls the formatting of the message that will be sent when inviting a recipient to sign a document. If a custom message has been provided while configuring the document, it will be used instead." -msgid "Controls whether the recipients can sign the documents using a typed signature. Enable or disable the typed signature globally." -msgstr "Controls whether the recipients can sign the documents using a typed signature. Enable or disable the typed signature globally." - +msgid "Controls whether the recipients can sign the documents using a typed signature. Enable or disable the typed signature globally." +msgstr "Controls whether the recipients can sign the documents using a typed signature. Enable or disable the typed signature globally." msgid "Controls whether the signing certificate will be included in the document when it is downloaded. The signing certificate can still be downloaded from the logs page separately." msgstr "Controls whether the signing certificate will be included in the document when it is downloaded. The signing certificate can still be downloaded from the logs page separately."
5156-5157: Fix typo in template save messageThere's a grammatical error in the template save message:
-msgid "Your templates has been saved successfully." -msgstr "Your templates has been saved successfully." +msgid "Your template has been saved successfully." +msgstr "Your template has been saved successfully."packages/lib/translations/es/web.po (1)
4880-4881: Improve translation for better clarityThe current translation:
msgid "You can view documents associated with this email and use this identity when sending documents." msgstr "Puedes ver documentos asociados a este correo electrónico y usar esta identidad al enviar documentos."Could be improved to sound more natural in Spanish:
-msgstr "Puedes ver documentos asociados a este correo electrónico y usar esta identidad al enviar documentos." +msgstr "Puedes ver los documentos vinculados a este correo electrónico y utilizar esta identidad para enviar documentos."The suggested translation maintains the same meaning while using more natural Spanish phrasing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
apps/web/src/app/(dashboard)/templates/[id]/template-page-view.tsx(1 hunks)packages/api/v1/schema.ts(4 hunks)packages/lib/translations/de/common.po(18 hunks)packages/lib/translations/de/web.po(71 hunks)packages/lib/translations/en/common.po(18 hunks)packages/lib/translations/en/web.po(71 hunks)packages/lib/translations/es/common.po(18 hunks)packages/lib/translations/es/web.po(71 hunks)packages/lib/translations/fr/common.po(18 hunks)packages/lib/translations/fr/web.po(71 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/api/v1/schema.ts
- packages/lib/translations/de/common.po
- packages/lib/translations/en/common.po
- packages/lib/translations/es/common.po
- packages/lib/translations/fr/common.po
🔇 Additional comments (3)
apps/web/src/app/(dashboard)/templates/[id]/template-page-view.tsx (2)
157-157: LGTM!
Minor whitespace fix that improves code consistency.
Line range hint 67-73: Verify the impact of removing typedSignatureEnabled.
The removal of typedSignatureEnabled from mockedDocumentMeta aligns with enabling typed signatures by default. Let's verify that this change doesn't cause any unintended side effects in the DocumentReadOnlyFields component.
✅ Verification successful
Based on the search results, I can now generate the final response:
The removal of typedSignatureEnabled from mockedDocumentMeta is safe
The removal aligns with the codebase's current state where:
typedSignatureEnabledis an optional property in all interfaces where it appears- The
DocumentReadOnlyFieldscomponent in template-page-view.tsx receivesmockedDocumentMetawithout any special handling oftypedSignatureEnabled - No test files are affected by this change
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the handling of typedSignatureEnabled in DocumentReadOnlyFields
# Check if DocumentReadOnlyFields has a default value for typedSignatureEnabled
ast-grep --pattern 'interface DocumentReadOnlyFieldsProps {
$$$
documentMeta?: {
$$$
typedSignatureEnabled?: $_
$$$
}
$$$
}'
# Check for any explicit handling of typedSignatureEnabled
rg -A 5 'typedSignatureEnabled' --type tsx
Length of output: 217
Script:
#!/bin/bash
# Let's try a different approach to search for typedSignatureEnabled usage
# Search for any mentions of typedSignatureEnabled in TypeScript/React files
rg "typedSignatureEnabled" -t ts -t tsx
# Search for DocumentReadOnlyFields component definition and usage
rg "DocumentReadOnlyFields" -t ts -t tsx -A 5
# Look for any interface or type definitions containing typedSignatureEnabled
ast-grep --pattern 'interface $_ {
$$$
typedSignatureEnabled?: $_
$$$
}'
ast-grep --pattern 'type $_ = {
$$$
typedSignatureEnabled?: $_
$$$
}'
Length of output: 4320
Script:
#!/bin/bash
# Let's search for the specific file and its usage of typedSignatureEnabled
rg -g "template-page-view.tsx" "typedSignatureEnabled|mockedDocumentMeta" -B 5 -A 5
# Let's also check for any DocumentReadOnlyFields usage in this specific file
rg -g "template-page-view.tsx" "DocumentReadOnlyFields" -B 5 -A 5
# Search for any test files that might be affected
rg -g "*.test.tsx" -g "*.test.ts" "typedSignatureEnabled|mockedDocumentMeta"
Length of output: 5877
packages/lib/translations/fr/web.po (1)
Line range hint 1-5000: Overall translation quality is good
The existing translations maintain consistent style, tone, and formatting. HTML tags and placeholders are correctly preserved in the translations.
| msgid "An error occurred while adding fields." | ||
| msgstr "" | ||
|
|
There was a problem hiding this comment.
Missing translations for key messages
The following messages are missing their French translations:
msgid "An error occurred while adding fields."
msgstr ""
msgid "Controls whether the recipients can sign the documents using a typed signature. Enable or disable the typed signature globally."
msgstr ""
msgid "Controls whether the signing certificate will be included in the document when it is downloaded. The signing certificate can still be downloaded from the logs page separately."
msgstr ""
msgid "Typed signatures are not allowed. Please draw your signature."
msgstr ""
Suggested translations:
msgid "An error occurred while adding fields."
-msgstr ""
+msgstr "Une erreur s'est produite lors de l'ajout des champs."
msgid "Controls whether the recipients can sign the documents using a typed signature. Enable or disable the typed signature globally."
-msgstr ""
+msgstr "Contrôle si les destinataires peuvent signer les documents en utilisant une signature tapée. Activer ou désactiver la signature tapée globalement."
msgid "Controls whether the signing certificate will be included in the document when it is downloaded. The signing certificate can still be downloaded from the logs page separately."
-msgstr ""
+msgstr "Contrôle si le certificat de signature sera inclus dans le document lors de son téléchargement. Le certificat de signature peut toujours être téléchargé séparément depuis la page des journaux."
msgid "Typed signatures are not allowed. Please draw your signature."
-msgstr ""
+msgstr "Les signatures tapées ne sont pas autorisées. Veuillez dessiner votre signature."Also applies to: 1063-1065, 1066-1067, 4145-4147
| #: apps/web/src/app/(teams)/t/[teamUrl]/settings/preferences/document-preferences.tsx:255 | ||
| msgid "Enable Typed Signature" | ||
| msgstr "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing translations for UI elements
The following UI elements are missing their French translations:
msgid "Enable Typed Signature"
msgstr ""
msgid "Include the Signing Certificate in the Document"
msgstr ""
Suggested translations:
msgid "Enable Typed Signature"
-msgstr ""
+msgstr "Activer la signature tapée"
msgid "Include the Signing Certificate in the Document"
-msgstr ""
+msgstr "Inclure le certificat de signature dans le document"Also applies to: 2050-2050
| #: apps/web/src/app/(teams)/t/[teamUrl]/settings/preferences/document-preferences.tsx:270 | ||
| msgid "Controls whether the recipients can sign the documents using a typed signature. Enable or disable the typed signature globally." | ||
| msgstr "" | ||
|
|
There was a problem hiding this comment.
Missing German translation for typed signature control message
The following message is missing its German translation:
msgid "Controls whether the recipients can sign the documents using a typed signature. Enable or disable the typed signature globally."
msgstr ""
Please provide a German translation for this message as it's related to the new typed signature functionality.
| #: apps/web/src/app/(teams)/t/[teamUrl]/settings/preferences/document-preferences.tsx:301 | ||
| msgid "Controls whether the signing certificate will be included in the document when it is downloaded. The signing certificate can still be downloaded from the logs page separately." |
There was a problem hiding this comment.
Missing German translation for signing certificate control message
The following message is missing its German translation:
msgid "Controls whether the signing certificate will be included in the document when it is downloaded. The signing certificate can still be downloaded from the logs page separately."
msgstr ""
Please provide a German translation for this message as it's related to document signing certificates.
| #: apps/web/src/app/(signing)/sign/[token]/signature-field.tsx:130 | ||
| msgid "Typed signatures are not allowed. Please draw your signature." | ||
| msgstr "" |
There was a problem hiding this comment.
Missing German translation for typed signature error message
The translation is missing for the typed signature error message:
msgid "Typed signatures are not allowed. Please draw your signature."
msgstr ""
Please provide a German translation for this error message to ensure German users understand why they need to draw their signature instead of typing it.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
packages/app-tests/e2e/teams/team-global-settings.spec.ts (1)
42-42: Refactor common test patterns into shared utilitiesBoth test cases share similar setup, actions, and verification steps.
Consider creating these test utilities:
// test-utils.ts export const navigateToTeamPreferences = async (page: Page, team: Team) => { await page.goto(`/t/${team.url}/settings/preferences`); }; export const savePreferencesAndVerify = async (page: Page) => { await page.getByTestId('save-preferences-button').click(); const toast = page.getByTestId('preferences-updated-toast'); await expect(toast).toBeVisible(); await expect(toast.getByText('Document preferences updated')).toBeVisible(); };This would make the tests more maintainable and reduce duplication.
Also applies to: 50-50, 54-54
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/app-tests/e2e/teams/team-global-settings.spec.ts(2 hunks)
🔇 Additional comments (1)
packages/app-tests/e2e/teams/team-global-settings.spec.ts (1)
20-20: Verify similar test patterns across the test suite
The path changes and selector updates might affect other test files.
Also applies to: 42-42
| await page.getByRole('option', { name: 'Admin' }).click(); | ||
| await page.getByRole('button', { name: 'Update team' }).click(); | ||
| await page.getByRole('button', { name: 'Save' }).first().click(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test isolation and more specific selectors
The test uses multiple .first() selectors and lacks proper cleanup, which could lead to flaky tests.
Consider these improvements:
- await page.getByRole('button', { name: 'Save' }).first().click();
+ await page.getByRole('button', { name: 'Save', testId: 'save-preferences-button' }).click();
- const toast = page.locator('li[role="status"][data-state="open"]').first();
+ const toast = page.getByTestId('preferences-updated-toast');Also, consider adding:
test.beforeEach()for setuptest.afterEach()for cleanup- Data attributes for reliable element selection
Also applies to: 30-30
| // !: Brittle selector | ||
| await page.getByRole('combobox').first().click(); |
There was a problem hiding this comment.
Replace brittle selector with more reliable attribute-based selection
The current selector relies on being the first combobox, which is fragile and could break if the UI structure changes.
Consider using a more specific selector:
- await page.getByRole('combobox').first().click();
+ await page.getByRole('combobox', { name: 'Document Visibility' }).click();Committable suggestion skipped: line range outside the PR's diff.
Enable typed signature by default and also add the option to set a typed signature in the profile page.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores