Skip to content

fix(timeout): allow inherit request timeout for yaml requests #8382

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
shubh-bruno wants to merge 5 commits into
base: main
Choose a base branch
from
+212 −47
Open

fix(timeout): allow inherit request timeout for yaml requests #8382

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
da9b45d
fix(timeout): allow inherit in timout for yaml requests
Jun 26, 2026
5f2592d
test: re-run
Jun 26, 2026
7a71697
test: yaml test cases for request timeout
Jun 29, 2026
e0f7846
test: re-run
Jun 29, 2026
7ff1b72
test(timeout): verify inherit persistence for bru and yml requests
Jun 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/bruno-converters/src/opencollection/items/graphql.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ export const toOpenCollectionGraphqlItem = (item: BrunoItem): GraphQLRequest =>

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,

@sachin-thakur-bruno sachin-thakur-bruno Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: please create const for inherit and use.

followRedirects: typeof brunoSettings.followRedirects === 'boolean' ? brunoSettings.followRedirects : true,
maxRedirects: typeof brunoSettings.maxRedirects === 'number' ? brunoSettings.maxRedirects : 5
};
Expand Down
4 changes: 2 additions & 2 deletions packages/bruno-converters/src/opencollection/items/http.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ export const fromOpenCollectionHttpItem = (ocRequest: HttpRequest): BrunoItem =>
if (ocRequest.settings) {
const settings: BrunoHttpItemSettings = {
encodeUrl: typeof ocRequest.settings.encodeUrl === 'boolean' ? ocRequest.settings.encodeUrl : true,
timeout: typeof ocRequest.settings.timeout === 'number' ? ocRequest.settings.timeout : 0,
timeout: typeof ocRequest.settings.timeout === 'number' || ocRequest.settings.timeout === 'inherit' ? ocRequest.settings.timeout : 0,
followRedirects: typeof ocRequest.settings.followRedirects === 'boolean' ? ocRequest.settings.followRedirects : true,
maxRedirects: typeof ocRequest.settings.maxRedirects === 'number' ? ocRequest.settings.maxRedirects : 5
};
Expand Down Expand Up @@ -221,7 +221,7 @@ export const toOpenCollectionHttpItem = (item: BrunoItem): HttpRequest => {

const settings: HttpRequestSettings = {
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,
followRedirects: typeof brunoSettings?.followRedirects === 'boolean' ? brunoSettings.followRedirects : true,
maxRedirects: typeof brunoSettings?.maxRedirects === 'number' ? brunoSettings.maxRedirects : 5
};
Expand Down
2 changes: 1 addition & 1 deletion packages/bruno-filestore/src/formats/yml/items/stringifyGraphQLRequest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ const stringifyGraphQLRequest = (item: BrunoItem): string => {
}

const timeout = httpSettings?.timeout;
if (isNumber(timeout)) {
if (isNumber(timeout) || timeout === 'inherit') {
settings.timeout = timeout;
} else {
settings.timeout = 0;
Expand Down
2 changes: 1 addition & 1 deletion packages/bruno-filestore/src/formats/yml/items/stringifyHttpRequest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ const stringifyHttpRequest = (item: BrunoItem): string => {
}

const timeout = httpSettings?.timeout;
if (isNumber(timeout)) {
if (isNumber(timeout) || timeout === 'inherit') {
settings.timeout = timeout;
} else {
settings.timeout = 0;

@coderabbitai coderabbitai Bot Jun 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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

@sachin-thakur-bruno sachin-thakur-bruno Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shubh-bruno we should add this.

@coderabbitai coderabbitai Bot Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

@shubh-bruno shubh-bruno Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

@coderabbitai coderabbitai Bot Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

Expand Down
Loading