Conversation
# Conflicts: # packages/core/src/fields/list.ts # packages/react-web/src/renderers/ListField.tsx
📝 WalkthroughWalkthroughRefactors hydration to a fetch-driven pattern with discriminated fetch contexts and hydrate callbacks, adds a component registry and page actions context, centralizes i18n helpers, updates field/list cloning to preserve functions, and adjusts hooks and renderers across React, Svelte, and Vue to support the new flows. Changes
Sequence Diagram(s)sequenceDiagram
participant Component
participant useDataForm
participant FetchHook
participant Service
participant BootstrapHook
Component->>useDataForm: mount(schema, hooks, options)
useDataForm->>useDataForm: set loading = true
alt fetch hook present
useDataForm->>FetchHook: call with { type, context, params, hydrate, component }
FetchHook->>Service: read/paginate based on type
Service-->>FetchHook: data/result
FetchHook->>useDataForm: hydrate(data/result)
useDataForm->>useDataForm: update state (values/rows)
end
alt bootstrap hook present (runs after hydrate)
useDataForm->>BootstrapHook: call with bootstrap context
BootstrapHook->>useDataForm: produce schema overrides / disable fields
useDataForm->>useDataForm: merge overrides
end
useDataForm->>Component: provide form API (includes setErrors) and data
useDataForm->>useDataForm: set loading = false
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can customize the high-level summary generated by CodeRabbit.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/svelte/src/use-data-form.ts (2)
71-72:⚠️ Potential issue | 🟠 MajorUpdate the dirty baseline before publishing hydrated state.
dirtyonly subscribes tostate. With the current order,state.set(currentState)recomputes against the oldinitialState, and the laterObject.assign(initialState, currentState)does not trigger another recompute. Hydrated forms will stay dirty until the next state change.Suggested fix
- state.set(currentState) - Object.assign(initialState, currentState) + Object.assign(initialState, currentState) + state.set(currentState)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/svelte/src/use-data-form.ts` around lines 71 - 72, The dirty baseline is updated after publishing the hydrated state, so the dirty subscriber recomputes against the old initialState; swap the operations so the baseline is updated first: call Object.assign(initialState, currentState) before invoking state.set(currentState) (or otherwise ensure initialState is updated prior to state.set) so the dirty computation uses the new baseline when the state is published.
38-91:⚠️ Potential issue | 🟠 MajorAlways clear
loadingand handle hook failures explicitly.If either hook rejects, execution skips Line 88 and
run()still bubbles an unhandled rejection from Line 91. The form can stay stuck inloading=trueon any fetch/bootstrap failure. Wrap the orchestration intry/finallyand add an explicit rejection path forrun().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/svelte/src/use-data-form.ts` around lines 38 - 91, The run function can leave loading=true when fetchHook or bootstrapHook reject and currently its unhandled rejection bubbles from the run() call; wrap the orchestration inside run in a try/finally so loading.set(false) always runs (and still set fieldOverrides/state inside the try), and add an explicit catch inside run to rethrow or return a rejected Promise with the original error (so callers can handle it). Specifically modify run (and its invocation run()) to: surround the existing fetchHook/ events/ bootstrapHook logic with try { ... } finally { loading.set(false) }, and in the try add a catch block that logs or wraps the error then rethrows; ensure run returns the Promise so the external run().catch(...) can handle failures. Ensure you still update fieldOverrides/state inside the try before finally runs.packages/react/src/use-data-form.ts (1)
278-304:⚠️ Potential issue | 🟠 MajorPotential stale closures in action
executecallbacks.The
actionsmemo captureserrors,dirty,valid,reset, andvalidateinside theexecutefunction, but these are not included in the dependency array. This can cause actions to operate on stale form state.🔧 Proposed fix to include missing dependencies
- }, [schema.actions, scope, handlers, state, component, permissions]); + }, [schema.actions, scope, handlers, state, component, permissions, errors, dirty, valid, reset, validate]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/use-data-form.ts` around lines 278 - 304, The memoized actions' execute closures capture form helpers and state (errors, dirty, valid, reset, validate, setErrors, valueRef) but the useMemo dependency array only lists [schema.actions, scope, handlers, state, component, permissions], causing stale closures; update the dependency array for the useMemo that produces actions to include errors, dirty, valid, reset, validate, setErrors, and valueRef.current (or valueRef) so execute uses fresh references, or alternatively avoid closing over mutable values by reading from refs/handlers at call-time (e.g., use valueRef.current and handlers[name] inside execute) and ensure all referenced symbols (actions, execute, handlers, state, component, errors, dirty, valid, reset, validate, setErrors, valueRef) are reflected in dependencies.
🧹 Nitpick comments (15)
packages/svelte/src/use-data-table.ts (1)
94-116: Hydration pattern looks solid.The stale-request guard via
fetchIdand thePromise.resolvewrapper to handle both sync/async hooks are well-implemented.Consider adding a
.catch()to surface fetch errors to the consumer. Currently, iffetchHookrejects, the error is silently swallowed and users cannot distinguish between an empty result and a failed request.♻️ Optional: Add error state handling
+ const error = writable<Error | null>(null) + function reload () { const id = ++fetchId const fetchHook = hooks?.fetch?.[scope] if (!fetchHook) return loading.set(true) + error.set(null) const hydrate = (result: { data: Record<string, unknown>[]; total: number }) => { if (fetchId !== id) return rows.set(result.data) total.set(result.total) } Promise.resolve( fetchHook({ type: 'collection', context: context ?? {}, params: { page: get(page), limit: get(limit), sort: get(sortField), order: get(sortOrder), filters: get(filters), }, hydrate, component, }), - ).finally(() => { + ).catch((err) => { + if (fetchId !== id) return + error.set(err instanceof Error ? err : new Error(String(err))) + }).finally(() => { if (fetchId !== id) return loading.set(false) }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/svelte/src/use-data-table.ts` around lines 94 - 116, The Promise.resolve wrapper around fetchHook currently swallows rejections; add error propagation and state handling by attaching a .catch handler to the Promise.resolve(fetchHook(...)) call that (1) checks the stale-request guard (fetchId !== id) before acting, (2) sets loading.set(false) as needed, and (3) surfaces the error to the consumer (e.g., call an onError callback or set an error store/state) so callers of useDataTable can distinguish failures from empty results; update references to hydrate, fetchHook, fetchId, id, and loading (and optionally rows/total) when implementing this error path.packages/demo/src/settings/hooks.ts (1)
13-26: Consider extracting duplicate fetch logic forScope.viewandScope.edit.The fetch handlers for
Scope.viewandScope.editare identical. This duplication can be reduced by extracting a shared helper.♻️ Proposed refactor to reduce duplication
+async function fetchRecord(context: FetchHookContext, service: ServiceContract) { + if (context.type !== FetchType.record || !context.context.id) { + return + } + const data = await service.read(context.context.id as string) + context.hydrate(data) +} + export function createDefault (service: ServiceContract) { return { bootstrap: { async [Scope.view] ({ schema }: BootstrapHookContext) { for (const field of Object.values(schema)) { field.disabled = true } }, }, fetch: { - async [Scope.view] (context: FetchHookContext) { - if (context.type !== FetchType.record || !context.context.id) { - return - } - const data = await service.read(context.context.id as string) - context.hydrate(data) - }, - async [Scope.edit] (context: FetchHookContext) { - if (context.type !== FetchType.record || !context.context.id) { - return - } - const data = await service.read(context.context.id as string) - context.hydrate(data) - }, + [Scope.view]: (ctx: FetchHookContext) => fetchRecord(ctx, service), + [Scope.edit]: (ctx: FetchHookContext) => fetchRecord(ctx, service), async [Scope.index] (context: FetchHookContext) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/demo/src/settings/hooks.ts` around lines 13 - 26, The handlers for Scope.view and Scope.edit duplicate the same fetch/hydrate logic; extract a shared async helper (e.g., fetchAndHydrateRecord(context: FetchHookContext)) that performs the early return when context.type !== FetchType.record or missing context.context.id, calls service.read(context.context.id as string), then calls context.hydrate(data), and invoke that helper from both the [Scope.view] and [Scope.edit] handlers so behavior and types for FetchHookContext, FetchType, service.read and context.hydrate are preserved.packages/core/src/types.ts (1)
210-217: Consider whetherPaginateParamsis appropriate forFetchHookContextwithtype: 'record'.For a single record fetch, including
PaginateParams(withpage,limit,sort,order,filters) seems semantically misaligned. Currently, callers pass placeholder values like{ page: 1, limit: 1 }which adds noise.Consider introducing a separate params type for record fetches (e.g., just
contextwith the record identifier) or documenting why the unified params shape is intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/types.ts` around lines 210 - 217, FetchHookContext currently uses PaginateParams for the 'record' variant, which is semantically wrong and forces callers to pass placeholder pagination values; change the type by replacing PaginateParams in the 'record' branch with a new minimal params type (e.g., RecordParams or RecordFetchParams) that contains only the record identifier(s) needed by your API (or make params optional/absent if the identifier lives in context), update the FetchHookContext declaration to reference this new type for the branch where type: 'record', and adjust any usages of FetchHookContext.fetch/hydrate, component, or callers to pass the new smaller params shape (or remove dummy {page, limit} values) so callers no longer need to supply pagination for single-record fetches.packages/core/src/fields/base.ts (1)
101-110: Line 103 is unreachable dead code.The check
typeof value === 'function'on line 103 will never execute because line 102 already returns early for functions. Sincetypeof someFunction !== 'object'istrue, functions are returned on line 102.♻️ Proposed simplification
function cloneWithFunctions(value: unknown): unknown { if (value === null || typeof value !== 'object') return value - if (typeof value === 'function') return value if (Array.isArray(value)) return value.map(cloneWithFunctions) const result: Record<string, unknown> = {} for (const key of Object.keys(value as object)) { const v = (value as Record<string, unknown>)[key] result[key] = typeof v === 'function' ? v : cloneWithFunctions(v) } return result }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/fields/base.ts` around lines 101 - 110, In cloneWithFunctions, the check "if (typeof value === 'function')" is unreachable because the prior "if (value === null || typeof value !== 'object') return value" already returns for functions; remove the redundant function-type branch from cloneWithFunctions and keep the early return for non-objects/null, the Array.isArray(value) branch, and the recursive object-cloning loop so functions remain returned as-is while objects/arrays are cloned.packages/vue/src/use-data-form.ts (1)
35-35: Comments are in Portuguese; consider using English for consistency.The inline comments on lines 35, 47, and 68 are in Portuguese ("hidrata os dados", "aplica hydrate e dispara events", "orquestra estado do schema"). For codebase consistency and international collaboration, English comments are typically preferred.
♻️ Suggested English translations
- // 1. fetch → hidrata os dados + // 1. fetch → hydrate data if (fetchHook) {- // 2. aplica hydrate e dispara events + // 2. apply hydration and fire events if (hydratedData) {- // 3. bootstrap → orquestra estado do schema (roda após hydrate) + // 3. bootstrap → orchestrate schema state (runs after hydrate) if (bootstrapHook) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vue/src/use-data-form.ts` at line 35, Replace the three Portuguese inline comments in use-data-form.ts with English equivalents for consistency: change the comment at the start of the data fetch block ("hidrata os dados") to "fetch → hydrate the data", the comment near the hydrate/events application ("aplica hydrate e dispara events") to "apply hydrate and emit events", and the comment around schema orchestration ("orquestra estado do schema") to "orchestrate schema state"; update the comments adjacent to the functions/blocks that perform these tasks (e.g., the fetch/hydrate block, the hydrate+events handler, and the schema orchestration logic) so the intent is clear in English.packages/core/src/fields/list.ts (1)
30-43: Consider stronger typing for fluent method parameters.The new
events(),hooks(), andhandlers()methods acceptRecord<string, unknown>, which is quite loose. If these correspond to specific contract types (e.g.,SchemaEvents,SchemaHooks,SchemaHandlers), consider using those types to provide better type safety and IDE autocompletion for consumers.💡 Example with stricter typing
- events(events: Record<string, unknown>): this { + events(events: SchemaEvents<any>): this { this._config.attrs = { ...this._config.attrs, events } return this }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/fields/list.ts` around lines 30 - 43, The methods events(), hooks(), and handlers() currently accept the loose Record<string, unknown>; replace those parameter types with the specific contract types (e.g., SchemaEvents, SchemaHooks, SchemaHandlers) used by your schema/field system so consumers get stronger typing and autocomplete; update the function signatures for events(), hooks(), and handlers() to use those types, adjust imports or generics where needed, and ensure assignments to this._config.attrs remain compatible (adding casts only if absolutely necessary).packages/react-web/src/renderers/list/useListComponent.ts (1)
13-18: Consider clarifyinginferScopebehavior for parameterized routes.
inferScopecompares paths using strict equality (path === route.path), which means it will match literal strings like'add'or':id'but not actual ID values like'123'or'abc-456/edit'.Based on the usage in
handleNavigation, it appears this is called with route constants rather than actual URLs, which makes this behavior correct. If this hook is ever used with actual path segments, the matching logic would need adjustment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-web/src/renderers/list/useListComponent.ts` around lines 13 - 18, The inferScope function currently uses strict equality between the incoming path and DEFAULT_SCOPES' route.path which fails for concrete parameterized segments (e.g., '123' vs ':id'); update this by either (A) changing inferScope to match parameterized routes (e.g., normalize/match route.path patterns like ':id' or '/:id/edit' against actual path segments) or (B) explicitly document/guard its intended use with route constants only—add a short comment above inferScope and, if used in handleNavigation, ensure callers pass route constants (not raw URLs) or perform normalization before calling inferScope; reference function name inferScope and DEFAULT_SCOPES and the consumer handleNavigation when making the change.packages/react-web/src/types.ts (1)
79-79: Consider using English for JSDoc/inline comments.The comment describing
DataTableEmptyStateInputis in Portuguese. For consistency with the rest of the codebase, use English.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-web/src/types.ts` at line 79, Update the JSDoc comment for the DataTableEmptyStateInput type to English to match the codebase language; locate the comment currently in Portuguese above DataTableEmptyStateInput and replace it with an English equivalent describing the props passed by the consumer and that `action` accepts a name or an ActionConfig from the schema.packages/react/src/use-data-form.ts (1)
39-40: Consider using English for code comments.Comments like "fetch → hidrata os dados", "aplica hydrate e dispara events", and "bootstrap → orquestra estado do schema" should be in English for consistency.
Also applies to: 51-51, 72-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/use-data-form.ts` around lines 39 - 40, Replace the Portuguese inline comments in packages/react/src/use-data-form.ts with clear English equivalents to keep code consistent; specifically update the comments near the fetchHook block ("fetch → hidrata os dados"), the comment around the hydrate/events application ("aplica hydrate e dispara events"), and the bootstrap/state-orchestration comment ("bootstrap → orquestra estado do schema") to concise English phrases (e.g., "fetch → hydrate data", "apply hydrate and emit events", "bootstrap → orchestrate schema state") while leaving surrounding logic (fetchHook, hydrate, event dispatching, bootstrap/orchestration code paths) unchanged.packages/react-web/src/components/defaults/Pagination.tsx (1)
35-35: Consider using English for code comments.The inline comments are in Portuguese. For consistency and broader maintainability, consider using English.
Also applies to: 65-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-web/src/components/defaults/Pagination.tsx` at line 35, Replace the Portuguese inline comments in the Pagination component with English equivalents to improve consistency and maintainability: locate the comments in the Pagination.tsx component (e.g., the comment reading "Esquerda: Itens por página" and the other comment noted by the reviewer) and update them to concise English descriptions (for example "Left: Items per page" or similar) preserving meaning and context near the related JSX elements or props.packages/react-web/src/renderers/SelectField.tsx (1)
24-24: Consider memoizinggetComponentresult if registry lookups are expensive.
getComponent('SelectInput')is called on every render. If the registry lookup has any overhead, consider usinguseMemoor ensuringgetComponentis internally memoized.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-web/src/renderers/SelectField.tsx` at line 24, The code calls getComponent('SelectInput') on every render which can be expensive; change CustomSelect to be obtained once by memoizing the lookup — e.g., wrap getComponent('SelectInput') in a React useMemo or ensure getComponent caches internally — so replace the repeated call that assigns const CustomSelect = getComponent('SelectInput') with a memoized result tied to no or appropriate dependencies (referencing CustomSelect and getComponent('SelectInput') to locate the site).packages/react-web/src/components/Table.tsx (1)
405-411: Inconsistent use of Position enum vs string literal.Line 353 uses
Position.headerbut line 406 uses the string'top'. UsePosition.topfor consistency and type safety.Proposed fix
{showTopActions && table.actions - .filter((a) => a.config.positions.includes('top')) + .filter((a) => a.config.positions.includes(Position.top)) .map((a) => <ResolvedActionButton🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-web/src/components/Table.tsx` around lines 405 - 411, The code mixes string literals and the Position enum for action positions; replace the string 'top' used in the filter at the showTopActions render with the enum value Position.top to match usage like Position.header and preserve type safety — update the filter on table.actions (the .filter((a) => a.config.positions.includes('top')) expression) to use Position.top and ensure Position is imported/available where ResolvedActionButton is rendered.packages/react-web/src/renderers/ListField.tsx (3)
68-71: Unused refsrawValueRefanddialogStateRef.These refs are created and updated but do not appear to be used within this component. If they're intended for use by child components or hooks, consider passing them explicitly; otherwise, remove to reduce noise.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-web/src/renderers/ListField.tsx` around lines 68 - 71, The refs rawValueRef and dialogStateRef in ListField are created and updated but never used; either remove the unused declarations and their assignments (delete const rawValueRef = useRef(rawValue); rawValueRef.current = rawValue; const dialogStateRef = useRef(dialogState); dialogStateRef.current = dialogState) or, if they were meant to be consumed by children or hooks, explicitly pass those refs into the child components or hooks that need them (e.g., as props or via context) so usage is visible; update any related code in the ListField component to reflect the removal or the new prop/context usage.
94-117: Modal lacks an explicit close button for accessibility.The modal can only be closed by clicking the overlay backdrop. For better accessibility (keyboard navigation, screen readers), consider adding an explicit close button inside the modal with proper ARIA attributes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-web/src/renderers/ListField.tsx` around lines 94 - 117, The modal markup rendered by createPortal lacks an explicit close control; add a visible, focusable close button inside the inner modal div (the element with style={styles.modal}) that calls closeDialog on click, includes an accessible label (e.g., aria-label="Close" or aria-label="Close dialog") and keyboard support (focusable via Tab and triggers close on Enter/Space), and ensure it is reachable by screen readers near the DataForm; optionally hook an onKeyDown for Escape to call closeDialog if not already handled elsewhere.
32-47: Reactive array wrapper may cause unexpected behavior.The
reactiveValuememo creates a custom array with overriddenpushandsplicemethods. However:
- The
splicesignature diverges from the native Array—whendeleteCountis omitted, it should default to removing all elements fromstartto the end, but heredeleteCount ?? next.lengthremovesnext.lengthelements which could exceed bounds.- Returning
next.lengthfrompushandremovedfromsplicereflects the local computation, butonChangeRef.current(next)triggers an async state update—callers expecting the array to be mutated in place will be surprised.If callers rely on standard Array semantics, consider documenting this behavior or using a proxy pattern that better mimics native semantics.
Potential fix for splice default
arr.splice = (start: number, deleteCount?: number, ...items: Record<string, unknown>[]) => { const next = [...rawValue] - const removed = next.splice(start, deleteCount ?? next.length, ...items) + const removed = next.splice(start, deleteCount ?? (next.length - start), ...items) onChangeRef.current(next) return removed }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-web/src/renderers/ListField.tsx` around lines 32 - 47, The reactive array wrapper overrides Array.push and Array.splice but misimplements splice default and can surprise callers because mutations aren’t in-place; change splice’s default deleteCount to (next.length - start) so removing until end behaves like native (use deleteCount ?? (next.length - start)), and update push/splice behavior to more closely mimic native semantics by either (a) applying mutations to the proxy/returned array synchronously before calling onChangeRef.current or (b) replace this pattern with a Proxy that intercepts mutations and forwards them to onChangeRef.current while reflecting changes immediately; locate and modify the reactiveValue creation and its push/splice implementations (and keep onChangeRef.current(next) calls) to ensure return values and in-place mutation semantics match native Array expectations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react-web/src/components/DataPage.tsx`:
- Around line 49-67: The DataPage component currently early-returns full-screen
loading/forbidden UIs (using loading, permitted, forbidden and
styles.loading/styles.forbidden), which breaks host-controlled layouts for pages
rendered with layout="flat" or bare mode; change the render logic to avoid early
returns so flat/bare pages are not replaced with a 100vh wrapper: detect
flat/bare (props.layout === 'flat' or props.bare) and for those cases render
inline/smaller loading or forbidden placeholders (reuse Icon and
styles.forbiddenText but remove minHeight: '100vh' and the full-screen
container) or render the existing children plus a lightweight overlay; otherwise
keep the current full-screen behavior for non-flat pages. Ensure you update the
conditional branches around loading/permitted/forbidden in DataPage to return
the children/layout path for flat/bare while showing a non-fullscreen indicator.
- Around line 74-84: The current DataPage uses resolvedHeaderEnd = headerEnd ??
registeredActions which drops registeredActions when headerEnd is provided;
instead compose both so nested-registered actions are not hidden: in the
DataPage render (look for headerEnd, registeredActions, resolvedHeaderEnd)
replace the nullish-coalescing behavior with a composed fragment/element that
renders registeredActions and headerEnd together (preserving desired order and
spacing) and use that composed value where resolvedHeaderEnd is used so both
sets of controls show.
In `@packages/react-web/src/components/defaults/Pagination.tsx`:
- Around line 9-14: The getVisiblePages function can produce fewer than 5 pages
near the end because start isn't re-adjusted after end is clamped; update
getVisiblePages to ensure a consistent 5-item sliding window by computing end =
Math.min(total, start + 4) and then, if (end - start + 1) < 5, shift start =
Math.max(1, end - 4) (or equivalently compute start = Math.max(1,
Math.min(current - 2, total - 4))) so the returned Array.from(...) always yields
up to 5 page numbers when total >= 5.
In `@packages/react-web/src/components/Form.tsx`:
- Around line 36-50: The effect in useLayoutEffect is stale because it only
depends on pageActions and props.schema.domain and never cleans up the
registered node; update the dependency array to include form.actions (or the
derived headerActions) so the effect re-runs when form.actions changes, remove
the eslint-disable comment, and add a cleanup that removes the previously
registered element—capture the return value from pageActions.register (or the
DOM node you passed) and call the corresponding unregister/remove API in the
cleanup function so the header buttons are deregistered when headerActions
becomes empty or on unmount; target symbols: useLayoutEffect,
pageActions.register, form.actions, headerActions, ResolvedActionButton.
In `@packages/react-web/src/components/Table.tsx`:
- Around line 332-347: The current lookup that finds a resolved action via
table.actions.find comparing config.variant, config.positions.join(), and
config.order is fragile (in the block that casts action as ActionConfig and
assigns const resolved). Change the matching to use a unique
identifier—preferably the action name (e.g., match a.config.name ===
config.name) or a dedicated unique id field on ActionConfig/a.config—so resolved
is selected deterministically; update any callers/fixtures to populate that
identifier and keep the existing fallback (return { ...rest }) if no match is
found.
- Around line 350-367: The useLayoutEffect is missing dependencies so
headerActions can become stale; update the dependency array to include
table.actions (or table if actions isn't stable) and the ResolvedActionButton
component reference so the effect re-runs when actions or the component change,
e.g., replace [pageActions, domain] with [pageActions, domain, table.actions,
ResolvedActionButton] (or [pageActions, domain, table] if actions is accessed
via table), and remove the eslint-disable comment; keep the logic that computes
headerActions and calls pageActions.register unchanged.
- Line 312: The search state (const [search, setSearch]) is never applied to the
table, so typing in ResolvedSearchBar doesn't filter results; hook the state up
to the table by calling the table filtering API whenever search changes (e.g.,
in a useEffect call table.setGlobalFilter(search) or
table.setFilter('<yourColumnId>', search) depending on whether you use a global
filter or a specific column filter). Ensure the effect runs on mount and when
search updates and that clearing search clears the table filter; keep setSearch
passed into ResolvedSearchBar as-is.
In `@packages/react-web/src/renderers/ListField.tsx`:
- Around line 55-61: The hook call uses a non-null assertion on schema (schema!)
before the component returns early when schema may be undefined; move the
useListComponent(...) invocation to after the existing early-return check (if
(!schema || proxy.hidden) return ...) so it only runs when schema is defined, or
alternatively remove the non-null assertion and update useListComponent’s
call/site types to accept SchemaProvide | undefined and handle undefined inside;
update references to tableComponent accordingly (useListComponent, schema,
tableComponent).
In `@packages/react-web/src/renderers/TextareaField.tsx`:
- Around line 97-102: The description style in TextareaField.tsx currently sets
both marginTop and margin: 0, which neutralizes the intended spacing; update the
description style (the description key in the style/object) to remove the
blanket margin: 0 so the marginTop: theme.spacing.xs takes effect
(alternatively, replace margin: 0 with a more specific shorthand like
marginBottom: 0 if you need to reset only the bottom margin while preserving
marginTop).
In `@packages/react-web/src/renderers/TextField.tsx`:
- Around line 96-101: The description style in TextField.tsx mistakenly uses
"margin: 0" which overrides "marginTop: theme.spacing.xs"; remove the "margin:
0" property (or replace it with an explicit "marginBottom" /
"marginLeft"/"marginRight" if needed) inside the description style object so
that description keeps marginTop: theme.spacing.xs (look for the "description"
style object and the theme references like theme.spacing.xs and
theme.colors.mutedForeground).
In `@packages/react-web/src/theme/default.ts`:
- Around line 63-69: The defaultTheme object is mutable at the type level;
update the declaration of defaultTheme to be readonly by adding "as const" while
still validating against Theme (i.e., declare it as const defaultTheme = { ... }
as const satisfies Theme); locate the defaultTheme symbol in this file and
change its declaration so nested tokens become readonly and TypeScript won't
allow assignments like defaultTheme.colors.primary = "...".
In `@packages/react/src/use-data-table.ts`:
- Around line 85-97: The effect/callback that calls fetchHook (the block using
fetchHook({ type: 'collection', context: context ?? {}, ... })) captures
`context` but does not list it in its dependency array; add `context` to the
dependency array (alongside hooks?.fetch, scope, page, limit, sortField,
sortOrder, filtersKey, component) so the fetch logic (and the fetchIdRef/current
check and setLoading(false) in the finally) will re-run when `context` changes.
- Around line 9-17: The initial state sets rows to value but leaves total as 0,
causing a first-render mismatch; update the useState for total to initialize
with value.length so total mirrors rows on mount (keep the existing useEffect
that overrides when hooks?.fetch?.[scope] is present); locate the declarations
of rows/setRows and total/setTotal in use-data-table.ts and change the total
initialization accordingly to use value.length.
- Around line 84-95: The current call uses Promise.resolve(fetchHook({...}))
which calls fetchHook synchronously and can throw before the promise chain runs,
leaving loading stuck; change it to defer execution by using
Promise.resolve().then(() => fetchHook({ type: 'collection', context: context ??
{}, params: { page, limit, sort: sortField, order: sortOrder, filters },
hydrate, component, })).finally(() => { if (fetchIdRef.current !== id) return;
setLoading(false); }); so synchronous throws are caught by the promise chain and
.finally always runs.
In `@packages/svelte/src/use-data-form.ts`:
- Around line 84-85: When you apply committed overrides (the spot where
fieldOverrides.set(currentOverrides) is called), snapshot a copy of
currentOverrides into a persistent variable (e.g., committedFieldOverrides) so
those override flags become the baseline; then update reset() so that when no
explicit values are passed it restores fieldOverrides from that committed
snapshot instead of clearing to {}. Ensure the snapshot is a defensive copy (not
the original object) and reference the symbols currentOverrides,
fieldOverrides.set(...), committedFieldOverrides (new variable) and reset() when
making the change.
---
Outside diff comments:
In `@packages/react/src/use-data-form.ts`:
- Around line 278-304: The memoized actions' execute closures capture form
helpers and state (errors, dirty, valid, reset, validate, setErrors, valueRef)
but the useMemo dependency array only lists [schema.actions, scope, handlers,
state, component, permissions], causing stale closures; update the dependency
array for the useMemo that produces actions to include errors, dirty, valid,
reset, validate, setErrors, and valueRef.current (or valueRef) so execute uses
fresh references, or alternatively avoid closing over mutable values by reading
from refs/handlers at call-time (e.g., use valueRef.current and handlers[name]
inside execute) and ensure all referenced symbols (actions, execute, handlers,
state, component, errors, dirty, valid, reset, validate, setErrors, valueRef)
are reflected in dependencies.
In `@packages/svelte/src/use-data-form.ts`:
- Around line 71-72: The dirty baseline is updated after publishing the hydrated
state, so the dirty subscriber recomputes against the old initialState; swap the
operations so the baseline is updated first: call Object.assign(initialState,
currentState) before invoking state.set(currentState) (or otherwise ensure
initialState is updated prior to state.set) so the dirty computation uses the
new baseline when the state is published.
- Around line 38-91: The run function can leave loading=true when fetchHook or
bootstrapHook reject and currently its unhandled rejection bubbles from the
run() call; wrap the orchestration inside run in a try/finally so
loading.set(false) always runs (and still set fieldOverrides/state inside the
try), and add an explicit catch inside run to rethrow or return a rejected
Promise with the original error (so callers can handle it). Specifically modify
run (and its invocation run()) to: surround the existing fetchHook/ events/
bootstrapHook logic with try { ... } finally { loading.set(false) }, and in the
try add a catch block that logs or wraps the error then rethrows; ensure run
returns the Promise so the external run().catch(...) can handle failures. Ensure
you still update fieldOverrides/state inside the try before finally runs.
---
Nitpick comments:
In `@packages/core/src/fields/base.ts`:
- Around line 101-110: In cloneWithFunctions, the check "if (typeof value ===
'function')" is unreachable because the prior "if (value === null || typeof
value !== 'object') return value" already returns for functions; remove the
redundant function-type branch from cloneWithFunctions and keep the early return
for non-objects/null, the Array.isArray(value) branch, and the recursive
object-cloning loop so functions remain returned as-is while objects/arrays are
cloned.
In `@packages/core/src/fields/list.ts`:
- Around line 30-43: The methods events(), hooks(), and handlers() currently
accept the loose Record<string, unknown>; replace those parameter types with the
specific contract types (e.g., SchemaEvents, SchemaHooks, SchemaHandlers) used
by your schema/field system so consumers get stronger typing and autocomplete;
update the function signatures for events(), hooks(), and handlers() to use
those types, adjust imports or generics where needed, and ensure assignments to
this._config.attrs remain compatible (adding casts only if absolutely
necessary).
In `@packages/core/src/types.ts`:
- Around line 210-217: FetchHookContext currently uses PaginateParams for the
'record' variant, which is semantically wrong and forces callers to pass
placeholder pagination values; change the type by replacing PaginateParams in
the 'record' branch with a new minimal params type (e.g., RecordParams or
RecordFetchParams) that contains only the record identifier(s) needed by your
API (or make params optional/absent if the identifier lives in context), update
the FetchHookContext declaration to reference this new type for the branch where
type: 'record', and adjust any usages of FetchHookContext.fetch/hydrate,
component, or callers to pass the new smaller params shape (or remove dummy
{page, limit} values) so callers no longer need to supply pagination for
single-record fetches.
In `@packages/demo/src/settings/hooks.ts`:
- Around line 13-26: The handlers for Scope.view and Scope.edit duplicate the
same fetch/hydrate logic; extract a shared async helper (e.g.,
fetchAndHydrateRecord(context: FetchHookContext)) that performs the early return
when context.type !== FetchType.record or missing context.context.id, calls
service.read(context.context.id as string), then calls context.hydrate(data),
and invoke that helper from both the [Scope.view] and [Scope.edit] handlers so
behavior and types for FetchHookContext, FetchType, service.read and
context.hydrate are preserved.
In `@packages/react-web/src/components/defaults/Pagination.tsx`:
- Line 35: Replace the Portuguese inline comments in the Pagination component
with English equivalents to improve consistency and maintainability: locate the
comments in the Pagination.tsx component (e.g., the comment reading "Esquerda:
Itens por página" and the other comment noted by the reviewer) and update them
to concise English descriptions (for example "Left: Items per page" or similar)
preserving meaning and context near the related JSX elements or props.
In `@packages/react-web/src/components/Table.tsx`:
- Around line 405-411: The code mixes string literals and the Position enum for
action positions; replace the string 'top' used in the filter at the
showTopActions render with the enum value Position.top to match usage like
Position.header and preserve type safety — update the filter on table.actions
(the .filter((a) => a.config.positions.includes('top')) expression) to use
Position.top and ensure Position is imported/available where
ResolvedActionButton is rendered.
In `@packages/react-web/src/renderers/list/useListComponent.ts`:
- Around line 13-18: The inferScope function currently uses strict equality
between the incoming path and DEFAULT_SCOPES' route.path which fails for
concrete parameterized segments (e.g., '123' vs ':id'); update this by either
(A) changing inferScope to match parameterized routes (e.g., normalize/match
route.path patterns like ':id' or '/:id/edit' against actual path segments) or
(B) explicitly document/guard its intended use with route constants only—add a
short comment above inferScope and, if used in handleNavigation, ensure callers
pass route constants (not raw URLs) or perform normalization before calling
inferScope; reference function name inferScope and DEFAULT_SCOPES and the
consumer handleNavigation when making the change.
In `@packages/react-web/src/renderers/ListField.tsx`:
- Around line 68-71: The refs rawValueRef and dialogStateRef in ListField are
created and updated but never used; either remove the unused declarations and
their assignments (delete const rawValueRef = useRef(rawValue);
rawValueRef.current = rawValue; const dialogStateRef = useRef(dialogState);
dialogStateRef.current = dialogState) or, if they were meant to be consumed by
children or hooks, explicitly pass those refs into the child components or hooks
that need them (e.g., as props or via context) so usage is visible; update any
related code in the ListField component to reflect the removal or the new
prop/context usage.
- Around line 94-117: The modal markup rendered by createPortal lacks an
explicit close control; add a visible, focusable close button inside the inner
modal div (the element with style={styles.modal}) that calls closeDialog on
click, includes an accessible label (e.g., aria-label="Close" or
aria-label="Close dialog") and keyboard support (focusable via Tab and triggers
close on Enter/Space), and ensure it is reachable by screen readers near the
DataForm; optionally hook an onKeyDown for Escape to call closeDialog if not
already handled elsewhere.
- Around line 32-47: The reactive array wrapper overrides Array.push and
Array.splice but misimplements splice default and can surprise callers because
mutations aren’t in-place; change splice’s default deleteCount to (next.length -
start) so removing until end behaves like native (use deleteCount ??
(next.length - start)), and update push/splice behavior to more closely mimic
native semantics by either (a) applying mutations to the proxy/returned array
synchronously before calling onChangeRef.current or (b) replace this pattern
with a Proxy that intercepts mutations and forwards them to onChangeRef.current
while reflecting changes immediately; locate and modify the reactiveValue
creation and its push/splice implementations (and keep onChangeRef.current(next)
calls) to ensure return values and in-place mutation semantics match native
Array expectations.
In `@packages/react-web/src/renderers/SelectField.tsx`:
- Line 24: The code calls getComponent('SelectInput') on every render which can
be expensive; change CustomSelect to be obtained once by memoizing the lookup —
e.g., wrap getComponent('SelectInput') in a React useMemo or ensure getComponent
caches internally — so replace the repeated call that assigns const CustomSelect
= getComponent('SelectInput') with a memoized result tied to no or appropriate
dependencies (referencing CustomSelect and getComponent('SelectInput') to locate
the site).
In `@packages/react-web/src/types.ts`:
- Line 79: Update the JSDoc comment for the DataTableEmptyStateInput type to
English to match the codebase language; locate the comment currently in
Portuguese above DataTableEmptyStateInput and replace it with an English
equivalent describing the props passed by the consumer and that `action` accepts
a name or an ActionConfig from the schema.
In `@packages/react/src/use-data-form.ts`:
- Around line 39-40: Replace the Portuguese inline comments in
packages/react/src/use-data-form.ts with clear English equivalents to keep code
consistent; specifically update the comments near the fetchHook block ("fetch →
hidrata os dados"), the comment around the hydrate/events application ("aplica
hydrate e dispara events"), and the bootstrap/state-orchestration comment
("bootstrap → orquestra estado do schema") to concise English phrases (e.g.,
"fetch → hydrate data", "apply hydrate and emit events", "bootstrap →
orchestrate schema state") while leaving surrounding logic (fetchHook, hydrate,
event dispatching, bootstrap/orchestration code paths) unchanged.
In `@packages/svelte/src/use-data-table.ts`:
- Around line 94-116: The Promise.resolve wrapper around fetchHook currently
swallows rejections; add error propagation and state handling by attaching a
.catch handler to the Promise.resolve(fetchHook(...)) call that (1) checks the
stale-request guard (fetchId !== id) before acting, (2) sets loading.set(false)
as needed, and (3) surfaces the error to the consumer (e.g., call an onError
callback or set an error store/state) so callers of useDataTable can distinguish
failures from empty results; update references to hydrate, fetchHook, fetchId,
id, and loading (and optionally rows/total) when implementing this error path.
In `@packages/vue/src/use-data-form.ts`:
- Line 35: Replace the three Portuguese inline comments in use-data-form.ts with
English equivalents for consistency: change the comment at the start of the data
fetch block ("hidrata os dados") to "fetch → hydrate the data", the comment near
the hydrate/events application ("aplica hydrate e dispara events") to "apply
hydrate and emit events", and the comment around schema orchestration
("orquestra estado do schema") to "orchestrate schema state"; update the
comments adjacent to the functions/blocks that perform these tasks (e.g., the
fetch/hydrate block, the hydrate+events handler, and the schema orchestration
logic) so the intent is clear in English.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4d84c580-6eaa-4922-9a0c-a8eaf51c600b
📒 Files selected for processing (48)
packages/core/src/fields/base.tspackages/core/src/fields/list.tspackages/core/src/index.tspackages/core/src/locales/pt-BR.tspackages/core/src/mock.tspackages/core/src/schema.tspackages/core/src/types.tspackages/demo/src/settings/hooks.tspackages/react-web/src/components/DataPage.tsxpackages/react-web/src/components/Dialog.tsxpackages/react-web/src/components/Form.tsxpackages/react-web/src/components/PageActionsContext.tsxpackages/react-web/src/components/Table.tsxpackages/react-web/src/components/defaults/ColumnSelector.tsxpackages/react-web/src/components/defaults/DebugPanel.tsxpackages/react-web/src/components/defaults/EmptyState.tsxpackages/react-web/src/components/defaults/Pagination.tsxpackages/react-web/src/components/registry.tspackages/react-web/src/contracts/component.tspackages/react-web/src/index.tspackages/react-web/src/renderers/DateField.tsxpackages/react-web/src/renderers/ListField.tsxpackages/react-web/src/renderers/MultiSelectField.tsxpackages/react-web/src/renderers/NumberField.tsxpackages/react-web/src/renderers/SelectField.tsxpackages/react-web/src/renderers/TextField.tsxpackages/react-web/src/renderers/TextareaField.tsxpackages/react-web/src/renderers/TimeField.tsxpackages/react-web/src/renderers/ToggleField.tsxpackages/react-web/src/renderers/list/useListComponent.tspackages/react-web/src/renderers/list/useListDialog.tspackages/react-web/src/support/Icon.tsxpackages/react-web/src/support/i18n.tspackages/react-web/src/theme/default.tspackages/react-web/src/types.tspackages/react/src/types.tspackages/react/src/use-data-form.tspackages/react/src/use-data-table.tspackages/svelte/src/types.tspackages/svelte/src/use-data-form.tspackages/svelte/src/use-data-table.tspackages/vue/src/types.tspackages/vue/src/use-data-form.tspackages/vue/src/use-data-table.tsplayground/react-native/tests/src/domain/person/hooks.test.tsplayground/react-web/tests/src/domain/person/hooks.test.tsplayground/sveltekit/tests/src/domain/person/hooks.test.tsplayground/vue-quasar/tests/src/domain/person/hooks.test.ts
Comment on lines
49
to
+67
| if (loading) { | ||
| return ( | ||
| <div style={styles.loading}>Loading...</div> | ||
| ); | ||
| ) | ||
| } | ||
|
|
||
| if (!permitted) { | ||
| if (forbidden) return <>{forbidden}</>; | ||
| if (forbidden) return <>{forbidden}</> | ||
| return ( | ||
| <div style={styles.forbidden}> | ||
| <Icon name="shield-off" size={32} color={theme.colors.mutedForeground} /> | ||
| <div style={styles.forbiddenText}>{t("common.forbidden")}</div> | ||
| <Icon | ||
| name="shield-off" | ||
| size={32} | ||
| color={theme.colors.mutedForeground} | ||
| /> | ||
| <div style={styles.forbiddenText}>{t('common.forbidden')}</div> | ||
| </div> | ||
| ); | ||
| ) | ||
| } |
There was a problem hiding this comment.
Don’t force full-screen loading/forbidden UIs in flat or bare mode.
These early returns bypass the new flat-layout path entirely. layout="flat"/bare pages still get minHeight: '100vh' and page chrome during loading or forbidden states, which breaks the new host-controlled layout contract exactly when the state flips.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-web/src/components/DataPage.tsx` around lines 49 - 67, The
DataPage component currently early-returns full-screen loading/forbidden UIs
(using loading, permitted, forbidden and styles.loading/styles.forbidden), which
breaks host-controlled layouts for pages rendered with layout="flat" or bare
mode; change the render logic to avoid early returns so flat/bare pages are not
replaced with a 100vh wrapper: detect flat/bare (props.layout === 'flat' or
props.bare) and for those cases render inline/smaller loading or forbidden
placeholders (reuse Icon and styles.forbiddenText but remove minHeight: '100vh'
and the full-screen container) or render the existing children plus a
lightweight overlay; otherwise keep the current full-screen behavior for
non-flat pages. Ensure you update the conditional branches around
loading/permitted/forbidden in DataPage to return the children/layout path for
flat/bare while showing a non-fullscreen indicator.
Comment on lines
+74
to
+84
| const resolvedHeaderEnd = headerEnd ?? registeredActions | ||
| return ( | ||
| <PageActionsContext.Provider value={contextValue}> | ||
| <div style={styles.flatScroll}> | ||
| <div style={{ ...styles.flatContainer, maxWidth }}> | ||
| <div style={styles.flatHeader}> | ||
| <div> | ||
| <div style={styles.flatTitle}>{resolvedTitle}</div> | ||
| {subtitle && <div style={styles.flatSubtitle}>{subtitle}</div>} | ||
| </div> | ||
| {resolvedHeaderEnd && <div style={styles.flatHeaderEnd}>{resolvedHeaderEnd}</div>} |
There was a problem hiding this comment.
Compose headerEnd with registered page actions instead of replacing them.
headerEnd ?? registeredActions hides the actions registered by nested children whenever the page also passes headerEnd. With the new form-header integration, that can remove the only save/cancel controls on customized flat pages.
♻️ Suggested composition
- const resolvedHeaderEnd = headerEnd ?? registeredActions
+ const resolvedHeaderEnd =
+ headerEnd != null || registeredActions != null
+ ? (
+ <>
+ {headerEnd}
+ {registeredActions}
+ </>
+ )
+ : null📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const resolvedHeaderEnd = headerEnd ?? registeredActions | |
| return ( | |
| <PageActionsContext.Provider value={contextValue}> | |
| <div style={styles.flatScroll}> | |
| <div style={{ ...styles.flatContainer, maxWidth }}> | |
| <div style={styles.flatHeader}> | |
| <div> | |
| <div style={styles.flatTitle}>{resolvedTitle}</div> | |
| {subtitle && <div style={styles.flatSubtitle}>{subtitle}</div>} | |
| </div> | |
| {resolvedHeaderEnd && <div style={styles.flatHeaderEnd}>{resolvedHeaderEnd}</div>} | |
| const resolvedHeaderEnd = | |
| headerEnd != null || registeredActions != null | |
| ? ( | |
| <> | |
| {headerEnd} | |
| {registeredActions} | |
| </> | |
| ) | |
| : null | |
| return ( | |
| <PageActionsContext.Provider value={contextValue}> | |
| <div style={styles.flatScroll}> | |
| <div style={{ ...styles.flatContainer, maxWidth }}> | |
| <div style={styles.flatHeader}> | |
| <div> | |
| <div style={styles.flatTitle}>{resolvedTitle}</div> | |
| {subtitle && <div style={styles.flatSubtitle}>{subtitle}</div>} | |
| </div> | |
| {resolvedHeaderEnd && <div style={styles.flatHeaderEnd}>{resolvedHeaderEnd}</div>} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-web/src/components/DataPage.tsx` around lines 74 - 84, The
current DataPage uses resolvedHeaderEnd = headerEnd ?? registeredActions which
drops registeredActions when headerEnd is provided; instead compose both so
nested-registered actions are not hidden: in the DataPage render (look for
headerEnd, registeredActions, resolvedHeaderEnd) replace the nullish-coalescing
behavior with a composed fragment/element that renders registeredActions and
headerEnd together (preserving desired order and spacing) and use that composed
value where resolvedHeaderEnd is used so both sets of controls show.
Comment on lines
+9
to
+14
| function getVisiblePages(current: number, total: number): number[] { | ||
| if (total <= 5) return Array.from({ length: total }, (_, i) => i + 1); | ||
| const start = Math.max(1, current - 2); | ||
| const end = Math.min(total, start + 4); | ||
| return Array.from({ length: end - start + 1 }, (_, i) => start + i); | ||
| } |
There was a problem hiding this comment.
Sliding window may show fewer than 5 pages near boundaries.
When current is near the end of pagination (e.g., current=9, total=10), the calculation yields start=7, end=10, producing only 4 page buttons instead of 5. The start is not re-adjusted after end is clamped.
🔧 Proposed fix to maintain consistent window size
function getVisiblePages(current: number, total: number): number[] {
if (total <= 5) return Array.from({ length: total }, (_, i) => i + 1);
- const start = Math.max(1, current - 2);
- const end = Math.min(total, start + 4);
+ let start = Math.max(1, current - 2);
+ let end = Math.min(total, start + 4);
+ // Re-adjust start if end was clamped
+ start = Math.max(1, end - 4);
return Array.from({ length: end - start + 1 }, (_, i) => start + i);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function getVisiblePages(current: number, total: number): number[] { | |
| if (total <= 5) return Array.from({ length: total }, (_, i) => i + 1); | |
| const start = Math.max(1, current - 2); | |
| const end = Math.min(total, start + 4); | |
| return Array.from({ length: end - start + 1 }, (_, i) => start + i); | |
| } | |
| function getVisiblePages(current: number, total: number): number[] { | |
| if (total <= 5) return Array.from({ length: total }, (_, i) => i + 1); | |
| let start = Math.max(1, current - 2); | |
| let end = Math.min(total, start + 4); | |
| // Re-adjust start if end was clamped | |
| start = Math.max(1, end - 4); | |
| return Array.from({ length: end - start + 1 }, (_, i) => start + i); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-web/src/components/defaults/Pagination.tsx` around lines 9 -
14, The getVisiblePages function can produce fewer than 5 pages near the end
because start isn't re-adjusted after end is clamped; update getVisiblePages to
ensure a consistent 5-item sliding window by computing end = Math.min(total,
start + 4) and then, if (end - start + 1) < 5, shift start = Math.max(1, end -
4) (or equivalently compute start = Math.max(1, Math.min(current - 2, total -
4))) so the returned Array.from(...) always yields up to 5 page numbers when
total >= 5.
Comment on lines
+36
to
+50
| useLayoutEffect(() => { | ||
| if (!pageActions) return | ||
| const headerActions = form.actions.filter((a) => a.config.positions.includes(Position.header)) | ||
| if (headerActions.length === 0) return | ||
| pageActions.register( | ||
| <div style={{ display: 'flex', gap: 8 }}> | ||
| {headerActions.map((a) => <ResolvedActionButton | ||
| key={a.name} | ||
| action={a} | ||
| domain={props.schema.domain} | ||
| />)} | ||
| </div> | ||
| ) | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [pageActions, props.schema.domain]) |
There was a problem hiding this comment.
Make the page-actions registration reactive and clear it on teardown.
The eslint-disable is masking a real stale-state bug here. This effect only reruns for pageActions/props.schema.domain, so header actions won’t update when form.actions changes, and a previously registered node is never cleared when the form unmounts or stops exposing header actions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-web/src/components/Form.tsx` around lines 36 - 50, The effect
in useLayoutEffect is stale because it only depends on pageActions and
props.schema.domain and never cleans up the registered node; update the
dependency array to include form.actions (or the derived headerActions) so the
effect re-runs when form.actions changes, remove the eslint-disable comment, and
add a cleanup that removes the previously registered element—capture the return
value from pageActions.register (or the DOM node you passed) and call the
corresponding unregister/remove API in the cleanup function so the header
buttons are deregistered when headerActions becomes empty or on unmount; target
symbols: useLayoutEffect, pageActions.register, form.actions, headerActions,
ResolvedActionButton.
| const ResolvedColumnSelector = components?.ColumnSelector ?? getComponent('ColumnSelector') ?? DefaultColumnSelector | ||
| const ResolvedSearchBar = components?.SearchBar ?? getComponent('SearchBar') as React.ComponentType<SearchBarProps> | undefined | ||
| const EmptyComponent = components?.EmptyState ?? getComponent('EmptyState') ?? DefaultEmptyState | ||
| const [search, setSearch] = useState('') |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if search is meant to be wired to table filtering
rg -n "setFilter.*search|search.*setFilter" --type tsRepository: devitools/ybyra
Length of output: 41
🏁 Script executed:
cd packages/react-web/src/components && wc -l Table.tsxRepository: devitools/ybyra
Length of output: 72
🏁 Script executed:
cd packages/react-web/src/components && sed -n '300,320p' Table.tsxRepository: devitools/ybyra
Length of output: 1308
🏁 Script executed:
cd packages/react-web/src/components && sed -n '420,440p' Table.tsxRepository: devitools/ybyra
Length of output: 676
🏁 Script executed:
cd packages/react-web/src/components && rg -n "table\.setFilter|setFilter" Table.tsxRepository: devitools/ybyra
Length of output: 41
🏁 Script executed:
cd packages/react-web/src/components && rg -n "search" Table.tsx -A 2 -B 2Repository: devitools/ybyra
Length of output: 1171
🏁 Script executed:
find packages -name "*.tsx" -o -name "*.ts" | xargs rg -l "SearchBar" | head -5Repository: devitools/ybyra
Length of output: 212
🏁 Script executed:
rg -n "SearchBarProps|interface SearchBar" packages/react-web/src --type ts -A 10Repository: devitools/ybyra
Length of output: 4969
🏁 Script executed:
rg -n "setFilter" packages/react-web/src --type ts -B 2 -A 2Repository: devitools/ybyra
Length of output: 41
🏁 Script executed:
cd packages/react-web/src && find . -name "*SearchBar*" -o -name "*searchBar*"Repository: devitools/ybyra
Length of output: 41
🏁 Script executed:
rg -n "useDataTable" packages/react-web/src --type ts -lRepository: devitools/ybyra
Length of output: 102
🏁 Script executed:
cat -n packages/react-web/src/hooks/useDataTable.ts | head -100Repository: devitools/ybyra
Length of output: 135
🏁 Script executed:
rg -n "setSearch\|search.*=" packages/react-web/src/components/Table.tsx -B 3 -A 3Repository: devitools/ybyra
Length of output: 41
🏁 Script executed:
find packages -name "*.ts" -o -name "*.tsx" | xargs rg -l "useDataTable" | head -5Repository: devitools/ybyra
Length of output: 240
🏁 Script executed:
rg -n "function useDataTable|const useDataTable|export.*useDataTable" packages --type ts -B 2 -A 20Repository: devitools/ybyra
Length of output: 9723
🏁 Script executed:
cd packages/react-web/src/components && sed -n '1,50p' Table.tsx | grep -E "import|from"Repository: devitools/ybyra
Length of output: 1179
🏁 Script executed:
cat -n packages/react-web/src/components/Table.tsx | sed -n '1,30p'Repository: devitools/ybyra
Length of output: 1569
🏁 Script executed:
find packages/react-web -name "use-data-table.ts" -o -name "useDataTable.ts"Repository: devitools/ybyra
Length of output: 41
🏁 Script executed:
rg -n "setFilter" packages/react --type ts -B 5 -A 5Repository: devitools/ybyra
Length of output: 7997
🏁 Script executed:
cat -n packages/react-web/src/components/Table.tsx | sed -n '310,450p'Repository: devitools/ybyra
Length of output: 5595
🏁 Script executed:
rg -n "useEffect.*search|search.*useEffect" packages/react-web/src/components/Table.tsx -B 3 -A 5Repository: devitools/ybyra
Length of output: 41
🏁 Script executed:
find packages -path "*/node_modules" -prune -o -name "*.tsx" -type f -exec grep -l "SearchBar" {} \; | grep -v node_modulesRepository: devitools/ybyra
Length of output: 102
🏁 Script executed:
cat -n packages/react-web/src/components/Table.tsx | sed -n '310,600p' | grep -E "useEffect|search|setFilter" -B 2 -A 2Repository: devitools/ybyra
Length of output: 915
🏁 Script executed:
rg -n "table\.(setFilter|filters)" packages/react-web/src/components/Table.tsx -B 3 -A 3Repository: devitools/ybyra
Length of output: 596
search state is unused for filtering.
The search state is initialized and passed to ResolvedSearchBar, but it's never connected to table.setFilter or any other filtering mechanism. The search functionality appears incomplete.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-web/src/components/Table.tsx` at line 312, The search state
(const [search, setSearch]) is never applied to the table, so typing in
ResolvedSearchBar doesn't filter results; hook the state up to the table by
calling the table filtering API whenever search changes (e.g., in a useEffect
call table.setGlobalFilter(search) or table.setFilter('<yourColumnId>', search)
depending on whether you use a global filter or a specific column filter).
Ensure the effect runs on mount and when search updates and that clearing search
clears the table filter; keep setSearch passed into ResolvedSearchBar as-is.
Comment on lines
63
to
+69
| fontWeight: { | ||
| regular: "400" as const, | ||
| medium: "500" as const, | ||
| semibold: "600" as const, | ||
| bold: "700" as const, | ||
| regular: "400", | ||
| medium: "500", | ||
| semibold: "600", | ||
| bold: "700", | ||
| }, | ||
| } as const; | ||
| } satisfies Theme; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In TypeScript, when an object literal changes from as consttosatisfies SomeInterface, do nested properties remain readonly or become mutable/widened? Is const theme = { ... } as const satisfies Theme valid, and what is the recommended pattern to preserve readonly theme tokens while still checking the object against an exported interface?
💡 Result:
-
Replacing
as constwithsatisfies Themedoes not make the object (or nested properties) readonly.satisfiesis only a compatibility check and “doesn’t chang[e] the resulting type of that expression” [2]. So if you write:const theme = { colors: { primary: "#09f" } } satisfies Theme;
the object’s type is still inferred like a normal object literal (properties are mutable, and string/number literals typically widen to
string/numberunless some other const-context inference applies). -
const theme = { ... } as const satisfies Themeis valid TypeScript and is a common pattern:as constprevents widening and makes object-literal propertiesreadonly[1], whilesatisfies Themechecks it againstThemewithout re-typing it asTheme[2]. -
The main gotcha: if
Themeis written with mutable shapes (e.g.string[], non-readonlyproperties), thenas const satisfies Thememay fail, becauseas constproducesreadonlyproperties/tuples/arrays [1]. In that case, either:- Make the exported contract readonly-friendly (recommended if you truly want immutable tokens):
export interface Theme { readonly colors: { readonly primary: string; }; // use readonly arrays/tuples where relevant // readonly space: readonly number[]; } export const theme = { colors: { primary: "#09f" }, } as const satisfies Theme;
- Or (often best for “design tokens”): export the value and derive the type from it, so the source of truth stays the object literal:
If you still need an interface for external consumption, keep it as a separate “shape” and validate with
export const theme = { colors: { primary: "#09f" }, } as const; export type Theme = typeof theme;
satisfies, but expect to model readonly-ness there too.
- Make the exported contract readonly-friendly (recommended if you truly want immutable tokens):
Sources: TypeScript 3.4 const assertions (as const) prevent widening and make object literal properties readonly [1]; TypeScript 4.9 satisfies checks compatibility “without changing the resulting type” [2].
[1] https://devblogs.microsoft.com/typescript/announcing-typescript-3-4/
[2] https://devblogs.microsoft.com/typescript/announcing-typescript-4-9/
🏁 Script executed:
cat packages/react-web/src/theme/default.tsRepository: devitools/ybyra
Length of output: 2062
Add as const to preserve defaultTheme immutability.
Without as const, the object's properties are mutable at the type level. Callers can reassign nested tokens like defaultTheme.colors.primary = "..." and the change affects every consumer. Use const defaultTheme = { ... } as const satisfies Theme; to prevent widening and make properties readonly while still checking the shape against the Theme interface.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-web/src/theme/default.ts` around lines 63 - 69, The
defaultTheme object is mutable at the type level; update the declaration of
defaultTheme to be readonly by adding "as const" while still validating against
Theme (i.e., declare it as const defaultTheme = { ... } as const satisfies
Theme); locate the defaultTheme symbol in this file and change its declaration
so nested tokens become readonly and TypeScript won't allow assignments like
defaultTheme.colors.primary = "...".
Comment on lines
+9
to
+17
| const [rows, setRows] = useState<Record<string, unknown>[]>(value); | ||
| const [loading, setLoading] = useState(false); | ||
| const [total, setTotal] = useState(0); | ||
|
|
||
| useEffect(() => { | ||
| if (hooks?.fetch?.[scope]) return; // fetch hook gerencia os dados | ||
| setRows(value); | ||
| setTotal(value.length); | ||
| }, [value, hooks?.fetch, scope]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "use-data-table.ts" packages/react/srcRepository: devitools/ybyra
Length of output: 95
🏁 Script executed:
cat -n packages/react/src/use-data-table.ts | head -50Repository: devitools/ybyra
Length of output: 2435
🏁 Script executed:
cd packages/react/src && ls -la use-data-table.tsRepository: devitools/ybyra
Length of output: 125
🏁 Script executed:
cat -n packages/react/src/use-data-table.tsRepository: devitools/ybyra
Length of output: 10554
Initialize total to match rows on first render.
When the component mounts, rows is seeded with value but total remains 0 until the effect runs. This creates a first-render mismatch where rows.length differs from total, breaking pagination calculations like totalPages = Math.ceil(total / limit) (line 104). This is particularly problematic during SSR where the initial render is output as-is. Initialize total with value.length to keep both state variables consistent on mount.
Proposed fix
- const [total, setTotal] = useState(0);
+ const [total, setTotal] = useState(value.length);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const [rows, setRows] = useState<Record<string, unknown>[]>(value); | |
| const [loading, setLoading] = useState(false); | |
| const [total, setTotal] = useState(0); | |
| useEffect(() => { | |
| if (hooks?.fetch?.[scope]) return; // fetch hook gerencia os dados | |
| setRows(value); | |
| setTotal(value.length); | |
| }, [value, hooks?.fetch, scope]); | |
| const [rows, setRows] = useState<Record<string, unknown>[]>(value); | |
| const [loading, setLoading] = useState(false); | |
| const [total, setTotal] = useState(value.length); | |
| useEffect(() => { | |
| if (hooks?.fetch?.[scope]) return; // fetch hook gerencia os dados | |
| setRows(value); | |
| setTotal(value.length); | |
| }, [value, hooks?.fetch, scope]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react/src/use-data-table.ts` around lines 9 - 17, The initial state
sets rows to value but leaves total as 0, causing a first-render mismatch;
update the useState for total to initialize with value.length so total mirrors
rows on mount (keep the existing useEffect that overrides when
hooks?.fetch?.[scope] is present); locate the declarations of rows/setRows and
total/setTotal in use-data-table.ts and change the total initialization
accordingly to use value.length.
Comment on lines
+84
to
+95
| Promise.resolve( | ||
| fetchHook({ | ||
| type: 'collection', | ||
| context: context ?? {}, | ||
| params: { page, limit, sort: sortField, order: sortOrder, filters }, | ||
| hydrate, | ||
| component, | ||
| }), | ||
| ).finally(() => { | ||
| if (fetchIdRef.current !== id) return; | ||
| setLoading(false); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/react/src/use-data-table.ts | sed -n '75,105p'Repository: devitools/ybyra
Length of output: 1194
🏁 Script executed:
# Search for fetchHook definition and its usage
rg -A 5 -B 5 'fetchHook' packages/react/src/use-data-table.tsRepository: devitools/ybyra
Length of output: 684
🏁 Script executed:
# Check the function signature and parameters
rg 'function.*fetchHook|const.*fetchHook|type.*fetchHook' packages/react/Repository: devitools/ybyra
Length of output: 1475
🏁 Script executed:
# Look for type definitions of the hooks parameter
rg -B 10 "fetchHook" packages/react/src/use-data-table.ts | head -40Repository: devitools/ybyra
Length of output: 693
🏁 Script executed:
# Check the interface/type for hooks and fetch
ast-grep --pattern 'interface $_ {
$$$
fetch $$$
}'Repository: devitools/ybyra
Length of output: 325
🏁 Script executed:
# Look for type definitions
rg 'type.*fetch|interface.*fetch' packages/react/src/ -A 5Repository: devitools/ybyra
Length of output: 41
🏁 Script executed:
# Check the full function signature
sed -n '1,75p' packages/react/src/use-data-table.ts | tail -30Repository: devitools/ybyra
Length of output: 998
🏁 Script executed:
# Look for FetchHookFn type definition
rg 'FetchHookFn' packages/core/src/types.ts -A 10Repository: devitools/ybyra
Length of output: 639
🏁 Script executed:
# Check if there are any other relevant type definitions
rg 'type FetchHookFn|interface FetchHookFn' packages/core/Repository: devitools/ybyra
Length of output: 191
🏁 Script executed:
# Check all fetchHook implementations in the codebase
rg 'fetch.*:.*\(' packages/ -A 3 | head -50Repository: devitools/ybyra
Length of output: 41
🏁 Script executed:
# Look for any synchronous fetch implementations
rg -B 5 -A 10 '\[.*Scope\.' packages/ | grep -A 10 'fetch:'Repository: devitools/ybyra
Length of output: 14873
Wrap fetchHook invocation in promise chain to guard against synchronous throws.
FetchHookFn type allows synchronous implementations ((ctx) => void). If such an implementation throws, Promise.resolve(fetchHook(...)) executes the call synchronously before entering the promise chain, so .finally() never runs and loading remains stuck at true. Use .then() to defer execution:
Proposed fix
- Promise.resolve(
- fetchHook({
- type: 'collection',
- context: context ?? {},
- params: { page, limit, sort: sortField, order: sortOrder, filters },
- hydrate,
- component,
- }),
- ).finally(() => {
- if (fetchIdRef.current !== id) return;
- setLoading(false);
- });
+ Promise.resolve()
+ .then(() => fetchHook({
+ type: 'collection',
+ context: context ?? {},
+ params: { page, limit, sort: sortField, order: sortOrder, filters },
+ hydrate,
+ component,
+ }))
+ .finally(() => {
+ if (fetchIdRef.current !== id) return;
+ setLoading(false);
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Promise.resolve( | |
| fetchHook({ | |
| type: 'collection', | |
| context: context ?? {}, | |
| params: { page, limit, sort: sortField, order: sortOrder, filters }, | |
| hydrate, | |
| component, | |
| }), | |
| ).finally(() => { | |
| if (fetchIdRef.current !== id) return; | |
| setLoading(false); | |
| }); | |
| Promise.resolve() | |
| .then(() => fetchHook({ | |
| type: 'collection', | |
| context: context ?? {}, | |
| params: { page, limit, sort: sortField, order: sortOrder, filters }, | |
| hydrate, | |
| component, | |
| })) | |
| .finally(() => { | |
| if (fetchIdRef.current !== id) return; | |
| setLoading(false); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react/src/use-data-table.ts` around lines 84 - 95, The current call
uses Promise.resolve(fetchHook({...})) which calls fetchHook synchronously and
can throw before the promise chain runs, leaving loading stuck; change it to
defer execution by using Promise.resolve().then(() => fetchHook({ type:
'collection', context: context ?? {}, params: { page, limit, sort: sortField,
order: sortOrder, filters }, hydrate, component, })).finally(() => { if
(fetchIdRef.current !== id) return; setLoading(false); }); so synchronous throws
are caught by the promise chain and .finally always runs.
Comment on lines
+85
to
97
| fetchHook({ | ||
| type: 'collection', | ||
| context: context ?? {}, | ||
| params: { page, limit, sort: sortField, order: sortOrder, filters }, | ||
| hydrate, | ||
| component, | ||
| }), | ||
| ).finally(() => { | ||
| if (fetchIdRef.current !== id) return; | ||
| setLoading(false); | ||
| }); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [hooks?.fetch, scope, page, limit, sortField, sortOrder, filtersKey, component]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/react/src/use-data-table.ts | sed -n '70,110p'Repository: devitools/ybyra
Length of output: 1543
🏁 Script executed:
cat -n packages/react/src/use-data-table.ts | head -70Repository: devitools/ybyra
Length of output: 3194
Add context to the dependency array to ensure fetches reflect context changes.
context is passed in the fetch payload (line 87) but omitted from the reload callback dependencies (line 97). When context changes, the callback continues using the previously captured value, preventing table reloads for new context.
Proposed fix
- }, [hooks?.fetch, scope, page, limit, sortField, sortOrder, filtersKey, component]);
+ }, [hooks?.fetch, scope, page, limit, sortField, sortOrder, filtersKey, component, context]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fetchHook({ | |
| type: 'collection', | |
| context: context ?? {}, | |
| params: { page, limit, sort: sortField, order: sortOrder, filters }, | |
| hydrate, | |
| component, | |
| }), | |
| ).finally(() => { | |
| if (fetchIdRef.current !== id) return; | |
| setLoading(false); | |
| }); | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [hooks?.fetch, scope, page, limit, sortField, sortOrder, filtersKey, component]); | |
| fetchHook({ | |
| type: 'collection', | |
| context: context ?? {}, | |
| params: { page, limit, sort: sortField, order: sortOrder, filters }, | |
| hydrate, | |
| component, | |
| }), | |
| ).finally(() => { | |
| if (fetchIdRef.current !== id) return; | |
| setLoading(false); | |
| }); | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [hooks?.fetch, scope, page, limit, sortField, sortOrder, filtersKey, component, context]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react/src/use-data-table.ts` around lines 85 - 97, The
effect/callback that calls fetchHook (the block using fetchHook({ type:
'collection', context: context ?? {}, ... })) captures `context` but does not
list it in its dependency array; add `context` to the dependency array
(alongside hooks?.fetch, scope, page, limit, sortField, sortOrder, filtersKey,
component) so the fetch logic (and the fetchIdRef/current check and
setLoading(false) in the finally) will re-run when `context` changes.
Comment on lines
84
to
85
| if (Object.keys(currentOverrides).length > 0) { | ||
| fieldOverrides.set(currentOverrides) |
There was a problem hiding this comment.
Persist the committed overrides as part of the reset baseline.
After hydration/bootstrap, currentOverrides becomes part of the initial form shape, but reset() still clears fieldOverrides to {}. That restores the hydrated values while dropping the hidden/disabled/state overrides computed here. Snapshot these overrides when you commit them and reuse that snapshot in reset() when no explicit values are supplied.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/svelte/src/use-data-form.ts` around lines 84 - 85, When you apply
committed overrides (the spot where fieldOverrides.set(currentOverrides) is
called), snapshot a copy of currentOverrides into a persistent variable (e.g.,
committedFieldOverrides) so those override flags become the baseline; then
update reset() so that when no explicit values are passed it restores
fieldOverrides from that committed snapshot instead of clearing to {}. Ensure
the snapshot is a defensive copy (not the original object) and reference the
symbols currentOverrides, fieldOverrides.set(...), committedFieldOverrides (new
variable) and reset() when making the change.
… constraints from `flatContainer` - Add `className?: string` to `PageProps` interface - Propagate `className` to the root element in both normal and flat modes - Remove `width: 100%` and `margin: 0 auto` from `flatContainer` style, giving layout control to the host app Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
wilcorrea
force-pushed
the
feat/slot-field-factory
branch
from
08cf036 to
e73d6e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
packages/react-web/src/components/DataPage.tsx (3)
76-76:⚠️ Potential issue | 🟠 Major
headerEnd ?? registeredActionsstill drops registered actions whenheaderEndis provided.This was flagged in a previous review—using nullish coalescing hides the actions registered by nested children whenever the page also passes
headerEnd. Consider composing both instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-web/src/components/DataPage.tsx` at line 76, The current resolvedHeaderEnd = headerEnd ?? registeredActions drops registeredActions whenever headerEnd is present; change resolvedHeaderEnd to compose both values instead (e.g., concatenate or render headerEnd alongside registeredActions) so nested-child registered actions are preserved; update the logic around resolvedHeaderEnd in the DataPage component to always include registeredActions (merging arrays or wrapping both render nodes) when headerEnd exists, referencing the symbols headerEnd, registeredActions and resolvedHeaderEnd.
57-69:⚠️ Potential issue | 🟠 MajorForbidden state also ignores layout mode.
Same issue as loading—the forbidden UI uses
minHeight: '100vh'regardless of whetherlayout="flat"was requested.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-web/src/components/DataPage.tsx` around lines 57 - 69, The forbidden UI in the DataPage component ignores the layout prop and always applies the full-viewport style from styles.forbidden; update the DataPage render logic that checks permitted/forbidden so the forbidden block respects the layout prop (e.g., layout === 'flat' vs default) by conditionally applying or merging styles instead of always using styles.forbidden — modify the code around the permitted/forbidden check in DataPage and adjust how styles.forbidden (or the inline style object used there) is chosen so the minHeight: '100vh' is omitted when layout is "flat".
51-55:⚠️ Potential issue | 🟠 MajorLoading state still uses full-screen styling, breaking flat/bare layouts.
The early return for loading bypasses the layout logic, so
layout="flat"pages still render withminHeight: '100vh'. Consider checking the layout mode and rendering an inline loading indicator for flat/bare modes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-web/src/components/DataPage.tsx` around lines 51 - 55, The loading early-return in DataPage currently always renders the full-screen styles.loading, which bypasses the layout prop; update the DataPage render so that when loading is true it checks the layout prop (e.g., layout === 'flat' or 'bare') and for flat/bare modes render an inline/loading spinner element within the normal page DOM (so it inherits the container layout) while keeping the existing full-screen return only for non-flat layouts; modify the loading conditional to branch on layout and adjust usage of styles.loading accordingly so flat/bare pages do not get minHeight:'100vh'.
🧹 Nitpick comments (1)
packages/react-web/src/components/DataPage.tsx (1)
129-129: Unused style definition:flatContainer.
flatContaineris defined but never used in the component. Consider removing it to avoid confusion.🧹 Proposed cleanup
flatScroll: { // No minHeight/backgroundColor/padding — host app controls layout }, - flatContainer: {}, flatHeader: {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-web/src/components/DataPage.tsx` at line 129, The style key flatContainer in the styles object of DataPage.tsx is defined but never used; remove the flatContainer entry from the styles object (or alternatively use it where intended) to eliminate the dead style definition and avoid confusion—search for the styles object and the flatContainer symbol in the file and delete that property (or apply it to the appropriate JSX element if the omission was accidental).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react-web/src/components/DataPage.tsx`:
- Around line 95-102: The card layout in the DataPage component returns children
without providing the PageActionsContext, so the register function (created as
register in DataPage) isn't available to children; update the card layout return
to wrap the inner content/children with <PageActionsContext.Provider value={{
register, unregister }}> (matching how the flat layout does it) so children can
register page actions—look for the DataPage component, the register/unregister
functions and the PageActionsContext symbol and ensure the provider is added
around the block that renders {children}.
- Around line 73-74: The DataPage component returns children directly when
(layout !== 'flat' && bare), which skips providing PageActionsContext and
prevents children from registering actions; modify the early-return path so that
even in bare mode you wrap children with the same PageActionsContext.Provider
used elsewhere in DataPage (ensuring you reuse the same value/initializer the
component uses for action registration) so PageActionsContext (and any methods
like registerAction/unregisterAction) remains available to children; locate this
in the DataPage component where the conditional return occurs and replace the
raw return of children with a provider-wrapped return using the existing
PageActionsContext value creation logic.
---
Duplicate comments:
In `@packages/react-web/src/components/DataPage.tsx`:
- Line 76: The current resolvedHeaderEnd = headerEnd ?? registeredActions drops
registeredActions whenever headerEnd is present; change resolvedHeaderEnd to
compose both values instead (e.g., concatenate or render headerEnd alongside
registeredActions) so nested-child registered actions are preserved; update the
logic around resolvedHeaderEnd in the DataPage component to always include
registeredActions (merging arrays or wrapping both render nodes) when headerEnd
exists, referencing the symbols headerEnd, registeredActions and
resolvedHeaderEnd.
- Around line 57-69: The forbidden UI in the DataPage component ignores the
layout prop and always applies the full-viewport style from styles.forbidden;
update the DataPage render logic that checks permitted/forbidden so the
forbidden block respects the layout prop (e.g., layout === 'flat' vs default) by
conditionally applying or merging styles instead of always using
styles.forbidden — modify the code around the permitted/forbidden check in
DataPage and adjust how styles.forbidden (or the inline style object used there)
is chosen so the minHeight: '100vh' is omitted when layout is "flat".
- Around line 51-55: The loading early-return in DataPage currently always
renders the full-screen styles.loading, which bypasses the layout prop; update
the DataPage render so that when loading is true it checks the layout prop
(e.g., layout === 'flat' or 'bare') and for flat/bare modes render an
inline/loading spinner element within the normal page DOM (so it inherits the
container layout) while keeping the existing full-screen return only for
non-flat layouts; modify the loading conditional to branch on layout and adjust
usage of styles.loading accordingly so flat/bare pages do not get
minHeight:'100vh'.
---
Nitpick comments:
In `@packages/react-web/src/components/DataPage.tsx`:
- Line 129: The style key flatContainer in the styles object of DataPage.tsx is
defined but never used; remove the flatContainer entry from the styles object
(or alternatively use it where intended) to eliminate the dead style definition
and avoid confusion—search for the styles object and the flatContainer symbol in
the file and delete that property (or apply it to the appropriate JSX element if
the omission was accidental).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 701ef64b-bea8-428e-b8d1-c1b3a1909bf4
📒 Files selected for processing (1)
packages/react-web/src/components/DataPage.tsx
Comment on lines
+73
to
+74
| if (layout !== 'flat' && bare) { | ||
| return <>{children}</> |
There was a problem hiding this comment.
bare mode doesn't provide PageActionsContext, preventing children from registering actions.
When bare=true and layout !== 'flat', the component returns children without wrapping them in PageActionsContext.Provider. If any child attempts to register page actions, it will either fail or use an outer/missing context.
Consider wrapping the bare return in the provider for consistency:
♻️ Proposed fix
if (layout !== 'flat' && bare) {
- return <>{children}</>
+ return (
+ <PageActionsContext.Provider value={contextValue}>
+ {children}
+ </PageActionsContext.Provider>
+ )
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (layout !== 'flat' && bare) { | |
| return <>{children}</> | |
| if (layout !== 'flat' && bare) { | |
| return ( | |
| <PageActionsContext.Provider value={contextValue}> | |
| {children} | |
| </PageActionsContext.Provider> | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-web/src/components/DataPage.tsx` around lines 73 - 74, The
DataPage component returns children directly when (layout !== 'flat' && bare),
which skips providing PageActionsContext and prevents children from registering
actions; modify the early-return path so that even in bare mode you wrap
children with the same PageActionsContext.Provider used elsewhere in DataPage
(ensuring you reuse the same value/initializer the component uses for action
registration) so PageActionsContext (and any methods like
registerAction/unregisterAction) remains available to children; locate this in
the DataPage component where the conditional return occurs and replace the raw
return of children with a provider-wrapped return using the existing
PageActionsContext value creation logic.
Comment on lines
95
to
+102
| return ( | ||
| <div style={styles.scroll}> | ||
| <div style={styles.scroll} className={className}> | ||
| <div style={{ ...styles.container, maxWidth }}> | ||
| <div style={styles.title}>{t(`${domain}.title`)} / {t(`common.scopes.${scope}`)}</div> | ||
| {children} | ||
| </div> | ||
| </div> | ||
| ); | ||
| ) |
There was a problem hiding this comment.
Card layout doesn't wrap children in PageActionsContext.Provider.
The card layout path (default) doesn't wrap children in PageActionsContext.Provider, meaning children cannot register page actions in card mode. The register function is created at line 48 but never provided to children here.
If action registration should be supported in card mode, wrap the return similarly to the flat layout:
♻️ Proposed fix
return (
- <div style={styles.scroll} className={className}>
- <div style={{ ...styles.container, maxWidth }}>
- <div style={styles.title}>{t(`${domain}.title`)} / {t(`common.scopes.${scope}`)}</div>
- {children}
+ <PageActionsContext.Provider value={contextValue}>
+ <div style={styles.scroll} className={className}>
+ <div style={{ ...styles.container, maxWidth }}>
+ <div style={styles.title}>{t(`${domain}.title`)} / {t(`common.scopes.${scope}`)}</div>
+ {children}
+ </div>
</div>
- </div>
+ </PageActionsContext.Provider>
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return ( | |
| <div style={styles.scroll}> | |
| <div style={styles.scroll} className={className}> | |
| <div style={{ ...styles.container, maxWidth }}> | |
| <div style={styles.title}>{t(`${domain}.title`)} / {t(`common.scopes.${scope}`)}</div> | |
| {children} | |
| </div> | |
| </div> | |
| ); | |
| ) | |
| return ( | |
| <PageActionsContext.Provider value={contextValue}> | |
| <div style={styles.scroll} className={className}> | |
| <div style={{ ...styles.container, maxWidth }}> | |
| <div style={styles.title}>{t(`${domain}.title`)} / {t(`common.scopes.${scope}`)}</div> | |
| {children} | |
| </div> | |
| </div> | |
| </PageActionsContext.Provider> | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-web/src/components/DataPage.tsx` around lines 95 - 102, The
card layout in the DataPage component returns children without providing the
PageActionsContext, so the register function (created as register in DataPage)
isn't available to children; update the card layout return to wrap the inner
content/children with <PageActionsContext.Provider value={{ register, unregister
}}> (matching how the flat layout does it) so children can register page
actions—look for the DataPage component, the register/unregister functions and
the PageActionsContext symbol and ensure the provider is added around the block
that renders {children}.
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 CodeRabbit
New Features
Improvements