fix(timeout): allow inherit request timeout for yaml requests#8382
fix(timeout): allow inherit request timeout for yaml requests#8382shubh-bruno wants to merge 5 commits into
Conversation
shubh-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno,
sid-bruno,
utkarsh-bruno and
vijayh-bruno
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughTimeout handling for HTTP and GraphQL requests now accepts ChangesTimeout inherit support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 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.
Actionable comments posted: 1
🤖 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/bruno-filestore/src/formats/yml/items/stringifyHttpRequest.ts`:
- Around line 121-124: Add regression tests for the timeout
serialization/deserialization path in stringifyHttpRequest and the corresponding
load logic to cover the new “inherit” support. Include cases for both HTTP and
GraphQL requests that round-trip timeout: 'inherit' and a numeric timeout, and
assert they do not fall back to 0. Place the coverage near the existing request
format tests so the behavior stays tied to the stringify/load contract.
🪄 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.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1d2a4cc9-5fa4-44f7-bd4b-9d23839109fd
📒 Files selected for processing (4)
packages/bruno-converters/src/opencollection/items/graphql.tspackages/bruno-converters/src/opencollection/items/http.tspackages/bruno-filestore/src/formats/yml/items/stringifyGraphQLRequest.tspackages/bruno-filestore/src/formats/yml/items/stringifyHttpRequest.ts
| if (isNumber(timeout) || timeout === 'inherit') { | ||
| settings.timeout = timeout; | ||
| } else { | ||
| settings.timeout = 0; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add regression coverage for the 'inherit' timeout round-trip.
This fixes a cross-layer serialization contract, but no matching tests are included here. Please add cases that save/load both HTTP and GraphQL requests with timeout: 'inherit' and with a numeric timeout so this path does not silently fall back to 0 again. As per coding guidelines, "Add tests for any new functionality or meaningful changes."
🤖 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/bruno-filestore/src/formats/yml/items/stringifyHttpRequest.ts`
around lines 121 - 124, Add regression tests for the timeout
serialization/deserialization path in stringifyHttpRequest and the corresponding
load logic to cover the new “inherit” support. Include cases for both HTTP and
GraphQL requests that round-trip timeout: 'inherit' and a numeric timeout, and
assert they do not fall back to 0. Place the coverage near the existing request
format tests so the behavior stays tied to the stringify/load contract.
Source: Coding guidelines
There was a problem hiding this comment.
@sachin-thakur-bruno agreed — adding regression coverage here would be the right follow-up.
Please cover both HTTP and GraphQL round-trips for:
timeout: 'inherit'- a numeric
timeout
and assert the value survives save/load without falling back to 0. Placing them next to the existing request format tests would keep the contract clear.
🐇👍
🧠 Learnings used
Learnt from: sanish-bruno
Repo: usebruno/bruno PR: 8387
File: packages/bruno-cli/src/utils/persist-variables.js:330-337
Timestamp: 2026-06-26T21:24:02.726Z
Learning: In the usebruno/bruno repository, the codebase is standardized to LF line endings. During code review, do not flag CRLF-preservation/line-ending change concerns for these text-based source/config files unless the file format or an explicit workflow/tooling requirement requires preserving CRLF (e.g., a Windows-specific file type or a mandated EOL rule).
There was a problem hiding this comment.
@coderabbitai e2e playwrights tests for this already exists but only for .bru files we just need to add fixtures for .yml and run the same tests against .yml requests file tests/request/settings/timeout.spec.ts
There was a problem hiding this comment.
@shubh-bruno that makes sense — thanks for the pointer.
Reusing the existing Playwright coverage in tests/request/settings/timeout.spec.ts and extending it with .yaml fixtures sounds like the right follow-up here, instead of adding a separate test path.
(ᵔᴥᵔ)
✏️ Learnings added
Learnt from: shubh-bruno
Repo: usebruno/bruno PR: 8382
File: packages/bruno-filestore/src/formats/yml/items/stringifyHttpRequest.ts:121-124
Timestamp: 2026-06-29T07:26:17.002Z
Learning: In the `usebruno/bruno` repository, regression coverage for request timeout save/load behavior is handled by the Playwright end-to-end test file `tests/request/settings/timeout.spec.ts`. Existing coverage already applies to `.bru` files; for YAML request timeout regressions, the expected approach is to add `.yaml` fixtures and reuse the same save tests rather than creating a separate test file.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: sanish-bruno
Repo: usebruno/bruno PR: 8387
File: packages/bruno-cli/src/utils/persist-variables.js:330-337
Timestamp: 2026-06-26T21:24:02.726Z
Learning: In the usebruno/bruno repository, the codebase is standardized to LF line endings. During code review, do not flag CRLF-preservation/line-ending change concerns for these text-based source/config files unless the file format or an explicit workflow/tooling requirement requires preserving CRLF (e.g., a Windows-specific file type or a mandated EOL rule).
| const settings: GraphQLRequestSettings = { | ||
| encodeUrl: typeof brunoSettings.encodeUrl === 'boolean' ? brunoSettings.encodeUrl : true, | ||
| timeout: typeof brunoSettings.timeout === 'number' ? brunoSettings.timeout : 0, | ||
| timeout: typeof brunoSettings.timeout === 'number' || brunoSettings.timeout === 'inherit' ? brunoSettings.timeout : 0, |
There was a problem hiding this comment.
nit: please create const for inherit and use.
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/request/settings/timeout.spec.ts`:
- Around line 114-140: The current timeout test only verifies the in-memory
inherit state and never exercises YAML persistence, so it can miss regressions
in save serialization. In timeout.spec.ts, after clicking the Reset to inherit
control in the request Settings tab, save the request, reopen/reload it, and
assert the Settings UI still shows the Inherit state before sending. Use the
existing helpers like setGlobalRequestTimeout, selectRequestPaneTab, and the
resetButton/inheritButton flow to locate the change, and keep the final request
execution to confirm the inherited timeout still resolves correctly after
reload.
🪄 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.yaml
Review profile: CHILL
Plan: Pro
Run ID: 01f424c9-5c22-4ca5-a99f-3c5f6042ea16
📒 Files selected for processing (15)
packages/bruno-common/src/utils/index.tspackages/bruno-converters/src/opencollection/common/index.tspackages/bruno-converters/src/opencollection/items/graphql.tspackages/bruno-converters/src/opencollection/items/http.tspackages/bruno-filestore/src/formats/yml/items/stringifyGraphQLRequest.tspackages/bruno-filestore/src/formats/yml/items/stringifyHttpRequest.tstests/request/settings/collection/requests-settings-bru/bruno.jsontests/request/settings/collection/requests-settings-bru/max-redirects.brutests/request/settings/collection/requests-settings-bru/no-redirects.brutests/request/settings/collection/requests-settings-bru/timeout.brutests/request/settings/collection/requests-settings-yml/opencollection.ymltests/request/settings/collection/requests-settings-yml/timeout.ymltests/request/settings/init-user-data/collection-security.jsontests/request/settings/init-user-data/preferences.jsontests/request/settings/timeout.spec.ts
✅ Files skipped from review due to trivial changes (3)
- tests/request/settings/collection/requests-settings-yml/opencollection.yml
- tests/request/settings/collection/requests-settings-yml/timeout.yml
- packages/bruno-converters/src/opencollection/common/index.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/bruno-filestore/src/formats/yml/items/stringifyGraphQLRequest.ts
- packages/bruno-filestore/src/formats/yml/items/stringifyHttpRequest.ts
- packages/bruno-converters/src/opencollection/items/graphql.ts
- packages/bruno-converters/src/opencollection/items/http.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/request/settings/timeout.spec.ts (1)
36-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffRestoring fixtures via
git checkoutmutates the tracked working tree — preferexecFileSyncand confirm parallel-safety.Two points on this cleanup hook:
- Both scenarios save into the committed fixture files (
timeout.bru/timeout.yml) and rely ongit checkoutto undo it. That pollutes the working tree mid-run and silently discards any local uncommitted edits a developer made to those fixtures. Per the E2E parallel-safety guideline ("No shared user data directories, ports, files..."), shared in-repo fixtures mutated in place aren't parallel-safe if the suite ever runs with multiple workers.- Building a shell string also trips the SAST
command-injectionhint. The args here are static (__dirname-derived), so it isn't exploitable, butexecFileSyncwith an argument array avoids the shell entirely and handles paths containing spaces more reliably across platforms.♻️ Suggested tweak to drop the shell
-import { execSync } from 'child_process'; +import { execFileSync } from 'child_process';test.afterAll(() => { - execSync(`git checkout -- "${bruRequestPath}" "${yamlRequestPath}"`); + execFileSync('git', ['checkout', '--', bruRequestPath, yamlRequestPath]); });Can you confirm whether this suite runs with
workers > 1? If so, copying the fixtures into an isolated temp dir would be the more robust fix.🤖 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 `@tests/request/settings/timeout.spec.ts` around lines 36 - 38, The cleanup in `timeout.spec.ts` uses `execSync` with `git checkout` to restore `bruRequestPath` and `yamlRequestPath`, which both mutates the tracked working tree and goes through a shell. Update the `test.afterAll` hook to use `execFileSync` with an argument array instead of a shell command, and avoid in-place restoration of shared fixtures if this suite can run with `workers > 1` by isolating the files in temp copies.Sources: Coding guidelines, Linters/SAST tools
🤖 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.
Nitpick comments:
In `@tests/request/settings/timeout.spec.ts`:
- Around line 36-38: The cleanup in `timeout.spec.ts` uses `execSync` with `git
checkout` to restore `bruRequestPath` and `yamlRequestPath`, which both mutates
the tracked working tree and goes through a shell. Update the `test.afterAll`
hook to use `execFileSync` with an argument array instead of a shell command,
and avoid in-place restoration of shared fixtures if this suite can run with
`workers > 1` by isolating the files in temp copies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a51dce8f-edd3-458a-bea7-0961c7dda03a
📒 Files selected for processing (1)
tests/request/settings/timeout.spec.ts
2 participants
Description
JIRA
Problem
Request-level timeout set to 'inherit' was reset to 0 on save for .yaml requests (.bru worked fine). The YAML stringifiers forced timeout with
isNumber(timeout) ? timeout : 0, dropping the 'inherit' string. The YAML parse path and both .bru paths already handled 'inherit' correctly, so the loss happened only on saveSolution
Allow 'inherit' to pass through alongside numbers in the YAML stringifiers and the OpenCollection converters.
Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
0.Tests
.bruand YAML requests, including new/updated fixtures.