Katleho/c p/blocking loader - #4853
Open
KatlehoMadaba wants to merge 15 commits into
Open
Conversation
…viders for improved type safety
…gressive feedback and control
…nt mutation and ensure safe API property assignment
…h context provider and loader display
KatlehoMadaba
marked this pull request as draft
April 22, 2026 10:36
Contributor
WalkthroughAdds a concurrent loader system with global and form-scoped providers, overlay components, and APIs; integrates form loaders into ConfigurableForm and exposes loader methods through form/data/context APIs and provider wiring. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App as App (GlobalLoaderProvider)
participant Form as ConfigurableForm (FormLoaderProvider)
participant FormCtx as FormLoader Context
participant GlobalCtx as GlobalLoader Context
User->>Form: trigger form operation (e.g., save)
Form->>FormCtx: showLoader("Saving...")
FormCtx->>FormCtx: create loader instance, add to activeLoaders
FormCtx-->>Form: return IFormLoaderInstance
Form->>Form: render FormLoader overlay (message)
Form->>FormCtx: loader.updateMessage("Processing...")
FormCtx->>FormCtx: update loader message state
FormCtx-->>Form: FormLoader re-renders with new message
Form->>FormCtx: loader.close()
FormCtx->>FormCtx: remove loader from activeLoaders
FormCtx-->>Form: FormLoader unmounts
User->>App: trigger app-level operation
App->>GlobalCtx: showLoader("Loading...", isBlocking?)
GlobalCtx->>GlobalCtx: create global loader, compute blocking state
GlobalCtx-->>App: render LoaderOverlay (blocking or non-blocking)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shesha-reactjs/src/providers/form/store/shaFormInstance.tsx (1)
819-841:⚠️ Potential issue | 🟡 MinorStale
formLoaderContextcaptured inuseStateinitializer.
useFormLoader()returns a fresh object on every render (both in the no-op branch at lines 39‑43 offormLoaderProvider.tsxand in the provider wherevalueis not memoized — see that file). TheShaFormInstanceis created only once via theuseStateinitializer, sothis.formLoaderContextis the first render's snapshot.In practice this happens to work because
showLoader/hideLoadersin the provider are wrapped inuseCallbackwith stable deps, so their identities don't change. But this is fragile — any future refactor that makes those callbacks non-stable (or that swaps the no-op for a real provider mid-lifecycle) will silently break loader calls issued via the form API.Consider either:
- storing a ref to the current context and dereferencing it inside
PublicFormApi.showLoader/hideLoaders, or- reading the context via a passed-in getter rather than capturing the value.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shesha-reactjs/src/providers/form/store/shaFormInstance.tsx` around lines 819 - 841, The formLoaderContext returned by useFormLoader() is captured once inside the useState initializer when creating the ShaFormInstance, causing a stale context to be held on the instance; update the implementation so the ShaFormInstance does not permanently close over that initial value — either store a mutable ref to the latest formLoaderContext (e.g., useRef and update it on each render) and have PublicFormApi.showLoader / hideLoaders dereference that ref at call time, or change the constructor to accept a getter function (e.g., getFormLoaderContext) and call that inside showLoader/hideLoaders so the current context is always used instead of the value captured in the useState initializer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shesha-reactjs/src/providers/dataContextProvider/dataContextBinder.tsx`:
- Around line 137-169: getFull currently coalesces getData() with {} and uses a
loose truthy check for api; remove the unnecessary "?? {}" since getData() is
non-null, replace the api check "if (!!api)" with a proper type guard "typeof
api === 'object' && api !== null" before calling Object.keys() and
Object.assign(), and delete redundant null checks that reference data (e.g., the
`data &&` checks when checking hasOwnProperty) so the function uses getData(),
getApi(), includeSetFieldValue and setFieldValue directly with the corrected
guards and tidy up arrow parens/trailing commas to satisfy ESLint.
In `@shesha-reactjs/src/providers/form/formApi.ts`:
- Around line 161-170: Replace unnecessary optional chaining on the private
field and use nullish coalescing: in showLoader(), call
this.#form.loaderApi.showLoader(message) and use the nullish coalescing operator
(??) to provide the no-op fallback so a falsy but valid return value isn't
overwritten; in hideLoaders(), call this.#form.loaderApi.hideLoaders() without
optional chaining. Update references to the private field and loaderApi methods
(showLoader, hideLoaders) accordingly.
In `@shesha-reactjs/src/providers/form/formLoader.tsx`:
- Around line 1-94: FormLoader duplicates LoaderOverlay and shared styles;
extract a single reusable LoaderOverlay component and shared useStyles hook
(move globalLoader/styles.ts into a shared styles module) that accepts a
positioning prop (e.g., position: 'absolute' | 'fixed' or mode: 'form' |
'global') and an isBlocking prop, then update both FormLoader and LoaderOverlay
to render that shared LoaderOverlay (preserving image/Spin fallback logic and
ARIA props). Also change the local image error handler signature in FormLoader
(handleImageError) to include an explicit return type ": void". Ensure you
reference and replace usages of FormLoader, LoaderOverlay, and the existing
useStyles from globalLoader/styles.ts to the new shared module.
In `@shesha-reactjs/src/providers/form/formLoaderProvider.tsx`:
- Around line 71-79: showLoader currently calls setActiveLoaders then mutates
loadersRef which can race with updateLoader's functional setter; to fix, set
loadersRef.current.set(loaderId, loaderInstance) before calling setActiveLoaders
and then call setActiveLoaders using a snapshot derived from loadersRef (so the
ref is the source of truth), and change the defaulting logic from message ||
'Loading...' to message ?? 'Loading...' to preserve intentionally empty strings;
update references to InternalFormLoaderInstance, showLoader, loadersRef,
updateLoader and setActiveLoaders accordingly.
- Around line 99-109: Wrap the context `value` in a useMemo to avoid recreating
it each render: memoize the object { activeLoaders, showLoader, hideLoaders }
inside the FormLoaderProvider using useMemo with dependencies [activeLoaders,
showLoader, hideLoaders], and add useMemo to the imports; return the memoized
value to FormLoaderContext.Provider so consumers only re-render when those
dependencies change.
- Around line 31-46: The fallback in useFormLoader currently creates a new
IFormLoaderInstance and FormLoaderContextValue on every call; hoist a single
module-scoped no-op object (e.g., NOOP_FORM_LOADER_INSTANCE) and a single
NOOP_FORM_LOADER_CONTEXT_VALUE to return instead of allocating noOpInstance and
the object inline inside useFormLoader so reference identity is stable for
consumers like useShaForm; update useFormLoader to return the module-scoped
NOOP_FORM_LOADER_CONTEXT_VALUE when context is falsy and keep the same method
names (updateMessage, close, showLoader, hideLoaders) intact.
In `@shesha-reactjs/src/providers/globalLoader/index.tsx`:
- Around line 64-118: The loadersRef ref is unused for reads and duplicates
state, so remove it and its mutations: delete the loadersRef declaration and all
references to loadersRef.current in updateLoader, showLoader, removeLoader, and
hideLoaders; rely solely on the activeLoaders state updated via setActiveLoaders
and the existing functions updateLoader, showLoader, removeLoader, hideLoaders
(and ensure updateLoader still updates state correctly by using setActiveLoaders
and finding/updating the loader by id).
- Around line 120-135: The provider currently recreates loaderApi and the
context value on every render which forces consumers to rerender; wrap creation
of loaderApi (the object containing showLoader and hideLoaders) and the value
passed to GlobalLoaderContext.Provider in a useMemo so the reference only
changes when showLoader/hideLoaders or active loader-derived values change, and
add useMemo to the React imports; keep showLoader and hideLoaders as the
dependencies (or include any values they close over) so the memoized loaderApi
and the Provider value remain stable across renders.
In `@shesha-reactjs/src/providers/globalLoader/loaderOverlay.tsx`:
- Around line 18-25: The root <div> in loaderOverlay.tsx currently uses
role="status" + aria-live and aria-label={message} while the inner text node is
aria-hidden, causing duplicate or suppressed announcements; pick one source of
truth — remove the aria-label={message} from the root element (leave
role="status" and aria-live) and also remove aria-hidden="true" from the inner
message container so the visible text becomes the accessible name announced by
the status region (alternatively, if you prefer the aria-label approach, keep
aria-label and ensure the inner text stays aria-hidden).
In `@shesha-reactjs/src/providers/globalLoader/styles.ts`:
- Around line 68-76: The blocking overlay currently uses a global selector that
disables pointer-events for everything (globalLoaderOverlayBlocking); change
that to only target overlay descendants except the content container so future
interactive elements remain clickable—e.g., instead of "* { pointer-events: none
}" scope the rule to exclude the contentContainer (make the overlay disable
pointer-events on non-content descendants and explicitly set contentContainer to
pointer-events: auto). Apply the same scoping change to the non-blocking overlay
variant (globalLoaderOverlayNonBlocking) so both overlays preserve pointer
events for elements inside the contentContainer.
In `@shesha-reactjs/src/providers/sourceFileManager/api-utils/form.ts`:
- Around line 96-110: The generated form API signature currently declares
showLoader but omits hideLoaders; update the form type in form.ts to include
hideLoaders alongside showLoader so the generated contract matches the
runtime/public API—add a method signature hideLoaders(): void to the exported
form API type (near the existing showLoader declaration) so callers can
type-check form.hideLoaders() properly.
In `@shesha-reactjs/src/providers/sourceFileManager/api-utils/pageContext.ts`:
- Around line 75-78: Replace the permissive any types in the generated page
context contract with unknown: change the index signature [key: string]: any to
[key: string]: unknown and update setFieldValue's signature value parameter from
any to unknown so consumers must narrow values before use; update any related
type assertions/usages in functions referencing setFieldValue or the page
context to perform proper type checks or casts where necessary (look for
setFieldValue and the index signature in pageContext.ts).
In `@shesha-reactjs/src/providers/subForm/index.tsx`:
- Around line 532-534: Sub-form API exposes showLoader but not hideLoaders; add
a delegate method hideLoaders on the subFormApi that calls
parentFormApi.hideLoaders() so callers (and IFormApi consumers) have the
matching cleanup API. Locate the sub-form API object where showLoader is defined
and add a hideLoaders: function(...) { return parentFormApi.hideLoaders(...); }
entry (matching signature of IFormApi.hideLoaders) to ensure type compatibility
and proper delegation.
---
Outside diff comments:
In `@shesha-reactjs/src/providers/form/store/shaFormInstance.tsx`:
- Around line 819-841: The formLoaderContext returned by useFormLoader() is
captured once inside the useState initializer when creating the ShaFormInstance,
causing a stale context to be held on the instance; update the implementation so
the ShaFormInstance does not permanently close over that initial value — either
store a mutable ref to the latest formLoaderContext (e.g., useRef and update it
on each render) and have PublicFormApi.showLoader / hideLoaders dereference that
ref at call time, or change the constructor to accept a getter function (e.g.,
getFormLoaderContext) and call that inside showLoader/hideLoaders so the current
context is always used instead of the value captured in the useState
initializer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a419155e-8e71-4d72-9a8c-5be1cd6ac819
📒 Files selected for processing (17)
shesha-reactjs/src/components/configurableForm/configurableFormRenderer.tsxshesha-reactjs/src/components/configurableForm/index.tsxshesha-reactjs/src/providers/dataContextProvider/contexts.tsshesha-reactjs/src/providers/dataContextProvider/dataContextBinder.tsxshesha-reactjs/src/providers/form/formApi.tsshesha-reactjs/src/providers/form/formLoader.tsxshesha-reactjs/src/providers/form/formLoaderProvider.tsxshesha-reactjs/src/providers/form/store/shaFormInstance.tsxshesha-reactjs/src/providers/globalLoader/index.tsxshesha-reactjs/src/providers/globalLoader/loaderOverlay.tsxshesha-reactjs/src/providers/globalLoader/styles.tsshesha-reactjs/src/providers/index.tsshesha-reactjs/src/providers/sheshaApplication/index.tsxshesha-reactjs/src/providers/sourceFileManager/api-utils/form.tsshesha-reactjs/src/providers/sourceFileManager/api-utils/loader.tsshesha-reactjs/src/providers/sourceFileManager/api-utils/pageContext.tsshesha-reactjs/src/providers/subForm/index.tsx
Comment on lines
137
to
+169
| const getFull: ContextGetFull = () => { | ||
| const data: IDataContextFull = getData(); | ||
| const data = getData(); | ||
| // Create a shallow copy to avoid mutating the original data object | ||
| const fullData: IDataContextFull = { ...(data ?? {}) }; | ||
| const api = getApi(); | ||
| return api | ||
| ? { ...data, api } | ||
| : data; | ||
| if (!!api) { | ||
| // Reserved property names: API properties (e.g., 'showLoader', 'hideLoaders') and 'setFieldValue' | ||
| // These are injected by DataContextBinder and should not be used as data field names | ||
| // Warn if collision detected | ||
| const apiKeys = Object.keys(api); | ||
| apiKeys.forEach(apiKey => { | ||
| if (data && Object.prototype.hasOwnProperty.call(data, apiKey)) { | ||
| console.warn( | ||
| `[DataContextBinder] Property name collision detected: '${apiKey}' is a reserved API property name. ` + | ||
| `The data field '${apiKey}' will be overwritten by the DataContext API method. ` + | ||
| `Please rename this field in your data model to avoid conflicts.` | ||
| ); | ||
| } | ||
| }); | ||
| // Spread api properties directly onto fullData for easy access (e.g., pageContext.showLoader()) | ||
| Object.assign(fullData, api); | ||
| } | ||
| if (includeSetFieldValue) { | ||
| if (data && Object.prototype.hasOwnProperty.call(data, 'setFieldValue')) { | ||
| console.warn( | ||
| `[DataContextBinder] Property name collision detected: 'setFieldValue' is a reserved property name. ` + | ||
| `The data field 'setFieldValue' will be overwritten by the DataContext API method. ` + | ||
| `Please rename this field in your data model to avoid conflicts.` | ||
| ); | ||
| } | ||
| fullData.setFieldValue = setFieldValue; | ||
| } | ||
| return fullData; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the dataContextBinder.tsx file
fd "dataContextBinder.tsx" shesha-reactjsRepository: shesha-io/shesha-framework
Length of output: 140
🏁 Script executed:
# Check the file size and structure
wc -l shesha-reactjs/src/providers/dataContextProvider/dataContextBinder.tsxRepository: shesha-io/shesha-framework
Length of output: 144
🏁 Script executed:
# Read the file to understand the types and implementation
cat -n shesha-reactjs/src/providers/dataContextProvider/dataContextBinder.tsxRepository: shesha-io/shesha-framework
Length of output: 10400
🏁 Script executed:
# Find and examine the contexts file where ContextGetData is defined
fd "contexts\.(ts|tsx)$" shesha-reactjs/src/providers/dataContextProviderRepository: shesha-io/shesha-framework
Length of output: 130
🏁 Script executed:
# Read the contexts file to see ContextGetData type
head -200 shesha-reactjs/src/providers/dataContextProvider/contexts.tsxRepository: shesha-io/shesha-framework
Length of output: 190
🏁 Script executed:
# Check for ESLint configuration to understand no-unnecessary-condition rule
fd "eslint" shesha-reactjs -t f | grep -E "(config|rc)" | head -5Repository: shesha-io/shesha-framework
Length of output: 168
🏁 Script executed:
# Read the contexts.ts file
head -250 shesha-reactjs/src/providers/dataContextProvider/contexts.tsRepository: shesha-io/shesha-framework
Length of output: 3677
🏁 Script executed:
# Get more context on ContextGetData and ContextGetFull types
grep -A 5 "type ContextGetData\|type ContextGetFull\|interface IDataContextFull" shesha-reactjs/src/providers/dataContextProvider/contexts.tsRepository: shesha-io/shesha-framework
Length of output: 1215
🏁 Script executed:
# Check ESLint config for no-unnecessary-condition rule
cat shesha-reactjs/eslint.config.mjs | head -100Repository: shesha-io/shesha-framework
Length of output: 4009
🏁 Script executed:
# Check if there are any test files that exercise getFull to understand data behavior
fd "dataContextBinder.*test\|\.spec\." shesha-reactjs/src/providers/dataContextProviderRepository: shesha-io/shesha-framework
Length of output: 52
🏁 Script executed:
# Verify the actual return type of getData by checking type inference
grep -B 5 -A 15 "const getData = useDeepCompareCallback" shesha-reactjs/src/providers/dataContextProvider/dataContextBinder.tsxRepository: shesha-io/shesha-framework
Length of output: 850
🏁 Script executed:
# Check ESLint typescript-eslint rules configuration more thoroughly
grep -A 10 "typescript-eslint\|no-unnecessary-condition" shesha-reactjs/eslint.config.mjsRepository: shesha-io/shesha-framework
Length of output: 4281
🏁 Script executed:
# Verify the actual return type signature of getData more carefully
grep -B 2 -A 8 "const getData = useDeepCompareCallback" shesha-reactjs/src/providers/dataContextProvider/dataContextBinder.tsxRepository: shesha-io/shesha-framework
Length of output: 385
🏁 Script executed:
# Look at where dataRef.current is set to understand its guaranteed type
grep -n "dataRef.current = " shesha-reactjs/src/providers/dataContextProvider/dataContextBinder.tsxRepository: shesha-io/shesha-framework
Length of output: 98
🏁 Script executed:
# Verify the exact line 140 ESLint failure by checking if the build logs exist
find shesha-reactjs -name "*.log" -o -name "build-output*" 2>/dev/null | head -5Repository: shesha-io/shesha-framework
Length of output: 52
🏁 Script executed:
# Double-check the type signature and contract of getData in the type definitions
grep -B 2 -A 2 "ContextGetData<" shesha-reactjs/src/providers/dataContextProvider/contexts.tsRepository: shesha-io/shesha-framework
Length of output: 549
🏁 Script executed:
# Verify the data parameter type in getFull signature
grep -B 5 "const getFull:" shesha-reactjs/src/providers/dataContextProvider/dataContextBinder.tsxRepository: shesha-io/shesha-framework
Length of output: 179
Add type guard for api and remove unnecessary null checks on data.
Line 140 fails ESLint because data from getData() is guaranteed to return a non-null object. Remove the ?? {} coalesce. Additionally, api is unknown and must be narrowed with a proper type guard before calling Object.keys() and Object.assign() on lines 146 and 157. Replace if (!!api) with typeof api === "object" && api !== null. This aligns with the coding guideline to prefer type guards over type casting. Also remove the redundant data && null checks on lines 148 and 160, and fix ESLint stylistic issues (arrow function parens, trailing commas).
Proposed fix
const getFull: ContextGetFull = () => {
const data = getData();
// Create a shallow copy to avoid mutating the original data object
- const fullData: IDataContextFull = { ...(data ?? {}) };
+ const fullData: IDataContextFull = { ...data };
const api = getApi();
- if (!!api) {
+ if (typeof api === "object" && api !== null) {
// Reserved property names: API properties (e.g., 'showLoader', 'hideLoaders') and 'setFieldValue'
// These are injected by DataContextBinder and should not be used as data field names
// Warn if collision detected
const apiKeys = Object.keys(api);
- apiKeys.forEach(apiKey => {
- if (data && Object.prototype.hasOwnProperty.call(data, apiKey)) {
+ apiKeys.forEach((apiKey) => {
+ if (Object.prototype.hasOwnProperty.call(data, apiKey)) {
console.warn(
`[DataContextBinder] Property name collision detected: '${apiKey}' is a reserved API property name. ` +
`The data field '${apiKey}' will be overwritten by the DataContext API method. ` +
- `Please rename this field in your data model to avoid conflicts.`
+ `Please rename this field in your data model to avoid conflicts.`,
);
}
});
// Spread api properties directly onto fullData for easy access (e.g., pageContext.showLoader())
Object.assign(fullData, api);
}
if (includeSetFieldValue) {
- if (data && Object.prototype.hasOwnProperty.call(data, 'setFieldValue')) {
+ if (Object.prototype.hasOwnProperty.call(data, 'setFieldValue')) {
console.warn(
`[DataContextBinder] Property name collision detected: 'setFieldValue' is a reserved property name. ` +
`The data field 'setFieldValue' will be overwritten by the DataContext API method. ` +
- `Please rename this field in your data model to avoid conflicts.`
+ `Please rename this field in your data model to avoid conflicts.`,
);
}
fullData.setFieldValue = setFieldValue;🧰 Tools
🪛 GitHub Actions: shesha-reactjs-build
[error] 140-140: ESLint (typescript-eslint/no-unnecessary-condition): Unnecessary conditional; expected left-hand side of ?? operator to be possibly null or undefined.
🪛 GitHub Check: build-attempt
[failure] 164-164:
Missing trailing comma
[failure] 160-160:
Unnecessary conditional, value is always truthy
[failure] 152-152:
Missing trailing comma
[failure] 148-148:
Unnecessary conditional, value is always truthy
[failure] 147-147:
Expected parentheses around arrow function argument
[failure] 140-140:
Unnecessary conditional, expected left-hand side of ?? operator to be possibly null or undefined
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shesha-reactjs/src/providers/dataContextProvider/dataContextBinder.tsx`
around lines 137 - 169, getFull currently coalesces getData() with {} and uses a
loose truthy check for api; remove the unnecessary "?? {}" since getData() is
non-null, replace the api check "if (!!api)" with a proper type guard "typeof
api === 'object' && api !== null" before calling Object.keys() and
Object.assign(), and delete redundant null checks that reference data (e.g., the
`data &&` checks when checking hasOwnProperty) so the function uses getData(),
getApi(), includeSetFieldValue and setFieldValue directly with the corrected
guards and tidy up arrow parens/trailing commas to satisfy ESLint.
Comment on lines
+161
to
+170
| showLoader = (message?: string): IFormLoaderInstanceApi => { | ||
| return this.#form?.loaderApi?.showLoader(message) || { | ||
| updateMessage: () => { /* no-op */ }, | ||
| close: () => { /* no-op */ }, | ||
| }; | ||
| }; | ||
|
|
||
| hideLoaders = (): void => { | ||
| this.#form?.loaderApi?.hideLoaders(); | ||
| }; |
Contributor
There was a problem hiding this comment.
Drop unnecessary optional chain on this.#form (build failure).
#form is assigned in the constructor and typed non-nullable, so this.#form?.loaderApi?... trips the no-unnecessary-condition rule (build-attempt failure at lines 162, 169). Also, the || {...} fallback in showLoader will fire if loaderApi.showLoader ever returns a falsy value — unlikely but prefer ?? for intent.
🛠️ Proposed fix
- showLoader = (message?: string): IFormLoaderInstanceApi => {
- return this.#form?.loaderApi?.showLoader(message) || {
- updateMessage: () => { /* no-op */ },
- close: () => { /* no-op */ },
- };
- };
-
- hideLoaders = (): void => {
- this.#form?.loaderApi?.hideLoaders();
- };
+ showLoader = (message?: string): IFormLoaderInstanceApi => {
+ return this.#form.loaderApi?.showLoader(message) ?? {
+ updateMessage: () => { /* no-op */ },
+ close: () => { /* no-op */ },
+ };
+ };
+
+ hideLoaders = (): void => {
+ this.#form.loaderApi?.hideLoaders();
+ };📝 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
| showLoader = (message?: string): IFormLoaderInstanceApi => { | |
| return this.#form?.loaderApi?.showLoader(message) || { | |
| updateMessage: () => { /* no-op */ }, | |
| close: () => { /* no-op */ }, | |
| }; | |
| }; | |
| hideLoaders = (): void => { | |
| this.#form?.loaderApi?.hideLoaders(); | |
| }; | |
| showLoader = (message?: string): IFormLoaderInstanceApi => { | |
| return this.#form.loaderApi?.showLoader(message) ?? { | |
| updateMessage: () => { /* no-op */ }, | |
| close: () => { /* no-op */ }, | |
| }; | |
| }; | |
| hideLoaders = (): void => { | |
| this.#form.loaderApi?.hideLoaders(); | |
| }; |
🧰 Tools
🪛 GitHub Check: build-attempt
[failure] 169-169:
Unnecessary optional chain on a non-nullish value
[failure] 162-162:
Unnecessary optional chain on a non-nullish value
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shesha-reactjs/src/providers/form/formApi.ts` around lines 161 - 170, Replace
unnecessary optional chaining on the private field and use nullish coalescing:
in showLoader(), call this.#form.loaderApi.showLoader(message) and use the
nullish coalescing operator (??) to provide the no-op fallback so a falsy but
valid return value isn't overwritten; in hideLoaders(), call
this.#form.loaderApi.hideLoaders() without optional chaining. Update references
to the private field and loaderApi methods (showLoader, hideLoaders)
accordingly.
Comment on lines
+1
to
+94
| import React, { FC, useState } from 'react'; | ||
| import { Spin } from 'antd'; | ||
| import { createStyles } from '@/styles'; | ||
|
|
||
| export interface FormLoaderProps { | ||
| message: string; | ||
| } | ||
|
|
||
| const useStyles = createStyles(({ css, cx }) => { | ||
| const contentContainer = "content-container"; | ||
| const loaderImage = "loader-image"; | ||
| const loaderMessage = "loader-message"; | ||
|
|
||
| // Form loader always blocks - overlay covering the entire form container | ||
| const formLoaderOverlay = cx("form-loader-overlay", css` | ||
| position: absolute; | ||
| top: 0; | ||
| left: 0; | ||
| right: 0; | ||
| bottom: 0; | ||
| background: rgba(0, 0, 0, 0.45); | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| z-index: 1000; | ||
| pointer-events: auto; | ||
| cursor: not-allowed; | ||
| border-radius: inherit; /* Inherit border radius from form container */ | ||
|
|
||
| * { | ||
| pointer-events: none; | ||
| } | ||
|
|
||
| .${contentContainer} { | ||
| text-align: center; | ||
| display: flex; | ||
| flex-direction: column; | ||
| align-items: center; | ||
| justify-content: center; | ||
| pointer-events: none; | ||
| } | ||
|
|
||
| .${loaderImage} { | ||
| margin-bottom: 12px; | ||
| } | ||
|
|
||
| .${loaderMessage} { | ||
| font-size: 14px; | ||
| color: rgba(255, 255, 255, 0.85); | ||
| } | ||
| `); | ||
|
|
||
| return { | ||
| formLoaderOverlay, | ||
| contentContainer, | ||
| loaderImage, | ||
| loaderMessage, | ||
| }; | ||
| }); | ||
|
|
||
| export const FormLoader: FC<FormLoaderProps> = ({ message }) => { | ||
| const { styles } = useStyles(); | ||
| const [useSpinFallback, setUseSpinFallback] = useState(false); | ||
|
|
||
| const handleImageError = () => { | ||
| setUseSpinFallback(true); | ||
| }; | ||
|
|
||
| return ( | ||
| <div | ||
| className={styles.formLoaderOverlay} | ||
| role="status" | ||
| aria-live="polite" | ||
| aria-busy="true" | ||
| aria-label={message} | ||
| > | ||
| <div className={styles.contentContainer}> | ||
| {!useSpinFallback ? ( | ||
| <img | ||
| src="/images/SheshaLoadingAnimation.gif" | ||
| alt="Loading..." | ||
| className={styles.loaderImage} | ||
| onError={handleImageError} | ||
| /> | ||
| ) : ( | ||
| <Spin size="large" className={styles.loaderImage} /> | ||
| )} | ||
| <div className={styles.loaderMessage} aria-hidden="true"> | ||
| {message} | ||
| </div> | ||
| </div> | ||
| </div> | ||
| ); | ||
| }; |
Contributor
There was a problem hiding this comment.
Substantial duplication with LoaderOverlay / globalLoader/styles.ts — extract a shared overlay.
FormLoader here and LoaderOverlay in shesha-reactjs/src/providers/globalLoader/loaderOverlay.tsx are ~95% identical: same image/Spin fallback state machine, same ARIA attributes, same content container structure, and the styles (contentContainer, loaderImage, loaderMessage) largely mirror those in shesha-reactjs/src/providers/globalLoader/styles.ts. The only meaningful differences are position: absolute vs fixed and z-index: 1000 vs 9999.
Consolidate into a single <LoaderOverlay mode="form" | "global" isBlocking /> (or a positioning prop) and share one style hook. This keeps ARIA/fallback behavior in lockstep and avoids two places to fix when the spinner changes.
Also flagged by build:
- Line 65: missing return type on
handleImageError— add: void.
♻️ Minimal fix for the return-type lint
- const handleImageError = () => {
+ const handleImageError = (): void => {
setUseSpinFallback(true);
};🧰 Tools
🪛 GitHub Check: build-attempt
[failure] 65-65:
Missing return type on function
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shesha-reactjs/src/providers/form/formLoader.tsx` around lines 1 - 94,
FormLoader duplicates LoaderOverlay and shared styles; extract a single reusable
LoaderOverlay component and shared useStyles hook (move globalLoader/styles.ts
into a shared styles module) that accepts a positioning prop (e.g., position:
'absolute' | 'fixed' or mode: 'form' | 'global') and an isBlocking prop, then
update both FormLoader and LoaderOverlay to render that shared LoaderOverlay
(preserving image/Spin fallback logic and ARIA props). Also change the local
image error handler signature in FormLoader (handleImageError) to include an
explicit return type ": void". Ensure you reference and replace usages of
FormLoader, LoaderOverlay, and the existing useStyles from
globalLoader/styles.ts to the new shared module.
Comment on lines
+31
to
+46
| export const useFormLoader = (): FormLoaderContextValue => { | ||
| const context = useContext(FormLoaderContext); | ||
| if (!context) { | ||
| // Return a no-op implementation if provider is not found | ||
| const noOpInstance: IFormLoaderInstance = { | ||
| updateMessage: () => { /* no-op */ }, | ||
| close: () => { /* no-op */ }, | ||
| }; | ||
| return { | ||
| activeLoaders: [], | ||
| showLoader: () => noOpInstance, | ||
| hideLoaders: () => { /* no-op */ }, | ||
| }; | ||
| } | ||
| return context; | ||
| }; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
No-op fallback allocates a new object per render.
useFormLoader() without a provider constructs a fresh FormLoaderContextValue (and a fresh noOpInstance) on every call. Since this value is typically consumed where reference stability matters (e.g., useShaForm captures it in a useState initializer — see shaFormInstance.tsx line 819), a reference change between renders is harmless here but wasteful elsewhere. Hoist the no-op object to module scope.
♻️ Proposed change
+const NOOP_LOADER_INSTANCE: IFormLoaderInstance = {
+ updateMessage: () => { /* no-op */ },
+ close: () => { /* no-op */ },
+};
+const NOOP_CONTEXT_VALUE: FormLoaderContextValue = {
+ activeLoaders: [],
+ showLoader: () => NOOP_LOADER_INSTANCE,
+ hideLoaders: () => { /* no-op */ },
+};
+
export const useFormLoader = (): FormLoaderContextValue => {
const context = useContext(FormLoaderContext);
- if (!context) {
- // Return a no-op implementation if provider is not found
- const noOpInstance: IFormLoaderInstance = {
- updateMessage: () => { /* no-op */ },
- close: () => { /* no-op */ },
- };
- return {
- activeLoaders: [],
- showLoader: () => noOpInstance,
- hideLoaders: () => { /* no-op */ },
- };
- }
- return context;
+ return context ?? NOOP_CONTEXT_VALUE;
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shesha-reactjs/src/providers/form/formLoaderProvider.tsx` around lines 31 -
46, The fallback in useFormLoader currently creates a new IFormLoaderInstance
and FormLoaderContextValue on every call; hoist a single module-scoped no-op
object (e.g., NOOP_FORM_LOADER_INSTANCE) and a single
NOOP_FORM_LOADER_CONTEXT_VALUE to return instead of allocating noOpInstance and
the object inline inside useFormLoader so reference identity is stable for
consumers like useShaForm; update useFormLoader to return the module-scoped
NOOP_FORM_LOADER_CONTEXT_VALUE when context is falsy and keep the same method
names (updateMessage, close, showLoader, hideLoaders) intact.
Comment on lines
+71
to
+79
| const showLoader = useCallback((message?: string): IFormLoaderInstance => { | ||
| const loaderId = nanoid(); | ||
| const loaderInstance: InternalFormLoaderInstance = { | ||
| id: loaderId, | ||
| message: message || 'Loading...', | ||
| }; | ||
|
|
||
| setActiveLoaders((prev) => [...prev, loaderInstance]); | ||
| loadersRef.current.set(loaderId, loaderInstance); |
Contributor
There was a problem hiding this comment.
showLoader mutates state synchronously after dispatching a state update — minor ordering risk.
loadersRef.current.set(loaderId, loaderInstance) is fine since the ref mirrors the intent, but note that updateLoader's functional setter reads from prev (state), not the ref. If a caller invokes instance.updateMessage before the setActiveLoaders batch from showLoader has committed, the updated.find(l => l.id === id) at line 58 will return undefined and the ref will not be updated for this id on that call — the state will still be correct via the next render. Not a bug today, but consider merging the ref-as-source-of-truth pattern (update the ref first, then derive state from it) for predictability.
Also, message || 'Loading...' will replace an intentionally empty string with the default; if that's not desired use message ?? 'Loading...'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shesha-reactjs/src/providers/form/formLoaderProvider.tsx` around lines 71 -
79, showLoader currently calls setActiveLoaders then mutates loadersRef which
can race with updateLoader's functional setter; to fix, set
loadersRef.current.set(loaderId, loaderInstance) before calling setActiveLoaders
and then call setActiveLoaders using a snapshot derived from loadersRef (so the
ref is the source of truth), and change the defaulting logic from message ||
'Loading...' to message ?? 'Loading...' to preserve intentionally empty strings;
update references to InternalFormLoaderInstance, showLoader, loadersRef,
updateLoader and setActiveLoaders accordingly.
Comment on lines
+18
to
+25
| return ( | ||
| <div | ||
| className={isBlocking ? styles.globalLoaderOverlayBlocking : styles.globalLoaderOverlay} | ||
| role="status" | ||
| aria-live="polite" | ||
| aria-busy="true" | ||
| aria-label={message} | ||
| > |
Contributor
There was a problem hiding this comment.
Possible duplicated announcement via ARIA.
With role="status" + aria-live="polite" + aria-label={message}, the message will be announced once via the label. The inner <div> at line 37‑39 renders the same message text, but since that subtree is inside a labelled status region, some screen readers can announce both the label and the accessible name derived from descendants. The aria-hidden="true" on the inner text mitigates this, but consider either dropping aria-label (letting the visible text name the region) or keeping the label and leaving the inner text purely visual — pick one source of truth.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shesha-reactjs/src/providers/globalLoader/loaderOverlay.tsx` around lines 18
- 25, The root <div> in loaderOverlay.tsx currently uses role="status" +
aria-live and aria-label={message} while the inner text node is aria-hidden,
causing duplicate or suppressed announcements; pick one source of truth — remove
the aria-label={message} from the root element (leave role="status" and
aria-live) and also remove aria-hidden="true" from the inner message container
so the visible text becomes the accessible name announced by the status region
(alternatively, if you prefer the aria-label approach, keep aria-label and
ensure the inner text stays aria-hidden).
Comment on lines
+68
to
+76
| const globalLoaderOverlayBlocking = cx("global-loader-overlay-blocking", base, css` | ||
| background: rgba(0, 0, 0, 0.45); | ||
| pointer-events: auto; | ||
| cursor: not-allowed; | ||
|
|
||
| * { | ||
| pointer-events: none; | ||
| } | ||
| `); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Blocking overlay disables pointer-events on its own spinner/message too.
The universal selector * { pointer-events: none; } inside globalLoaderOverlayBlocking disables pointer events on the content card as well, which is fine today but means any future interactive element (cancel button, retry link) placed inside the overlay will silently stop working. Consider scoping the disable to a specific class (e.g., exclude .${contentContainer} descendants that should remain clickable) to avoid a foot-gun later. Same applies to the non-blocking variant at lines 58‑65.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shesha-reactjs/src/providers/globalLoader/styles.ts` around lines 68 - 76,
The blocking overlay currently uses a global selector that disables
pointer-events for everything (globalLoaderOverlayBlocking); change that to only
target overlay descendants except the content container so future interactive
elements remain clickable—e.g., instead of "* { pointer-events: none }" scope
the rule to exclude the contentContainer (make the overlay disable
pointer-events on non-content descendants and explicitly set contentContainer to
pointer-events: auto). Apply the same scoping change to the non-blocking overlay
variant (globalLoaderOverlayNonBlocking) so both overlays preserve pointer
events for elements inside the contentContainer.
Comment on lines
+96
to
+110
| /** | ||
| * Show loader overlay | ||
| * @param message Optional message to display | ||
| * @returns Loader instance with methods for progressive feedback | ||
| * @example | ||
| * const loader = form.showLoader("Saving..."); | ||
| * try { | ||
| * await http.post('/api/save', data); | ||
| * loader.close(); | ||
| * } catch (error) { | ||
| * loader.updateMessage("Failed to save"); | ||
| * setTimeout(() => loader.close(), 2000); | ||
| * } | ||
| */ | ||
| showLoader: (message?: string) => { updateMessage(message: string): void; close(): void; }; |
Contributor
There was a problem hiding this comment.
Keep the generated form API contract in sync with runtime.
This template adds showLoader, but the runtime/public form API also exposes hideLoaders; generated source files won’t type-check calls to form.hideLoaders() unless it is documented here too.
Proposed fix
showLoader: (message?: string) => { updateMessage(message: string): void; close(): void; };
+
+ /**
+ * Hides all currently displayed form loaders immediately
+ */
+ hideLoaders: () => void;
/** Set validation errors. Need for display validation errors in the ValidationErrors component */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shesha-reactjs/src/providers/sourceFileManager/api-utils/form.ts` around
lines 96 - 110, The generated form API signature currently declares showLoader
but omits hideLoaders; update the form type in form.ts to include hideLoaders
alongside showLoader so the generated contract matches the runtime/public
API—add a method signature hideLoaders(): void to the exported form API type
(near the existing showLoader declaration) so callers can type-check
form.hideLoaders() properly.
Comment on lines
+75
to
+78
| setFieldValue?: (name: string, value: any) => void; | ||
|
|
||
| // Allow additional properties from the data model | ||
| [key: string]: any; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid any in the generated page-context contract.
Use unknown for arbitrary field values so scripts must narrow before use. As per coding guidelines, “Eliminate the any type; use unknown type instead for values with unknown types, forcing explicit type checking”.
Proposed fix
- setFieldValue?: (name: string, value: any) => void;
+ setFieldValue?: (name: string, value: unknown) => void;
// Allow additional properties from the data model
- [key: string]: any;
+ [key: string]: unknown;📝 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
| setFieldValue?: (name: string, value: any) => void; | |
| // Allow additional properties from the data model | |
| [key: string]: any; | |
| setFieldValue?: (name: string, value: unknown) => void; | |
| // Allow additional properties from the data model | |
| [key: string]: unknown; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shesha-reactjs/src/providers/sourceFileManager/api-utils/pageContext.ts`
around lines 75 - 78, Replace the permissive any types in the generated page
context contract with unknown: change the index signature [key: string]: any to
[key: string]: unknown and update setFieldValue's signature value parameter from
any to unknown so consumers must narrow values before use; update any related
type assertions/usages in functions referencing setFieldValue or the page
context to perform proper type checks or casts where necessary (look for
setFieldValue and the index signature in pageContext.ts).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shesha-reactjs/src/providers/form/store/shaFormInstance.tsx (1)
820-841: 🧹 Nitpick | 🔵 TrivialConsider updating reused form instances with the current loader context.
If
useShaForm({ form: existingInstance })is ever called with a pre-existing form, that instance will keep its old or uninitializedformLoaderContexteven when rendered underFormLoaderProvider. This would causeformApi.showLoader()to silently fail for reused instances.Currently, no usages of this pattern exist in the codebase (only
form: undefinedis passed), so this is a preventive suggestion rather than an active issue. However, the code structure permits this problematic usage, and the suggested setter methods would guard against future issues.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shesha-reactjs/src/providers/form/store/shaFormInstance.tsx` around lines 820 - 841, If a pre-existing ShaFormInstance is passed into useShaForm it never gets the current formLoaderContext, so add a small update after the instance is resolved: ensure ShaFormInstance exposes a setter (e.g. setFormLoaderContext) or a public property and, in the hook that creates/uses the instance (useShaForm / the useState initializer around ShaFormInstance), call instance.setFormLoaderContext(formLoaderContext) (or assign instance.formLoaderContext = formLoaderContext) and ensure this runs whenever formLoaderContext or the passed-in form instance changes (useEffect watching [formLoaderContext, form]) so reused instances get the active loader and formApi.showLoader() works.
♻️ Duplicate comments (4)
shesha-reactjs/src/providers/dataContextProvider/dataContextBinder.tsx (1)
142-158:⚠️ Potential issue | 🟠 MajorNarrow
apiwith a proper type guard before callingObject.keys/Object.assign.
apiis typed asunknown, so!!apionly narrows it to a truthy value (could still be a string, number, etc.). CallingObject.keys(api)on a primitive will throw at runtime for non-object truthy values, and TypeScript will (correctly) complain here. Use an object type guard.Proposed fix
- if (!!api) { + if (typeof api === 'object' && api !== null) { // Reserved property names: API properties (e.g., 'showLoader', 'hideLoaders') and 'setFieldValue' // These are injected by DataContextBinder and should not be used as data field names // Warn if collision detected const apiKeys = Object.keys(api);As per coding guidelines: "Prefer type guards over type casting for type checking" and "Leverage TypeScript to its full potential as a type system".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shesha-reactjs/src/providers/dataContextProvider/dataContextBinder.tsx` around lines 142 - 158, The code uses api (typed unknown) with Object.keys and Object.assign without narrowing; add a runtime/type guard in DataContextBinder that ensures api is a non-null plain object (e.g., typeof api === 'object' && api !== null and not an Array) and, once narrowed to Record<string, unknown> (or Record<string, any>), only then call Object.keys(api) and Object.assign(fullData, api); keep the existing collision check logic (using data and apiKey) inside that guarded block so primitives won't reach Object.keys/Object.assign and TypeScript will be satisfied.shesha-reactjs/src/providers/globalLoader/loaderOverlay.tsx (1)
19-39:⚠️ Potential issue | 🟡 MinorUse one live-region announcement source.
aria-label={message}already names thestatusregion, while the image still contributes"Loading..."and the visible message is hidden. Prefer letting the visible message be the live-region content and make the image decorative.♿ Proposed cleanup
<div className={isBlocking ? styles.globalLoaderOverlayBlocking : styles.globalLoaderOverlay} role="status" aria-live="polite" aria-busy="true" - aria-label={message} > <div className={styles.contentContainer}> {!useSpinFallback ? ( <img src="/images/SheshaLoadingAnimation.gif" - alt="Loading..." + alt="" + aria-hidden="true" className={styles.loaderImage} onError={handleImageError} /> @@ - <div className={styles.loaderMessage} aria-hidden="true"> + <div className={styles.loaderMessage}> {message} </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shesha-reactjs/src/providers/globalLoader/loaderOverlay.tsx` around lines 19 - 39, The live region currently has aria-label={message} while the visible message is aria-hidden and the image still has an accessible alt, creating duplicate announcements; update the loader overlay (the container using role="status") to remove aria-label={message}, make the visible message element (styles.loaderMessage) NOT aria-hidden so it is the single live-region content, and make the image decorative by clearing its accessible text (set its alt to an empty string or role="presentation" for the <img> rendered when !useSpinFallback and ensure Spin (when used) is also hidden from assistive tech if it is purely decorative); keep handleImageError logic intact but ensure the image no longer provides accessible text.shesha-reactjs/src/providers/globalLoader/index.tsx (2)
1-1: 🛠️ Refactor suggestion | 🟠 MajorRemove the write-only loader map.
loadersRef.currentis only written/cleared here and never read, so it duplicatesactiveLoaderswithout adding behavior.♻️ Proposed removal
-import React, { FC, PropsWithChildren, createContext, useContext, useState, useCallback, useRef } from 'react'; +import React, { FC, PropsWithChildren, createContext, useContext, useState, useCallback } from 'react'; @@ export const GlobalLoaderProvider: FC<PropsWithChildren> = ({ children }) => { const [activeLoaders, setActiveLoaders] = useState<InternalLoaderInstance[]>([]); - const loadersRef = useRef<Map<string, InternalLoaderInstance>>(new Map()); @@ const updateLoader = useCallback((id: string, updates: Partial<InternalLoaderInstance>) => { - setActiveLoaders((prev) => { - const updated = prev.map((loader) => - loader.id === id ? { ...loader, ...updates } : loader, - ); - // Update ref - const loader = updated.find((l) => l.id === id); - if (loader) { - loadersRef.current.set(id, loader); - } - return updated; - }); + setActiveLoaders((prev) => + prev.map((loader) => loader.id === id ? { ...loader, ...updates } : loader) + ); }, []); @@ const removeLoader = useCallback((id: string) => { setActiveLoaders((prev) => prev.filter((loader) => loader.id !== id)); - loadersRef.current.delete(id); }, []); @@ }; setActiveLoaders((prev) => [...prev, loaderInstance]); - loadersRef.current.set(loaderId, loaderInstance); @@ const hideLoaders = useCallback(() => { setActiveLoaders([]); - loadersRef.current.clear(); }, []);Also applies to: 64-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shesha-reactjs/src/providers/globalLoader/index.tsx` at line 1, Remove the write-only loadersRef and its useRef import and rely solely on the activeLoaders state: delete the useRef import, remove the loadersRef variable and all assignments/clears to loadersRef.current (e.g., where loadersRef.current is set or reset), and ensure any logic that intended to read loadersRef uses activeLoaders and setActiveLoaders instead; verify exported/context functions (start/stop loader handlers) update activeLoaders only and that no other code expects loadersRef to exist.
1-1: 🧹 Nitpick | 🔵 TrivialMemoize the API object passed through context.
loaderApiand{ loaderApi }are recreated every render, causing all consumers to see a new context reference even when the API callbacks did not change.♻️ Proposed memoization
-import React, { FC, PropsWithChildren, createContext, useContext, useState, useCallback, useRef } from 'react'; +import React, { FC, PropsWithChildren, createContext, useContext, useState, useCallback, useRef, useMemo } from 'react'; @@ - const loaderApi: LoaderApi = { - showLoader, - hideLoaders, - }; + const loaderApi = useMemo<LoaderApi>( + () => ({ showLoader, hideLoaders }), + [showLoader, hideLoaders], + ); + + const contextValue = useMemo<GlobalLoaderContextValue>( + () => ({ loaderApi }), + [loaderApi], + ); @@ - <GlobalLoaderContext.Provider value={{ loaderApi }}> + <GlobalLoaderContext.Provider value={contextValue}>Based on learnings, For useMemo/useCallback dependency arrays, include all values read inside the hook that can change.
Also applies to: 120-131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shesha-reactjs/src/providers/globalLoader/index.tsx` at line 1, The context currently recreates the API object each render (loaderApi and the `{ loaderApi }` value), so memoize the object and its callbacks: wrap mutable callbacks in useCallback (referencing their names like startLoading/stopLoading/toggleLoading or whatever functions are defined) and wrap the final context value `loaderApi` in useMemo so `{ loaderApi }` is stable; ensure all dynamic values used inside those hooks (e.g., isLoading state, setIsLoading, refs) are included in the respective dependency arrays so the memo updates only when real dependencies change and consumers stop receiving new references every render.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shesha-reactjs/src/providers/form/formLoaderProvider.tsx`:
- Line 1: Remove the unused write-only ref by deleting loadersRef and any writes
to it; instead keep all loader state in activeLoaders and update activeLoaders
directly. Locate the FormLoaderProvider (and helper functions
registerLoader/unregisterLoader or addLoader/removeLoader) and remove the useRef
declaration for loadersRef and any assignments like loadersRef.current = ...,
updating the register/unregister logic to setState on activeLoaders only
(preserve existing API that consumers use). Ensure any places that previously
referenced loadersRef (if any) now read from activeLoaders and that no dead
variables or imports remain.
---
Outside diff comments:
In `@shesha-reactjs/src/providers/form/store/shaFormInstance.tsx`:
- Around line 820-841: If a pre-existing ShaFormInstance is passed into
useShaForm it never gets the current formLoaderContext, so add a small update
after the instance is resolved: ensure ShaFormInstance exposes a setter (e.g.
setFormLoaderContext) or a public property and, in the hook that creates/uses
the instance (useShaForm / the useState initializer around ShaFormInstance),
call instance.setFormLoaderContext(formLoaderContext) (or assign
instance.formLoaderContext = formLoaderContext) and ensure this runs whenever
formLoaderContext or the passed-in form instance changes (useEffect watching
[formLoaderContext, form]) so reused instances get the active loader and
formApi.showLoader() works.
---
Duplicate comments:
In `@shesha-reactjs/src/providers/dataContextProvider/dataContextBinder.tsx`:
- Around line 142-158: The code uses api (typed unknown) with Object.keys and
Object.assign without narrowing; add a runtime/type guard in DataContextBinder
that ensures api is a non-null plain object (e.g., typeof api === 'object' &&
api !== null and not an Array) and, once narrowed to Record<string, unknown> (or
Record<string, any>), only then call Object.keys(api) and
Object.assign(fullData, api); keep the existing collision check logic (using
data and apiKey) inside that guarded block so primitives won't reach
Object.keys/Object.assign and TypeScript will be satisfied.
In `@shesha-reactjs/src/providers/globalLoader/index.tsx`:
- Line 1: Remove the write-only loadersRef and its useRef import and rely solely
on the activeLoaders state: delete the useRef import, remove the loadersRef
variable and all assignments/clears to loadersRef.current (e.g., where
loadersRef.current is set or reset), and ensure any logic that intended to read
loadersRef uses activeLoaders and setActiveLoaders instead; verify
exported/context functions (start/stop loader handlers) update activeLoaders
only and that no other code expects loadersRef to exist.
- Line 1: The context currently recreates the API object each render (loaderApi
and the `{ loaderApi }` value), so memoize the object and its callbacks: wrap
mutable callbacks in useCallback (referencing their names like
startLoading/stopLoading/toggleLoading or whatever functions are defined) and
wrap the final context value `loaderApi` in useMemo so `{ loaderApi }` is
stable; ensure all dynamic values used inside those hooks (e.g., isLoading
state, setIsLoading, refs) are included in the respective dependency arrays so
the memo updates only when real dependencies change and consumers stop receiving
new references every render.
In `@shesha-reactjs/src/providers/globalLoader/loaderOverlay.tsx`:
- Around line 19-39: The live region currently has aria-label={message} while
the visible message is aria-hidden and the image still has an accessible alt,
creating duplicate announcements; update the loader overlay (the container using
role="status") to remove aria-label={message}, make the visible message element
(styles.loaderMessage) NOT aria-hidden so it is the single live-region content,
and make the image decorative by clearing its accessible text (set its alt to an
empty string or role="presentation" for the <img> rendered when !useSpinFallback
and ensure Spin (when used) is also hidden from assistive tech if it is purely
decorative); keep handleImageError logic intact but ensure the image no longer
provides accessible text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dd6369e7-351d-4026-a6ad-ce5aac992f14
📒 Files selected for processing (8)
shesha-reactjs/src/providers/dataContextProvider/dataContextBinder.tsxshesha-reactjs/src/providers/form/formApi.tsshesha-reactjs/src/providers/form/formLoader.tsxshesha-reactjs/src/providers/form/formLoaderProvider.tsxshesha-reactjs/src/providers/form/store/shaFormInstance.tsxshesha-reactjs/src/providers/globalLoader/index.tsxshesha-reactjs/src/providers/globalLoader/loaderOverlay.tsxshesha-reactjs/src/providers/subForm/index.tsx
| @@ -0,0 +1,110 @@ | |||
| import React, { FC, PropsWithChildren, createContext, useContext, useState, useCallback, useRef } from 'react'; | |||
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove the write-only loader ref.
loadersRef is kept in sync but never read, so activeLoaders already carries all state needed by the provider.
♻️ Proposed simplification
-import React, { FC, PropsWithChildren, createContext, useContext, useState, useCallback, useRef } from 'react';
+import React, { FC, PropsWithChildren, createContext, useContext, useState, useCallback } from 'react';
@@
export const FormLoaderProvider: FC<PropsWithChildren> = ({ children }) => {
const [activeLoaders, setActiveLoaders] = useState<InternalFormLoaderInstance[]>([]);
- const loadersRef = useRef<Map<string, InternalFormLoaderInstance>>(new Map());
@@
const updateLoader = useCallback((id: string, updates: Partial<InternalFormLoaderInstance>) => {
- setActiveLoaders((prev) => {
- const updated = prev.map((loader) =>
- loader.id === id ? { ...loader, ...updates } : loader,
- );
- // Update ref
- const loader = updated.find((l) => l.id === id);
- if (loader) {
- loadersRef.current.set(id, loader);
- }
- return updated;
- });
+ setActiveLoaders((prev) =>
+ prev.map((loader) => loader.id === id ? { ...loader, ...updates } : loader)
+ );
}, []);
@@
const removeLoader = useCallback((id: string) => {
setActiveLoaders((prev) => prev.filter((loader) => loader.id !== id));
- loadersRef.current.delete(id);
}, []);
@@
setActiveLoaders((prev) => [...prev, loaderInstance]);
- loadersRef.current.set(loaderId, loaderInstance);
@@
const hideLoaders = useCallback(() => {
setActiveLoaders([]);
- loadersRef.current.clear();
}, []);Also applies to: 49-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shesha-reactjs/src/providers/form/formLoaderProvider.tsx` at line 1, Remove
the unused write-only ref by deleting loadersRef and any writes to it; instead
keep all loader state in activeLoaders and update activeLoaders directly. Locate
the FormLoaderProvider (and helper functions registerLoader/unregisterLoader or
addLoader/removeLoader) and remove the useRef declaration for loadersRef and any
assignments like loadersRef.current = ..., updating the register/unregister
logic to setState on activeLoaders only (preserve existing API that consumers
use). Ensure any places that previously referenced loadersRef (if any) now read
from activeLoaders and that no dead variables or imports remain.
KatlehoMadaba
marked this pull request as ready for review
April 22, 2026 11:53
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
2 participants
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
Chores