Add expression caching, virtual scrolling, and developer tooling#283
Add expression caching, virtual scrolling, and developer tooling#283
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
…dd shared cache Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
| // 2. Strict mode enabled ("use strict") | ||
| // 3. Limited scope (only varNames variables available) | ||
| // 4. No access to global objects (process, window, etc.) | ||
| return new Function(...varNames, `"use strict"; return (${expression});`) as CompiledExpression; |
Check warning
Code scanning / CodeQL
Unsafe code constructed from library input Medium
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
📦 Bundle Size Report
Size Limits
|
2 similar comments
📦 Bundle Size Report
Size Limits
|
📦 Bundle Size Report
Size Limits
|
|
✅ All checks passed!
|
|
✅ All checks passed!
|
1 similar comment
|
✅ All checks passed!
|
There was a problem hiding this comment.
Pull request overview
This PR implements performance optimizations and developer tooling enhancements for ObjectUI, focusing on expression caching for reduced parsing overhead, virtual scrolling for large datasets, schema validation utilities, and CLI commands for development workflows.
Changes:
- Expression Caching: Added
ExpressionCacheclass with hybrid LFU/LRU eviction policy (default 1000 entries), integrated intoExpressionEvaluatorwith a global cache for convenience functions - Virtual Scrolling: Implemented
VirtualGridcomponent using@tanstack/react-virtualfor rendering only visible rows, and optimized AG Grid with automaticrowBufferanddebounceVerticalScrollbarsettings for datasets >1000 rows - Schema Validation: Exported
validateSchema()andsafeValidateSchema()helper functions from@object-ui/types/zodfor convenient schema validation - CLI Tooling: Added
objectui validate,objectui create plugin, andobjectui analyzecommands with formatted output and error handling
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/evaluator/ExpressionCache.ts | New cache implementation with LFU eviction and metadata tracking for compiled expressions |
| packages/core/src/evaluator/ExpressionEvaluator.ts | Integrated cache into evaluator, added cache statistics methods, shared global cache for convenience functions |
| packages/core/src/evaluator/tests/ExpressionCache.test.ts | Comprehensive tests for cache functionality including eviction, hit counting, and statistics |
| packages/core/src/evaluator/index.ts | Export ExpressionCache for external use |
| packages/plugin-grid/src/VirtualGrid.tsx | New virtual scrolling grid component with configurable row height, overscan, and custom cell renderers |
| packages/plugin-grid/src/VirtualGrid.test.tsx | Basic existence tests (minimal coverage) |
| packages/plugin-grid/src/index.tsx | Export VirtualGrid component and types |
| packages/plugin-grid/package.json | Add @tanstack/react-virtual dependency |
| packages/plugin-aggrid/src/VirtualScrolling.ts | Documentation file explaining AG Grid's built-in virtual scrolling |
| packages/plugin-aggrid/src/AgGridImpl.tsx | Add automatic virtual scrolling optimizations based on dataset size |
| packages/types/src/zod/index.zod.ts | Add validateSchema and safeValidateSchema helper functions |
| packages/cli/src/commands/validate.ts | New command to validate schema files with formatted error output |
| packages/cli/src/commands/create-plugin.ts | New command to scaffold plugin structure |
| packages/cli/src/commands/analyze.ts | New command for bundle size analysis and performance recommendations |
| packages/cli/src/cli.ts | Integrate new commands into CLI interface |
| packages/cli/package.json | Add @object-ui/types dependency |
| pnpm-lock.yaml | Add @tanstack/react-virtual@3.13.18 and update dependency graph |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
| rowBuffer: gridOptions.rowBuffer ?? 10, | ||
| debounceVerticalScrollbar: gridOptions.debounceVerticalScrollbar ?? (rowData.length > 1000), |
There was a problem hiding this comment.
The debounceVerticalScrollbar setting is automatically enabled for datasets larger than 1000 rows (line 246), but this is applied to gridOptions which the user can override. This creates an inconsistency where:
- If gridOptions.debounceVerticalScrollbar is undefined, it automatically becomes true for large datasets
- If gridOptions.debounceVerticalScrollbar is explicitly set to false, it stays false (due to the ?? operator)
- If gridOptions.debounceVerticalScrollbar is explicitly set to true, it stays true
The spread operator on line 227 (...gridOptions) means that if a user passes gridOptions: { debounceVerticalScrollbar: false }, it will be overridden on line 246. This is actually correct behavior (auto-optimization unless explicitly disabled), but it should be documented.
Consider adding a comment explaining this behavior, or documenting in the component's JSDoc that automatic optimizations can be disabled by explicitly setting these options to false in gridOptions.
| > | ||
| {columns.map((column, index) => ( | ||
| <div | ||
| key={index} |
There was a problem hiding this comment.
The column headers are using array index as the key (line 96), which is an anti-pattern in React. While this works when the columns array is static, it can cause issues if:
- The columns array order changes dynamically
- Columns are added or removed
- The grid needs to update with a new columns configuration
Using index as a key can lead to incorrect component reuse and potential rendering bugs. Instead, consider using a more stable identifier:
key={column.accessorKey || column.header || index}This ensures that if the same column appears in a different position, React can properly track and update it. The accessorKey is the most stable identifier since it's unique per column.
| key={index} | |
| key={column.accessorKey || column.header || index} |
| "@object-ui/fields": "workspace:*", | ||
| "@object-ui/react": "workspace:*", | ||
| "@object-ui/types": "workspace:*", | ||
| "@tanstack/react-virtual": "^3.11.3", |
There was a problem hiding this comment.
The installed version of @tanstack/react-virtual is 3.13.18, but the package.json specifies ^3.11.3. While this works due to semver, there's a discrepancy between what was requested in the PR description ("Added @tanstack/react-virtual@^3.11.3") and what was actually installed (3.13.18).
This is not necessarily a problem, but for clarity and predictability, consider:
- Updating package.json to use the exact version that was tested: "@tanstack/react-virtual": "^3.13.18", OR
- Ensuring tests pass with the minimum specified version (3.11.3)
This ensures other developers installing from the package.json get the same version used during development.
| import { ComponentRegistry } from '@object-ui/core'; | ||
| import { ObjectGrid } from './ObjectGrid'; | ||
| import { VirtualGrid } from './VirtualGrid'; | ||
|
|
There was a problem hiding this comment.
The VirtualGrid component is not documented in the README. The plugin-grid README should be updated to include information about when and how to use VirtualGrid versus the standard ObjectGrid.
Based on the PR description, VirtualGrid is specifically designed for handling 10,000+ items without DOM bloat. Users need guidance on:
- When to use VirtualGrid vs ObjectGrid
- How to use VirtualGrid with schema-based rendering
- Example configurations
- Performance characteristics and limitations
This is especially important since VirtualGrid is a new feature that addresses a specific performance use case.
| /** | |
| * ObjectGrid vs VirtualGrid | |
| * | |
| * - {@link ObjectGrid}: | |
| * - General-purpose data grid integrated with the ObjectUI schema renderer. | |
| * - Best suited for small-to-medium datasets (typically up to a few thousand rows), | |
| * where rendering all rows in the DOM at once is acceptable. | |
| * - Prioritizes feature richness and simplicity over extreme-scale performance. | |
| * | |
| * - {@link VirtualGrid}: | |
| * - Specialized grid implementation designed for very large datasets | |
| * (10,000+ items) without causing DOM bloat. | |
| * - Only the visible rows (plus a small overscan window) are mounted in the DOM, | |
| * dramatically reducing memory usage and layout/paint cost. | |
| * - Recommended when: | |
| * - You need to render tens of thousands of records in a single grid. | |
| * - Scroll performance degrades with {@link ObjectGrid}. | |
| * - You care more about scroll smoothness and initial render time than about | |
| * having every row present in the DOM simultaneously. | |
| * | |
| * Schema-based usage | |
| * ------------------- | |
| * This plugin registers the `'object-grid'` view type in the {@link ComponentRegistry}. | |
| * In schema-driven UIs, you typically render an ObjectGrid like: | |
| * | |
| * ```jsonc | |
| * { | |
| * "type": "object-grid", | |
| * "id": "users-grid", | |
| * "props": { | |
| * "columns": [ | |
| * { "key": "id", "title": "ID" }, | |
| * { "key": "name", "title": "Name" } | |
| * ] | |
| * } | |
| * } | |
| * ``` | |
| * | |
| * {@link VirtualGrid} exposes `VirtualGridProps` and `VirtualGridColumn` for direct | |
| * use when you need manual, code-level control over a virtualized grid instance. | |
| * A typical configuration in React/TypeScript may look like: | |
| * | |
| * ```tsx | |
| * import { VirtualGrid, VirtualGridColumn } from '@object-ui/plugin-grid'; | |
| * | |
| * const columns: VirtualGridColumn[] = [ | |
| * { key: 'id', title: 'ID', width: 80 }, | |
| * { key: 'name', title: 'Name', flex: 1 } | |
| * ]; | |
| * | |
| * <VirtualGrid | |
| * columns={columns} | |
| * data={hugeDataArray} // 10k+ items | |
| * rowHeight={32} | |
| * overscan={8} | |
| * /> | |
| * ``` | |
| * | |
| * Performance characteristics & limitations | |
| * ---------------------------------------- | |
| * - Virtualized rendering: | |
| * - Only a subset of rows is rendered at any time. | |
| * - Greatly improves performance for very large datasets. | |
| * - Layout assumptions: | |
| * - Typically assumes a fixed or predictable row height (`rowHeight`). | |
| * - Highly dynamic row heights may reduce virtualization accuracy. | |
| * - Feature parity: | |
| * - Some advanced features that depend on the full DOM (e.g., complex | |
| * row-measurement-based layouts) may not be available or may behave | |
| * differently compared to {@link ObjectGrid}. | |
| * | |
| * Exported surface | |
| * ---------------- | |
| * - {@link ObjectGrid} & {@link ObjectGridProps}: | |
| * - Use when you want a standard, schema-friendly data grid for typical | |
| * dataset sizes. | |
| * - {@link VirtualGrid}, {@link VirtualGridProps}, {@link VirtualGridColumn}: | |
| * - Use when you explicitly need a virtualized grid for 10,000+ items and | |
| * want to avoid DOM bloat, with the understanding of the constraints above. | |
| */ |
|
|
||
| export const VIRTUAL_SCROLLING_DOCS = { | ||
| enabled: true, | ||
| automatic: true, | ||
| recommendedSettings: { | ||
| rowBuffer: 10, | ||
| rowHeight: 40, | ||
| debounceVerticalScrollbar: true, | ||
| animateRows: false, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
The VirtualScrolling.ts file exports a VIRTUAL_SCROLLING_DOCS constant that is never used in the codebase. This appears to be documentation-only code that exists purely to document AG Grid's virtual scrolling capabilities.
Consider either:
- Removing this file and moving the documentation to a markdown file (e.g., docs/virtual-scrolling.md) if it's only for reference
- Actually using this constant somewhere in the code (e.g., to display help text or recommendations to users)
- Exporting this from the plugin-aggrid index.ts if it's meant to be public API
Unused exported constants can confuse developers about the intended usage and may indicate incomplete implementation.
| export const VIRTUAL_SCROLLING_DOCS = { | |
| enabled: true, | |
| automatic: true, | |
| recommendedSettings: { | |
| rowBuffer: 10, | |
| rowHeight: 40, | |
| debounceVerticalScrollbar: true, | |
| animateRows: false, | |
| }, | |
| }; |
| files.push({ | ||
| path: fullPath.replace(distDir + '/', ''), |
There was a problem hiding this comment.
The path manipulation on line 44 uses string concatenation with a hardcoded forward slash, which may not work correctly on Windows:
path: fullPath.replace(distDir + '/', '')On Windows, paths use backslashes, so this replacement may fail. Use path.relative() instead for cross-platform compatibility:
import { relative } from 'path';
// ...
path: relative(distDir, fullPath)This ensures the code works correctly on all operating systems.
| return ( | ||
| <div className={className}> | ||
| {/* Header */} | ||
| <div | ||
| className={`grid border-b sticky top-0 bg-background z-10 ${headerClassName}`} | ||
| style={{ | ||
| gridTemplateColumns: columns | ||
| .map((col) => col.width || '1fr') | ||
| .join(' '), | ||
| }} | ||
| > | ||
| {columns.map((column, index) => ( | ||
| <div | ||
| key={index} | ||
| className={`px-4 py-2 font-semibold text-sm ${ | ||
| column.align === 'center' | ||
| ? 'text-center' | ||
| : column.align === 'right' | ||
| ? 'text-right' | ||
| : 'text-left' | ||
| }`} | ||
| > | ||
| {column.header} | ||
| </div> | ||
| ))} | ||
| </div> | ||
|
|
||
| {/* Virtual scrolling container */} | ||
| <div | ||
| ref={parentRef} | ||
| className="overflow-auto" | ||
| style={{ | ||
| height: typeof height === 'number' ? `${height}px` : height, | ||
| contain: 'strict' | ||
| }} | ||
| > | ||
| <div | ||
| style={{ | ||
| height: `${virtualizer.getTotalSize()}px`, | ||
| width: '100%', | ||
| position: 'relative', | ||
| }} | ||
| > | ||
| {items.map((virtualRow) => { | ||
| const row = data[virtualRow.index]; | ||
| const rowClasses = | ||
| typeof rowClassName === 'function' | ||
| ? rowClassName(row, virtualRow.index) | ||
| : rowClassName || ''; | ||
|
|
||
| return ( | ||
| <div | ||
| key={virtualRow.key} | ||
| className={`grid border-b hover:bg-muted/50 cursor-pointer ${rowClasses}`} | ||
| style={{ | ||
| position: 'absolute', | ||
| top: 0, | ||
| left: 0, | ||
| width: '100%', | ||
| height: `${virtualRow.size}px`, | ||
| transform: `translateY(${virtualRow.start}px)`, | ||
| gridTemplateColumns: columns | ||
| .map((col) => col.width || '1fr') | ||
| .join(' '), | ||
| }} | ||
| onClick={() => onRowClick?.(row, virtualRow.index)} | ||
| > | ||
| {columns.map((column, colIndex) => { | ||
| const value = row[column.accessorKey]; | ||
| const cellContent = column.cell | ||
| ? column.cell(value, row) | ||
| : value; | ||
|
|
||
| return ( | ||
| <div | ||
| key={colIndex} | ||
| className={`px-4 py-2 text-sm flex items-center ${ | ||
| column.align === 'center' | ||
| ? 'text-center justify-center' | ||
| : column.align === 'right' | ||
| ? 'text-right justify-end' | ||
| : 'text-left justify-start' | ||
| }`} | ||
| > | ||
| {cellContent} | ||
| </div> | ||
| ); | ||
| })} | ||
| </div> | ||
| ); | ||
| })} | ||
| </div> | ||
| </div> | ||
|
|
||
| {/* Footer info */} | ||
| <div className="px-4 py-2 text-xs text-muted-foreground border-t"> | ||
| Showing {items.length} of {data.length} rows (virtual scrolling enabled) | ||
| </div> | ||
| </div> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
The VirtualGrid component lacks accessibility attributes for screen readers. The grid structure should include appropriate ARIA roles and labels to be accessible to users with disabilities.
Consider adding:
role="grid"on the main containerrole="row"on each row elementrole="columnheader"on header cellsrole="gridcell"on data cellsaria-labeloraria-labelledbyto identify the gridaria-rowcountandaria-rowindexfor virtual scrolling awarenessaria-colcountandaria-colindexfor column identification
Example:
<div role="grid" aria-label="Data grid" aria-rowcount={data.length} className={className}>
<div role="row" className="...">
{columns.map((column) => (
<div role="columnheader" aria-colindex={index + 1}>
{column.header}
</div>
))}
</div>
{/* ... rows with role="row" and cells with role="gridcell" */}
</div>This is especially important for a data grid component that users may rely on for navigating tabular data.
| describe('VirtualGrid', () => { | ||
| it('should be exported', () => { | ||
| expect(VirtualGrid).toBeDefined(); | ||
| expect(typeof VirtualGrid).toBe('function'); | ||
| }); | ||
|
|
||
| it('should have the correct display name', () => { | ||
| // Verify it's a React component | ||
| expect(VirtualGrid.name).toBe('VirtualGrid'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The VirtualGrid component lacks proper test coverage. The existing tests only verify that the component is exported and has the correct name, but do not test any actual functionality.
Based on the test patterns found in the codebase (packages/components/src/tests/test-utils.tsx and packages/react/src/tests/LazyPluginLoader.test.tsx), this component should have tests that:
- Render the component with sample data and columns
- Verify that virtual scrolling is working (only visible rows are rendered)
- Test row click handlers
- Test custom cell renderers
- Verify accessibility attributes
This is particularly important because VirtualGrid is a new, performance-critical component for handling large datasets.
| private evictLFU(): void { | ||
| let oldestKey: string | null = null; | ||
| let oldestTime = Infinity; | ||
| let lowestHits = Infinity; | ||
|
|
||
| // Find the entry with lowest hit count, or oldest if tied | ||
| for (const [key, metadata] of this.cache.entries()) { | ||
| if (metadata.hitCount < lowestHits || | ||
| (metadata.hitCount === lowestHits && metadata.compiledAt < oldestTime)) { | ||
| oldestKey = key; | ||
| oldestTime = metadata.compiledAt; | ||
| lowestHits = metadata.hitCount; | ||
| } | ||
| } | ||
|
|
||
| if (oldestKey) { | ||
| this.cache.delete(oldestKey); | ||
| } | ||
| } |
There was a problem hiding this comment.
The PR description claims "LFU eviction" but the implementation is actually a hybrid LFU/LRU (Least Frequently Used with Least Recently Used as tiebreaker). The evictLFU method finds the entry with the lowest hitCount, and uses compiledAt as a tiebreaker when multiple entries have the same hitCount.
While this is a reasonable eviction strategy, the terminology is imprecise. Consider either:
- Renaming the method to
evictLFUWithLRUTiebreakerorevictHybridfor clarity - Updating the PR description and documentation to reflect that this is a hybrid approach, not pure LFU
- Documenting this behavior in the class-level JSDoc comment
The current implementation is correct, but the naming could be more accurate to avoid confusion.
| // Try JSON first, then YAML | ||
| try { | ||
| schema = JSON.parse(fileContent); | ||
| console.log(chalk.gray(`Reading schema from: ${schemaPath} (detected as JSON)`)); | ||
| } catch { | ||
| schema = loadYaml(fileContent); | ||
| console.log(chalk.gray(`Reading schema from: ${schemaPath} (detected as YAML)`)); |
There was a problem hiding this comment.
The auto-detection of file format (lines 45-52) has a potential issue: if the file is neither valid JSON nor valid YAML, the YAML parser error will be thrown and caught by the outer try-catch, resulting in a generic "Error reading or parsing schema file" message.
This could confuse users who provide an invalid file, as they won't know if the issue is with JSON or YAML parsing. Consider adding a more specific error message when both parsers fail:
try {
schema = JSON.parse(fileContent);
console.log(chalk.gray(`Reading schema from: ${schemaPath} (detected as JSON)`));
} catch (jsonError) {
try {
schema = loadYaml(fileContent);
console.log(chalk.gray(`Reading schema from: ${schemaPath} (detected as YAML)`));
} catch (yamlError) {
throw new Error(`Failed to parse file as JSON or YAML. JSON error: ${jsonError.message}. YAML error: ${yamlError.message}`);
}
}This provides clearer feedback when a file cannot be parsed in either format.
| // Try JSON first, then YAML | |
| try { | |
| schema = JSON.parse(fileContent); | |
| console.log(chalk.gray(`Reading schema from: ${schemaPath} (detected as JSON)`)); | |
| } catch { | |
| schema = loadYaml(fileContent); | |
| console.log(chalk.gray(`Reading schema from: ${schemaPath} (detected as YAML)`)); | |
| // Try JSON first, then YAML with detailed error reporting | |
| try { | |
| schema = JSON.parse(fileContent); | |
| console.log(chalk.gray(`Reading schema from: ${schemaPath} (detected as JSON)`)); | |
| } catch (jsonError) { | |
| try { | |
| schema = loadYaml(fileContent); | |
| console.log(chalk.gray(`Reading schema from: ${schemaPath} (detected as YAML)`)); | |
| } catch (yamlError) { | |
| const jsonMessage = | |
| jsonError instanceof Error ? jsonError.message : String(jsonError); | |
| const yamlMessage = | |
| yamlError instanceof Error ? yamlError.message : String(yamlError); | |
| throw new Error( | |
| `Failed to parse file as JSON or YAML. JSON error: ${jsonMessage}. YAML error: ${yamlMessage}` | |
| ); | |
| } |
Implements performance optimizations for large datasets and developer experience improvements: expression caching to eliminate redundant parsing, virtual scrolling for efficient list rendering, schema validation helpers, and CLI tooling.
Expression Caching
Added
ExpressionCachewith LFU eviction (default: 1000 entries) integrated intoExpressionEvaluator. Convenience functions now share a global cache instance.Impact: Eliminates redundant Function constructor calls for frequently evaluated expressions.
Virtual Scrolling
VirtualGridcomponent using@tanstack/react-virtualrenders only visible rows. Configurable height, row height, overscan, and custom cell renderers.rowBuffer: 10anddebounceVerticalScrollbarfor datasets >1000 rows. AG Grid's built-in virtualization documented.Impact: Handles 10,000+ items without DOM bloat.
Schema Validation
Exported
validateSchema()(throws) andsafeValidateSchema()(returns result object) from@object-ui/types/zod:CLI Commands
objectui validate <schema>- Validates JSON/YAML schemas with formatted error outputobjectui create plugin <name>- Scaffolds plugin structure via@object-ui/create-pluginobjectui analyze- Reports bundle size breakdown and performance recommendationsobjectui generate --from <source>- Placeholder for OpenAPI/Prisma codegenFiles Changed
packages/core/src/evaluator/ExpressionCache.ts(new)packages/core/src/evaluator/ExpressionEvaluator.ts(shared cache)packages/plugin-grid/src/VirtualGrid.tsx(new)packages/plugin-aggrid/src/AgGridImpl.tsx(virtual scroll config)packages/types/src/zod/index.zod.ts(validation exports)packages/cli/src/commands/{validate,create-plugin,analyze}.ts(new)Dependencies
@tanstack/react-virtual@^3.11.3toplugin-gridOriginal prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.