fix(timeout): allow inherit request timeout for yaml requests#8746
fix(timeout): allow inherit request timeout for yaml requests#8746shubh-bruno wants to merge 2 commits into
Conversation
WalkthroughChangesTimeout preservation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
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
🧹 Nitpick comments (3)
tests/utils/page/preferences.ts (2)
60-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAction helper uses
expect(...)instead of a wait — should synchronize only.Per the project's Playwright testing guide, action helpers (outside spec files) should synchronize with waits, and leave assertions to the specs that own pass/fail.
setRequestTimeoutPreferenceusesexpect(...).toHaveValue(...)to wait for the debounced value commit; a helper failure here surfaces as a helper assertion failure rather than a spec assertion, muddying failure attribution.♻️ Proposed fix using a non-assertion wait
- await preferences.requestTimeoutInput().fill(value); - // Wait for the value to commit before closing so the debounced save flushes the new value on unmount - await expect(preferences.requestTimeoutInput()).toHaveValue(value, { timeout: 5000 }); + await preferences.requestTimeoutInput().fill(value); + // Wait for the value to commit before closing so the debounced save flushes the new value on unmount + await preferences.requestTimeoutInput().evaluate( + (el, expected) => new Promise<void>((resolve) => { + const check = () => ((el as HTMLInputElement).value === expected ? resolve() : requestAnimationFrame(check)); + check(); + }), + value + );As per path instructions, "Keep assertions in specs: action helpers should synchronize (waits) but not
expect(...); specs own pass/fail assertions."🤖 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/utils/page/preferences.ts` around lines 60 - 72, Replace the expect(...).toHaveValue(...) assertion in setRequestTimeoutPreference with a non-assertion Playwright wait that synchronizes until requestTimeoutInput() reflects value. Keep the existing timeout and subsequent tab-closing flow unchanged, leaving pass/fail assertions to the specs.Source: Path instructions
12-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the existing close-icon testid instead of a CSS class.
locators.ts'stabs.closeTabalready targets the same kind of element via.getByTestId('request-tab-close-icon').openTabCloseIconinstead falls back to a.close-iconCSS class selector, which is more fragile to styling refactors than the testid already used elsewhere for tab close buttons.♻️ Proposed fix
- openTabCloseIcon: () => page.locator('.request-tab').filter({ hasText: 'Preferences' }).locator('.close-icon'), + openTabCloseIcon: () => page.locator('.request-tab').filter({ hasText: 'Preferences' }).getByTestId('request-tab-close-icon'),Please confirm the Preferences tab's close icon actually renders the same
request-tab-close-icontestid as other request tabs before applying.🤖 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/utils/page/preferences.ts` around lines 12 - 18, Update openTabCloseIcon in the Preferences tab locators to target the existing request-tab-close-icon testid, first confirming that the Preferences tab renders this same testid as other request tabs. Remove the fragile .close-icon selector while preserving the current Preferences tab scoping.Source: Path instructions
tests/request/settings/timeout.spec.ts (1)
30-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoth tests duplicate nearly the entire flow — consider a shared parameterized helper.
The bru (30-88) and yaml (90-146) tests differ only in collection/request names and the serialized file path/extension; everything else (verify custom timeout, send, change global pref, reset-to-inherit, save, assert file content, reopen, re-send) is duplicated. Extracting a local helper taking
{ collectionName, requestName, settingsFilePath }would cut duplication and prevent the two flows from drifting apart over time.Based on the retrieved learning that YAML timeout coverage should reuse the same save/load test cases in this same file (rather than a separate file), a shared parameterized helper keeps both formats colocated while removing the duplication.
🤖 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 30 - 146, Extract the duplicated timeout persistence flow from the two tests into a local parameterized helper, accepting collection name, request name, and serialized settings file path or equivalent format-specific values. Reuse this helper from both bru and yaml tests while preserving all existing assertions and format-specific file paths, keeping both cases in this file.Source: Learnings
🤖 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-common/src/utils/index.ts`:
- Around line 57-62: Update resolveTimeoutSetting to preserve only
TIMEOUT_INHERIT and finite, positive numeric timeout values; return 0 for NaN,
positive or negative Infinity, zero, negative numbers, and other invalid inputs.
Add coverage for these malformed numeric cases while retaining the existing
valid-value behavior.
---
Nitpick comments:
In `@tests/request/settings/timeout.spec.ts`:
- Around line 30-146: Extract the duplicated timeout persistence flow from the
two tests into a local parameterized helper, accepting collection name, request
name, and serialized settings file path or equivalent format-specific values.
Reuse this helper from both bru and yaml tests while preserving all existing
assertions and format-specific file paths, keeping both cases in this file.
In `@tests/utils/page/preferences.ts`:
- Around line 60-72: Replace the expect(...).toHaveValue(...) assertion in
setRequestTimeoutPreference with a non-assertion Playwright wait that
synchronizes until requestTimeoutInput() reflects value. Keep the existing
timeout and subsequent tab-closing flow unchanged, leaving pass/fail assertions
to the specs.
- Around line 12-18: Update openTabCloseIcon in the Preferences tab locators to
target the existing request-tab-close-icon testid, first confirming that the
Preferences tab renders this same testid as other request tabs. Remove the
fragile .close-icon selector while preserving the current Preferences tab
scoping.
🪄 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 Plus
Run ID: 71d6c060-410d-4c0a-8571-03d3cf5c80e7
📒 Files selected for processing (19)
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/fixtures/collections/requests-settings-bru/bruno.jsontests/request/settings/fixtures/collections/requests-settings-bru/max-redirects.brutests/request/settings/fixtures/collections/requests-settings-bru/no-redirects.brutests/request/settings/fixtures/collections/requests-settings-bru/timeout.brutests/request/settings/fixtures/collections/requests-settings-yml/opencollection.ymltests/request/settings/fixtures/collections/requests-settings-yml/timeout.ymltests/request/settings/init-user-data/collection-security.jsontests/request/settings/init-user-data/preferences.jsontests/request/settings/timeout.spec.tstests/utils/page/index.tstests/utils/page/locators.tstests/utils/page/preferences.tstests/utils/page/request-settings.ts
| export const TIMEOUT_INHERIT = 'inherit' as const; | ||
|
|
||
| // Normalize a request timeout setting for serialization: keep numbers and the | ||
| // "inherit" sentinel as-is; fall back to 0 for anything else (null/undefined/invalid). | ||
| export const resolveTimeoutSetting = (value: unknown): number | typeof TIMEOUT_INHERIT => | ||
| typeof value === 'number' || value === TIMEOUT_INHERIT ? value : 0; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file outline =="
ast-grep outline packages/bruno-common/src/utils/index.ts || true
echo
echo "== relevant lines =="
sed -n '1,90p' packages/bruno-common/src/utils/index.ts
echo
echo "== usage/search =="
rg -n "resolveTimeoutSetting|TIMEOUT_INHERIT|timeout" packages/bruno-common -S | head -n 120Repository: usebruno/bruno
Length of output: 2501
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== bruno-common package opt-in check =="
cat packages/bruno-common/package.json | jq '{type, dependencies, devDependencies, scripts}' 2>/dev/null || cat packages/bruno-common/package.json
echo
echo "== TypeScript files under bruno-common =="
git ls-files 'packages/bruno-common/**/*.ts' | sed -n '1,80p'
echo
echo "== tests mentioning timeout/resolver =="
rg -n "resolveTimeoutSetting|TIMEOUT_INHERIT|Timeout|timeout=|requestTimeout|timeout" packages/bruno-common -S || true
echo
echo "== deterministic numeric classifier probe =="
node - <<'JS'
const v = [NaN, Infinity, -Infinity, -1, 0, 1, 1.5, '1', null, undefined];
for (const value of v) {
const resolved = typeof value === 'number' || value === 'inherit' ? value : 0;
console.log(String(value).padEnd(12), 'type=', typeof value, 'resolved=', resolved);
}
JSRepository: usebruno/bruno
Length of output: 4013
Restrict resolveTimeoutSetting to valid timeout values.
Line 61 preserves every number, including NaN, Infinity, -Infinity, and negative values, while the comment says invalid values should fall back to 0. Add bounds/finite checks for finite positive timeout values and cover these malformed cases.
🤖 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-common/src/utils/index.ts` around lines 57 - 62, Update
resolveTimeoutSetting to preserve only TIMEOUT_INHERIT and finite, positive
numeric timeout values; return 0 for NaN, positive or negative Infinity, zero,
negative numbers, and other invalid inputs. Add coverage for these malformed
numeric cases while retaining the existing valid-value behavior.
| }) => { | ||
| // Navigate to the test collection and request | ||
| await expect(page.locator('#sidebar-collection-name').getByText('settings-test')).toBeVisible(); | ||
| test('should persist inherit timeout for a bru request', async ({ launchElectronApp, createTmpDir }) => { |
There was a problem hiding this comment.
Please add test.step in test
bijin-bruno
left a comment
bijin-bruno
left a comment
There was a problem hiding this comment.
@shubh-bruno Please verify if we need a corresponding CLI handling
| export const TIMEOUT_INHERIT = 'inherit' as const; | ||
|
|
||
| // Normalize a request timeout setting for serialization: keep numbers and the | ||
| // "inherit" sentinel as-is; fall back to 0 for anything else (null/undefined/invalid). | ||
| export const resolveTimeoutSetting = (value: unknown): number | typeof TIMEOUT_INHERIT => | ||
| typeof value === 'number' || value === TIMEOUT_INHERIT ? value : 0; |
3 participants
Description
BRU-3653
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
New Features
Bug Fixes
Tests