Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…NT, MIN, MAX, TODAY, NOW, DATEADD, DATEDIFF, IF, AND, OR, NOT, SWITCH, CONCAT, LEFT, RIGHT, TRIM, UPPER, LOWER) Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
…le triggers Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
… and batch progress tracking Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
…Excel formula injection protection Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds three core integration capabilities across the monorepo: formula functions for the expression evaluator, DataSource-backed “live” report export utilities, and a transaction manager for multi-step operations (with rollback/batch/optimistic support).
Changes:
- Introduces
FormulaFunctionsand injects them intoExpressionEvaluator(plus tests and exports). - Adds
LiveReportExporterhelpers (exportWithLiveData, Excel-with-formulas TSV export, schedule trigger) and exports them fromplugin-report(plus tests). - Adds
TransactionManagerto@object-ui/core/actionsand exports it (plus tests).
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/plugin-report/src/index.tsx | Re-exports new live export APIs/types from LiveReportExporter. |
| packages/plugin-report/src/LiveReportExporter.ts | Implements DataSource-integrated export, TSV “Excel with formulas”, and schedule trigger helper. |
| packages/plugin-report/src/tests/LiveReportExporter.test.ts | Adds unit tests for live export, TSV export, and scheduling trigger behavior. |
| packages/core/src/evaluator/index.ts | Exports FormulaFunctions from evaluator barrel. |
| packages/core/src/evaluator/FormulaFunctions.ts | Implements built-in formula registry (aggregation/date/logic/string). |
| packages/core/src/evaluator/tests/FormulaFunctions.test.ts | Adds coverage for built-ins and ExpressionEvaluator integration + custom registrations. |
| packages/core/src/evaluator/ExpressionEvaluator.ts | Injects formulas into evaluation context; adds registerFunction() and formula sharing behavior. |
| packages/core/src/actions/index.ts | Exports TransactionManager from actions barrel. |
| packages/core/src/actions/TransactionManager.ts | Implements transaction execution with rollback recording, optimistic updates, and batch operations with retries/progress. |
| packages/core/src/actions/tests/TransactionManager.test.ts | Adds unit tests for transaction execution, rollback, optimistic updates, and batching. |
| format: exportFormat, | ||
| ...report.exportConfigs?.[exportFormat], | ||
| ...exportConfig, |
There was a problem hiding this comment.
mergedConfig sets format: exportFormat but then spreads report.exportConfigs?.[exportFormat] and exportConfig, both of which can contain a format key and silently desync mergedConfig.format from the actual exportFormat passed to exportReport. Consider omitting format from spread inputs (or forcing mergedConfig.format = exportFormat after merging) to keep config consistent.
| format: exportFormat, | |
| ...report.exportConfigs?.[exportFormat], | |
| ...exportConfig, | |
| ...report.exportConfigs?.[exportFormat], | |
| ...exportConfig, | |
| format: exportFormat, |
| // Build TSV content with BOM | ||
| const tsvContent = '\uFEFF' + [ | ||
| headers.join('\t'), | ||
| ...rows.map(r => r.join('\t')), | ||
| ].join('\n'); |
There was a problem hiding this comment.
The TSV generation joins cells with tabs/newlines without escaping, so any value containing \t, \n, or quotes will corrupt row/column structure in Excel. ReportExportEngine.exportAsExcel() already handles escaping/quoting for these cases; consider reusing that escaping logic here (including for formula results when they produce text).
| return { style: 'percent', minimumFractionDigits: 0 }; | ||
| } | ||
| const decimals = (format.match(/0/g) || []).length; | ||
| return { | ||
| minimumFractionDigits: Math.max(0, decimals - 1), | ||
| maximumFractionDigits: Math.max(0, decimals - 1), |
There was a problem hiding this comment.
inferLocaleOptions() infers decimal places by counting all 0 characters in the format string, which miscalculates for formats that include multiple integer-place zeros (e.g. 0000.00 would be treated as 5 fraction digits). Consider deriving fraction digits from the substring after the decimal point only, or requiring explicit Intl.NumberFormatOptions instead of a spreadsheet-style format string.
| return { style: 'percent', minimumFractionDigits: 0 }; | |
| } | |
| const decimals = (format.match(/0/g) || []).length; | |
| return { | |
| minimumFractionDigits: Math.max(0, decimals - 1), | |
| maximumFractionDigits: Math.max(0, decimals - 1), | |
| // Keep existing behavior for percent-style formats to avoid changing semantics. | |
| return { style: 'percent', minimumFractionDigits: 0 }; | |
| } | |
| let decimals = 0; | |
| const decimalPointIndex = format.indexOf('.'); | |
| if (decimalPointIndex !== -1) { | |
| const fractionalPart = format.slice(decimalPointIndex + 1); | |
| decimals = (fractionalPart.match(/0/g) || []).length; | |
| } | |
| return { | |
| minimumFractionDigits: decimals, | |
| maximumFractionDigits: decimals, |
| function downloadFile(content: string, filename: string, mimeType: string): void { | ||
| const blob = new Blob([content], { type: `${mimeType};charset=utf-8` }); | ||
| const url = URL.createObjectURL(blob); | ||
| const link = document.createElement('a'); | ||
| link.href = url; | ||
| link.download = filename; | ||
| document.body.appendChild(link); | ||
| link.click(); | ||
| document.body.removeChild(link); | ||
| URL.revokeObjectURL(url); | ||
| } |
There was a problem hiding this comment.
downloadFile() is duplicated here and in ReportExportEngine.ts (same Blob/URL/anchor logic). Consider extracting a shared helper (e.g. downloadFile.ts) and reusing it to avoid divergence (this file also uses different Excel injection sanitization than sanitizeCSVValue).
| date.setDate(date.getDate() + amount); | ||
| break; | ||
| case 'month': | ||
| case 'months': | ||
| date.setMonth(date.getMonth() + amount); | ||
| break; | ||
| case 'year': | ||
| case 'years': | ||
| date.setFullYear(date.getFullYear() + amount); | ||
| break; | ||
| case 'hour': | ||
| case 'hours': | ||
| date.setHours(date.getHours() + amount); | ||
| break; | ||
| case 'minute': | ||
| case 'minutes': | ||
| date.setMinutes(date.getMinutes() + amount); |
There was a problem hiding this comment.
DATEADD uses local-time mutators (setDate, setMonth, setFullYear, etc.). When dateStr is an ISO string (often UTC), mixing local mutators with UTC-based inputs can cause off-by-one-hour/day issues around timezones/DST and make behavior environment-dependent. Consider using the UTC variants (setUTCDate/setUTCMonth/...) or doing arithmetic on UTC timestamps to keep results stable.
| date.setDate(date.getDate() + amount); | |
| break; | |
| case 'month': | |
| case 'months': | |
| date.setMonth(date.getMonth() + amount); | |
| break; | |
| case 'year': | |
| case 'years': | |
| date.setFullYear(date.getFullYear() + amount); | |
| break; | |
| case 'hour': | |
| case 'hours': | |
| date.setHours(date.getHours() + amount); | |
| break; | |
| case 'minute': | |
| case 'minutes': | |
| date.setMinutes(date.getMinutes() + amount); | |
| date.setUTCDate(date.getUTCDate() + amount); | |
| break; | |
| case 'month': | |
| case 'months': | |
| date.setUTCMonth(date.getUTCMonth() + amount); | |
| break; | |
| case 'year': | |
| case 'years': | |
| date.setUTCFullYear(date.getUTCFullYear() + amount); | |
| break; | |
| case 'hour': | |
| case 'hours': | |
| date.setUTCHours(date.getUTCHours() + amount); | |
| break; | |
| case 'minute': | |
| case 'minutes': | |
| date.setUTCMinutes(date.getUTCMinutes() + amount); |
| if (result.success) { | ||
| break; | ||
| } else { | ||
| lastError = result.error; | ||
| // Only retry on conflict if enabled | ||
| if (config.retryOnConflict && attempt < maxRetries) { | ||
| await delay(this.retryDelay * Math.pow(2, attempt)); | ||
| continue; | ||
| } | ||
| break; | ||
| } | ||
| } catch (error) { | ||
| lastError = (error as Error).message; | ||
| if (config.retryOnConflict && attempt < maxRetries) { | ||
| await delay(this.retryDelay * Math.pow(2, attempt)); | ||
| continue; | ||
| } | ||
| result = { success: false, error: lastError }; | ||
| break; |
There was a problem hiding this comment.
retryOnConflict currently retries on any ActionResult.success === false or thrown error, without checking that the failure is actually a conflict. This makes retryOnConflict behave like a generic retry and can amplify non-conflict failures. Consider adding a conflict predicate (e.g. isConflict(result/error) or a status/code on ActionResult) and only retry when it matches.
| const maxRetries = config.maxRetries ?? this.maxRetries; | ||
| const actionResults: ActionResult[] = []; | ||
|
|
||
| this.operations = []; | ||
| let completed = 0; |
There was a problem hiding this comment.
executeTransaction() resets this.operations on the manager instance and uses it for rollback. If the same TransactionManager instance is used for concurrent transactions, this shared mutable state can interleave and rollback the wrong operations. Consider scoping the operations list to the executeTransaction call (or preventing concurrent execution per instance).
| // Inject formula functions into the evaluation context | ||
| const formulaObj = this.formulas.toObject(); | ||
| const mergedContext = { ...formulaObj, ...contextObj }; | ||
|
|
||
| // Build safe function with context variables | ||
| const varNames = Object.keys(contextObj); | ||
| const varValues = Object.values(contextObj); | ||
| const varNames = Object.keys(mergedContext); | ||
| const varValues = Object.values(mergedContext); |
There was a problem hiding this comment.
In evaluateExpression, mergedContext spreads formulaObj first and then contextObj, which allows user-provided context keys (e.g. SUM) to shadow built-in formulas and break expression evaluation unexpectedly. Consider giving formulas precedence (spread context first) or explicitly preventing/throwing on collisions with reserved formula names.
Implements the three pillars of the
@objectstack/clientFull-Stack Integration roadmap: expression formula functions, DataSource-integrated report export, and transactional multi-step operations with rollback.Expression Engine —
FormulaFunctionsNew
FormulaFunctionsclass auto-injected intoExpressionEvaluatorcontext. 20 built-in functions across 4 categories:SUM,AVG,COUNT,MIN,MAX(supports nested arrays)TODAY,NOW,DATEADD,DATEDIFFIF,AND,OR,NOT,SWITCHCONCAT,LEFT,RIGHT,TRIM,UPPER,LOWERCustom functions via
evaluator.registerFunction('DOUBLE', (n) => n * 2).Report Export —
LiveReportExporterexportWithLiveData()— Fetches viaDataSource.find()then routes to any export formatexportExcelWithFormulas()— TSV with formula cells (=SUM(A2:A{ROW})), column formatting, and aggregation rowscreateScheduleTrigger()— Returns a workflow-invokable async function that exports all scheduled formats and fires a completion callbackTransaction Manager
executeTransaction()— Sequential action execution with automatic rollback of completed operations on failure. SupportsretryOnConflictwith exponential backoff.applyOptimisticUpdate()→confirmOptimisticUpdate()/rollbackOptimisticUpdate()lifecycle for instant UI feedbackexecuteBatch()— Batch CRUD with progress events, triesDataSource.bulk()first with per-item fallback and retryTest coverage
85 new tests across 3 test files (48 formula, 13 report export, 24 transaction). Full suite green: 2728 passed.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.