Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughWizard persistence moved from storing a single step to persisting full wizard state (stepId, formData, visitedSteps) in sessionStorage; the hook restores state on init, tracks visited steps, persists on step changes/unload/unmount, and clears persisted wizard fields on completion/reset. Changes
Sequence DiagramsequenceDiagram
autonumber
actor User
participant WizardHook as "Wizard Hook"
participant FormHook as "Form Hook"
participant StorageAPI as "Storage API"
participant SessionStorage as "sessionStorage"
Note over User,SessionStorage: Initialization
User->>WizardHook: Mount (persistStep enabled)
WizardHook->>StorageAPI: loadWizardState(wizardId, componentName?)
StorageAPI->>SessionStorage: read key
SessionStorage-->>StorageAPI: return serialized state
StorageAPI-->>WizardHook: IWizardPersistedState
WizardHook->>WizardHook: set current step from stepId
WizardHook->>FormHook: setFormData({ values: formData, mergeValues:false })
WizardHook->>WizardHook: restore visitedSteps Set
Note over User,SessionStorage: Navigation / Save
User->>WizardHook: Navigate / change step
WizardHook->>WizardHook: update visitedSteps & currentStep
WizardHook->>FormHook: read formData
WizardHook->>StorageAPI: saveWizardState(stepId, formData, visitedSteps)
StorageAPI->>SessionStorage: write serialized state
Note over User,SessionStorage: Completion / Cleanup
User->>WizardHook: Complete or Reset
WizardHook->>StorageAPI: clearWizardState()
StorageAPI->>SessionStorage: remove key
WizardHook->>FormHook: clear wizard-related fields (set undefined, mergeValues:true)
WizardHook->>WizardHook: set hasBeenClearedRef to avoid re-saving
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
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/designer-components/wizard/hooks.ts (1)
105-122:⚠️ Potential issue | 🟠 MajorMove persisted form restoration to an effect instead of the render phase.
Calling
setFormDatainsideuseMemo(line 109) mutates form state during the render phase. While this may provide immediate initialization for UX purposes (similar to the pattern in formComponentHooks.ts), it violates React's render-phase purity principle. Move the form restoration logic to auseEffectthat runs after the initial step is set.Note: The saved step resolution at line 112 is not affected by the form restoration—
visibleStepsis computed independently and does not depend on form data, so the fallback to index 0 is appropriate defensive coding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shesha-reactjs/src/designer-components/wizard/hooks.ts` around lines 105 - 122, The render-phase mutation occurs because setFormData is being called inside the useMemo that computes initial step; change this by removing the setFormData call from the useMemo (the block using loadWizardState, visibleSteps and returning step index) and instead restore form values in a useEffect that runs after mount: call loadWizardState(actionsOwnerId, actionOwnerName) in the effect, and if savedState.formData and setFormData exist call setFormData({ values: savedState.formData, mergeValues: false }); keep the saved step resolution (visibleSteps.findIndex(...) and returning 0) inside the useMemo, and ensure the effect depends only on actionsOwnerId/actionOwnerName/setFormData so restoration happens after the initial step is computed.
🤖 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/designer-components/wizard/hooks.ts`:
- Around line 406-415: The current reset clears the entire form via
setFormData({ values: {}, mergeValues: false }) which triggers
shaFormInstance.resetFields() and wipes unrelated inputs; instead, scope the
reset to wizard fields only by computing the wizard field keys (e.g., derive
wizardFieldKeys from the wizard steps/fields) and call setFormData with only
those keys set to their empty/default values and mergeValues: true (or call the
form instance resetFields(wizardFieldKeys) if available) so only wizard-related
model entries are cleared; keep the existing clearWizardState(actionsOwnerId,
actionOwnerName) and setVisitedSteps(new Set()) calls unchanged.
- Around line 179-197: The two separate useEffects cause a race where
setVisitedSteps queues a state update but saveWizardState reads the stale
visitedStepsRef.current; fix by merging into one effect (the existing useEffect
blocks that watch currentStep?.id) so when currentStep?.id exists you
synchronously add it to visitedStepsRef.current and call setVisitedSteps(newSet)
and then call saveWizardState(actionsOwnerId, currentStep.id,
currentFormDataRef.current, actionOwnerName,
Array.from(visitedStepsRef.current)); ensure the combined effect depends on
currentStep?.id, persistStep, actionsOwnerId, actionOwnerName, and formMode so
the ref is updated before persisting and the saved snapshot always includes the
current step.
- Around line 199-215: The effect that registers handleBeforeUnload only saves
state on page unload; update the cleanup returned from that useEffect to also
call saveWizardState on component unmount so pending edits aren't lost: inside
the return cleanup of the useEffect that declares handleBeforeUnload, invoke
saveWizardState(actionsOwnerId, currentStep.id, currentFormDataRef.current,
actionOwnerName, Array.from(visitedStepsRef.current)) guarded by the same
conditions (persistStep && formMode !== 'designer' && currentStep?.id) before
removing the beforeunload listener; keep the existing handleBeforeUnload and
listener logic intact.
In `@shesha-reactjs/src/designer-components/wizard/utils.ts`:
- Around line 154-158: Update the IWizardPersistedState declaration to use
formData: unknown instead of any, and update the function in utils.ts that
validates/deserializes the persisted state (the block that currently checks only
stepId and formData) to also validate visitedSteps is an array of strings (e.g.
Array.isArray(saved.visitedSteps) && saved.visitedSteps.every(s => typeof s ===
'string')); coerce or default visitedSteps to [] if missing or invalid so
subsequent code (hooks.ts:130 new Set(savedState.visitedSteps)) never receives a
non-iterable value; ensure the function signature that currently repeats
formData uses unknown too.
---
Outside diff comments:
In `@shesha-reactjs/src/designer-components/wizard/hooks.ts`:
- Around line 105-122: The render-phase mutation occurs because setFormData is
being called inside the useMemo that computes initial step; change this by
removing the setFormData call from the useMemo (the block using loadWizardState,
visibleSteps and returning step index) and instead restore form values in a
useEffect that runs after mount: call loadWizardState(actionsOwnerId,
actionOwnerName) in the effect, and if savedState.formData and setFormData exist
call setFormData({ values: savedState.formData, mergeValues: false }); keep the
saved step resolution (visibleSteps.findIndex(...) and returning 0) inside the
useMemo, and ensure the effect depends only on
actionsOwnerId/actionOwnerName/setFormData so restoration happens after the
initial step is computed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e2723b03-f594-48f0-a010-4f70051c1654
📒 Files selected for processing (2)
shesha-reactjs/src/designer-components/wizard/hooks.tsshesha-reactjs/src/designer-components/wizard/utils.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/designer-components/wizard/hooks.ts`:
- Around line 161-178: allWizardFieldNames currently builds its list from
visibleSteps so hidden/permissioned steps drop out and Reset Steps won't clear
their values; change the logic that collects field names (allWizardFieldNames)
to iterate the full wizard step definition instead of visibleSteps (use the
canonical steps source for the wizard rather than visibleSteps), still using
componentsTreeToFlatStructure(toolbox, step.components) to extract
component.propertyName values (skip components with component.context) so Reset
Steps clears values for every defined step; apply the same fix to the other
occurrence mentioned (around lines 447-455).
- Around line 221-249: The cleanup/save logic in the useEffect
(handleBeforeUnload and returned cleanup) can re-persist state after
clearWizardState is called by done() because executeActionIfConfigured is async;
add a ref flag (e.g., skipPersistRef) initialized false and check it in both
handleBeforeUnload and the cleanup before calling saveWizardState; set
skipPersistRef.current = true in the done() success callback (where
clearWizardState is invoked) so subsequent unmount/beforeunload handlers skip
saving, and keep using existing symbols: useEffect, handleBeforeUnload,
saveWizardState, clearWizardState, done(), executeActionIfConfigured,
persistStep, currentFormDataRef, visitedStepsRef, actionsOwnerId,
actionOwnerName, currentStep.id.
In `@shesha-reactjs/src/designer-components/wizard/utils.ts`:
- Around line 162-175: The type-guard isWizardPersistedState currently only
checks for the presence of formData but not its type, allowing primitives
through and causing mismatches with saveWizardState/loadWizardState; update
isWizardPersistedState to explicitly validate that state.formData has the
expected shape/type (e.g., an object or the specific IFormData shape you
restore) and adjust the contract between saveWizardState and loadWizardState so
formData is either always present and validated or explicitly allowed to be
undefined (and serialized consistently), ensuring callers can safely narrow to
IWizardPersistedState.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7cc8732e-2640-47e6-bb0b-7a731ec10f67
📒 Files selected for processing (2)
shesha-reactjs/src/designer-components/wizard/hooks.tsshesha-reactjs/src/designer-components/wizard/utils.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/designer-components/wizard/hooks.ts`:
- Around line 202-220: The persistence short-circuited by hasBeenClearedRef
stays true after a reset, so reset that flag when a new draft is started or
before the next save; specifically, update the step-change effect that calls
saveWizardState (the useEffect that reads currentStep?.id and updates
visitedStepsRef and setVisitedSteps) to also set hasBeenClearedRef.current =
false (or ensure the same reset happens when currentFormDataRef is first
modified after a clear) so the beforeunload/unmount saver and subsequent
saveWizardState calls will run normally again.
- Around line 454-462: The code builds clearedWizardData using literal dotted
keys (allWizardFieldNames) which leaves nested paths like "address.city" intact;
change to build a real nested object instead before calling setFormData: for
each fieldName in allWizardFieldNames (and where fieldName may be dotted) create
the nested path in clearedWizardData and assign undefined at the leaf (i.e.,
implement or use a small setDeep helper or lodash.set to turn "a.b.c" into { a:
{ b: { c: undefined } } }); then call setFormData({ values: clearedWizardData,
mergeValues: true }) so deepMergeValues will clear nested fields as intended
(refer to setFormData, allWizardFieldNames, clearedWizardData).
In `@shesha-reactjs/src/designer-components/wizard/utils.ts`:
- Around line 188-206: The current saveWizardState function serializes formData
with JSON.stringify which loses runtime types like moment; update persistence to
use a custom serializer that converts moment instances into a stable
representation (e.g., a marker plus ISO string) when calling JSON.stringify in
saveWizardState (reference: saveWizardState, sessionStorage.setItem), and ensure
the complementary load/rehydration path (where stored value is parsed and passed
to setFormData) uses JSON.parse with a reviver or an explicit rehydration
routine to detect that marker and reconstruct moment objects (or other special
types) back into moment instances before calling setFormData; keep the storage
key logic via getWizardStateStorageKey unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 42cb4a80-49f6-4652-ba6d-2b28f587e955
📒 Files selected for processing (2)
shesha-reactjs/src/designer-components/wizard/hooks.tsshesha-reactjs/src/designer-components/wizard/utils.ts
There was a problem hiding this comment.
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/designer-components/wizard/hooks.ts (1)
103-123: 🧹 Nitpick | 🔵 TrivialConsider extracting initial state loading to avoid duplicate sessionStorage reads.
loadWizardStateis called twice during initialization—once ingetInitialStep(Line 106) and again in thevisitedStepsinitializer (Line 129). While sessionStorage reads are synchronous and this works correctly, consolidating these into a single load would be cleaner and more efficient.Suggested approach
+ // Load persisted state once during initialization + const initialPersistedState = useMemo(() => { + if (persistStep && formMode !== 'designer') { + return loadWizardState(actionsOwnerId, actionOwnerName); + } + return null; + }, []); + const getInitialStep = useMemo(() => { // If persistStep is enabled and we're not in designer mode, try to load from sessionStorage if (persistStep && formMode !== 'designer') { - const savedState = loadWizardState(actionsOwnerId, actionOwnerName); + const savedState = initialPersistedState; if (savedState) { // ... } } // ... - }, []); + }, [initialPersistedState]); const [visitedSteps, setVisitedSteps] = useState<Set<string>>(() => { - if (persistStep && formMode !== 'designer') { - const savedState = loadWizardState(actionsOwnerId, actionOwnerName); - if (savedState?.visitedSteps) { - return new Set(savedState.visitedSteps); - } + if (initialPersistedState?.visitedSteps) { + return new Set(initialPersistedState.visitedSteps); } return new Set(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shesha-reactjs/src/designer-components/wizard/hooks.ts` around lines 103 - 123, Extract the sessionStorage read into a single variable (e.g., persistedWizardState) at the top of the hook and reuse it in both getInitialStep (the useMemo that currently calls loadWizardState) and the visitedSteps initializer so loadWizardState is only invoked once; update getInitialStep to reference that shared persistedWizardState when persistStep && formMode !== 'designer' and use the same persistedWizardState when restoring formData and calculating stepIndex (visibleSteps.findIndex(...)), ensuring you still honor the existing fallback behavior (return 0 when persistence is on and nothing found, otherwise use getDefaultStepIndex(defaultActiveStep)).
♻️ Duplicate comments (1)
shesha-reactjs/src/designer-components/wizard/hooks.ts (1)
202-223:⚠️ Potential issue | 🟠 MajorRe-enable persistence after Reset/Done by clearing the guard flag.
After
done()or "Reset Steps" setshasBeenClearedRef.current = true, subsequent step changes skip persisting (Lines 214-222 have no guard, but beforeunload/unmount at Lines 228, 242 check the flag). If a user resets, then continues filling the wizard without completing, those edits are lost on page close.Reset the flag at the start of the step-change effect to re-enable persistence for the new session:
Proposed fix
// Track visited steps and persist state when step changes useEffect(() => { if (!currentStep?.id) return; + + // Re-enable persistence for new session after reset/done + hasBeenClearedRef.current = false; // Update visited steps immediately const nextVisited = new Set(visitedStepsRef.current);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shesha-reactjs/src/designer-components/wizard/hooks.ts` around lines 202 - 223, The step-change effect stops persisting after a reset/done because hasBeenClearedRef.current remains true; to fix, reset that guard at the start of the useEffect that handles currentStep changes (the effect that updates visitedStepsRef, setVisitedSteps and calls saveWizardState) by setting hasBeenClearedRef.current = false so subsequent step changes will resume calling saveWizardState(actionsOwnerId, currentStep.id, currentFormDataRef.current, actionOwnerName, Array.from(nextVisited)); keep the rest of the effect logic intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@shesha-reactjs/src/designer-components/wizard/hooks.ts`:
- Around line 103-123: Extract the sessionStorage read into a single variable
(e.g., persistedWizardState) at the top of the hook and reuse it in both
getInitialStep (the useMemo that currently calls loadWizardState) and the
visitedSteps initializer so loadWizardState is only invoked once; update
getInitialStep to reference that shared persistedWizardState when persistStep &&
formMode !== 'designer' and use the same persistedWizardState when restoring
formData and calculating stepIndex (visibleSteps.findIndex(...)), ensuring you
still honor the existing fallback behavior (return 0 when persistence is on and
nothing found, otherwise use getDefaultStepIndex(defaultActiveStep)).
---
Duplicate comments:
In `@shesha-reactjs/src/designer-components/wizard/hooks.ts`:
- Around line 202-223: The step-change effect stops persisting after a
reset/done because hasBeenClearedRef.current remains true; to fix, reset that
guard at the start of the useEffect that handles currentStep changes (the effect
that updates visitedStepsRef, setVisitedSteps and calls saveWizardState) by
setting hasBeenClearedRef.current = false so subsequent step changes will resume
calling saveWizardState(actionsOwnerId, currentStep.id,
currentFormDataRef.current, actionOwnerName, Array.from(nextVisited)); keep the
rest of the effect logic intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c4d2293b-3b6e-46dc-818e-b305aa3da180
📒 Files selected for processing (2)
shesha-reactjs/src/designer-components/wizard/hooks.tsshesha-reactjs/src/designer-components/wizard/utils.ts
There was a problem hiding this comment.
Actionable comments posted: 2
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/designer-components/wizard/hooks.ts (1)
103-123:⚠️ Potential issue | 🟠 MajorSide effect in
useMemoviolates React rules.Calling
setFormDatainsideuseMemo(line 110) is a React anti-pattern.useMemomust be a pure computation without side effects. This can cause:
- Unpredictable behavior in React Strict Mode (runs twice in development)
- The side effect executes during render, not after mount
- If the component re-renders before effects run, form data may be set prematurely
Move the form data restoration to a
useEffectthat runs on mount:Proposed fix
const getInitialStep = useMemo(() => { if (persistStep && formMode !== 'designer') { const savedState = loadWizardState(actionsOwnerId, actionOwnerName); if (savedState) { - // Restore form data - if (savedState.formData && setFormData) { - setFormData({ values: savedState.formData, mergeValues: false }); - } const stepIndex = visibleSteps.findIndex(step => step.id === savedState.stepId); if (stepIndex !== -1) { return stepIndex; } } return 0; } return getDefaultStepIndex(defaultActiveStep); }, []); + // Restore persisted form data on mount + useEffect(() => { + if (persistStep && formMode !== 'designer') { + const savedState = loadWizardState(actionsOwnerId, actionOwnerName); + if (savedState?.formData && setFormData) { + setFormData({ values: savedState.formData, mergeValues: true }); + } + } + }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shesha-reactjs/src/designer-components/wizard/hooks.ts` around lines 103 - 123, getInitialStep currently performs a side effect (calling setFormData) inside a useMemo — move the mutation out of the memo so the memo stays a pure computation. Keep useMemo(getInitialStep) responsible only for returning the initial step index (use loadWizardState, persistStep, formMode, visibleSteps, getDefaultStepIndex to compute the index) and remove any setFormData calls; then add a useEffect that runs on mount (or when actionsOwnerId/actionOwnerName/persistStep/formMode change) which calls loadWizardState and, if savedState and setFormData exist, calls setFormData({ values: savedState.formData, mergeValues: false }) and any other restoration logic. Ensure the useEffect uses proper deps and that getInitialStep reads only pure values (no side effects).
🤖 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/designer-components/wizard/hooks.ts`:
- Around line 106-110: The code calls loadWizardState twice (in getInitialStep
and the visitedSteps initializer) and restores form data with setFormData({
values: ..., mergeValues: false }) which can wipe unrelated form fields; change
to a single loadWizardState call used for both getInitialStep and visitedSteps
initialization (cache the result in a local variable or closure) and restore
formData using mergeValues: true (or a scoped merge option) so setFormData
merges values instead of resetting fields; update references to loadWizardState,
getInitialStep, visitedSteps, and setFormData accordingly to use the cached
state and merged restore.
- Around line 145-148: The useEffect with no dependency array (useEffect(() => {
currentFormDataRef.current = currentFormData; visitedStepsRef.current =
visitedSteps; });) runs on every render which is unconventional; either add an
explicit inline comment above this useEffect explaining that the omission of
deps is intentional because you need refs updated every render, or move the ref
updates to the places where the corresponding state changes occur (update
currentFormDataRef.current inside the setter or at the end of functions that
change currentFormData, and update visitedStepsRef.current where visitedSteps is
modified) so the refs are kept in sync without a deps-less effect; reference the
symbols currentFormDataRef, currentFormData, visitedStepsRef, visitedSteps and
the useEffect hook when making the change.
---
Outside diff comments:
In `@shesha-reactjs/src/designer-components/wizard/hooks.ts`:
- Around line 103-123: getInitialStep currently performs a side effect (calling
setFormData) inside a useMemo — move the mutation out of the memo so the memo
stays a pure computation. Keep useMemo(getInitialStep) responsible only for
returning the initial step index (use loadWizardState, persistStep, formMode,
visibleSteps, getDefaultStepIndex to compute the index) and remove any
setFormData calls; then add a useEffect that runs on mount (or when
actionsOwnerId/actionOwnerName/persistStep/formMode change) which calls
loadWizardState and, if savedState and setFormData exist, calls setFormData({
values: savedState.formData, mergeValues: false }) and any other restoration
logic. Ensure the useEffect uses proper deps and that getInitialStep reads only
pure values (no side effects).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 99abef49-5b17-41f3-969c-1b2bae95c3c4
📒 Files selected for processing (1)
shesha-reactjs/src/designer-components/wizard/hooks.ts
IvanIlyichev
left a comment
There was a problem hiding this comment.
Hi @HackGenesis. Build fails
|
@IvanIlyichev the build error is now fixed |
IvanIlyichev
left a comment
There was a problem hiding this comment.
Hi @HackGenesis. The proposed change introduces data inconsistency. The correct implementation should be an auto-save on the back end; otherwise, the user may lose the data.
Problems with the current implementation:
- The application can show outdated information. On the first visit, the wizard saves data to local storage and ignores any changes made in another browser or by another user.
- The wizard uses the component ID as a storage key, meaning it does not account for business objects. As a result, it cannot be used as an editor for different entities—it will display the data of the first opened entity until the Done button is pressed.
#4574 (comment)
Summary by CodeRabbit
New Features
Bug Fixes & Reliability