Skip to content

Conversation

@Amogh-Bharadwaj
Copy link
Contributor

In a query where they are having aliases for columns (i.e column A in source maps to column B in target), the upsert key columns for upsert qrep mode need to column B (i,e the destination columns).
Currently we list source columns in the UI, this PR changes this to allow typing whatever columns are needed

useEffect(() => {
const uniqueColsArr = Array.from(uniqueColumnsSet);
// Parse comma-separated values and update the config
const columnsArray = upsertColumnsText
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can memo this just to not have to repeat the expression so much

Copy link
Contributor

@ilidemi ilidemi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude review

Summary

Changes the upsert key columns selector from a multi-select dropdown (selecting from source columns) to a free-text input field. This allows users to type destination column names when using aliases.

Issues Found

  1. Duplicate parsing logic - The comma-separated column parsing is done 3 times:

    • In handleTextChange
    • In the useEffect
    • In the JSX for the preview display

    Should extract to a helper function or memoize.

  2. Double state update - handleTextChange calls setting.stateHandler() AND the useEffect also updates the setter. This causes redundant updates on every keystroke. The useEffect dependency on upsertColumnsText means both fire.

  3. Inline styles - Uses inline style={{...}} objects instead of the project's existing patterns (e.g., SelectTheme, CSS modules, or styled components).

  4. No input validation feedback - If a user enters invalid column names, there's no way to know until the mirror fails. The old dropdown at least ensured valid source columns.

  5. Removed unused imports - Good cleanup of ReactSelect, SelectTheme, Badge, Icon.

  6. Preview is redundant - The "Columns: ..." preview just echoes back what the user typed with normalized whitespace. Not particularly useful.

Suggested Fix for the Double Update

const handleTextChange = (value: string) => {
  setUpsertColumnsText(value);
  // Remove: setting.stateHandler(columnsArray, setter);
};

Let only the useEffect handle the state update, or remove the useEffect and only use handleTextChange.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants