warehouse creation bug for postgres#1700
Conversation
WalkthroughAdded runtime guards: a type check using Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/Destinations/Destinations.tsx (1)
99-115:⚠️ Potential issue | 🟡 MinorClear stale warehouse state when the response is empty/invalid.
Good fix on the array guard. One gap remains: if
data.warehousesbecomes empty (or non-array) after previously holding data,warehouseis never reset and stale UI can persist.Suggested fix
useEffect(() => { - if (data && data.warehouses && Array.isArray(data.warehouses) && data.warehouses.length > 0) { + if (data && Array.isArray(data.warehouses) && data.warehouses.length > 0) { const w_house = data.warehouses[0]; setWarehouse({ airbyteWorkspaceId: w_house.airbyte_destination.workspaceId, airbyteDockerRepository: w_house.airbyte_docker_repository, tag: w_house.airbyte_docker_image_tag, destinationId: w_house.airbyte_destination.destinationId, destinationDefinitionId: w_house.airbyte_destination.destinationDefinitionId, name: w_house.name, wtype: w_house.wtype, icon: w_house.airbyte_destination.icon, connectionConfiguration: w_house.airbyte_destination.connectionConfiguration, ssl: w_house.ssl, } as Warehouse); + } else { + setWarehouse(undefined); } }, [data]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Destinations/Destinations.tsx` around lines 99 - 115, The useEffect that sets warehouse from data.warehouses only updates state when a valid array entry exists, so when data.warehouses becomes empty/invalid it leaves stale warehouse state; inside the same useEffect (the one referencing useEffect, data, setWarehouse, and warehouse) add an else branch that clears the warehouse (e.g., setWarehouse(undefined) or null depending on Warehouse state typing) whenever data.warehouses is missing, not an array, or has length === 0 so the UI resets correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/components/Destinations/Destinations.tsx`:
- Around line 99-115: The useEffect that sets warehouse from data.warehouses
only updates state when a valid array entry exists, so when data.warehouses
becomes empty/invalid it leaves stale warehouse state; inside the same useEffect
(the one referencing useEffect, data, setWarehouse, and warehouse) add an else
branch that clears the warehouse (e.g., setWarehouse(undefined) or null
depending on Warehouse state typing) whenever data.warehouses is missing, not an
array, or has length === 0 so the UI resets correctly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/components/Destinations/Destinations.tsx
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1700 +/- ##
=======================================
Coverage 75.33% 75.33%
=======================================
Files 107 107
Lines 7962 7962
Branches 1866 1866
=======================================
Hits 5998 5998
Misses 1927 1927
Partials 37 37 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/helpers/ConnectorConfigInput.tsx`:
- Around line 138-141: The code uses `exclude[0] in data?.properties` which can
throw if `data?.properties` is undefined and uses `||` losing valid falsy
discriminators; guard the `in` check by first verifying `data?.properties` is
non-null (e.g., `if (data?.properties && exclude[0] in data.properties)`) and
replace the `||` with nullish coalescing `??` when computing the discriminator
(e.g., compute `const val = data.properties[exclude[0]]?.const ??
data.properties[exclude[0]]?.enum?.[0]` and then `dropdownEnums.push(val)`),
using the existing symbols `exclude`, `data?.properties`, and `dropdownEnums`.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/helpers/ConnectorConfigInput.tsx
| if (exclude[0] in data?.properties) { | ||
| dropdownEnums.push( | ||
| data?.properties[exclude[0]]?.const || data?.properties[exclude[0]]?.enum[0] | ||
| data?.properties[exclude[0]]?.const || data?.properties[exclude[0]]?.enum?.[0] | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the file and the specific lines mentioned
cat -n src/helpers/ConnectorConfigInput.tsx | sed -n '130,150p'Repository: DalgoT4D/webapp
Length of output: 857
🏁 Script executed:
# Let's search for falsy const values in schema files
rg -nP '"const"\s*:\s*(false|0|""|null)' --type json --type yamlRepository: DalgoT4D/webapp
Length of output: 41
🏁 Script executed:
# Check for similar patterns with `in` operator and optional chaining on data.properties
rg -nP 'in\s+data\?\.properties' --type ts --type tsxRepository: DalgoT4D/webapp
Length of output: 86
🏁 Script executed:
# Look for patterns using || with .const or .enum
rg -nP '\.const\s*\|\|' --type ts --type tsx -A 2Repository: DalgoT4D/webapp
Length of output: 86
🏁 Script executed:
# Get broader context around lines 138-141 to understand the function
cat -n src/helpers/ConnectorConfigInput.tsx | sed -n '120,160p'Repository: DalgoT4D/webapp
Length of output: 1527
🏁 Script executed:
# Search for falsy const values in all files (broader search)
rg '"const"\s*:\s*(false|0|""|null)' -g '*.json' -g '*.yaml' -g '*.yml'Repository: DalgoT4D/webapp
Length of output: 41
🏁 Script executed:
# Search for const || patterns in TypeScript/TSX files
fd -e ts -e tsx | xargs rg '\.const\s*\|\|' -A 2Repository: DalgoT4D/webapp
Length of output: 277
🏁 Script executed:
# Search for `in` operator used with data or object properties
rg 'in\s+\w+\?' -A 1 -B 1 | head -50Repository: DalgoT4D/webapp
Length of output: 1242
🏁 Script executed:
# Check what data?.properties actually represents by looking at the broader function context
cat -n src/helpers/ConnectorConfigInput.tsx | sed -n '129,200p'Repository: DalgoT4D/webapp
Length of output: 3041
🏁 Script executed:
# Look for where traverseSpecs is called to understand what data is passed
rg 'traverseSpecs\(' src/helpers/ConnectorConfigInput.tsx -B 2 -A 2Repository: DalgoT4D/webapp
Length of output: 880
Guard the in operator check and use nullish coalescing instead of logical OR.
Line 138: exclude[0] in data?.properties can throw a TypeError when data?.properties is undefined. The in operator requires a non-null object on the right-hand side, and optional chaining returns undefined if data is null or undefined.
Line 140: Using || instead of ?? means valid falsy const discriminators (e.g., 0, false, '') would incorrectly fall back to enum?.[0].
Suggested fix
- if (exclude[0] in data?.properties) {
- dropdownEnums.push(
- data?.properties[exclude[0]]?.const || data?.properties[exclude[0]]?.enum?.[0]
- );
- }
+ const properties = data?.properties;
+ if (properties && exclude[0] in properties) {
+ dropdownEnums.push(
+ properties[exclude[0]]?.const ?? properties[exclude[0]]?.enum?.[0]
+ );
+ }📝 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 (exclude[0] in data?.properties) { | |
| dropdownEnums.push( | |
| data?.properties[exclude[0]]?.const || data?.properties[exclude[0]]?.enum[0] | |
| data?.properties[exclude[0]]?.const || data?.properties[exclude[0]]?.enum?.[0] | |
| ); | |
| const properties = data?.properties; | |
| if (properties && exclude[0] in properties) { | |
| dropdownEnums.push( | |
| properties[exclude[0]]?.const ?? properties[exclude[0]]?.enum?.[0] | |
| ); | |
| } |
🧰 Tools
🪛 Biome (2.4.4)
[error] 138-138: Unsafe usage of optional chaining.
(lint/correctness/noUnsafeOptionalChaining)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/helpers/ConnectorConfigInput.tsx` around lines 138 - 141, The code uses
`exclude[0] in data?.properties` which can throw if `data?.properties` is
undefined and uses `||` losing valid falsy discriminators; guard the `in` check
by first verifying `data?.properties` is non-null (e.g., `if (data?.properties
&& exclude[0] in data.properties)`) and replace the `||` with nullish coalescing
`??` when computing the discriminator (e.g., compute `const val =
data.properties[exclude[0]]?.const ?? data.properties[exclude[0]]?.enum?.[0]`
and then `dropdownEnums.push(val)`), using the existing symbols `exclude`,
`data?.properties`, and `dropdownEnums`.
1 participant
Summary by CodeRabbit