fix(sales): improve deal document printing - #8827
#8827Open
Khuslen122 wants to merge 2 commits into
mainerxes/erxes:mainfrom
fix/sales-print-preview-performanceerxes/erxes:fix/sales-print-preview-performanceCopy head branch name to clipboard
Open
fix(sales): improve deal document printing#8827Khuslen122 wants to merge 2 commits intomainerxes/erxes:mainfrom fix/sales-print-preview-performanceerxes/erxes:fix/sales-print-preview-performanceCopy head branch name to clipboard
Khuslen122 wants to merge 2 commits into
mainerxes/erxes:mainfrom
fix/sales-print-preview-performanceerxes/erxes:fix/sales-print-preview-performanceCopy head branch name to clipboard
Conversation
There was a problem hiding this comment.
Sorry @Khuslen122, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
Reviewer's GuideFrontend reimplements the deal print dialog to use a modern Sheet UI, server-side document processing via GraphQL, and a richer table selection; backend optimizes deal document generation with concurrent IO, shared field metadata fetching, and concurrency-limited replacement, improving robustness and performance for printing deal documents. Sequence diagram for the updated deal print workflowsequenceDiagram
actor User
participant PrintDialog
participant Apollo as Apollo_useLazyQuery
participant SalesAPI as sales_api_PROCESS_DOCUMENT
participant PrintWindow
User->>PrintDialog: Open PrintDialog
PrintDialog->>PrintDialog: useDeals(stageId, limit=DEALS_LIMIT)
User->>PrintDialog: Submit form (copies, width, brandId, branchId, departmentId, documentId)
PrintDialog->>PrintDialog: print()
alt missing documentId or selectedDealIds
PrintDialog->>PrintDialog: toast(error)
else valid form
PrintDialog->>PrintWindow: window.open()
PrintDialog->>PrintWindow: showPrintLoading()
PrintDialog->>Apollo: processDocument(PROCESS_DOCUMENT)
Apollo-->>SalesAPI: PROCESS_DOCUMENT query
SalesAPI-->>Apollo: documentsProcess HTML
alt html missing
PrintDialog->>PrintWindow: close()
PrintDialog->>PrintDialog: toast(print-document-failed)
else html present
PrintDialog->>PrintWindow: printHtml(html, title, width)
PrintWindow->>PrintWindow: buildPrintReadyHtml()
PrintWindow->>PrintWindow: waitForPrintDocument()
PrintWindow->>PrintWindow: waitForPrintContent()
PrintWindow->>PrintWindow: print()
PrintWindow->>PrintWindow: close() after afterprint
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughThe PR parallelizes deal document enrichment and replacement, batches product field lookups, and replaces the sales deal print dialog with a Sheet-based workflow that processes selected deals and prints generated HTML. ChangesDeal document processing and printing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PrintDialog
participant PrintDealsRecordTable
participant PROCESS_DOCUMENT
participant PrintWindow
PrintDialog->>PrintDealsRecordTable: Read selected deal IDs
PrintDialog->>PROCESS_DOCUMENT: Submit document ID, replacer IDs, and print config
PROCESS_DOCUMENT-->>PrintDialog: Return generated HTML
PrintDialog->>PrintWindow: Load prepared HTML and wait for resources
PrintWindow-->>PrintDialog: Reach complete state
PrintDialog->>PrintWindow: Trigger print
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
frontend/plugins/sales_ui/src/modules/deals/boards/components/common/Print.tsx (3)
636-645: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNumeric inputs can't be cleared while typing.
Number.parseInt('') || 1coerces an empty field back to1(or the default width) on every keystroke that empties it, so users can't delete-and-retype. Keeping the raw string in the field and coercing on submit avoids this.Also applies to: 659-669
🤖 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 `@frontend/plugins/sales_ui/src/modules/deals/boards/components/common/Print.tsx` around lines 636 - 645, Update the numeric inputs in the Print component’s field handlers to preserve the raw event.target.value, including an empty string, during editing instead of immediately applying Number.parseInt with a fallback. Move numeric coercion and the minimum/default-width handling to the submit or final-value processing path for both affected inputs.
289-323: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo timeout when waiting for images/fonts.
waitForPrintDocumenthasPRINT_DOCUMENT_LOAD_TIMEOUT, but this wait can stall indefinitely if an image or font never settles, leaving the popup stuck on the loading screen with no way to recover. Race it against the same timeout.♻️ Proposed timeout race
- await Promise.all([ - printWindow.document.fonts?.ready ?? Promise.resolve(), - ...pendingImages, - ]); + await Promise.race([ + Promise.all([ + printWindow.document.fonts?.ready ?? Promise.resolve(), + ...pendingImages, + ]), + new Promise<void>((resolve) => + window.setTimeout(resolve, PRINT_DOCUMENT_LOAD_TIMEOUT), + ), + ]);🤖 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 `@frontend/plugins/sales_ui/src/modules/deals/boards/components/common/Print.tsx` around lines 289 - 323, Update waitForPrintContent to race the combined font-readiness, image-settling, and animation-frame wait against the existing PRINT_DOCUMENT_LOAD_TIMEOUT used by waitForPrintDocument. Ensure the timeout rejects or otherwise aborts the wait so a never-settling image or font cannot leave the print popup loading indefinitely, while preserving normal completion when all resources settle in time.
25-28: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valuePrefer root-barrel imports for shared library constants.
The constants exist under
frontend/libs/ui-modules/src/modules/documents/constants/index.ts, but the shared library is typically consumed via root imports; re-exportPAPER_SIZES/PAPER_TYPESfromui-modulesso Module Federation path mappings and consumers use the stable shared package entry.🤖 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 `@frontend/plugins/sales_ui/src/modules/deals/boards/components/common/Print.tsx` around lines 25 - 28, Re-export PAPER_SIZES and PAPER_TYPES from the ui-modules root barrel, then update the Print.tsx import to consume both constants from ui-modules instead of the nested documents/constants path. Preserve the existing constant usage and remove the deep shared-library import.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@frontend/plugins/sales_ui/src/modules/deals/boards/components/common/Print.tsx`:
- Line 512: Reset selectedDealIds whenever the sheet’s open state or stageId
changes, including the fixed tableId RecordTable selection so stale rows cannot
persist across close/reopen or stage changes. Update the effect or lifecycle
logic surrounding selectedDealIds and the RecordTable configuration while
preserving selection during an unchanged open stage.
- Around line 492-499: The useDeals call in Print should account for stages
containing more than DEALS_LIMIT deals instead of silently presenting
deals.length as the complete set. Add pagination to load all deals, or expose a
clear truncation indicator near the selected/deals.length counter when the limit
is reached; keep the existing behavior for stages within the limit.
- Around line 365-372: Replace the Blob URL navigation in the print flow with
rendering the generated HTML through a tightly sandboxed iframe using srcdoc,
allowing only the minimal capability needed for printing (such as allow-modals).
Update the surrounding print readiness flow to wait for the sandboxed document,
and ensure untrusted values passed through buildPrintReadyHtml are escaped or
sanitized before insertion.
---
Nitpick comments:
In
`@frontend/plugins/sales_ui/src/modules/deals/boards/components/common/Print.tsx`:
- Around line 636-645: Update the numeric inputs in the Print component’s field
handlers to preserve the raw event.target.value, including an empty string,
during editing instead of immediately applying Number.parseInt with a fallback.
Move numeric coercion and the minimum/default-width handling to the submit or
final-value processing path for both affected inputs.
- Around line 289-323: Update waitForPrintContent to race the combined
font-readiness, image-settling, and animation-frame wait against the existing
PRINT_DOCUMENT_LOAD_TIMEOUT used by waitForPrintDocument. Ensure the timeout
rejects or otherwise aborts the wait so a never-settling image or font cannot
leave the print popup loading indefinitely, while preserving normal completion
when all resources settle in time.
- Around line 25-28: Re-export PAPER_SIZES and PAPER_TYPES from the ui-modules
root barrel, then update the Print.tsx import to consume both constants from
ui-modules instead of the nested documents/constants path. Preserve the existing
constant usage and remove the deep shared-library import.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7d068329-8aa7-4776-9cff-bb9ee57dc821
📒 Files selected for processing (5)
backend/gateway/src/locales/en/sales.jsonbackend/gateway/src/locales/mn/sales.jsonbackend/plugins/sales_api/src/modules/sales/documents/dealContent.tsbackend/plugins/sales_api/src/modules/sales/utils.tsfrontend/plugins/sales_ui/src/modules/deals/boards/components/common/Print.tsx
Comment on lines
+365
to
+372
| const printReadyHtml = buildPrintReadyHtml(html, width); | ||
| const printUrl = URL.createObjectURL( | ||
| new Blob([printReadyHtml], { type: 'text/html;charset=utf-8' }), | ||
| ); | ||
|
|
||
| try { | ||
| printWindow.location.replace(printUrl); | ||
| await waitForPrintDocument(printWindow, printUrl, errorMessage); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how documentsProcess builds the returned HTML and whether values are escaped
fd -t f 'prepareContent*' -x cat -n {} \;
rg -n 'prepareContent' -g '!**/node_modules/**' -C3Repository: erxes/erxes
Length of output: 2031
Render the printable HTML in a tightly sandboxed iframe.
Blob URLs adopt the opener page’s origin, so <script> or event-handler markup in the printable HTML runs with access to the app origin. opener = null does not change that behavior. Render the document into srcdoc with a restrictive sandbox (for example, allow-modals only, or the minimal allowed set) and avoid passing untrusted attribute values through buildPrintReadyHtml without escaping/sanitization.
🤖 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
`@frontend/plugins/sales_ui/src/modules/deals/boards/components/common/Print.tsx`
around lines 365 - 372, Replace the Blob URL navigation in the print flow with
rendering the generated HTML through a tightly sandboxed iframe using srcdoc,
allowing only the minimal capability needed for printing (such as allow-modals).
Update the surrounding print readiness flow to wait for the sandboxed document,
and ensure untrusted values passed through buildPrintReadyHtml are escaped or
sanitized before insertion.
Comment on lines
+492
to
499
| const { deals = [], loading } = useDeals({ | ||
| variables: { | ||
| stageId, | ||
| limit: DEALS_LIMIT, | ||
| }, | ||
| skip: !open, | ||
| fetchPolicy: 'network-only', | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Deals beyond DEALS_LIMIT are silently dropped.
The stage may hold more than 100 deals, yet the counter renders selected/deals.length as if it were the full set. Consider surfacing the truncation (e.g. a "showing first 100" hint) or paginating.
Also applies to: 749-755
🤖 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
`@frontend/plugins/sales_ui/src/modules/deals/boards/components/common/Print.tsx`
around lines 492 - 499, The useDeals call in Print should account for stages
containing more than DEALS_LIMIT deals instead of silently presenting
deals.length as the complete set. Add pagination to load all deals, or expose a
clear truncation indicator near the selected/deals.length counter when the limit
is reached; keep the existing behavior for stages within the limit.
| }, | ||
| }); | ||
|
|
||
| const [selectedDealIds, setSelectedDealIds] = useState<string[]>([]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
selectedDealIds is never reset when the sheet closes or stageId changes.
The state (and the RecordTable row selection keyed by the fixed tableId) survives close/reopen, so reopening on a different stage can submit replacerIds for deals that aren't in the current list. Clear the selection when open or stageId changes.
🐛 Proposed reset
const [selectedDealIds, setSelectedDealIds] = useState<string[]>([]);
+
+ useEffect(() => {
+ setSelectedDealIds([]);
+ }, [open, stageId]);Also applies to: 600-608
🤖 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
`@frontend/plugins/sales_ui/src/modules/deals/boards/components/common/Print.tsx`
at line 512, Reset selectedDealIds whenever the sheet’s open state or stageId
changes, including the fixed tableId RecordTable selection so stale rows cannot
persist across close/reopen or stage changes. Update the effect or lifecycle
logic surrounding selectedDealIds and the RecordTable configuration while
preserving selection during an unchanged open stage.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
backend/plugins/sales_api/src/modules/sales/documents/dealContent.ts (2)
12-12: 🚀 Performance & Scalability | 🔵 TrivialVerify downstream capacity for the new concurrency level.
DOCUMENT_PROCESS_CONCURRENCY = 8limits deals, but eachbuildDealReplacerstarts several TRPC/database operations in parallel. The effective downstream fan-out can therefore be much higher than eight; please validate this against core-service and database capacity, or use a shared/configurable limiter if needed.Also applies to: 181-195
🤖 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 `@backend/plugins/sales_api/src/modules/sales/documents/dealContent.ts` at line 12, Review DOCUMENT_PROCESS_CONCURRENCY together with buildDealReplacer’s parallel TRPC/database work and validate the resulting downstream fan-out against core-service and database capacity. If eight concurrent deals exceed capacity, replace the fixed limit with the existing shared or configurable limiter, ensuring the effective operation concurrency is bounded.
456-502: 📐 Maintainability & Code Quality | 🔵 TrivialPreserve observability for empty and no-match returns.
Replacing the previous console warnings with silent early returns can make invalid deal IDs or malformed content difficult to diagnose. Retain a structured debug log or metric for these cases without restoring noisy
console.warncalls.🤖 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 `@backend/plugins/sales_api/src/modules/sales/documents/dealContent.ts` around lines 456 - 502, Preserve structured observability in the early-return paths surrounding deal lookup and content validation: add or retain a debug log or metric when replacerIds are empty or no deals match. Update the relevant deal-processing function without restoring console.warn calls, and keep both existing return [] behaviors unchanged.
🤖 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.
Nitpick comments:
In `@backend/plugins/sales_api/src/modules/sales/documents/dealContent.ts`:
- Line 12: Review DOCUMENT_PROCESS_CONCURRENCY together with buildDealReplacer’s
parallel TRPC/database work and validate the resulting downstream fan-out
against core-service and database capacity. If eight concurrent deals exceed
capacity, replace the fixed limit with the existing shared or configurable
limiter, ensuring the effective operation concurrency is bounded.
- Around line 456-502: Preserve structured observability in the early-return
paths surrounding deal lookup and content validation: add or retain a debug log
or metric when replacerIds are empty or no deals match. Update the relevant
deal-processing function without restoring console.warn calls, and keep both
existing return [] behaviors unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 387e3996-0aa1-47ad-affd-8dc7b782be14
📒 Files selected for processing (2)
backend/plugins/sales_api/src/modules/sales/documents/dealContent.tsfrontend/plugins/sales_ui/src/modules/deals/boards/components/common/Print.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/plugins/sales_ui/src/modules/deals/boards/components/common/Print.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Improve sales deal document printing UX and backend processing for generating deal-based documents.
New Features:
Bug Fixes:
Enhancements:
Summary by CodeRabbit