UX Rescue Phase 2A: rebuild the Form Designer as a unified authoring studio - #153
Merged
Merged
Conversation
Replaces the 7-route form creation/design/review flow with one studio at /forms/designer/new and /forms/designer/:formId, closing the critical gap identified in form-designer-gap-analysis.md: condition and formula builders now exist (previously no UI could set visibility/ required conditions or calculation formulas despite the domain model and evaluation engine already supporting them). Reuses existing autosave, undo/redo, and preview/evaluation logic unchanged. Two small backend additions only: copy-schema-from-an- existing-form, and template preview-before-use — no parallel form engine, no new services beyond the existing FormVersionService/ FormTemplateService split. Backend: 990 unit + 263 integration tests passing, 0 failed, 0 skipped. Frontend: 343 tests passing; typecheck/lint/build/audit clean. Does not close #144 — see phase2a-form-designer-compliance-ledger.md for the full Verified/Partial/Missing breakdown (8 explicit Missing items, none data/security risk). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @henter36, your pull request is larger than the review limit of 150000 diff characters
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe PR adds a unified form designer studio with schema editing, validation, comparison, responsive workspaces, migrated routes, copy/template APIs, consolidated access checks, automated coverage, and Phase 2A documentation. ChangesUnified Form Designer Studio
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Reviewer's GuideRebuilds the form designer UX into a single unified studio at /forms/designer/*, introduces typed condition and formula builders with cycle detection, adds backend support for copying forms and previewing templates, and wires all existing flows, routes, permissions, and tests to the new studio without introducing a parallel engine or new services. Sequence diagram for unified studio start flow and backend copy/template endpointssequenceDiagram
actor User
participant FormDesignerStudioPage
participant StudioStartFlow
participant api_forms as api.forms
participant api_templates as api.formTemplates
User->>FormDesignerStudioPage: open /forms/designer/new
FormDesignerStudioPage->>StudioStartFlow: render start flow
User->>StudioStartFlow: choose blank form
StudioStartFlow->>api_forms: create(request) // CreateFormRequest
api_forms-->>StudioStartFlow: FormDetail
StudioStartFlow->>api_forms: createVersion(formId)
api_forms-->>StudioStartFlow: FormVersionDetail
StudioStartFlow->>FormDesignerStudioPage: onCreated(formId,versionId)
FormDesignerStudioPage->>FormDesignerStudioPage: navigate /forms/designer/:formId?versionId=
User->>StudioStartFlow: choose template
StudioStartFlow->>api_templates: list()
api_templates-->>StudioStartFlow: FormTemplateListItem[]
User->>StudioStartFlow: preview template
StudioStartFlow->>api_templates: getSchema(templateId)
api_templates-->>StudioStartFlow: FormTemplateSchema
User->>StudioStartFlow: use template
StudioStartFlow->>api_templates: createForm(templateId,CreateFormRequest)
api_templates-->>StudioStartFlow: FormDetail
StudioStartFlow->>FormDesignerStudioPage: onCreated(formId,versionId)
User->>StudioStartFlow: choose copy existing form
StudioStartFlow->>api_forms: list({search,pageSize})
api_forms-->>StudioStartFlow: FormsList
User->>StudioStartFlow: select sourceFormId,sourceVersionId
StudioStartFlow->>api_forms: copyFromExistingForm(sourceFormId,sourceVersionId,CreateFormRequest)
api_forms-->>StudioStartFlow: FormVersionDetail
StudioStartFlow->>FormDesignerStudioPage: onCreated(formId,versionId)
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
FormVersionService's constructor had 8 parameters (Sonar S107, max 7). formScope + effectiveAccess were always used together (load a form in scope, then check a capability on it) across every method in the service, so they are now a single coherent collaborator, IFormVersionAccessGuard, instead of two separate injected services. No transaction boundary, version numbering, optimistic concurrency, or publish/draft lifecycle behavior changed — verified by the existing FormsVersionIntegrationTests suite plus new unit tests for the guard itself and a new integration test covering the "reject invalid schema on submit" path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
FormDesignerStudioPage's main workspace component had Cognitive Complexity 30 (Sonar S3776, max 15), a 6-level-deep inline function nest in the mobile label-rename handler (S2004), and a nested ternary choosing between the field library / read-only notice / outline (S3358). - studioWorkspaceHelpers.ts: pulls the branching logic out of mutation callbacks and effects (version reseed sync, conflict-schema sync, review decision dispatch, save-as-new-version, error-message resolution) into named functions with their own, separate complexity budget — the effects/mutations in the component now each reduce to a single delegated call. - useStudioFieldCommands.ts: extracts the add-field/add-page guard clauses into their own hook. - StudioSidePanel.tsx: owns the field-library/outline tab switch, resolving the nested ternary into a single decision function. - StudioMobileWorkspace.tsx: owns the mobile branch's rendering, and replaces the inline nested page->section->field map chain with the existing flat updateFieldInSchema/renamePageTitle helpers (the same ones the desktop canvas already uses), eliminating the deep nesting entirely rather than just relocating it. Undo/redo, dirty-state guarding, autosave/manual-save semantics, and URL/selection synchronization are unchanged — the hook call order and dependencies are identical, only the callback bodies moved. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
StudioOutline had a map > map(&&) > ternary > map chain (page -> section -> field, Sonar S2004, max 4 levels) and StudioMobileReview had a similar page -> field nesting. Both are split into small named components (OutlinePage/OutlineSection/OutlineField; MobileReviewPageSection/MobileReviewPageTitleField/ MobileReviewFieldLabelField/MobileReviewStatusSummary), each with at most one level of mapping in its own body. MobileReviewPageTitleField/MobileReviewFieldLabelField now own their draft text as local state instead of the parent tracking every open input in one shared Record keyed by id — equivalent behavior (once edited, the local override persists the same way), simpler to reason about per-row. Also fixes S6819: the mobile advisory banner used role="status" on a div; it is now a semantic <output aria-live="polite"> element, which is the accessible-by-default alternative Sonar and the task both call for. Keyboard navigation, selection, and expand/collapse in the outline are unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- ValidationPanel.locateIssue was Cognitive Complexity 17 (Sonar S3776, max 15): a nested page/section scan followed by a flattened field scan with a compound boolean condition. Split into findPageOrSectionLocation / matchesIssueField / findFieldLocation, each independently simple; locateIssue itself is now two lines. Also replaces the `issue.fieldKey && ... === issue.fieldKey...` guard with `issue.fieldKey?.toLowerCase()` (S6582). - versionDiff.diffField was Cognitive Complexity 16: a 7-branch if-chain building changedProperties. Converted to a declarative FIELD_PROPERTY_CHECKS table (label + comparator) filtered/mapped in one pass, in the exact original order, plus a hasRequiredChanged helper. Also adds a "قواعد التحقق" (validation rules) check to the same table, and covers renamed/validation/formula/condition/ reordered/unchanged-form scenarios in versionDiff.test.ts. Diff output order and displayed text are unchanged; verified against the existing test suite plus the new scenarios. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- ConditionBuilder and FormulaBuilder used role="group" on a <div>
(Sonar S6819); both are now <fieldset><legend>...</legend> — the
native, accessible-by-default grouping element the rule asks for.
`getByRole('group', { name })` still resolves correctly since
<fieldset> exposes that role with its <legend> as the accessible
name, so no test changes were needed.
- Replaced array-index React keys (S6479) with content-derived keys:
nested condition groups and formula argument nodes have no id of
their own (mirroring the server's shape), so their key is the
node's own JSON content — neither component holds per-instance
local state, so two structurally-identical siblings sharing a key
is harmless. Condition predicates, field-issue rows, and recent-
field-type buttons use natural composite/unique identifiers
(predicate content, code+path, and the type itself, which is
deduplicated before insertion) instead of array position.
- StudioInspector: replaced the `field.number ?? {}` spread fallback
(S7744, "the empty object is useless") with `...field.number`
directly — spreading undefined/null into an object literal is a
no-op in JS, so the empty-object literal added nothing. Choice
option rows now key off `option.value`, which the existing
duplicate-value guard already keeps unique.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Removed `event.returnValue = ''` (Sonar S1874, deprecated). `event.preventDefault()` alone is the modern, spec-compliant way to trigger the browser's generic beforeunload prompt; the project has no documented legacy-browser target (no browserslist/legacy build config) that would require the old fallback. Added useUnsavedChangesGuard.test.ts covering the hook's actual observable behavior (whether a dispatched beforeunload event ends up defaultPrevented) rather than mocking addEventListener: no-op when clean, blocks when dirty, stops blocking after unmount, and reacts to the dirty flag changing across re-renders. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Replaced `await waitFor(() => expect(screen.getByText(...)))` with `expect(await screen.findByText(...))` (Sonar S9020) — findBy already retries until the element appears or times out, so the outer waitFor was redundant. waitFor is kept only for non-DOM assertions (mock call checks) where findBy doesn't apply. Also gives the three tests that combine userEvent typing, the 800ms autosave debounce, and a waitFor an explicit 10s test timeout instead of vitest's 5s default — these were already timing-marginal before this change and became visibly flaky under concurrent system load while the backend integration suite was running; not a regression introduced by the findBy swap itself. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (9)
docs/ux-rescue/phase2a-form-designer-completion-report.md-26-27 (1)
26-27: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the component inventory counts.
Line 26 lists 10 studio components, not 8; line 27 lists 10 designer modules/components, not 7. Update the counts or the lists so the completion report accurately reflects the delivered scope.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/ux-rescue/phase2a-form-designer-completion-report.md` around lines 26 - 27, Correct the inventory counts in the completion report: update the studio component count to 10 and the designer module/component count to 10, while preserving the listed names.docs/ux-rescue/phase2a-form-designer-compliance-ledger.md-98-98 (1)
98-98: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCount
Maxamong the verified formula operations.The schema contract documents
Maxas a supported backend function, so “Verified للستة الأولى” is inconsistent with the seven supported requested operations through الحد الأعلى. Change this to “للسبعة الأولى” or clarify the intended subset.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/ux-rescue/phase2a-form-designer-compliance-ledger.md` at line 98, Update the formula-operations verification entry in the compliance ledger to count “Max” among the supported verified operations, changing “Verified للستة الأولى” to “Verified للسبعة الأولى” unless the schema contract confirms a narrower intended subset.docs/ux-rescue/phase2a-form-designer-compliance-ledger.md-90-90 (1)
90-90: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the malformed compliance table rows.
These rows have only three cells in a four-column table, so Markdown rendering drops the evidence column. Add the missing fourth cell or split the status/evidence text correctly on Lines 90, 164, 203, and 205.
Also applies to: 164-164, 203-203, 205-205
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/ux-rescue/phase2a-form-designer-compliance-ledger.md` at line 90, The compliance table rows at the entries labeled 40, 164, 203, and 205 have only three cells instead of the required four. Update each row to include a separate fourth evidence cell, preserving the existing status and evidence content while ensuring every row matches the table’s column structure.Source: Linters/SAST tools
src/frontend/src/forms/designer/ConditionBuilder.test.tsx-8-24 (1)
8-24: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSelf-reference exclusion test never actually exercises the exclusion logic.
excludeFieldKeyis hardcoded to'self_field'(Line 21), butFIELDSnever contains a field keyedself_field(Lines 8-12). The assertion that no option hasvalue === 'self_field'is trivially true whether or notwouldCreateSelfReferencefiltering works — a regression here would go undetected.🧪 Proposed fix — make `self_field` an actual candidate so exclusion is meaningfully tested
const FIELDS: ConditionableField[] = [ + { key: 'self_field', labelAr: 'الحقل الحالي', type: 0 }, { key: 'text_field', labelAr: 'حقل نصي', type: 0 }, { key: 'number_field', labelAr: 'حقل رقمي', type: 2 }, { key: 'choice_field', labelAr: 'حقل اختيار', type: 7, choiceOptions: [{ value: 'a', labelAr: 'أ', order: 0, isActive: true }, { value: 'b', labelAr: 'ب', order: 1, isActive: true }] }, ]Also applies to: 45-53
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/forms/designer/ConditionBuilder.test.tsx` around lines 8 - 24, Add a field keyed "self_field" to the FIELDS fixture used by Harness, with valid field metadata, while keeping Harness.excludeFieldKey set to "self_field". Ensure the self-reference exclusion test can observe and verify that this candidate is omitted rather than passing trivially.src/frontend/src/App.route-redirects.test.tsx-20-20 (1)
20-20: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the preserved
versionId, not just the destination route.The probe matches regardless of query string, so an implementation that drops
?versionId=v1still passes.Proposed test update
-import { MemoryRouter, Route, Routes } from 'react-router' +import { MemoryRouter, Route, Routes, useLocation } from 'react-router' function TargetProbe({ label }: Readonly<{ label: string }>) { - return <div>{label}</div> + const location = useLocation() + return <div data-location={`${location.pathname}${location.search}`}>{label}</div> } - expect(screen.getByText('landed-in-studio')).toBeInTheDocument() + expect(screen.getByText('landed-in-studio')).toHaveAttribute( + 'data-location', + '/forms/designer/f1?versionId=v1', + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/App.route-redirects.test.tsx` at line 20, Update the route redirect assertion in the test to verify the destination includes the preserved versionId query parameter, such as versionId=v1, rather than only matching the landed-in-studio text. Keep the existing destination assertion while adding an exact URL or query-string check that fails when the redirect drops versionId.src/frontend/src/pages/forms/studio/FormDesignerStudioPage.tsx-326-339 (1)
326-339: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
canValidateis hardcodedtruehere but gated oncanEditfor mobile.Read-only viewers get an enabled “التحقق” button on desktop/tablet that will likely fail server-side. Consider
canValidate={canEdit}(orhasAllowedAction(allowedActions, 'Validate')) for consistency withStudioMobileWorkspace.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/pages/forms/studio/FormDesignerStudioPage.tsx` around lines 326 - 339, Update the FormDesignerStudioPage validation capability passed to the studio component so canValidate is restricted to users who can edit, matching the mobile workspace behavior; replace the hardcoded true value while preserving the existing validation handler.src/frontend/src/pages/forms/studio/StudioInspector.tsx-116-116 (1)
116-116: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSubstring matching attributes issues to the wrong field.
i.path.includes(field.key)misfires whenever one key is a substring of another (e.g.agematchingmanager_age), so unrelated validation issues appear under this field. Match on a parsed path segment instead of a raw substring.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/pages/forms/studio/StudioInspector.tsx` at line 116, Update the fieldIssues filtering in StudioInspector to parse each issue path into segments and match field.key against an exact segment, rather than using substring includes. Preserve case-insensitive matching while preventing keys such as “age” from matching “manager_age”.src/frontend/src/pages/forms/studio/StudioConflictBanner.tsx-29-31 (1)
29-31: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCompare stays permanently disabled if the server schema fetch fails.
The caller derives
isLoadingServerSchemafrom!conflictServerSchema(FormDesignerStudioPage.tsxline 345) andsyncConflictServerSchemaswallows fetch errors, so a failed load is indistinguishable from loading and the button never re-enables. Consider an explicit loading/error flag from the caller and a retry affordance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/pages/forms/studio/StudioConflictBanner.tsx` around lines 29 - 31, Update StudioConflictBanner and its FormDesignerStudioPage caller to track server-schema loading explicitly rather than deriving isLoadingServerSchema from !conflictServerSchema. Ensure syncConflictServerSchema exposes or propagates fetch failure state, allowing the compare button to re-enable after errors, and add a retry affordance for failed schema loads.src/frontend/src/pages/forms/studio/StudioCanvas.tsx-216-230 (1)
216-230: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winIncomplete ARIA tab pattern in the studio shell. Both tablists declare
role="tablist"/role="tab"without an associatedrole="tabpanel",aria-controls, or arrow-key roving focus, so screen-reader users get a tab widget that doesn't behave like one.
src/frontend/src/pages/forms/studio/StudioCanvas.tsx#L216-L230: give the page tabsid/aria-controlspointing at the page content region (markedrole="tabpanel") and add arrow-key navigation, or drop the roles and keeparia-pressedbuttons.src/frontend/src/pages/forms/studio/StudioSidePanel.tsx#L58-L66: apply the same fix to the library/outline tabs, wiringaria-controlsto the panel rendered byresolveSidePanelContent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/pages/forms/studio/StudioCanvas.tsx` around lines 216 - 230, The studio’s ARIA tab widgets are incomplete because their tabs lack associated panels, controls, and keyboard navigation. In src/frontend/src/pages/forms/studio/StudioCanvas.tsx lines 216-230, either implement the full tab pattern by adding tab IDs, aria-controls, arrow-key roving focus, and a role="tabpanel" page content region, or remove tab roles and use aria-pressed buttons; in src/frontend/src/pages/forms/studio/StudioSidePanel.tsx lines 58-66, apply the same chosen approach and wire the library/outline tabs to the panel rendered by resolveSidePanelContent.
🧹 Nitpick comments (11)
src/frontend/src/forms/designer/versionDiff.ts (1)
18-20: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
stableJsonis not actually order-stable.
JSON.stringifypreserves each object's own key-insertion order, so two semantically-equal objects built via different code paths (e.g., one deserialized from a server response, another freshly constructed with a different literal property order) can serialize differently and be flagged as "changed" even though nothing meaningfully differs. This affects everystableJson-based comparison here (text/number/file/validationRulessettings, options,visibilityCondition,formula,requiredCondition), and would surface as spurious entries in the version-compare UI.Consider a canonical comparison (recursively sort object keys before stringifying, or use a proper deep-equal utility) instead of raw
JSON.stringify.Since this depends on how
before/afterschema objects are actually constructed elsewhere (e.g.studioSchemaOps.ts, server DTO serialization), please confirm whether key ordering can actually diverge between the two.Also applies to: 52-59, 77-79
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/forms/designer/versionDiff.ts` around lines 18 - 20, Update stableJson to canonicalize values before serialization by recursively sorting object keys, while preserving array order and the existing null handling. Ensure all stableJson-based comparisons produce identical results for semantically equal objects with different key insertion orders.src/frontend/src/pages/forms/FormsListPage.tsx (1)
104-106: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAvoid nesting a button inside the navigation link.
Use the
Linkitself as the interactive control; nested interactive elements are invalid and may behave inconsistently for keyboard users.Proposed update
- <Link to="/forms/designer/new"> - <button type="button">نموذج جديد (استوديو التصميم)</button> - </Link> + <Link to="/forms/designer/new">نموذج جديد (استوديو التصميم)</Link>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/pages/forms/FormsListPage.tsx` around lines 104 - 106, Update the new-form navigation control in FormsListPage to use the Link directly as the interactive element, removing the nested button while preserving its destination and displayed label.src/frontend/src/pages/forms/studio/StudioOutline.tsx (1)
4-18: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSelected field isn't exposed to assistive tech.
Selection is communicated only through a CSS class swap; add
aria-currentso screen-reader users can tell which field is selected.♻️ Proposed fix
<button type="button" className={isSelected ? 'studio-outline-field' : 'studio-outline-field secondary'} + aria-current={isSelected ? 'true' : undefined} onClick={onSelect} >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/pages/forms/studio/StudioOutline.tsx` around lines 4 - 18, Update the OutlineField button to expose its selection state through aria-current, setting it to the selected state when isSelected is true and leaving it unset or false otherwise, while preserving the existing className and onSelect behavior.src/frontend/src/pages/forms/studio/StudioStartFlow.tsx (1)
251-261: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSearch fires a request on every keystroke.
queryKeyincludes the rawsearchvalue with no debounce, so once the 2-character threshold is met, every keystroke issues a newapi.forms.listcall.♻️ Proposed fix: debounce the search term
+ const [debouncedSearch, setDebouncedSearch] = useState('') + useEffect(() => { + const id = setTimeout(() => setDebouncedSearch(search), 300) + return () => clearTimeout(id) + }, [search]) + const searchQuery = useQuery({ - queryKey: ['forms-search', search], - queryFn: () => api.forms.list({ search, pageSize: 10 }), - enabled: search.trim().length >= 2, + queryKey: ['forms-search', debouncedSearch], + queryFn: () => api.forms.list({ search: debouncedSearch, pageSize: 10 }), + enabled: debouncedSearch.trim().length >= 2, })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/pages/forms/studio/StudioStartFlow.tsx` around lines 251 - 261, Debounce the form search term used by the searchQuery in StudioStartFlow, so api.forms.list is not called for every keystroke once the two-character threshold is reached. Introduce or reuse a debounced search value, use it in both queryKey and queryFn, and keep the existing minimum-length enabled condition applied to the debounced value.src/frontend/src/index.css (1)
2200-2210: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeprecated
clipproperty flagged by stylelint.♻️ Proposed fix
.sr-only { position: absolute; width: 1px; height: 1px; padding: 0; margin: -1px; overflow: hidden; - clip: rect(0, 0, 0, 0); + clip-path: inset(50%); white-space: nowrap; border: 0; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/index.css` around lines 2200 - 2210, Update the .sr-only accessibility utility to replace the deprecated clip declaration with the supported clip-path equivalent, preserving the existing visually-hidden behavior and stylelint compliance.Source: Linters/SAST tools
src/frontend/src/pages/forms/studio/FormDesignerStudioPage.tsx (2)
84-91: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid mutating the previous
URLSearchParamsand prefer named version statuses.Mutating and returning
prevrelies on React Router re-serializing the same instance; a fresh copy is the safer, idiomatic form. Also,status === 0 || status === 2is opaque — a named constant/enum for draft/changes-requested would read better here and in the rest of the studio.♻️ Suggested change
- if (resolved) { - setSearchParams((prev) => { prev.set('versionId', resolved.id); return prev }, { replace: true }) - } + if (resolved) { + setSearchParams((prev) => { + const next = new URLSearchParams(prev) + next.set('versionId', resolved.id) + return next + }, { replace: true }) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/pages/forms/studio/FormDesignerStudioPage.tsx` around lines 84 - 91, Update the setSearchParams callback in FormDesignerStudioPage to return a new URLSearchParams copy after setting versionId, rather than mutating and returning prev. Replace the numeric status checks in the editable-version selection with the existing named constants or enum values for draft and changes-requested statuses, reusing those symbols consistently in the studio.
234-239: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
isSavingAsNewVersionduplicatessaveAsNewVersionMutation.isPending.The extra state (plus
onMutate/onSettled) can be dropped in favour of the mutation's own pending flag passed toStudioConflictBanner.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/pages/forms/studio/FormDesignerStudioPage.tsx` around lines 234 - 239, Remove the redundant isSavingAsNewVersion state and its onMutate/onSettled handlers from saveAsNewVersionMutation. Use saveAsNewVersionMutation.isPending directly when passing the saving status to StudioConflictBanner, preserving the existing mutation and success navigation behavior.src/frontend/src/pages/forms/studio/StudioInspector.tsx (1)
46-54: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueEffect keyed on the
selectedFieldobject identity resets in-progress drafts.
selectedFieldis a fresh object after any schema change (e.g. toggling “إلزامي”), so unrelated edits re-run this effect and overwrite drafts the user is still typing. Depending onselectedField?.idwould scope the reset to actual selection changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/pages/forms/studio/StudioInspector.tsx` around lines 46 - 54, Update the draft-reset useEffect associated with selectedField to depend on selectedField?.id instead of the selectedField object identity, so schema updates do not overwrite in-progress edits while switching fields still resets all draft values and errors.src/frontend/src/pages/forms/studio/StudioCanvas.tsx (1)
91-135: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
FieldRowreimplementsInlineEditableText.The label draft/commit/Escape logic duplicates the component defined above at lines 16-62; reuse it (passing the trigger label) instead of a second copy.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/pages/forms/studio/StudioCanvas.tsx` around lines 91 - 135, Replace the duplicated label editing state and commit/Escape handling in FieldRow with the existing InlineEditableText component defined above. Pass field.labelAr as the displayed value and trigger label, and connect its save callback to onRenameLabel while preserving trimming and unchanged-value behavior.src/frontend/src/pages/forms/studio/FormDesignerStudioPage.test.tsx (1)
26-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
validateVersionis mocked but never exercised.Consider a case covering the validate →
ValidationPanelpath (and a read-only version wherecanEditis false), which are the branches most likely to regress.Also applies to: 152-192
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/pages/forms/studio/FormDesignerStudioPage.test.tsx` at line 26, Add tests in FormDesignerStudioPage.test.tsx that exercise the mocked validateVersion flow through ValidationPanel, including the read-only canEdit=false case. Assert validation is triggered and the resulting panel or read-only behavior is rendered, while preserving existing editable-path coverage.src/frontend/src/pages/forms/studio/StudioFieldLibrary.tsx (1)
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
aria-labelon a role-lessdivis ignored by assistive tech.Use a landmark/region element so the label is exposed, e.g.
<section aria-label="مكتبة الحقول">(or addrole="group").🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/pages/forms/studio/StudioFieldLibrary.tsx` at line 16, Update the role-less container in StudioFieldLibrary to use a semantic section or an explicit group/region role so the existing Arabic aria-label is exposed to assistive technologies, while preserving the current styling class and contents.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/permissions-matrix.md`:
- Line 417: Update the Forms.ManageTemplates row in the permissions matrix to
align with CreateFormFromTemplateAsync: clearly state that template-based form
creation requires both Forms.ManageTemplates and Forms.Create, and remove the
contradictory claim that Forms.ManageTemplates is unnecessary.
In `@docs/ux-rescue/phase2a-form-designer-autosave.md`:
- Around line 23-25: Correct the autosave concurrency documentation for
`inFlightRef` and `abortRef`: do not claim that debounced saves are serialized
or that aborting prevents the server from processing an outdated request. Either
update the hook’s debounced `saveNow` flow to await the existing `inFlightRef`
before sending the next save, or revise the documentation to describe possible
concurrent writes, 409 responses, and retry handling accurately.
In `@docs/ux-rescue/phase2a-form-designer-compliance-ledger.md`:
- Line 214: Revise the risk statement in the compliance ledger to classify
published option-key changes as a data-integrity risk, consistent with the
migration-policy gap recorded on line 73. Alternatively, add an explicit
migration or immutability safeguard for keys used by published schemas, then
retain the existing statement only if that safeguard is documented.
- Line 105: Update row 55 in the compliance ledger from Verified to Partial
because previewLogic.ts and the server evaluators are separate implementations.
Describe the outcome as sharing the same tested rules rather than the same
engine, unless the implementation adds a shared evaluator or automated parity
contract.
In `@docs/ux-rescue/phase2a-form-designer-performance.md`:
- Line 5: Update the “لا N+1 عند تحميل الاستوديو” statement to describe exactly
two API requests, not exactly two database or EF Core queries. Remove the
unsupported claim that these requests prove query count or absence of N+1
behavior, and avoid asserting query-count verification until the
interceptor-based test referenced later exists.
In `@src/frontend/src/forms/designer/ConditionBuilder.tsx`:
- Around line 172-193: Replace the value-derived keys in the predicate and
nested-group renderers with stable array-index keys, using the existing map
indices in the top-level and nested collections. Update the key on the predicate
row and the key generated by conditionGroupKey for nested groups; preserve the
existing append and filter-removal behavior.
In `@src/frontend/src/forms/designer/FormulaBuilder.tsx`:
- Around line 41-43: Update formulaNodeKey and the function-argument rendering
in FormulaNodeEditor to use each argument’s stable array index as its React key
instead of JSON.stringify(node). Preserve the controlled input behavior while
ensuring keys remain unchanged during edits and are unique for duplicate
newly-created arguments.
In `@src/frontend/src/forms/designer/studioSchemaOps.ts`:
- Around line 3-14: The cloning flow must remap internal field-key references
when duplicating groups. Update cloneWithNewIds and the
duplicateSection/duplicatePage paths to build an old-key-to-new-key map for all
copied fields, then rewrite visibilityCondition, requiredCondition, and formula
fieldReference nodes targeting keys in that map; leave duplicateField behavior
unchanged because its siblings are not copied.
In `@src/frontend/src/pages/forms/studio/FormDesignerStudioPage.tsx`:
- Around line 268-274: Reorder the guards in FormDesignerStudioPage so the
versionQuery.isError branch runs before the loading condition that checks
!history and !schema. Preserve the existing formatted API error response and
loading behavior for non-error states.
- Around line 97-110: Handle versionsQuery failures and the
no-version/no-permission state in FormDesignerStudioPage before the existing
version-selection spinner: render an alert using formatApiError for
versionsQuery.isError, and render a non-loading terminal message or error state
when versionsQuery.data is empty and canDesign is false. Preserve
CreateFirstVersion for empty versions when canDesign is true and keep the normal
version-selection flow for available versions.
In `@src/frontend/src/pages/forms/studio/StudioCanvas.tsx`:
- Around line 273-291: Update the field rendering in StudioCanvas so it passes a
lazy resolveDependents callback to FieldRow instead of eagerly evaluating
fieldDependents(field.key) for every row. Update FieldRow to invoke
resolveDependents only while confirmingDelete is true, preserving the existing
dependent data used by delete confirmation.
In `@src/frontend/src/pages/forms/studio/StudioInspector.tsx`:
- Around line 315-319: Update the options mapping in the choice editor to use a
stable key instead of option.value, so editing the “مفتاح الخيار” input does not
remount the row or lose focus. Modify the key on the mapped div while preserving
the existing updateOption behavior and validation display.
In `@src/frontend/src/pages/forms/studio/StudioMobileReview.tsx`:
- Around line 5-21: Synchronize the local draft state with updated source values
in both MobileReviewPageTitleField and MobileReviewFieldLabelField: add an
effect that updates draft when titleAr or labelAr changes, respectively.
Preserve the existing editing and trimmed onBlur commit behavior.
In `@src/frontend/src/pages/forms/studio/studioWorkspaceHelpers.ts`:
- Around line 39-47: The JSON parsing fallback logic is duplicated across the
studio flow. Keep parseSchema in
src/frontend/src/pages/forms/studio/studioWorkspaceHelpers.ts:39-47 as the
single exported implementation, and in
src/frontend/src/pages/forms/studio/StudioStartFlow.tsx:29-36 remove the local
copy and import parseSchema from './studioWorkspaceHelpers'.
---
Minor comments:
In `@docs/ux-rescue/phase2a-form-designer-completion-report.md`:
- Around line 26-27: Correct the inventory counts in the completion report:
update the studio component count to 10 and the designer module/component count
to 10, while preserving the listed names.
In `@docs/ux-rescue/phase2a-form-designer-compliance-ledger.md`:
- Line 98: Update the formula-operations verification entry in the compliance
ledger to count “Max” among the supported verified operations, changing
“Verified للستة الأولى” to “Verified للسبعة الأولى” unless the schema contract
confirms a narrower intended subset.
- Line 90: The compliance table rows at the entries labeled 40, 164, 203, and
205 have only three cells instead of the required four. Update each row to
include a separate fourth evidence cell, preserving the existing status and
evidence content while ensuring every row matches the table’s column structure.
In `@src/frontend/src/App.route-redirects.test.tsx`:
- Line 20: Update the route redirect assertion in the test to verify the
destination includes the preserved versionId query parameter, such as
versionId=v1, rather than only matching the landed-in-studio text. Keep the
existing destination assertion while adding an exact URL or query-string check
that fails when the redirect drops versionId.
In `@src/frontend/src/forms/designer/ConditionBuilder.test.tsx`:
- Around line 8-24: Add a field keyed "self_field" to the FIELDS fixture used by
Harness, with valid field metadata, while keeping Harness.excludeFieldKey set to
"self_field". Ensure the self-reference exclusion test can observe and verify
that this candidate is omitted rather than passing trivially.
In `@src/frontend/src/pages/forms/studio/FormDesignerStudioPage.tsx`:
- Around line 326-339: Update the FormDesignerStudioPage validation capability
passed to the studio component so canValidate is restricted to users who can
edit, matching the mobile workspace behavior; replace the hardcoded true value
while preserving the existing validation handler.
In `@src/frontend/src/pages/forms/studio/StudioCanvas.tsx`:
- Around line 216-230: The studio’s ARIA tab widgets are incomplete because
their tabs lack associated panels, controls, and keyboard navigation. In
src/frontend/src/pages/forms/studio/StudioCanvas.tsx lines 216-230, either
implement the full tab pattern by adding tab IDs, aria-controls, arrow-key
roving focus, and a role="tabpanel" page content region, or remove tab roles and
use aria-pressed buttons; in
src/frontend/src/pages/forms/studio/StudioSidePanel.tsx lines 58-66, apply the
same chosen approach and wire the library/outline tabs to the panel rendered by
resolveSidePanelContent.
In `@src/frontend/src/pages/forms/studio/StudioConflictBanner.tsx`:
- Around line 29-31: Update StudioConflictBanner and its FormDesignerStudioPage
caller to track server-schema loading explicitly rather than deriving
isLoadingServerSchema from !conflictServerSchema. Ensure
syncConflictServerSchema exposes or propagates fetch failure state, allowing the
compare button to re-enable after errors, and add a retry affordance for failed
schema loads.
In `@src/frontend/src/pages/forms/studio/StudioInspector.tsx`:
- Line 116: Update the fieldIssues filtering in StudioInspector to parse each
issue path into segments and match field.key against an exact segment, rather
than using substring includes. Preserve case-insensitive matching while
preventing keys such as “age” from matching “manager_age”.
---
Nitpick comments:
In `@src/frontend/src/forms/designer/versionDiff.ts`:
- Around line 18-20: Update stableJson to canonicalize values before
serialization by recursively sorting object keys, while preserving array order
and the existing null handling. Ensure all stableJson-based comparisons produce
identical results for semantically equal objects with different key insertion
orders.
In `@src/frontend/src/index.css`:
- Around line 2200-2210: Update the .sr-only accessibility utility to replace
the deprecated clip declaration with the supported clip-path equivalent,
preserving the existing visually-hidden behavior and stylelint compliance.
In `@src/frontend/src/pages/forms/FormsListPage.tsx`:
- Around line 104-106: Update the new-form navigation control in FormsListPage
to use the Link directly as the interactive element, removing the nested button
while preserving its destination and displayed label.
In `@src/frontend/src/pages/forms/studio/FormDesignerStudioPage.test.tsx`:
- Line 26: Add tests in FormDesignerStudioPage.test.tsx that exercise the mocked
validateVersion flow through ValidationPanel, including the read-only
canEdit=false case. Assert validation is triggered and the resulting panel or
read-only behavior is rendered, while preserving existing editable-path
coverage.
In `@src/frontend/src/pages/forms/studio/FormDesignerStudioPage.tsx`:
- Around line 84-91: Update the setSearchParams callback in
FormDesignerStudioPage to return a new URLSearchParams copy after setting
versionId, rather than mutating and returning prev. Replace the numeric status
checks in the editable-version selection with the existing named constants or
enum values for draft and changes-requested statuses, reusing those symbols
consistently in the studio.
- Around line 234-239: Remove the redundant isSavingAsNewVersion state and its
onMutate/onSettled handlers from saveAsNewVersionMutation. Use
saveAsNewVersionMutation.isPending directly when passing the saving status to
StudioConflictBanner, preserving the existing mutation and success navigation
behavior.
In `@src/frontend/src/pages/forms/studio/StudioCanvas.tsx`:
- Around line 91-135: Replace the duplicated label editing state and
commit/Escape handling in FieldRow with the existing InlineEditableText
component defined above. Pass field.labelAr as the displayed value and trigger
label, and connect its save callback to onRenameLabel while preserving trimming
and unchanged-value behavior.
In `@src/frontend/src/pages/forms/studio/StudioFieldLibrary.tsx`:
- Line 16: Update the role-less container in StudioFieldLibrary to use a
semantic section or an explicit group/region role so the existing Arabic
aria-label is exposed to assistive technologies, while preserving the current
styling class and contents.
In `@src/frontend/src/pages/forms/studio/StudioInspector.tsx`:
- Around line 46-54: Update the draft-reset useEffect associated with
selectedField to depend on selectedField?.id instead of the selectedField object
identity, so schema updates do not overwrite in-progress edits while switching
fields still resets all draft values and errors.
In `@src/frontend/src/pages/forms/studio/StudioOutline.tsx`:
- Around line 4-18: Update the OutlineField button to expose its selection state
through aria-current, setting it to the selected state when isSelected is true
and leaving it unset or false otherwise, while preserving the existing className
and onSelect behavior.
In `@src/frontend/src/pages/forms/studio/StudioStartFlow.tsx`:
- Around line 251-261: Debounce the form search term used by the searchQuery in
StudioStartFlow, so api.forms.list is not called for every keystroke once the
two-character threshold is reached. Introduce or reuse a debounced search value,
use it in both queryKey and queryFn, and keep the existing minimum-length
enabled condition applied to the debounced value.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 77d8107e-919e-4b57-9afe-3fe0aaf76e6b
📒 Files selected for processing (72)
README.mddocs/implementation-plan.mddocs/permissions-matrix.mddocs/ux-rescue/phase2a-form-designer-accessibility.mddocs/ux-rescue/phase2a-form-designer-architecture.mddocs/ux-rescue/phase2a-form-designer-autosave.mddocs/ux-rescue/phase2a-form-designer-completion-report.mddocs/ux-rescue/phase2a-form-designer-compliance-ledger.mddocs/ux-rescue/phase2a-form-designer-performance.mddocs/ux-rescue/phase2a-form-designer-route-transition.mddocs/ux-rescue/phase2a-form-designer-schema-contract.mddocs/ux-rescue/phase2a-form-designer-scope.mddocs/ux-rescue/phase2a-form-designer-test-matrix.mddocs/ux-rescue/phase2a-form-designer-validation.mddocs/ux-rescue/rescue-roadmap.mddocs/ux-rescue/screen-and-route-inventory.mddocs/ux-rescue/task-metrics-baseline.mdsrc/backend/Baseera.Api/Endpoints/ApiEndpoints.cssrc/backend/Baseera.Application/DependencyInjection/ApplicationServiceCollectionExtensions.cssrc/backend/Baseera.Application/Forms/FormTemplateService.cssrc/backend/Baseera.Application/Forms/FormVersionAccessGuard.cssrc/backend/Baseera.Application/Forms/FormVersionDtos.cssrc/backend/Baseera.Application/Forms/FormVersionService.cssrc/backend/tests/Baseera.IntegrationTests/FormsVersionIntegrationTests.cssrc/backend/tests/Baseera.UnitTests/Forms/Versions/FormVersionAccessGuardTests.cssrc/frontend/src/App.route-redirects.test.tsxsrc/frontend/src/App.tsxsrc/frontend/src/api/client.tssrc/frontend/src/forms/designer/ConditionBuilder.test.tsxsrc/frontend/src/forms/designer/ConditionBuilder.tsxsrc/frontend/src/forms/designer/DesignerCanvas.tsxsrc/frontend/src/forms/designer/DesignerPalette.tsxsrc/frontend/src/forms/designer/DesignerPropertiesPanel.tsxsrc/frontend/src/forms/designer/DesignerToolbar.tsxsrc/frontend/src/forms/designer/FormulaBuilder.test.tsxsrc/frontend/src/forms/designer/FormulaBuilder.tsxsrc/frontend/src/forms/designer/ValidationPanel.test.tsxsrc/frontend/src/forms/designer/ValidationPanel.tsxsrc/frontend/src/forms/designer/VersionCompare.tsxsrc/frontend/src/forms/designer/fieldDependencies.test.tssrc/frontend/src/forms/designer/fieldDependencies.tssrc/frontend/src/forms/designer/fieldLibrary.tssrc/frontend/src/forms/designer/studioSchemaOps.tssrc/frontend/src/forms/designer/useResponsiveStudioLayout.tssrc/frontend/src/forms/designer/useUnsavedChangesGuard.test.tssrc/frontend/src/forms/designer/useUnsavedChangesGuard.tssrc/frontend/src/forms/designer/versionDiff.test.tssrc/frontend/src/forms/designer/versionDiff.tssrc/frontend/src/index.csssrc/frontend/src/pages/form-campaigns/FormCampaignWizardPage.tsxsrc/frontend/src/pages/forms/FormsListPage.tsxsrc/frontend/src/pages/forms/studio/FormDesignerStudioPage.test.tsxsrc/frontend/src/pages/forms/studio/FormDesignerStudioPage.tsxsrc/frontend/src/pages/forms/studio/StudioCanvas.tsxsrc/frontend/src/pages/forms/studio/StudioConflictBanner.tsxsrc/frontend/src/pages/forms/studio/StudioFieldLibrary.tsxsrc/frontend/src/pages/forms/studio/StudioInspector.tsxsrc/frontend/src/pages/forms/studio/StudioMobileReview.tsxsrc/frontend/src/pages/forms/studio/StudioMobileWorkspace.tsxsrc/frontend/src/pages/forms/studio/StudioOutline.tsxsrc/frontend/src/pages/forms/studio/StudioReviewPanel.tsxsrc/frontend/src/pages/forms/studio/StudioSidePanel.tsxsrc/frontend/src/pages/forms/studio/StudioStartFlow.tsxsrc/frontend/src/pages/forms/studio/StudioTopBar.tsxsrc/frontend/src/pages/forms/studio/studioWorkspaceHelpers.tssrc/frontend/src/pages/forms/studio/useStudioFieldCommands.tssrc/frontend/src/pages/forms/templates/FormTemplatesPage.tsxsrc/frontend/src/pages/forms/versions/FormDesignerPage.test.tsxsrc/frontend/src/pages/forms/versions/FormDesignerPage.tsxsrc/frontend/src/pages/forms/versions/FormVersionComparePage.tsxsrc/frontend/src/pages/forms/versions/FormVersionDetailPage.tsxsrc/frontend/src/pages/forms/versions/FormVersionsPage.tsx
💤 Files with no reviewable changes (6)
- src/frontend/src/forms/designer/DesignerToolbar.tsx
- src/frontend/src/forms/designer/DesignerCanvas.tsx
- src/frontend/src/pages/forms/versions/FormDesignerPage.test.tsx
- src/frontend/src/forms/designer/DesignerPalette.tsx
- src/frontend/src/pages/forms/versions/FormDesignerPage.tsx
- src/frontend/src/forms/designer/DesignerPropertiesPanel.tsx
SonarCloud re-analysis of the previous commits surfaced two new findings from the refactor itself: - FormVersionService.ValidateAsync: 'form' was assigned from LoadViewableAsync but never used afterward (the old code needed it to call the now-inlined view-capability check separately) — S1481, unused local variable. Stopped capturing it. - StudioWorkspace was still Cognitive Complexity 19 (down from 30, but above the 15 limit) — the desktop/tablet JSX tree's many small ternaries and && checks (preview toggle, tablet panel toggles, undo/redo availability, conflict banner, validation/review panel visibility) were still counted in the orchestrator. Extracted StudioDesktopWorkspace (+ its StudioDesktopEditor sub-component) the same way StudioMobileWorkspace already was, so that render tree's complexity is attributed to it instead. StudioWorkspace now only wires up state/hooks and picks mobile vs. desktop. Also moved classifyIssues() out of StudioWorkspace entirely — both StudioMobileWorkspace and StudioDesktopWorkspace now compute errors/warnings themselves from the raw issues list, removing that call (and its complexity contribution) from the orchestrator too. No behavior change: same props, same hook order, same guards. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CodeRabbit flagged that the earlier Sonar remediation pass introduced a regression: ConditionBuilder/FormulaBuilder/StudioInspector switched array-index keys to content-derived keys (JSON.stringify / option.value) to satisfy a lint rule, but those lists hold editable text inputs — a key that changes on every keystroke forces React to remount the row and drop focus, so a user could not type more than one character into a predicate value, formula argument, or choice-option field. Revert to index keys, which are safe here (append/filter-only or swap-based reorder, no per-row local state), and add regression tests that type into each affected input and assert focus is retained. Also fixes a related regression in StudioMobileReview: the page-title/ field-label draft inputs seeded local state once and never resynced when the underlying schema changed externally (e.g. after a conflict reload), so a blur could commit stale text over freshly reloaded data.
…ions/pages duplicateSection/duplicatePage assign every duplicated field a new key (cloneWithNewIds) but left visibilityCondition/requiredCondition predicates and formula fieldReference nodes pointing at the original keys, so a condition or formula referencing a sibling field inside the duplicated group silently kept referencing the untouched original after duplication instead of its own copy. Build an old-key -> new-key map up front for the group being duplicated (including nested repeating-table columns) and rewrite condition/formula references through it; duplicateField gets the same treatment for its own repeating-table columns, since those undergo the same key-changing clone. References to fields outside the duplicated group are left untouched, matching existing behavior.
FormDesignerStudioPage never checked versionsQuery.isError, so a failed version-list request (or zero versions with no create permission) left the user on a permanent "جاري تحديد الإصدار…" spinner with no way out. Handle both cases explicitly with an error message. StudioCanvas called fieldDependents(field.key) — a full-schema scan — for every field on every render, even though the result is only shown once a row enters delete-confirmation. Compute it lazily inside FieldRow, gated on confirmingDelete, instead of eagerly for the whole list. StudioStartFlow had its own copy of the JSON-parse-with-fallback schema logic already exported from studioWorkspaceHelpers; import the shared one instead of maintaining two copies that can drift.
Five accuracy fixes raised in review, each a doc overclaiming a guarantee the code doesn't actually provide: - permissions-matrix.md: "using a template doesn't require Forms.ManageTemplates" contradicted CreateFormFromTemplateAsync, which checks both Forms.ManageTemplates and Forms.Create. - autosave.md: the debounced autosave path doesn't await inFlightRef (only flush() does) and abortRef only cancels the client-side request — the server may still process an "aborted" save, so parallel saves are not strictly prevented, just resolved via an occasional 409 rather than silent data loss. - compliance-ledger.md #55: preview and runtime use separate evaluator implementations tested for parity manually, not literally "the same engine" with an automated parity contract — downgraded to Partial. - compliance-ledger.md summary: one of the eight "Missing" items (no migration policy for renaming a published option key) is a real data- integrity risk, not just a UX/test/perf gap — called out explicitly. - performance.md: "no N+1" was verified at the HTTP-request level (two requests, no per-field loop), not at the EF Core query level, which was never measured.
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 (4)
src/frontend/src/pages/forms/studio/StudioInspector.tsx (4)
297-300: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winGenerate collision-free option values.
choice.options.length + 1can reuse an existing value after deletion. For example, removingoption_1from[option_1, option_2]and adding an option creates a secondoption_2, producing an invalid choice schema.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/pages/forms/studio/StudioInspector.tsx` around lines 297 - 300, Update addOption to derive the new option value from existing option values rather than choice.options.length + 1, selecting the next collision-free option_N identifier after deletions. Preserve the current label, order, and active-state behavior.
116-116: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMatch validation paths by field segments, not substrings.
includes(field.key)misattributes issues when one key is contained in another, such asnameandsurname. Parse the path or compare the exact field-key segment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/pages/forms/studio/StudioInspector.tsx` at line 116, Update the fieldIssues filter in StudioInspector to match field.key against an exact segment of each validation path rather than using substring includes, preventing keys such as name from matching surname. Parse or split each path using the established path format, compare the relevant segment exactly, and preserve case-insensitive matching.
21-25: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAvoid committing twice when Enter is pressed.
blur()invokes the input’sonBlurcommit, thencommit()runs again immediately. This can create duplicate schema/history updates and duplicate autosave work. Use either the blur path or the explicit commit, not both.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/pages/forms/studio/StudioInspector.tsx` around lines 21 - 25, Update commitOnEnter so pressing Enter triggers only one commit: either rely on event.currentTarget.blur() and remove the explicit commit() call, or commit explicitly without blurring. Preserve the existing Enter-key guard and avoid duplicate onBlur/schema history updates.
46-54: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not reset drafts on every selected-field object change.
Any schema edit creates a new
selectedFieldobject; while the user is typing an unblurred label, key, description, or default value, changing another inspector control can overwrite that draft with the previous schema value. Reset on field identity changes or preserve dirty draft state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/pages/forms/studio/StudioInspector.tsx` around lines 46 - 54, Update the selectedField synchronization effect in StudioInspector so drafts are reset only when the selected field identity changes, not whenever the selectedField object is recreated by schema edits. Preserve unblurred user edits to the label, key, description, and default value while other inspector controls modify the same field, while still initializing drafts for a newly selected field.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/ux-rescue/phase2a-form-designer-compliance-ledger.md`:
- Line 105: Update the ledger entry for preview/runtime parity to remove the
claim that the cited tests are manual. State that the separate preview and
runtime implementations have automated test coverage, while no automated
cross-engine parity contract exists; mention manual comparison only if it is
explicitly documented elsewhere.
In `@src/frontend/src/pages/forms/studio/StudioInspector.tsx`:
- Around line 315-319: Add an editor-only stable ID to each option in the
StudioInspector editor state, preserve it across reorder and removal operations,
and assign one when options are created or loaded. Update the choice.options
rendering to use that ID for the React key and keyErrors lookup, and adjust
duplicate-key validation/update logic to track errors by stable option ID rather
than array index.
---
Outside diff comments:
In `@src/frontend/src/pages/forms/studio/StudioInspector.tsx`:
- Around line 297-300: Update addOption to derive the new option value from
existing option values rather than choice.options.length + 1, selecting the next
collision-free option_N identifier after deletions. Preserve the current label,
order, and active-state behavior.
- Line 116: Update the fieldIssues filter in StudioInspector to match field.key
against an exact segment of each validation path rather than using substring
includes, preventing keys such as name from matching surname. Parse or split
each path using the established path format, compare the relevant segment
exactly, and preserve case-insensitive matching.
- Around line 21-25: Update commitOnEnter so pressing Enter triggers only one
commit: either rely on event.currentTarget.blur() and remove the explicit
commit() call, or commit explicitly without blurring. Preserve the existing
Enter-key guard and avoid duplicate onBlur/schema history updates.
- Around line 46-54: Update the selectedField synchronization effect in
StudioInspector so drafts are reset only when the selected field identity
changes, not whenever the selectedField object is recreated by schema edits.
Preserve unblurred user edits to the label, key, description, and default value
while other inspector controls modify the same field, while still initializing
drafts for a newly selected field.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 81df7ddb-6c8c-4475-957d-0c8946a0537a
📒 Files selected for processing (21)
docs/permissions-matrix.mddocs/ux-rescue/phase2a-form-designer-autosave.mddocs/ux-rescue/phase2a-form-designer-compliance-ledger.mddocs/ux-rescue/phase2a-form-designer-performance.mdsrc/backend/Baseera.Application/Forms/FormVersionService.cssrc/frontend/src/forms/designer/ConditionBuilder.test.tsxsrc/frontend/src/forms/designer/ConditionBuilder.tsxsrc/frontend/src/forms/designer/FormulaBuilder.test.tsxsrc/frontend/src/forms/designer/FormulaBuilder.tsxsrc/frontend/src/forms/designer/studioSchemaOps.test.tssrc/frontend/src/forms/designer/studioSchemaOps.tssrc/frontend/src/pages/forms/studio/FormDesignerStudioPage.test.tsxsrc/frontend/src/pages/forms/studio/FormDesignerStudioPage.tsxsrc/frontend/src/pages/forms/studio/StudioCanvas.tsxsrc/frontend/src/pages/forms/studio/StudioDesktopWorkspace.tsxsrc/frontend/src/pages/forms/studio/StudioInspector.test.tsxsrc/frontend/src/pages/forms/studio/StudioInspector.tsxsrc/frontend/src/pages/forms/studio/StudioMobileReview.test.tsxsrc/frontend/src/pages/forms/studio/StudioMobileReview.tsxsrc/frontend/src/pages/forms/studio/StudioMobileWorkspace.tsxsrc/frontend/src/pages/forms/studio/StudioStartFlow.tsx
🚧 Files skipped from review as they are similar to previous changes (9)
- src/frontend/src/forms/designer/FormulaBuilder.test.tsx
- src/frontend/src/forms/designer/ConditionBuilder.test.tsx
- src/frontend/src/pages/forms/studio/StudioMobileWorkspace.tsx
- src/frontend/src/pages/forms/studio/StudioCanvas.tsx
- src/frontend/src/pages/forms/studio/FormDesignerStudioPage.tsx
- src/frontend/src/pages/forms/studio/StudioStartFlow.tsx
- src/backend/Baseera.Application/Forms/FormVersionService.cs
- src/frontend/src/pages/forms/studio/StudioMobileReview.tsx
- src/frontend/src/forms/designer/studioSchemaOps.ts
added 3 commits
July 29, 2026 10:07
…re correct Reverting to array-index keys (previous commit) to fix the CodeRabbit-caught focus-loss regression brought SonarCloud's "no array index in keys" rule (S6479) back on these exact 4 lines. It's a false positive in each case: every affected list only supports append and filter-based removal (or, for StudioInspector's options, an index-preserving swap), holds no per-row local component state, and a content-derived key was the thing that actually broke user-facing behavior. Suppress with NOSONAR plus an explanation pointing at the regression test that would catch a re-break, rather than reintroducing the bug to satisfy the linter.
CodeRabbit's second look at StudioInspector's choice-option list found that index-as-key (the previous commit's fix for the focus-loss bug) still has a correctness problem: keyErrors is also indexed by array position, so removing or reordering an option can leave a duplicate-key error message attached to the wrong row once positions shift. Fix it properly instead of re-suppressing the linter: generate a stable id per option at creation time (not derived from editable content, so it doesn't cause the earlier focus bug either), keep it in lockstep with add/remove/move, and key both the row and its error message off that id. Reset the id set when the selected field itself changes. Drops the now-unneeded NOSONAR suppression on this file — the key is genuinely stable, not index-based, so S6479 no longer applies. Also fixes wording in phase2a-form-designer-compliance-ledger.md #55: the referenced *EvaluatorTests.cs files are automated test suites, not a manual comparison — reworded to "separate automated coverage, no automated parity contract" instead of implying a manual check was performed.
A separate CodeRabbit finding on StudioWorkspace's own versionQuery (not the version-list query already fixed): the isLoading/!history/!schema guard ran before the isError check, but history is only seeded from a successful query, so on failure !history stays permanently true and the earlier guard always wins — the isError branch was dead code and a failed version load showed an endless spinner instead of the error message. Swap the order.
|
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
/forms/designer/new,/forms/designer/:formId) that merges creation, page/section/field design, condition/formula authoring, validation, preview, autosave, and review into a single place.docs/ux-rescue/form-designer-gap-analysis.md: Condition Builder and Formula Builder did not exist anywhere in the UI despite the domain model and evaluation engine already supporting typed conditions/formulas server-side. Both are new, typed, safe (no free-text expressions), with client-side cycle/self-reference detection backed by the server'svalidateVersionas the final authority.useFormDesignerAutosave), undo/redo (historyStore), preview/evaluation engine (previewLogic), and the server'sFormSchemaValidator/FormDependencyGraph. No parallel form engine was built.POST /api/v1/forms/copy-from/{sourceFormId}/{sourceVersionId}) and template preview-before-use (GET /api/v1/form-templates/{id}/schema) — both reuse existingFormVersionService/FormTemplateServicelogic and scope/permission checks, no new services, no migrations./forms/:formId/versions/:versionId/edit) and the confirmed-dead/forms/:formId/versions/newnow redirect into the studio (capability moved first, redirects tested) rather than being deleted outright; every other route disposition is documented indocs/ux-rescue/phase2a-form-designer-route-transition.md.docs/ux-rescue/phase2a-form-designer-compliance-ledger.mdfor the full Verified/Partial/Missing breakdown across all 95 checklist items from the task brief — 8 explicitMissingitems remain (multi-select in canvas, institutional context in preview, formula result-type display, a documented 200-field performance budget, an automated query-count test, an automated accessibility audit, manual screen-reader testing, and a migration policy for renaming a published option key). None are data-integrity or security risks.Test plan
dotnet test Baseera.UnitTests— 990 passed, 0 failed, 0 skippeddotnet test Baseera.IntegrationTests(real SQL Server) — 263 passed, 0 failed, 0 skippednpm run test— 343 passed (63 files), including new coverage for cycle detection, condition/formula builders, validation panel, the full new-form→studio flow, autosave states, the 409 conflict UX (all three recovery options), mobile review-only mode, and route redirectsnpm run typecheck,npm run lint— clean (only pre-existing warnings in unrelated files)npm run build— succeedsnpm run check:ux-routes— passes (65 routes, up from 62, fully reconciled with the inventory doc)npm audit --audit-level=high— 0 vulnerabilitiesbash scripts/check-nuget-vulnerabilities.sh— no High/Criticalgitleaks detect— no leaksgit diff --check— cleanFull details, numbers, and the remaining Phase 2B scope are in
docs/ux-rescue/phase2a-form-designer-completion-report.md.🤖 Generated with Claude Code
Summary by Sourcery
Introduce a unified Form Designer studio that consolidates form creation, editing, validation, preview, and review into new /forms/designer routes, while adding backend support to copy existing forms and preview templates, updating routes, permissions docs, tests, and UX documentation accordingly.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Documentation
Tests