fix(@redhat-cloud-services/frontend-components-config-utilities,@redhat-cloud-services/frontend-components-config): revert reload nav on CRD change - #2395
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting WalkthroughThe development proxy removes CRD interceptor server startup, routing, and teardown. Proxy handling retains CRD refresh and response interception while removing browser reload signaling and request-header stripping. ChangesCRD interception and proxy behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
LightOfHeaven1994
left a comment
LightOfHeaven1994
left a comment
There was a problem hiding this comment.
LGTM, thank you!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/config-utils/src/proxy.ts (1)
241-245: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve cache bypass for intercepted responses.
Without stripping conditional headers, the upstream can return
304 Not Modified; there is then no body formodifyRequestto replace, so CRD changes remain stale in the browser. Restore conditional-request removal and cache-prevention headers forisInterceptAbleRequestresponses, and add a regression test covering a CRD update followed by a conditional navigation request.Proposed fix
+ onProxyReq: (proxyReq, req) => { + if (FEOFeaturesEnabled && frontendCrdRef.current && isInterceptAbleRequest(req.url)) { + proxyReq.removeHeader('if-none-match'); + proxyReq.removeHeader('if-modified-since'); + } + }, onProxyRes: (proxyRes, req, res) => { if (FEOFeaturesEnabled && frontendCrdRef.current && isInterceptAbleRequest(req.url)) { + delete proxyRes.headers.etag; + delete proxyRes.headers['last-modified']; + delete proxyRes.headers['cache-control'];As per coding guidelines, “All functions in
@redhat-cloud-services/frontend-components-config-utilitiesmust have comprehensive unit tests.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/config-utils/src/proxy.ts` around lines 241 - 245, Update the intercepted-response path in the onProxyRes handler to remove conditional request headers and apply cache-prevention headers whenever isInterceptAbleRequest(req.url) is true, ensuring upstream responses include a body for modifyRequest. Add a unit regression test covering a CRD update followed by a conditional navigation request and verify the updated CRD content is returned.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/config/src/bin/dev-proxy-script.ts`:
- Line 238: Update the routesConfigPath assignment in the dev-proxy startup flow
to pass a per-invocation filename or temporary-directory-scoped path to
createRoutesConfig instead of the shared /tmp/routes.json default. Ensure
concurrent dev-proxy sessions generate and bind-mount distinct routes files.
- Line 85: Update createRoutesConfig to replace fecConfig: any with a concrete
type exposing the required routes and routesPath fields. Replace the fixed
temporary routes.json location with a unique per-run temporary filename under
os.tmpdir(), ensuring concurrent dev-proxy instances do not overwrite one
another’s mounted routes file.
---
Outside diff comments:
In `@packages/config-utils/src/proxy.ts`:
- Around line 241-245: Update the intercepted-response path in the onProxyRes
handler to remove conditional request headers and apply cache-prevention headers
whenever isInterceptAbleRequest(req.url) is true, ensuring upstream responses
include a body for modifyRequest. Add a unit regression test covering a CRD
update followed by a conditional navigation request and verify the updated CRD
content is returned.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 3ca2aa2f-466c-47da-a2e7-bc327a0e9edc
📒 Files selected for processing (3)
packages/config-utils/src/proxy.tspackages/config/src/bin/crd-interceptor-server.tspackages/config/src/bin/dev-proxy-script.ts
💤 Files with no reviewable changes (1)
- packages/config/src/bin/crd-interceptor-server.ts
| filename: string = 'routes.json', | ||
| interceptorPort?: number, | ||
| ): string { | ||
| function createRoutesConfig(fecConfig: any, cdnPath: string, port: string, SPAFallback: boolean, filename: string = 'routes.json'): string { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline packages/config/src/bin/dev-proxy-script.ts --items all --match createRoutesConfig
rg -n --type ts -C2 'interface .*Config|type .*Config|routesPath|validateFECConfig' packages/config/src packages/config-utils/srcRepository: RedHatInsights/frontend-components
Length of output: 13570
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' packages/config/src/bin/dev-proxy-script.ts | cat -n
printf '\n---\n'
sed -n '220,340p' packages/config/src/bin/dev-proxy-script.ts | cat -n
printf '\n---\n'
sed -n '1,260p' packages/config-utils/src/proxy.ts | cat -nRepository: RedHatInsights/frontend-components
Length of output: 24778
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- dev-proxy-script.ts: lines 220-360 ---'
sed -n '220,360p' packages/config/src/bin/dev-proxy-script.ts | cat -n
printf '\n%s\n' '--- all createRoutesConfig call sites ---'
rg -n 'createRoutesConfig\(' packages/config/src/bin/dev-proxy-script.ts packages/config/src/bin -C 2
printf '\n%s\n' '--- routes.json / mount references ---'
rg -n 'routes.json|routesConfigPath|custom_routes|/config/' packages/config/src/bin/dev-proxy-script.ts packages/config-utils/src/proxy.ts -C 2Repository: RedHatInsights/frontend-components
Length of output: 10917
Replace fecConfig: any and stop using a fixed temp filename.
- Use a concrete config type for the
routes/routesPathfields instead ofany. os.tmpdir()/routes.jsonis shared across runs, so concurrent dev-proxy instances can overwrite each other’s mounted routes file.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { exec, execSync } from 'child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/config/src/bin/dev-proxy-script.ts` at line 85, Update
createRoutesConfig to replace fecConfig: any with a concrete type exposing the
required routes and routesPath fields. Replace the fixed temporary routes.json
location with a unique per-run temporary filename under os.tmpdir(), ensuring
concurrent dev-proxy instances do not overwrite one another’s mounted routes
file.
Sources: Coding guidelines, Path instructions
| try { | ||
| cdnPath = getCdnPath(fecConfig, webpackConfig, cwd); | ||
| routesConfigPath = createRoutesConfig(fecConfig, cdnPath, staticPort, SPAFallback, 'routes.json', interceptorPort); | ||
| routesConfigPath = createRoutesConfig(fecConfig, cdnPath, staticPort, SPAFallback); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Use a per-invocation routes filename.
This now defaults every dev-proxy process to /tmp/routes.json. Concurrent sessions overwrite the file bind-mounted into an existing proxy container, causing it to use another session’s routes. Pass a PID- or temp-directory-scoped filename.
Proposed fix
- routesConfigPath = createRoutesConfig(fecConfig, cdnPath, staticPort, SPAFallback);
+ routesConfigPath = createRoutesConfig(fecConfig, cdnPath, staticPort, SPAFallback, `routes-${process.pid}.json`);📝 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.
| routesConfigPath = createRoutesConfig(fecConfig, cdnPath, staticPort, SPAFallback); | |
| routesConfigPath = createRoutesConfig(fecConfig, cdnPath, staticPort, SPAFallback, `routes-${process.pid}.json`); |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { exec, execSync } from 'child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/config/src/bin/dev-proxy-script.ts` at line 238, Update the
routesConfigPath assignment in the dev-proxy startup flow to pass a
per-invocation filename or temporary-directory-scoped path to createRoutesConfig
instead of the shared /tmp/routes.json default. Ensure concurrent dev-proxy
sessions generate and bind-mount distinct routes files.
Chromatic Build
|
charlesmulder
changed the title
charlesmulder
changed the title
a87b9da to
574fe2c
Compare
…at-cloud-services/frontend-components-config): revert reload nav on CRD change
574fe2c to
60c771f
Compare
3 participants
Reverts #2391
Summary by CodeRabbit