Fix the "null" string in the station update form #1074
Conversation
✅ Deploy Preview for antenna-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Warning Rate limit exceeded@mihow has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 5 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughUpdated utilities and a model: functions now treat null like undefined when serializing or adding query params; two deployment model fields and their getters were changed to allow Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where the string "null" was appearing in the station update form by adding null checks alongside existing undefined checks in three utility functions.
Key Changes:
- Added
&& value !== nullconditions to prevent null values from being converted to the string "null" - Applied the fix consistently across URL parameter handling, form data conversion, and boolean-to-string conversion functions
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
ui/src/utils/getAppRoute.ts |
Adds null check when setting URL search parameters to prevent "null" strings in filters |
ui/src/data-services/hooks/deployments/utils.ts |
Adds null check when appending deployment field values to FormData to prevent "null" strings in form submissions |
ui/src/components/filtering/utils.ts |
Adds null check in boolean-to-string conversion to return empty string instead of "null" |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) | ||
| Object.entries(filters).forEach(([name, value]) => { | ||
| if (value !== undefined) { | ||
| if (value !== undefined && value !== null) { |
There was a problem hiding this comment.
The type definition for filters specifies Partial<Record<FilterType, string | undefined>> which doesn't include null. If null values are possible at runtime (as this fix suggests), the type should be updated to Partial<Record<FilterType, string | undefined | null>> to accurately reflect the possible values and improve type safety.
| } | ||
| } | ||
|
|
||
| export const booleanToString = (value?: boolean) => |
There was a problem hiding this comment.
The parameter type is value?: boolean which doesn't include null. If null values are possible at runtime (as this fix suggests), the type should be updated to value?: boolean | null to accurately reflect the possible values and improve type safety.
| export const booleanToString = (value?: boolean) => | |
| export const booleanToString = (value?: boolean | null) => |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ui/src/utils/getAppRoute.ts (1)
29-31: Consider aligningfilterstype with newnullhandlingThe extra
value !== nullguard is good for avoiding"null"in the query string. Ifnullis now a legitimate “no value” signal for filters, you may want to reflect that in the type:
- Change
filters?: Partial<Record<FilterType, string | undefined>>to allowstring | null | undefined, or- Drop the
nullcheck ifnullshould never appear infilters.Not blocking, but aligning the type with runtime behavior will make this intent clearer and catch misuses at compile time.
ui/src/components/filtering/utils.ts (1)
15-16: UpdatebooleanToStringparameter type to matchnullhandlingThe added
value !== nullcheck correctly preventsnullfrom being stringified and aligns with the rest of the PR. To keep the type signature in sync with this behavior, consider widening the parameter type, e.g.:
export const booleanToString = (value?: boolean | null) => ...
orexport const booleanToString = (value: boolean | null | undefined) => ...so that callers and the type system both acknowledge
nullas an expected input.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ui/src/components/filtering/utils.ts(1 hunks)ui/src/data-services/hooks/deployments/utils.ts(1 hunks)ui/src/utils/getAppRoute.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Upload results
- GitHub Check: Redirect rules
- GitHub Check: Header rules
- GitHub Check: Pages changed
🔇 Additional comments (1)
ui/src/data-services/hooks/deployments/utils.ts (1)
17-19: Null guard correctly prevents"null"from being sent in form dataThe added
value !== nullcheck here is consistent with treatingnullas "no value" and avoids stringifying it, while still allowing valid falsy values like0orfalseto be appended.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ui/src/data-services/models/deployment-details.ts (2)
71-73: Consider normalizingundefinedtonullindataSourceSubdirDepending on how
ServerDeploymentDetailsis shaped,this._deployment.data_source_subdirmight beundefinedwhen absent. Since the getter’s return type isstring | null, you could normalize at the boundary to avoid ever returningundefined:- get dataSourceSubdir(): string | null { - return this._deployment.data_source_subdir - } + get dataSourceSubdir(): string | null { + return this._deployment.data_source_subdir ?? null + }This keeps the public API aligned with the annotated type and consistently treats “missing” as
null.
75-77: Apply the same undefined→null normalization fordataSourceRegexFor symmetry with
dataSourceSubdirand to avoid returningundefinedfrom astring | nullgetter, you can also coalesce here:- get dataSourceRegex(): string | null { - return this._deployment.data_source_regex - } + get dataSourceRegex(): string | null { + return this._deployment.data_source_regex ?? null + }This keeps the null-handling semantics consistent across both fields.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ui/src/data-services/hooks/deployments/utils.ts(1 hunks)ui/src/data-services/models/deployment-details.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ui/src/data-services/hooks/deployments/utils.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Redirect rules
- GitHub Check: Header rules
- GitHub Check: Pages changed
🔇 Additional comments (1)
ui/src/data-services/models/deployment-details.ts (1)
14-15: Nullable data source fields look correctAllowing
dataSourceSubdiranddataSourceRegexto bestring | null(while still optional) matches the intent of supporting realnullvalues from the backend/UI and should help avoid accidental'null'stringification down the line. No issues from a typing perspective here.
Summary
The
data_source_regexfield in the Station / Deployment form often gets saved with the string "null". This PR ensures the frontend keeps nulls asnullrather than converting it to a string. A future enhancement could be to post/patch JSON data instead of form data -- for fields other than the image field. We also need to ensure "clearing" the field works so that PATCH sendsnullinstead of omitting the field from the request.Important: It's unclear when this happens! We are not able to manually reproduce the issue, but it happens regularly. I have updated all existing deployments in the production database that contained "null" in this field (48 in total).
Screenshots
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.