Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull Request Overview
This PR upgrades OpenSign to version 2.30.0, introducing significant improvements to document conversion, widget management, and code quality. Key changes include a rewritten DOCX-to-PDF conversion endpoint with concurrency controls, enhanced widget validation in signature flows, and numerous code optimizations across the codebase.
- Server: Improved DOCX-to-PDF conversion with memory-based processing, concurrency limiting, and timeout handling
- Frontend: Enhanced widget validation, formula support for number widgets, improved accessibility, and code refactoring
- Dependencies: Updated multiple packages including AWS SDK, Parse Server, and development tools
Reviewed Changes
Copilot reviewed 29 out of 30 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/OpenSignServer/package.json | Version bump to 2.30.0 and dependency updates |
| apps/OpenSignServer/index.js | Added security configuration for authentication adapters |
| apps/OpenSignServer/cloud/parsefunction/saveAsTemplate.js | Reset widget response field when saving templates |
| apps/OpenSignServer/cloud/customRoute/docxtopdf.js | Complete rewrite with memory storage, concurrency limiting, and improved error handling |
| apps/OpenSign/src/utils/widgetUtils.js | Added applyNumberFormulasToPages utility function |
| apps/OpenSign/src/primitives/Tooltip.jsx | Added focus outline for accessibility |
| apps/OpenSign/src/pages/TemplatePlaceholder.jsx | Refactored widget position handling and added formula recalculation |
| apps/OpenSign/src/pages/SignyourselfPdf.jsx | Enhanced widget validation with guided tours for unsigned widgets |
| apps/OpenSign/src/pages/PlaceHolderSign.jsx | Code refactoring and formula recalculation integration |
| apps/OpenSign/src/pages/PdfRequestFiles.jsx | Fixed trailing comma formatting |
| apps/OpenSign/src/constant/saveFileSize.js | Extracted common headers and removed console.log statements |
| apps/OpenSign/src/constant/Utils.js | Multiple improvements including widget order changes, formula support, and code cleanup |
| apps/OpenSign/src/components/pdf/* | Various UI improvements, refactoring, and bug fixes across PDF components |
| apps/OpenSign/public/locales/*/translation.json | Added translations for formula-related features |
| apps/OpenSign/package.json | Version bump to 2.30.0 and ESLint update |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| checkSigned = requiredWidgets[i]?.options?.response; | ||
| if (!checkSigned) { | ||
| const checkSignUrl = requiredWidgets[i]?.pos?.SignUrl; | ||
| const checkSignUrl = requiredWidgets[i]?.SignUrl; |
There was a problem hiding this comment.
The code now references requiredWidgets[i]?.SignUrl but the structure has been flattened from requiredWidgets[i]?.pos?.SignUrl. The pos property was removed, which could cause this check to fail if widgets still have the nested structure, potentially allowing unsigned widgets to pass validation.
| const checkSignUrl = requiredWidgets[i]?.SignUrl; | |
| const checkSignUrl = requiredWidgets[i]?.SignUrl || requiredWidgets[i]?.pos?.SignUrl; |
| // (tracks how many images have finished loading) | ||
| setLoadedImages((prev) => prev + 1); | ||
| } | ||
| setImageLoaders({}); |
There was a problem hiding this comment.
This line clears all image loaders immediately after incrementing loadedImages, which could cause the loading spinner to disappear prematurely for other images that are still loading. The loader state should only be cleared for the specific image that finished loading, not all loaders.
| setImageLoaders({}); | |
| setImageLoaders((prevLoaders) => { | |
| const newLoaders = { ...prevLoaders }; | |
| delete newLoaders[key]; | |
| return newLoaders; | |
| }); |
| return "/^[a-zA-Z0-9._-]+@[a-zA-Z0-9.-]+.[a-zA-Z]{2,}$/"; | ||
| case "number": | ||
| return "/^\\d+$/"; | ||
| return "^\\d+(?:\\.\\d+)?$"; // "/^\\d+$/"; |
There was a problem hiding this comment.
The regex pattern for number validation is missing the leading and trailing slashes that are present in other regex patterns in this switch statement. While the current pattern works with RegexParser, this inconsistency could cause confusion. Either add the slashes for consistency or document why this pattern differs from others.
| return "^\\d+(?:\\.\\d+)?$"; // "/^\\d+$/"; | |
| return "/^\\d+(?:\\.\\d+)?$/"; |
| props.pos.type | ||
| ) | ||
| ) { | ||
| if (props?.data?.Role !== "prefill") { |
There was a problem hiding this comment.
The condition for showing the page copy modal has been changed from checking specific widget types to checking if Role is not 'prefill'. This means the copy functionality is now available for ALL widgets in non-prefill roles, including those that may not have been intended to support copying (like dropdowns, radios, etc.). This could be intentional but represents a significant behavior change that should be verified.
| if (props?.data?.Role !== "prefill") { | |
| // Only allow copy modal for specific widget types and non-prefill role | |
| const allowedCopyTypes = ["text", "signature", "date", "checkbox"]; // Add/remove types as needed | |
| if ( | |
| props?.data?.Role !== "prefill" && | |
| allowedCopyTypes.includes(props?.pos?.type) | |
| ) { |
| ); | ||
| //condiiton is used to store copied prefill image base64 url in redux for display image | ||
| if (props?.data?.Role === "prefill") { | ||
| if (props?.pos?.type === "image") { |
There was a problem hiding this comment.
This condition was changed from checking if props?.data?.Role === 'prefill' to checking if props?.pos?.type === 'image'. This means prefill images for other widget types (signature, stamp, etc.) will no longer be stored in Redux, which could break image display for those widget types when copied.
| if (props?.pos?.type === "image") { | |
| if (props?.data?.Role === "prefill") { |
| export const applyNumberFormulasToPages = (pages = []) => { | ||
| // Guard: if not an array or empty, just return as-is | ||
| if (!Array.isArray(pages) || pages.length === 0) { | ||
| return pages; | ||
| } | ||
|
|
||
| // Clone pages and each widget.options so we do not mutate caller's data. | ||
| // (Shallow clone is sufficient for our current usage since we only touch `options`.) | ||
| const clonedPages = pages.map((page) => ({ | ||
| ...page, | ||
| pos: (page?.pos || []).map((widget) => ({ | ||
| ...widget, | ||
| options: { ...widget.options } | ||
| })) | ||
| })); | ||
|
|
||
| // Return the cloned, updated pages | ||
| return clonedPages; | ||
| }; |
There was a problem hiding this comment.
The applyNumberFormulasToPages function is documented as applying number formulas but currently only clones the data structure without actually implementing any formula calculation logic. This is called throughout the codebase expecting formula evaluation, but it's not performing that operation. This appears to be an incomplete implementation.
No description provided.