Skip to content

scroll form when the error happens (DP)#25760

Merged
siddhant1 merged 4 commits intomainfrom
sid/dp-fixes
Feb 9, 2026
Merged

scroll form when the error happens (DP)#25760
siddhant1 merged 4 commits intomainfrom
sid/dp-fixes

Conversation

@siddhant1
Copy link
Member

@siddhant1 siddhant1 commented Feb 9, 2026

Describe your changes:

Fixes

I worked on ... because ...

Type of change:

  • Bug fix
  • Improvement
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: I updated the migration scripts or explained why it is not needed.

Summary by Gitar

  • Fixed form validation UX:
    • Added automatic scroll-to-error in useFormDrawerWithRef when validation fails
  • New prop:
    • className in DrawerBodyConfig enables targeting scroll containers for error fields
  • Editor styling refactor:
    • Consolidated .block-editor-wrapper styles from add-domain-form.less to global form.less
  • Editor configuration:
    • Set max-height 120px and standardized toolbar styling across all rich text editors

This will update automatically on new commits.


@github-actions
Copy link
Contributor

github-actions bot commented Feb 9, 2026

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

@github-actions
Copy link
Contributor

github-actions bot commented Feb 9, 2026

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

@gitar-bot
Copy link

gitar-bot bot commented Feb 9, 2026

Code Review ✅ Approved 3 resolved / 3 findings

Clean PR that adds scroll-to-error functionality for drawer forms and consolidates editor styling. All initial review findings (className override, error swallowing, unscoped styles) were properly addressed in follow-up commits. The final code is well-structured with correct className merging, proper error handling separation, and scoped CSS.

✅ 3 resolved
Bug: Global .block-editor-wrapper styles may break non-form editors

📄 openmetadata-ui/src/main/resources/ui/src/styles/components/form.less:141
The new global rule .block-editor-wrapper .om-block-editor { max-height: 120px; padding: @size-sm; } in form.less applies to all block editors application-wide, not just those in forms.

This could cause visual regressions in:

  • Bubble-menu editors (.block-editor-wrapper--bubble-menu): Used for inline description editing — these have no max-height by design, and 120px would severely truncate them.
  • Non-bar-menu read-only editors: Task diff views, description previews, and other places where the editor needs to expand to fit content.
  • Padding conflicts: The bar-menu variant explicitly sets padding-left: 24px; padding-right: 24px; padding-top: 8px, and this new rule adds padding: @size-sm which could override those values depending on CSS load order.

Additionally, the existing p:last-child { padding-bottom: 30vh } in block-editor.less (line 28) gets effectively overridden by p:last-child { padding-bottom: 75px } here (same specificity, load order wins).

Suggested fix: Scope the new styles to form contexts only, e.g.:

.new-form-style .block-editor-wrapper {
  .om-block-editor {
    max-height: 120px;
    padding: @size-sm;
    p:last-child {
      padding-bottom: 75px;
    }
  }
  .bar-menu-wrapper {
    padding: @size-xxs;
    .bar-menu-wrapper--format--button--icon {
      width: @size-mlg;
      height: @size-mlg;
    }
  }
}

This ensures only editors within .new-form-style forms are affected.

Bug: ...body spread can override className, breaking scroll-to-error

📄 openmetadata-ui/src/main/resources/ui/src/components/common/atoms/drawer/useFormDrawer.tsx:114
In useFormDrawer, the body config is constructed as:

body: {
  children: form,
  loading: loading || isSubmitting,
  className: 'drawer-form-content',
  ...body,   // <-- consumer's body config spreads AFTER className
},

Since ...body comes after className: 'drawer-form-content', if any consumer passes body: { className: 'my-class' }, it will silently override the drawer-form-content class. This would break the scroll-to-error functionality because createScrollToErrorHandler() defaults to querying .drawer-form-content.

Suggested fix: Place ...body before the className, or merge the classNames:

body: {
  ...body,
  children: form,
  loading: loading || isSubmitting,
  className: clsx('drawer-form-content', body?.className),
},

This ensures the critical drawer-form-content class is always present while allowing consumers to add additional classes.

Edge Case: Catch block swallows all errors, not just validation failures

📄 openmetadata-ui/src/main/resources/ui/src/components/common/atoms/drawer/useFormDrawer.tsx:199
The try-catch in handleSubmit catches all exceptions from formRef.validateFields() and only calls scrollToError() without re-throwing:

try {
  const values = await formRef.validateFields();
  if (formRef?.submit) {
    formRef.submit();
  } else {
    await onSubmit(values);
  }
} catch {
  scrollToError();
}

If validateFields() or onSubmit() throws a non-validation error (e.g., a network error from onSubmit, or an unexpected runtime error), it will be silently swallowed and only scrollToError() is called—which won't find any error elements and will do nothing.

Suggested fix: Re-throw errors that aren't validation failures, or separate the validation try-catch from the submit call:

try {
  const values = await formRef.validateFields();
  // Move submit outside the try-catch, or use a separate try-catch
  if (formRef?.submit) {
    formRef.submit();
  } else {
    await onSubmit(values);
  }
} catch (error) {
  scrollToError();
  // Optionally check if it's a validation error before swallowing
  // Ant Design's validateFields rejects with { errorFields, values }
}

In practice, this is mitigated because formRef.submit() typically handles its own error flow and onSubmit errors may be handled by the caller. But it's worth being explicit about which errors are expected.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@github-actions
Copy link
Contributor

github-actions bot commented Feb 9, 2026

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 65%
65.83% (56038/85119) 45.18% (29339/64931) 47.95% (8849/18455)

@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 9, 2026

@siddhant1 siddhant1 enabled auto-merge (squash) February 9, 2026 12:16
@siddhant1 siddhant1 merged commit 567e914 into main Feb 9, 2026
28 of 29 checks passed
@siddhant1 siddhant1 deleted the sid/dp-fixes branch February 9, 2026 12:46
siddhant1 added a commit that referenced this pull request Feb 9, 2026
* scroll when the error happens

* description editor update

* fix errors

* locate

---------

Co-authored-by: Siddhant <siddhant@MacBook-Pro.local>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

safe to test Add this label to run secure Github workflows on PRs UI UI specific issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants