fix(sources): drop empty reader_options before submit (#1648)#1705
Open
SarthakB11 wants to merge 2 commits into
Open
fix(sources): drop empty reader_options before submit (#1648)#1705SarthakB11 wants to merge 2 commits into
SarthakB11 wants to merge 2 commits into
Conversation
When adding an Excel (or other File-based) source in Dalgo, submitting
the form without typing anything into reader_options produced:
Error: Failed to check the source connection: Field 'reader_options'
is not valid JSON object.
Airbyte's File source declares reader_options as a JSON-encoded string
field with no default; react-hook-form therefore serialized the empty
input as '' which the Dalgo backend then tried to parse as JSON and
rejected. The documented workaround was to manually type '{}'.
This patch extends cleanConfig() with the same strip-on-empty pattern
already used for start_date: if reader_options comes through as an
empty string, drop it from the payload so the backend falls back to
its own default.
Fixes DalgoT4D#1648
|
@SarthakB11 is attempting to deploy a commit to the support-9965's projects Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughMoved Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
🧹 Nitpick comments (1)
src/components/Sources/SourceForm.tsx (1)
286-309: Add a small regression test forcleanConfigbehavior.Given this was a production bug, please add a focused test that asserts
reader_options: ''is removed while valid non-empty values remain unchanged.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Sources/SourceForm.tsx` around lines 286 - 309, Add a focused unit test for the cleanConfig helper to prevent regressions: import cleanConfig from SourceForm, then assert that when called with { reader_options: '' } the returned object does not have reader_options, while { reader_options: '{"foo":1}' } is preserved unchanged; also include assertions that start_date values of '', null, and undefined are removed but valid start_date strings remain. Use the project's Jest/React Testing setup and give the test a clear name like "cleanConfig removes empty reader_options and empty start_date but preserves valid values".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/Sources/SourceForm.tsx`:
- Around line 286-309: Add a focused unit test for the cleanConfig helper to
prevent regressions: import cleanConfig from SourceForm, then assert that when
called with { reader_options: '' } the returned object does not have
reader_options, while { reader_options: '{"foo":1}' } is preserved unchanged;
also include assertions that start_date values of '', null, and undefined are
removed but valid start_date strings remain. Use the project's Jest/React
Testing setup and give the test a clear name like "cleanConfig removes empty
reader_options and empty start_date but preserves valid values".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 568fe147-b43b-4b11-bfe9-e689c1b3031c
📒 Files selected for processing (1)
src/components/Sources/SourceForm.tsx
Move cleanConfig out of the SourceForm component body to module scope since it's a pure function with no state deps. Export it for direct testing. 8 test cases covering reader_options strip, start_date strip (empty/null/undefined), valid-value preservation, input immutability, and passthrough of unrelated fields.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/components/Sources/SourceForm.tsx (1)
55-56: Harden empty-check for whitespace-onlyreader_options.Consider treating trimmed-empty strings as empty too, to avoid accidental
" "submissions causing the same backend rejection.♻️ Suggested patch
- if (cleaned.reader_options === '') { + if ( + typeof cleaned.reader_options === 'string' && + cleaned.reader_options.trim() === '' + ) { delete cleaned.reader_options; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Sources/SourceForm.tsx` around lines 55 - 56, The empty-check for reader_options should treat whitespace-only strings as empty; update the condition around cleaned.reader_options in SourceForm.tsx (the block that currently reads "if (cleaned.reader_options === '') { delete cleaned.reader_options; }") to first confirm it's a string and then check trimmed value (e.g., use cleaned.reader_options.trim() === '') so whitespace-only values are deleted as well; also ensure you guard against null/undefined before calling trim.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/ISSUE_TEMPLATE/bug_report.md:
- Around line 34-37: Normalize example values for the template fields "Device",
"OS", "Browser", and "Version" to use spaced, more readable forms (e.g., change
"iPhone6" to "iPhone 6", "iOS8.1" to "iOS 8.1", and "safari" to "Safari") so the
examples are consistent and clearer; update the example text for the "Device",
"OS", "Browser", and "Version" lines accordingly and keep capitalization
consistent.
In `@README.md`:
- Line 60: Update the README sentence that currently reads "The env varibales
will be picked up from the .env file during the yarn build stage. This will copy
all the env variables starting with NEXT*PUBLIC* (that get embedd into the
javascript code). The env varibales starting without NEXT*PUBLIC* will be picked
up during run time." by correcting the three typos: change both occurrences of
"varibales" to "variables" and change "embedd" to "embedded" so the sentence
uses "variables" and "embedded" consistently.
In `@src/components/Notifications/Notifications.module.css`:
- Around line 11-12: In Notifications.module.css remove the redundant duplicate
declaration of border-bottom (the second "border-bottom: 15px solid `#fff`;") so
only a single border-bottom rule remains; edit the CSS in
Notifications.module.css to delete the duplicate line associated with the
border-bottom property.
---
Nitpick comments:
In `@src/components/Sources/SourceForm.tsx`:
- Around line 55-56: The empty-check for reader_options should treat
whitespace-only strings as empty; update the condition around
cleaned.reader_options in SourceForm.tsx (the block that currently reads "if
(cleaned.reader_options === '') { delete cleaned.reader_options; }") to first
confirm it's a string and then check trimmed value (e.g., use
cleaned.reader_options.trim() === '') so whitespace-only values are deleted as
well; also ensure you guard against null/undefined before calling trim.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 79c06cca-f3d8-48e1-a378-f580435380db
📒 Files selected for processing (16)
.github/ISSUE_TEMPLATE/bug_report.md.github/ISSUE_TEMPLATE/c4gt.md.github/ISSUE_TEMPLATE/feature_request.md.github/ISSUE_TEMPLATE/task.mdCODE_OF_CONDUCT.mdREADME.mdcypress/support/component-index.htmljest.config.tsnext.config.jssentry.client.config.tssentry.edge.config.tssentry.server.config.tssrc/components/Notifications/Notifications.module.csssrc/components/Sources/SourceForm.tsxsrc/components/Sources/__tests__/cleanConfig.test.tstsconfig.json
💤 Files with no reviewable changes (2)
- .github/ISSUE_TEMPLATE/feature_request.md
- .github/ISSUE_TEMPLATE/task.md
✅ Files skipped from review due to trivial changes (6)
- tsconfig.json
- sentry.client.config.ts
- sentry.edge.config.ts
- jest.config.ts
- sentry.server.config.ts
- cypress/support/component-index.html
Comment on lines
+34
to
+37
| - Device: [e.g. iPhone6] | ||
| - OS: [e.g. iOS8.1] | ||
| - Browser [e.g. stock browser, safari] | ||
| - Version [e.g. 22] |
There was a problem hiding this comment.
Normalize example device/OS wording for consistency.
Minor docs quality nit: examples like iPhone6 / iOS8.1 are less readable than spaced forms.
✏️ Suggested wording tweak
-- Device: [e.g. iPhone6]
-- OS: [e.g. iOS8.1]
+- Device: [e.g. iPhone 6]
+- OS: [e.g. iOS 8.1]📝 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.
Suggested change
| - Device: [e.g. iPhone6] | |
| - OS: [e.g. iOS8.1] | |
| - Browser [e.g. stock browser, safari] | |
| - Version [e.g. 22] | |
| - Device: [e.g. iPhone 6] | |
| - OS: [e.g. iOS 8.1] | |
| - Browser [e.g. stock browser, safari] | |
| - Version [e.g. 22] |
🧰 Tools
🪛 LanguageTool
[grammar] ~34-~34: Ensure spelling is correct
Context: ...following information):** - Device: [e.g. iPhone6] - OS: [e.g. iOS8.1] - Browser [e.g. st...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/ISSUE_TEMPLATE/bug_report.md around lines 34 - 37, Normalize example
values for the template fields "Device", "OS", "Browser", and "Version" to use
spaced, more readable forms (e.g., change "iPhone6" to "iPhone 6", "iOS8.1" to
"iOS 8.1", and "safari" to "Safari") so the examples are consistent and clearer;
update the example text for the "Device", "OS", "Browser", and "Version" lines
accordingly and keep capitalization consistent.
|
|
||
| The env varibales will be picked up from the .env file during the yarn build stage. This will copy all the env variables starting with NEXT_PUBLIC_ (that get embedd into the javascript code). The env varibales starting without NEXT_PUBLIC_ will be picked up during run time. | ||
| Run the script: | ||
| The env varibales will be picked up from the .env file during the yarn build stage. This will copy all the env variables starting with NEXT*PUBLIC* (that get embedd into the javascript code). The env varibales starting without NEXT*PUBLIC* will be picked up during run time. |
There was a problem hiding this comment.
Fix spelling errors in documentation.
The documentation contains three spelling errors:
- "varibales" should be "variables" (appears twice)
- "embedd" should be "embedded"
📝 Proposed fix for spelling errors
-The env varibales will be picked up from the .env file during the yarn build stage. This will copy all the env variables starting with NEXT*PUBLIC* (that get embedd into the javascript code). The env varibales starting without NEXT*PUBLIC* will be picked up during run time.
+The env variables will be picked up from the .env file during the yarn build stage. This will copy all the env variables starting with NEXT*PUBLIC* (that get embedded into the javascript code). The env variables starting without NEXT*PUBLIC* will be picked up during run time.📝 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.
Suggested change
| The env varibales will be picked up from the .env file during the yarn build stage. This will copy all the env variables starting with NEXT*PUBLIC* (that get embedd into the javascript code). The env varibales starting without NEXT*PUBLIC* will be picked up during run time. | |
| The env variables will be picked up from the .env file during the yarn build stage. This will copy all the env variables starting with NEXT*PUBLIC* (that get embedded into the javascript code). The env variables starting without NEXT*PUBLIC* will be picked up during run time. |
🧰 Tools
🪛 LanguageTool
[grammar] ~60-~60: Ensure spelling is correct
Context: ...Step 2: Build the Docker image The env varibales will be picked up from the .env file du...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~60-~60: Ensure spelling is correct
Context: ...es starting with NEXTPUBLIC (that get embedd into the javascript code). The env vari...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~60-~60: Ensure spelling is correct
Context: ...bedd into the javascript code). The env varibales starting without NEXTPUBLIC will be p...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 60, Update the README sentence that currently reads "The
env varibales will be picked up from the .env file during the yarn build stage.
This will copy all the env variables starting with NEXT*PUBLIC* (that get embedd
into the javascript code). The env varibales starting without NEXT*PUBLIC* will
be picked up during run time." by correcting the three typos: change both
occurrences of "varibales" to "variables" and change "embedd" to "embedded" so
the sentence uses "variables" and "embedded" consistently.
Comment on lines
+11
to
+12
| border-bottom: 15px solid #fff; | ||
| border-bottom: 15px solid #fff; |
There was a problem hiding this comment.
Remove the duplicate border-bottom declaration.
Line 12 duplicates the border-bottom property from line 11. The second declaration is redundant and should be removed.
🧹 Proposed fix
border-bottom: 15px solid `#fff`;
- border-bottom: 15px solid `#fff`;
border-left: 15px solid transparent;📝 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.
Suggested change
| border-bottom: 15px solid #fff; | |
| border-bottom: 15px solid #fff; | |
| border-bottom: 15px solid `#fff`; | |
| border-left: 15px solid transparent; |
🧰 Tools
🪛 Stylelint (17.6.0)
[error] 11-11: Unexpected duplicate "border-bottom" (declaration-block-no-duplicate-properties)
(declaration-block-no-duplicate-properties)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Notifications/Notifications.module.css` around lines 11 - 12,
In Notifications.module.css remove the redundant duplicate declaration of
border-bottom (the second "border-bottom: 15px solid `#fff`;") so only a single
border-bottom rule remains; edit the CSS in Notifications.module.css to delete
the duplicate line associated with the border-bottom property.
Author
|
@himanshudube97 @Ishankoradia — friendly bump when one of you has a moment. Small fix for #1648 (DAL-661); CI green and CodeRabbit's nits are on unrelated formatter-touched files. Happy to iterate. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1648.
Symptom
When a user adds an Excel (or other File-type) source in Dalgo without typing anything into the `reader_options` field, form submission fails with:
The documented workaround on the issue is to manually type `{}` into the field.
Cause
Airbyte's File source declares `reader_options` as a JSON-encoded string field with no default. react-hook-form serializes an untouched input as `''`, which the Dalgo backend then tries to parse as JSON and rejects.
Fix
Extend `cleanConfig()` in `src/components/Sources/SourceForm.tsx` with the same strip-on-empty pattern already used for `start_date`: if `reader_options` comes through as an empty string, delete it from the payload before sending, so the backend falls back to its own default (equivalent to `{}`).
```diff
```
`cleanConfig` is called on both the check-connection WebSocket payload (`onSubmit`) and the create/update HTTP payload (`handleSaveSource`), so both paths are covered.
Scope
Verification
Summary by CodeRabbit