Skip to content

feat(webui): add SQL syntax highlighting to metric form editors#1341

Open
mohamadyasser118 wants to merge 2 commits intocybertec-postgresql:masterfrom
mohamadyasser118:feat/sql-editor-syntax-highlighting
Open

feat(webui): add SQL syntax highlighting to metric form editors#1341
mohamadyasser118 wants to merge 2 commits intocybertec-postgresql:masterfrom
mohamadyasser118:feat/sql-editor-syntax-highlighting

Conversation

@mohamadyasser118
Copy link
Copy Markdown
Contributor

As a follow-up to the web improvements and SQL display, I replaced plain OutlinedInput fields with react-simple-code-editor and prismjs for SQLs and InitSQL fields in the metric form. I see that this makes it easy to write SQLs.

Changes:

  • SQLs field: full editor and syntax highlighting
  • InitSQL field: same editor with smaller min-height
  • I Used react-simple-code-editor (lightweight) over CodeMirror
    to avoid unnecessary bundle size increase
  • Integrated with react-hook-form via Controller

SQLs Before:

before

SQLs After:

after

initSQL Before:

initSQL before

initSQL After:

initSQL after

AI & Automation Policy

  • I am the human author and take full personal responsibility for every change in this PR.
  • No AI or automated generative tool was used in any part of this PR OR I have disclosed all tool(s) below.
    Drafted and reviewed with the assistance Cluade Sonnet 4.6

Checklist

  • Code compiles and existing tests pass locally.
  • New or updated tests are included where applicable.
  • Documentation is updated where applicable.

@pashagolub
Copy link
Copy Markdown
Collaborator

PR Review: feat(webui): add SQL syntax highlighting to metric form editors

Summary

The intent is good — replacing plain OutlinedInput with a syntax-highlighted editor for SQL fields. However, there are several correctness, accessibility, and architecture issues that need to be addressed.


Critical Issues

1. Duplicate imports from react-hook-form

In both changed files, Controller and useFormContext are imported in two separate statements from the same package:

// MetricFormStepSQL.tsx
import { useFormContext } from "react-hook-form";   // line 2
import { Controller } from "react-hook-form";        // line 9  ← duplicate

These must be merged into one import:

import { useFormContext, Controller } from "react-hook-form";

2. field.onBlur and field.ref are not wired

When using Controller, the field object exposes { value, onChange, onBlur, ref, name }. Only value and onChange are passed to <Editor>. Missing onBlur means validation-on-blur won't trigger. Missing ref means react-hook-form can't programmatically focus the field on validation errors. Fix:

render={({ field }) => (
  <Editor
    value={field.value ?? ""}
    onValueChange={field.onChange}
    onBlur={field.onBlur}  // ← add this
    // Editor doesn't accept ref directly; wrap in a div with ref={field.ref} if needed
    ...
  />
)}

3. Missing aria-describedby on the Editor — accessibility regression

The original OutlinedInput had aria-describedby="SQLs-error" so screen readers associated the error message with the input. The new Editor drops this. The react-simple-code-editor passes unknown props down to its internal <textarea>, so this works:

<Editor
  textareaProps={{ "aria-describedby": "SQLs-error" }}
  ...
/>

Architecture Issues

4. Duplicate side-effect imports

Both MetricFormStepSQL.tsx and MetricFormStepSettings.tsx have:

import "prismjs/components/prism-sql";
import "prismjs/themes/prism.css";

Global CSS and language registrations should be imported once — in main.tsx or a shared setup file. Importing them in two component files is redundant and can cause subtle ordering issues with CSS.

5. The project already has react-syntax-highlighter which bundles prismjs

Adding prismjs as a direct dependency when react-syntax-highlighter (already in package.json) already ships its own prism bundle risks version drift and duplicate code in the bundle. The PR description mentions avoiding bundle size increase, but this actually adds it. Consider extracting the prism reference from react-syntax-highlighter's dependency or auditing the actual bundle impact.


UX / Theming Issues

6. Hardcoded colors break dark mode and MUI theming

// InputLabel
sx={{ backgroundColor: "white", px: 0.5 }}

// Editor styles
backgroundColor: "#fafafa",
color: "#333",
border: hasError ? "1px solid #d32f2f" : "1px solid rgba(0,0,0,0.23)",

These hardcoded values won't adapt to MUI's theme (dark mode, custom palettes, high-contrast). They should use theme tokens:

// Use MUI's useTheme() or sx prop with theme callback
backgroundColor: theme.palette.background.paper,
color: theme.palette.text.primary,
border: hasError
  ? `1px solid ${theme.palette.error.main}`
  : `1px solid ${theme.palette.divider}`,

7. shrink is permanently true on the InputLabel

The original label floated naturally — up when the field had content, inline when empty. The PR forces shrink unconditionally, losing this behavior. While necessary since Editor isn't an MUI input and can't signal focus/filled state, the backgroundColor: "white" hack to cover the border is fragile. Consider using a fieldset/legend approach, or wrapping the editor in an MUI OutlinedInput-like container using the inputComponent slot.


Minor Issues

8. value ?? "" is unnecessary for SQLs

SQLs is typed as string (non-nullable) in MetricFormValues, so the ?? "" fallback is dead code. It's correct for InitSQL (typed string | null | undefined), but not for SQLs.

9. InitSQL field lacks error state display

The SQLs field properly passes error={hasError} to FormControl and renders a <FormHelperText>. The InitSQL field in MetricFormStepSettings.tsx does neither — the original code also didn't, but since this PR restructures the field anyway, it should be made consistent.

10. Prism's prism.css light theme is globally injected

prismjs/themes/prism.css adds global styles for a light syntax theme. If the app ever supports dark mode, this will look inconsistent. A scoped CSS-in-JS approach (e.g., via a sx prop or styled) would be more maintainable.


Overall

The feature is welcome but the implementation has correctness gaps (missing onBlur/ref, broken accessibility) and architectural concerns (duplicate imports, hardcoded theme values, redundant dependencies). Not ready for merging. Your Claude Sonnet 4.6

@pashagolub pashagolub force-pushed the feat/sql-editor-syntax-highlighting branch from 7ed1a40 to 1bd5f6b Compare April 29, 2026 15:31
@pashagolub pashagolub self-assigned this Apr 29, 2026
@pashagolub pashagolub added the wontfix This will not be worked on label Apr 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

wontfix This will not be worked on

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants