Fix Primary key selection #1631
Conversation
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CreateConnectionForm
participant Stream
User->>CreateConnectionForm: Toggle incremental sync OFF
CreateConnectionForm->>Stream: Inspect stream.cursorFieldConfig.sourceDefinedCursor
alt sourceDefinedCursor == true
CreateConnectionForm->>Stream: Keep cursorField
else
CreateConnectionForm->>Stream: Clear cursorField
end
CreateConnectionForm->>Stream: Inspect stream.primaryKeyConfig.sourceDefinedPrimaryKey
alt sourceDefinedPrimaryKey == true
CreateConnectionForm->>Stream: Keep primaryKey
else
CreateConnectionForm->>Stream: Clear primaryKey
end
User->>CreateConnectionForm: Change destination sync mode away from append_dedup
CreateConnectionForm->>Stream: Update destinationSyncMode (do not modify primaryKey)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Since when source defined is true, we can't select any other keys as primary key. So now The simple solution is to clear primary keys and cursor if it is not source defined, otherwise it will remain as it is. |
|
@MohitAgrawal16 |
Ok....I will check and fix it. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1631 +/- ##
=======================================
Coverage ? 75.96%
=======================================
Files ? 102
Lines ? 7773
Branches ? 1875
=======================================
Hits ? 5905
Misses ? 1836
Partials ? 32 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/components/Connections/CreateConnectionForm.tsx (1)
158-164: Primary key options for source-defined streams should come from the stream, not from configUsing
el.config.primaryKeyto populate options can leave the Primary Key menu empty (especially in “create” flows), which matches the reported “cannot select primary key” behavior after mode toggles. For source-defined PKs, derive options fromel.stream.sourceDefinedPrimaryKeyand prefer persisted config only to initialize the current selection.Apply this diff to fix the options source and keep behavior consistent for both create and edit:
- if (primaryKeyObj.sourceDefinedPrimaryKey) { - stream.primaryKey = el.config.primaryKey.flat(); - primaryKeyObj.primaryKeyOptions = el.config.primaryKey.flat(); - } else { + if (primaryKeyObj.sourceDefinedPrimaryKey) { + const sdpk = (el.stream.sourceDefinedPrimaryKey || []).flat(); + // Prefer persisted config if available (edit), otherwise use source-defined defaults. + stream.primaryKey = + (el.config.primaryKey && el.config.primaryKey.length > 0) + ? el.config.primaryKey.flat() + : sdpk; + primaryKeyObj.primaryKeyOptions = sdpk; + } else {
🧹 Nitpick comments (2)
src/components/Connections/CreateConnectionForm.tsx (2)
485-505: Align bulk toggle semantics with per-stream toggle for cursor/PK resets
handleIncrementalAllStreamsdoes not mirror the new conditional reset semantics you added insetStreamIncr. Today, turning OFF incremental for all streams leaves user-defined cursor/PK values intact, while turning it off for a single stream clears them. This inconsistency can confuse users and cause the “Incremental All” switch availability to differ based on which control they used.Apply this diff to conditionally clear user-defined values when turning incremental OFF in bulk:
const sourceStreamsSlice: Array<SourceStream> = sourceStreams.map((stream: SourceStream) => { @@ - return { - ...stream, - syncMode: checked ? 'incremental' : 'full_refresh', - destinationSyncMode: destinationMode, - }; + const nextCursorField = + !checked && !stream.cursorFieldConfig.sourceDefinedCursor ? '' : stream.cursorField; + const nextPrimaryKey = + !checked && !stream.primaryKeyConfig.sourceDefinedPrimaryKey ? [] : stream.primaryKey; + + return { + ...stream, + syncMode: checked ? 'incremental' : 'full_refresh', + destinationSyncMode: destinationMode, + cursorField: nextCursorField, + primaryKey: nextPrimaryKey, + }; });
41-44: Typo: rename PrimayKeyConfig to PrimaryKeyConfigMinor but worthwhile cleanup to avoid confusion and improve readability.
-type PrimayKeyConfig = { +type PrimaryKeyConfig = { sourceDefinedPrimaryKey: boolean; primaryKeyOptions: string[]; }; @@ - primaryKeyConfig: PrimayKeyConfig; + primaryKeyConfig: PrimaryKeyConfig;Also applies to: 60-60
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Connections/CreateConnectionForm.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Ishankoradia
PR: DalgoT4D/webapp#1623
File: src/components/Connections/Connections.tsx:816-853
Timestamp: 2025-08-03T14:08:39.068Z
Learning: In src/components/Connections/StreamSelectionDialog.tsx, the confirm button is disabled until at least one stream is selected, preventing users from proceeding with an empty stream selection. This UI validation makes server-side validation for empty selectedStreamsForClear array redundant in the clearSelectedStreams function.
📚 Learning: 2025-08-03T14:08:39.068Z
Learnt from: Ishankoradia
PR: DalgoT4D/webapp#1623
File: src/components/Connections/Connections.tsx:816-853
Timestamp: 2025-08-03T14:08:39.068Z
Learning: In src/components/Connections/StreamSelectionDialog.tsx, the confirm button is disabled until at least one stream is selected, preventing users from proceeding with an empty stream selection. This UI validation makes server-side validation for empty selectedStreamsForClear array redundant in the clearSelectedStreams function.
Applied to files:
src/components/Connections/CreateConnectionForm.tsx
🔇 Additional comments (1)
src/components/Connections/CreateConnectionForm.tsx (1)
441-442: Re-verify the Append ↔ Append/Dedup toggle UX; primary key selectability depends on multiple gatesEven after not clearing
primaryKeyinsetDestinationSyncMode, the Primary Key control is disabled unless ALL conditions are true: stream selected, supports incremental, syncMode is incremental, and destinationSyncMode is append_dedup. Also, if options are empty (see source-defined fix above), the user still can’t select anything.Please validate manually:
- Start with a stream in incremental mode.
- Toggle Destination between Append and Append/Dedup.
- Ensure when in Append/Dedup, the Primary Key menu is enabled and populated (for non source-defined PKs).
- For source-defined PKs, verify the selected keys remain visible and the checkboxes are disabled (by design).
If you still see a disabled or empty menu in Append/Dedup for non source-defined PKs, the options source fix (proposed above) is likely the missing piece.
Also applies to: 708-714, 745-785
| if (!stream.cursorFieldConfig.sourceDefinedCursor) { | ||
| // Reset to empty cursor field if not source-defined | ||
| stream.cursorField = ''; | ||
| } | ||
|
|
||
| if (!stream.primaryKeyConfig.sourceDefinedPrimaryKey) { | ||
| // Reset primary key to empty array if not source-defined | ||
| stream.primaryKey = []; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid mutating React state objects in-place; compute next values and update immutably
Directly mutating stream before calling updateThisStreamTo_ risks subtle bugs and stale renders. Compute the next values and pass them in a single immutable update.
Apply this diff to make the update atomic and immutable:
- if (!checked) {
- if (!stream.cursorFieldConfig.sourceDefinedCursor) {
- // Reset to empty cursor field if not source-defined
- stream.cursorField = '';
- }
-
- if (!stream.primaryKeyConfig.sourceDefinedPrimaryKey) {
- // Reset primary key to empty array if not source-defined
- stream.primaryKey = [];
- }
- }
-
- updateThisStreamTo_(stream, {
- ...stream,
- syncMode: checked ? 'incremental' : 'full_refresh',
- destinationSyncMode: destinationMode,
- });
+ const nextCursorField =
+ !checked && !stream.cursorFieldConfig.sourceDefinedCursor ? '' : stream.cursorField;
+ const nextPrimaryKey =
+ !checked && !stream.primaryKeyConfig.sourceDefinedPrimaryKey ? [] : stream.primaryKey;
+
+ updateThisStreamTo_(stream, {
+ ...stream,
+ syncMode: checked ? 'incremental' : 'full_refresh',
+ destinationSyncMode: destinationMode,
+ cursorField: nextCursorField,
+ primaryKey: nextPrimaryKey,
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!stream.cursorFieldConfig.sourceDefinedCursor) { | |
| // Reset to empty cursor field if not source-defined | |
| stream.cursorField = ''; | |
| } | |
| if (!stream.primaryKeyConfig.sourceDefinedPrimaryKey) { | |
| // Reset primary key to empty array if not source-defined | |
| stream.primaryKey = []; | |
| } | |
| const nextCursorField = | |
| !checked && !stream.cursorFieldConfig.sourceDefinedCursor ? '' : stream.cursorField; | |
| const nextPrimaryKey = | |
| !checked && !stream.primaryKeyConfig.sourceDefinedPrimaryKey ? [] : stream.primaryKey; | |
| updateThisStreamTo_(stream, { | |
| ...stream, | |
| syncMode: checked ? 'incremental' : 'full_refresh', | |
| destinationSyncMode: destinationMode, | |
| cursorField: nextCursorField, | |
| primaryKey: nextPrimaryKey, | |
| }); |
🤖 Prompt for AI Agents
In src/components/Connections/CreateConnectionForm.tsx around lines 423 to 431,
avoid mutating the stream object in-place; instead compute the next stream
object immutably and call updateThisStreamTo_ once with that object. Concretely,
build a newStream = { ...stream, cursorField:
stream.cursorFieldConfig.sourceDefinedCursor ? stream.cursorField : '',
primaryKey: stream.primaryKeyConfig.sourceDefinedPrimaryKey ? stream.primaryKey
: [] } (ensuring any nested objects are spread as needed) and pass newStream to
updateThisStreamTo_ so the update is atomic and no in-place mutation occurs.
2 participants
Fixes: #1628
Here we can solve it by two ways:
1.Either keep the source defined primary key and cursor field always checked.
2. or clear all keys when changing to non-incremental and when again changed to incremental, update primary key and cursor field according to source defined.
I had pushed commits with 1st approach.
Summary by CodeRabbit