ECOPROJECT-4393 | fix: populate existing network configuration when editing environment#571
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 11 minutes and 58 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR introduces network configuration support via a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/ui/environment/views/DiscoverySourceSetupModal.tsx (1)
402-413: Move edit-form hydration mapping out of the view layer.This block performs business/data-mapping logic in a
viewscomponent. Prefer moving this transformation into the environment VM (or a dedicated hook) and keep the view focused on render + state wiring.As per coding guidelines,
Views in src/ui/*/views/ must render only with no business logic, calling vm hook at top.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/environment/views/DiscoverySourceSetupModal.tsx` around lines 402 - 413, The view DiscoverySourceSetupModal is doing business/data-mapping (calling getNetworkConfig and building the initialValues object, then calling setNetworkConfigType/setIpAddress/setSubnetMask/setDefaultGateway/setDns/setInitialValues); move that hydration out of the view into the environment VM or a dedicated hook (e.g., add a method on the Environment VM like hydrateDiscoverySourceForm(src) or create a hook useDiscoverySourceFormValues(src) that returns { networkConfigType, ipAddress, subnetMask, defaultGateway, dns, initialValues } where initialValues composes sshKey, environmentName, ...proxyConfig and ...networkConfig); then in the view call the VM method/hook at the top and use its returned values to set state (invoke setNetworkConfigType, setIpAddress, etc. and setInitialValues) so the component contains only render/state wiring.src/ui/environment/helpers/__tests__/proxyConfig.test.ts (1)
65-80: Add a regression test for whitespace-only proxy values.Since
enableProxyis inferred, add coverage forhttpUrl: " "/httpsUrl: " "/noProxy: " "so behavior is explicit and protected.🧪 Suggested test case
+ it("should keep proxy disabled when values are whitespace-only", () => { + const source: Partial<Source> = { + infra: { + proxy: { + httpUrl: " ", + httpsUrl: " ", + noProxy: " ", + }, + }, + }; + + const config = getProxyConfig(source as Source); + expect(config.enableProxy).toBe(false); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/environment/helpers/__tests__/proxyConfig.test.ts` around lines 65 - 80, Add a regression test that supplies whitespace-only strings for infra.proxy.httpUrl, httpsUrl, and noProxy and assert that getProxyConfig treats them as empty (trimmed) values so enableProxy is false; specifically, update or add a test around getProxyConfig with httpUrl: " ", httpsUrl: " ", noProxy: " " and expect config.enableProxy toBe(false) and config.noProxy toBe("") or null per your normalization. If necessary, update getProxyConfig to trim proxy fields (httpUrl, httpsUrl, noProxy) before computing enableProxy and before returning the config so whitespace-only values are treated as unset.
🤖 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/ui/environment/helpers/proxyConfig.ts`:
- Line 30: The enableProxy calculation currently uses raw string truthiness
(enableProxy: Boolean(httpProxy || httpsProxy || noProxy)); change it to
consider trimmed values so whitespace-only strings are treated as empty: derive
trimmedHttp = httpProxy?.trim(), trimmedHttps = httpsProxy?.trim(), trimmedNo =
noProxy?.trim() (or call .trim() inline) and set enableProxy to
Boolean(trimmedHttp || trimmedHttps || trimmedNo) so the form won't mark proxy
enabled for whitespace-only input; update any nearby references in
proxyConfig.ts that rely on the original variables accordingly.
---
Nitpick comments:
In `@src/ui/environment/helpers/__tests__/proxyConfig.test.ts`:
- Around line 65-80: Add a regression test that supplies whitespace-only strings
for infra.proxy.httpUrl, httpsUrl, and noProxy and assert that getProxyConfig
treats them as empty (trimmed) values so enableProxy is false; specifically,
update or add a test around getProxyConfig with httpUrl: " ", httpsUrl: " ",
noProxy: " " and expect config.enableProxy toBe(false) and config.noProxy
toBe("") or null per your normalization. If necessary, update getProxyConfig to
trim proxy fields (httpUrl, httpsUrl, noProxy) before computing enableProxy and
before returning the config so whitespace-only values are treated as unset.
In `@src/ui/environment/views/DiscoverySourceSetupModal.tsx`:
- Around line 402-413: The view DiscoverySourceSetupModal is doing
business/data-mapping (calling getNetworkConfig and building the initialValues
object, then calling
setNetworkConfigType/setIpAddress/setSubnetMask/setDefaultGateway/setDns/setInitialValues);
move that hydration out of the view into the environment VM or a dedicated hook
(e.g., add a method on the Environment VM like hydrateDiscoverySourceForm(src)
or create a hook useDiscoverySourceFormValues(src) that returns {
networkConfigType, ipAddress, subnetMask, defaultGateway, dns, initialValues }
where initialValues composes sshKey, environmentName, ...proxyConfig and
...networkConfig); then in the view call the VM method/hook at the top and use
its returned values to set state (invoke setNetworkConfigType, setIpAddress,
etc. and setInitialValues) so the component contains only render/state wiring.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 53bfbb62-dddb-4b88-9874-79dfd943be3c
📒 Files selected for processing (5)
src/ui/environment/helpers/__tests__/networkConfig.test.tssrc/ui/environment/helpers/__tests__/proxyConfig.test.tssrc/ui/environment/helpers/networkConfig.tssrc/ui/environment/helpers/proxyConfig.tssrc/ui/environment/views/DiscoverySourceSetupModal.tsx
…diting environment Environment edit form was not loading existing vmNetwork configuration, causing it to default to DHCP and lose static network settings. Add getNetworkConfig helper to extract network configuration from Source objects and update DiscoverySourceSetupModal to properly load existing values when editing an environment. Signed-off-by: Guillaume Vincent <gvincent@redhat.com>
rh-gvincent
force-pushed
the
ECOPROJECT-4393
branch
from
5ca671d to
cc2f053
Compare
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rh-gvincent The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
2 participants
Environment edit form was not loading existing vmNetwork configuration, causing it to default to DHCP and lose static network settings.
Add getNetworkConfig helper to extract network configuration from Source objects and update DiscoverySourceSetupModal to properly load existing values when editing an environment.
Summary by CodeRabbit
New Features
Tests
Chores