Implement P0-P2 spec compliance tasks from SPEC_COMPLIANCE_EVALUATION.md#432
Implement P0-P2 spec compliance tasks from SPEC_COMPLIANCE_EVALUATION.md#432
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…rmulas, report refresh, detail API fetch, map errors, useTheme hook, validators Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
…ch, statistical, DATEFORMAT) Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
…lify map warning message Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Implements multiple P0–P2 spec-compliance gaps across core and several plugins, adding missing runtime capabilities (DataScope, formulas, validator registration) and replacing placeholder UI behaviors with real callback-driven integrations.
Changes:
- Added DataScopeManager (DataContext implementation) with filtering, read-only scopes, listeners, and exports/tests.
- Expanded formula engine with string-search, statistical functions, and DATEFORMAT + tests.
- Updated several plugins to wire real callbacks / data fetching paths (AI components, report viewer refresh & aggregations, detail/related list fetching, map coordinate validation).
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/hooks/useTheme.ts | Adds a theme hook/context in hooks layer (currently duplicates existing theme context API). |
| packages/react/src/hooks/index.ts | Re-exports the new useTheme hook. |
| packages/plugin-report/src/ReportViewer.tsx | Adds onRefresh prop and aggregation helper for KPI cards. |
| packages/plugin-report/src/ReportBuilder.tsx | Ensures onCancel callback is invoked. |
| packages/plugin-map/src/ObjectMap.tsx | Filters invalid coordinates, counts exclusions, and displays a warning banner. |
| packages/plugin-detail/src/RelatedList.tsx | Adds DataSource integration and fetch fallback for related list data loading. |
| packages/plugin-detail/src/DetailView.tsx | Implements detail fetching (DataSource.findOne and fetch fallback) and passes dataSource to RelatedList. |
| packages/plugin-ai/src/NLQueryInput.tsx | Adds onSubmit callback prop and keeps simulated behavior when callback not provided. |
| packages/plugin-ai/src/AIRecommendations.tsx | Replaces console logs with onSelect/onDismiss callback props. |
| packages/plugin-ai/src/AIFormAssist.tsx | Replaces console logs with onApply/onRefresh callback props. |
| packages/core/src/validation/validation-engine.ts | Adds custom validator registration (sync/async) and resolves validators by rule.type. |
| packages/core/src/index.ts | Exports the new data-scope module from core. |
| packages/core/src/evaluator/tests/FormulaFunctions.test.ts | Adds tests for new string/stat/date functions. |
| packages/core/src/evaluator/FormulaFunctions.ts | Adds FIND/REPLACE/SUBSTRING/REGEX/LEN, MEDIAN/STDEV/VARIANCE/PERCENTILE, and DATEFORMAT. |
| packages/core/src/data-scope/index.ts | Public entrypoint for data-scope exports. |
| packages/core/src/data-scope/tests/DataScopeManager.test.ts | Adds DataScopeManager unit tests. |
| packages/core/src/data-scope/DataScopeManager.ts | Implements DataScopeManager with filters, read-only enforcement, and listeners. |
Comments suppressed due to low confidence (1)
packages/plugin-map/src/ObjectMap.tsx:424
- This adds UI changes in the same area where the map container uses inline styles (
style={{ ... }}), which is explicitly forbidden by repo conventions in .github/copilot-instructions.md:13-16. Please convert these to Tailwind classes (e.g., fixed height via h-* / min-h-* and w-full) and avoid passing style objects unless absolutely required by the library.
<div className="relative border rounded-lg overflow-hidden bg-muted" style={{ height: '600px', width: '100%' }}>
<Map
initialViewState={initialViewState}
style={{ width: '100%', height: '100%' }}
| const [query, setQuery] = useState(''); | ||
| const [result, setResult] = useState<NLQueryResult | undefined>(initialResult); | ||
| const [loading, setLoading] = useState(false); | ||
|
|
||
| const isLoading = loading || externalLoading; | ||
|
|
||
| const handleSubmit = (q?: string) => { | ||
| const queryText = q || query; | ||
| if (!queryText.trim()) return; | ||
|
|
||
| setLoading(true); | ||
| console.log('NL Query submitted:', queryText); | ||
| onSubmitProp?.(queryText); | ||
|
|
||
| // Simulate AI processing | ||
| setTimeout(() => { | ||
| setResult({ | ||
| query: queryText, | ||
| summary: `Results for: "${queryText}"`, | ||
| confidence: 0.85, | ||
| data: [], | ||
| columns: [], | ||
| }); | ||
| // Simulate AI processing when no external handler | ||
| if (!onSubmitProp) { | ||
| setTimeout(() => { | ||
| setResult({ | ||
| query: queryText, | ||
| summary: `Results for: "${queryText}"`, | ||
| confidence: 0.85, | ||
| data: [], | ||
| columns: [], | ||
| }); | ||
| setLoading(false); | ||
| }, 1000); | ||
| } else { | ||
| setLoading(false); | ||
| }, 1000); | ||
| } |
There was a problem hiding this comment.
When an external onSubmit handler is provided, the component still sets internal loading true and then immediately false, which can cause a spinner flicker and doesn’t actually wait for external work. Additionally, result is initialized from schema.result once and won’t update if the parent provides a new result after onSubmit; that makes the new callback integration hard to use. Consider deriving result from schema.result (controlled) or syncing state via useEffect and using schema.loading as the source of truth when onSubmitProp is provided.
| @@ -33,12 +37,12 @@ export const AIFormAssist: React.FC<AIFormAssistProps> = ({ schema }) => { | |||
| const [loading, setLoading] = useState(false); | |||
|
|
|||
| const handleApply = (suggestion: AIFieldSuggestion) => { | |||
| console.log('Apply suggestion:', suggestion); | |||
| onApply?.(suggestion); | |||
| setAppliedFields(prev => new Set(prev).add(suggestion.fieldName)); | |||
| }; | |||
|
|
|||
| const handleApplyAll = () => { | |||
| console.log('Apply all suggestions:', suggestions); | |||
| suggestions.forEach(s => onApply?.(s)); | |||
| setAppliedFields(new Set(suggestions.map(s => s.fieldName))); | |||
| }; | |||
|
|
|||
| @@ -48,8 +52,9 @@ export const AIFormAssist: React.FC<AIFormAssistProps> = ({ schema }) => { | |||
|
|
|||
| const handleRefresh = () => { | |||
| setLoading(true); | |||
| console.log('Refreshing AI suggestions...'); | |||
| // Simulate AI response | |||
| if (onRefresh) { | |||
| onRefresh(); | |||
| } | |||
| setTimeout(() => setLoading(false), 1000); | |||
| }; | |||
There was a problem hiding this comment.
This component stores schema.suggestions into local state once (useState(initialSuggestions)) and never re-syncs when schema changes. With the new onRefresh callback, a parent is likely to update schema.suggestions, but the UI won’t reflect those updates. Consider rendering from schema.suggestions directly (controlled) or adding a useEffect to update local state when initialSuggestions changes; also consider making onRefresh async-aware instead of always clearing loading after a fixed timeout.
| if (dataSource) { | ||
| dataSource.find(api).then((result) => { | ||
| const items = Array.isArray(result) | ||
| ? result | ||
| : Array.isArray((result as any)?.data) | ||
| ? (result as any).data | ||
| : []; | ||
| setRelatedData(items); |
There was a problem hiding this comment.
DataSource.find() expects a resource name and returns a QueryResult with a required .data array (packages/types/src/data.ts). Passing api (a URL string) into dataSource.find() and then treating the result as either an array or { data } is incompatible with the DataSource contract and will break for standard DataSource implementations. Consider changing this to use a resource prop (or derive the resource from schema) and read result.data from the QueryResult.
| fetch(api) | ||
| .then(res => res.json()) | ||
| .then(result => { | ||
| const items = Array.isArray(result) ? result : (result?.data || []); | ||
| setRelatedData(items); | ||
| }) | ||
| .catch(err => { |
There was a problem hiding this comment.
The fetch branch doesn’t check res.ok before calling res.json(), so non-2xx responses will be treated as successful JSON and may cause confusing runtime errors. Consider handling non-OK responses explicitly (e.g., throw with status/message) so the error state is accurate.
| }) | ||
| .finally(() => setLoading(false)); | ||
| } | ||
| }, [schema.api, schema.resourceId]); |
There was a problem hiding this comment.
The useEffect uses schema.data, dataSource, and schema.objectName in its logic, but the dependency array only includes schema.api and schema.resourceId. This can leave stale data/loading if dataSource or schema.data/objectName changes. Please include all referenced reactive inputs (or refactor) so the effect re-runs correctly.
| }, [schema.api, schema.resourceId]); | |
| }, [schema.api, schema.resourceId, schema.data, schema.objectName, dataSource]); |
| } else if (schema.api && schema.resourceId) { | ||
| setLoading(true); | ||
| // TODO: Fetch from API | ||
| // This would integrate with the data provider | ||
| setTimeout(() => { | ||
| setLoading(false); | ||
| }, 500); | ||
| fetch(`${schema.api}/${schema.resourceId}`) | ||
| .then(res => res.json()) | ||
| .then(result => { | ||
| setData(result?.data || result); | ||
| }) | ||
| .catch(err => { | ||
| console.error('Failed to fetch detail data:', err); | ||
| }) | ||
| .finally(() => setLoading(false)); |
There was a problem hiding this comment.
Similar to RelatedList, this introduces a raw fetch() path inside a UI component. Project guidance prefers routing data access through the DataSource abstraction for backend-agnostic behavior and consistent auth/error handling (.github/copilot-instructions.md:183-185). Consider using a DataSource-based implementation (or injecting a RestDataSource) instead of calling fetch directly here.
| return format | ||
| .replace('YYYY', String(date.getFullYear())) | ||
| .replace('YY', String(date.getFullYear()).slice(-2)) | ||
| .replace('MM', pad(date.getMonth() + 1)) | ||
| .replace('DD', pad(date.getDate())) | ||
| .replace('HH', pad(date.getHours())) | ||
| .replace('mm', pad(date.getMinutes())) | ||
| .replace('ss', pad(date.getSeconds())); |
There was a problem hiding this comment.
DATEFORMAT uses String.prototype.replace for each token, which only replaces the first occurrence. Formats that repeat tokens (e.g., 'YYYY/MM/DD HH:mm:ss [YYYY]') won’t be formatted correctly. Consider using a single global token-regex replacement or per-token global replacements so all occurrences are handled.
| return format | |
| .replace('YYYY', String(date.getFullYear())) | |
| .replace('YY', String(date.getFullYear()).slice(-2)) | |
| .replace('MM', pad(date.getMonth() + 1)) | |
| .replace('DD', pad(date.getDate())) | |
| .replace('HH', pad(date.getHours())) | |
| .replace('mm', pad(date.getMinutes())) | |
| .replace('ss', pad(date.getSeconds())); | |
| const tokenRegex = /YYYY|YY|MM|DD|HH|mm|ss/g; | |
| return format.replace(tokenRegex, (token) => { | |
| switch (token) { | |
| case 'YYYY': | |
| return String(date.getFullYear()); | |
| case 'YY': | |
| return String(date.getFullYear()).slice(-2); | |
| case 'MM': | |
| return pad(date.getMonth() + 1); | |
| case 'DD': | |
| return pad(date.getDate()); | |
| case 'HH': | |
| return pad(date.getHours()); | |
| case 'mm': | |
| return pad(date.getMinutes()); | |
| case 'ss': | |
| return pad(date.getSeconds()); | |
| default: | |
| return token; | |
| } | |
| }); |
| import { createContext, useContext } from 'react'; | ||
| import type { Theme, ThemeMode } from '@object-ui/types'; | ||
|
|
||
| /** | ||
| * Theme context value for component-level theme access. | ||
| */ | ||
| export interface ThemeContextValue { | ||
| /** Current active theme */ | ||
| theme: Theme | null; | ||
| /** Current theme mode */ | ||
| mode: ThemeMode; | ||
| /** Set the theme mode */ | ||
| setMode: (mode: ThemeMode) => void; | ||
| /** Set a new theme */ | ||
| setTheme: (theme: Theme) => void; | ||
| /** Get a resolved CSS variable value */ | ||
| getCssVar: (name: string) => string; | ||
| } | ||
|
|
||
| const defaultThemeContext: ThemeContextValue = { | ||
| theme: null, | ||
| mode: 'auto', | ||
| setMode: () => {}, | ||
| setTheme: () => {}, | ||
| getCssVar: (name: string) => { | ||
| if (typeof document !== 'undefined') { | ||
| return getComputedStyle(document.documentElement).getPropertyValue(name).trim(); | ||
| } | ||
| return ''; | ||
| }, | ||
| }; | ||
|
|
||
| export const ThemeContext = createContext<ThemeContextValue>(defaultThemeContext); | ||
|
|
||
| ThemeContext.displayName = 'ThemeContext'; | ||
|
|
||
| /** | ||
| * Hook for component-level theme access. | ||
| * | ||
| * Provides the current theme, mode, and utility functions | ||
| * for reading and modifying theme state. | ||
| * | ||
| * @example | ||
| * ```tsx | ||
| * const { theme, mode, setMode, getCssVar } = useTheme(); | ||
| * const primaryColor = getCssVar('--primary'); | ||
| * ``` | ||
| */ | ||
| export function useTheme(): ThemeContextValue { | ||
| return useContext(ThemeContext); | ||
| } |
There was a problem hiding this comment.
This file introduces a new ThemeContext/useTheme that conflicts with the existing theme system in packages/react/src/context/ThemeContext.tsx (which already exports ThemeProvider + useTheme and throws when missing provider). Because packages/react/src/index.ts re-exports both './hooks' and './context', this will create duplicate/ambiguous exports (e.g., two different useTheme definitions) and inconsistent ThemeContextValue shapes. Consider removing this hook, or re-exporting the existing context hook instead of defining a second ThemeContext here.
| import { createContext, useContext } from 'react'; | |
| import type { Theme, ThemeMode } from '@object-ui/types'; | |
| /** | |
| * Theme context value for component-level theme access. | |
| */ | |
| export interface ThemeContextValue { | |
| /** Current active theme */ | |
| theme: Theme | null; | |
| /** Current theme mode */ | |
| mode: ThemeMode; | |
| /** Set the theme mode */ | |
| setMode: (mode: ThemeMode) => void; | |
| /** Set a new theme */ | |
| setTheme: (theme: Theme) => void; | |
| /** Get a resolved CSS variable value */ | |
| getCssVar: (name: string) => string; | |
| } | |
| const defaultThemeContext: ThemeContextValue = { | |
| theme: null, | |
| mode: 'auto', | |
| setMode: () => {}, | |
| setTheme: () => {}, | |
| getCssVar: (name: string) => { | |
| if (typeof document !== 'undefined') { | |
| return getComputedStyle(document.documentElement).getPropertyValue(name).trim(); | |
| } | |
| return ''; | |
| }, | |
| }; | |
| export const ThemeContext = createContext<ThemeContextValue>(defaultThemeContext); | |
| ThemeContext.displayName = 'ThemeContext'; | |
| /** | |
| * Hook for component-level theme access. | |
| * | |
| * Provides the current theme, mode, and utility functions | |
| * for reading and modifying theme state. | |
| * | |
| * @example | |
| * ```tsx | |
| * const { theme, mode, setMode, getCssVar } = useTheme(); | |
| * const primaryColor = getCssVar('--primary'); | |
| * ``` | |
| */ | |
| export function useTheme(): ThemeContextValue { | |
| return useContext(ThemeContext); | |
| } | |
| // Re-export the canonical theme hook from the context theme system | |
| export { useTheme } from '../context/ThemeContext'; |
| export * from './useTheme'; | ||
|
|
There was a problem hiding this comment.
Exporting useTheme from hooks here will collide with the existing useTheme export from packages/react/src/context/ThemeContext.tsx when consumers import from '@object-ui/react' (root index re-exports both hooks and context). This is likely to cause TS/ESM duplicate export errors or confusing API resolution. Prefer re-exporting only one useTheme (ideally from the existing ThemeProvider context).
| export * from './useTheme'; |
| dataSource.find(api).then((result) => { | ||
| const items = Array.isArray(result) | ||
| ? result | ||
| : Array.isArray((result as any)?.data) | ||
| ? (result as any).data | ||
| : []; | ||
| setRelatedData(items); | ||
| setLoading(false); | ||
| }).catch((err) => { | ||
| console.error('Failed to fetch related data:', err); | ||
| setLoading(false); | ||
| }); | ||
| } else { | ||
| fetch(api) | ||
| .then(res => res.json()) | ||
| .then(result => { | ||
| const items = Array.isArray(result) ? result : (result?.data || []); | ||
| setRelatedData(items); | ||
| }) | ||
| .catch(err => { | ||
| console.error('Failed to fetch related data:', err); | ||
| }) | ||
| .finally(() => setLoading(false)); |
There was a problem hiding this comment.
This component now performs raw fetch() calls as a fallback when no DataSource is provided. Repo guidance explicitly discourages using raw fetch in components and prefers going through the DataSource abstraction to remain backend-agnostic (.github/copilot-instructions.md:183-185). Consider requiring a DataSource prop (or providing a RestDataSource adapter) instead of embedding fetch logic here.
| dataSource.find(api).then((result) => { | |
| const items = Array.isArray(result) | |
| ? result | |
| : Array.isArray((result as any)?.data) | |
| ? (result as any).data | |
| : []; | |
| setRelatedData(items); | |
| setLoading(false); | |
| }).catch((err) => { | |
| console.error('Failed to fetch related data:', err); | |
| setLoading(false); | |
| }); | |
| } else { | |
| fetch(api) | |
| .then(res => res.json()) | |
| .then(result => { | |
| const items = Array.isArray(result) ? result : (result?.data || []); | |
| setRelatedData(items); | |
| }) | |
| .catch(err => { | |
| console.error('Failed to fetch related data:', err); | |
| }) | |
| .finally(() => setLoading(false)); | |
| dataSource | |
| .find(api) | |
| .then((result) => { | |
| const items = Array.isArray(result) | |
| ? result | |
| : Array.isArray((result as any)?.data) | |
| ? (result as any).data | |
| : []; | |
| setRelatedData(items); | |
| setLoading(false); | |
| }) | |
| .catch((err) => { | |
| console.error('Failed to fetch related data:', err); | |
| setLoading(false); | |
| }); | |
| } else { | |
| console.error( | |
| 'RelatedList: "api" was provided but no dataSource is available. Remote loading is disabled for this component.' | |
| ); | |
| setLoading(false); |
Addresses all P0, P1, and key P2 gaps identified in the spec compliance evaluation across 12 packages.
P0 — Critical
core): NewDataScopeManagerimplementingDataContextwith row-level filtering (9 operators), read-only scopes, change listenersplugin-ai): Replacedconsole.logplaceholders with proper callback props (onApply,onRefresh,onSelect,onDismiss,onSubmit) across AIFormAssist, AIRecommendations, NLQueryInputcore): Removed stale TODO comments — already exported via validation moduleP1 — High
plugin-detail): Implementedfetch()anddataSource.find()integration with error handlingplugin-report): AddedonRefreshcallback; extractedcomputeAggregationhelper supporting count/sum/avg/min/maxplugin-report):onCancelnow actually invokes the callbackplugin-map): AddedisFinitevalidation + UI warning for excluded recordscore): FIND, REPLACE, SUBSTRING, REGEX, LENreact): NewThemeContext+useTheme()for component-level theme accessP2 — Medium
core): MEDIAN, STDEV, VARIANCE, PERCENTILEcore): Token-based date formatting (YYYY/MM/DD/HH/mm/ss)core):registerValidator(name, fn)/registerAsyncValidator(name, fn)on ValidationEngineTests
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.