fix(sources): drop empty reader_options before submit (#1648)#1705
fix(sources): drop empty reader_options before submit (#1648)#1705SarthakB11 wants to merge 1 commit into
Conversation
|
@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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe source form now exports a pure ChangesSource configuration cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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
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
| - 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.
| - 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.
| 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.
| 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.
| 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.
|
@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. |
Signed-off-by: Sarthak Bhardwaj <sarthak.bhardwaj21b@iiitg.ac.in>
c60ab5a to
b3d0b6f
Compare
|
Trimmed the PR to just the fix. Reverted the unrelated Prettier reformats CodeRabbit flagged, so the diff is now only SourceForm.tsx and the new cleanConfig test. |
1 participant
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