feat: collection-level and app-level proxy settings updates
#6514
Conversation
lohit-bruno
requested review from
bijin-bruno,
helloanoop and
naman-bruno
as code owners
|
Warning Rate limit exceeded@lohit-bruno has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 1 minutes and 46 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (1)
WalkthroughThis PR refactors proxy configuration from a flat shape to a nested model with Changes
Sequence Diagram(s)sequenceDiagram
participant UI as App UI (Preferences/Collection)
participant Store as PreferencesStore (electron-store)
participant Transform as transformBrunoConfig
participant Utils as cert-utils / proxy-util
participant FS as File parsers (YML) / Converters
Note over UI,Store: Save flow (new nested proxy model)
UI->>Store: save preferences (proxy: {disabled?, inherit, config})
Store->>Transform: transformBrunoConfigBeforeSave(preferences)
Transform-->>Store: cleaned preferences (omit false flags)
Store->>FS: persist to disk
Note over Store,Utils: Read & runtime resolution
Store->>Transform: transformBrunoConfigAfterRead(rawPrefs)
Transform-->>Store: normalized preferences (migrated if needed)
Store->>Utils: getCertsAndProxyConfig(normalized preferences)
Utils->>Utils: derive proxyMode from disabled/inherit/config
Utils-->>UI: returns proxy/cert configuration for requests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bruno-filestore/src/formats/yml/common/auth-oauth2.ts (1)
86-92: Add tests for PKCE transformation in YML format.The
buildPkcelogic correctly implements the "enabled by default" pattern:
pkce: true→{}(enabled)pkce: false→{ disabled: true }The bidirectional transformation in
toBrunoOAuth2properly inverts this back to a boolean. However, tests exist for PKCE in Postman conversion but are missing for the YML format'sbuildPkceandtoBrunoOAuth2bidirectional transformation. Add test coverage to prevent regression.
🧹 Nitpick comments (2)
packages/bruno-electron/tests/store/proxy-preferences.spec.js (1)
14-14: Unused import:savePreferences
savePreferencesis imported but never used in this test file. Remove it to keep imports clean.-const { getPreferences, savePreferences } = require('../../src/store/preferences'); +const { getPreferences } = require('../../src/store/preferences');packages/bruno-filestore/src/formats/yml/stringifyCollection.ts (1)
21-29: Duplicated proxy format validation logic.The same validation check (
'inherit' in brunoConfig.proxy && brunoConfig.proxy.config) appears twice. Consider extracting to a helper function for DRY compliance.+const isValidNewerProxyFormat = (proxy: any): boolean => { + return proxy + && typeof proxy === 'object' + && 'inherit' in proxy + && proxy.config + && typeof proxy.config === 'object'; +}; + const hasCollectionConfig = (brunoConfig: any): boolean => { // protobuf const hasProtobuf = ( brunoConfig.protobuf?.protofFiles?.length > 0 || brunoConfig.protobuf?.importPaths?.length > 0 ); - // proxy - check if proxy is configured in newer format - // Valid newer format: has 'inherit' property and 'config' object - const isValidProxyFormat = brunoConfig.proxy - && typeof brunoConfig.proxy === 'object' - && 'inherit' in brunoConfig.proxy - && brunoConfig.proxy.config - && typeof brunoConfig.proxy.config === 'object'; - - const hasProxy = isValidProxyFormat; + const hasProxy = isValidNewerProxyFormat(brunoConfig.proxy);Also applies to: 87-93
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
package.jsonpackages/bruno-app/src/components/CollectionSettings/ProxySettings/index.jspackages/bruno-app/src/components/Preferences/ProxySettings/index.jspackages/bruno-converters/package.jsonpackages/bruno-electron/src/app/collection-watcher.jspackages/bruno-electron/src/app/collections.jspackages/bruno-electron/src/ipc/collection.jspackages/bruno-electron/src/ipc/network/cert-utils.jspackages/bruno-electron/src/store/preferences.jspackages/bruno-electron/src/utils/proxy-util.jspackages/bruno-electron/src/utils/transformBrunoConfig.jspackages/bruno-electron/tests/store/proxy-preferences.spec.jspackages/bruno-electron/tests/utils/transform-bruno-config.spec.jspackages/bruno-filestore/package.jsonpackages/bruno-filestore/src/formats/yml/common/auth-oauth2.tspackages/bruno-filestore/src/formats/yml/parseCollection.tspackages/bruno-filestore/src/formats/yml/stringifyCollection.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation. No tabs, just spaces
Stick to single quotes for strings. For JSX/TSX attributes, use double quotes (e.g., )
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single params
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses
Space before and after the arrow in arrow functions.() => {}is good
No space between function name and parentheses.func()notfunc ()
Semicolons go at the end of the line, not on a new line
Names for functions need to be concise and descriptive
Add in JSDoc comments to add more details to the abstractions if needed
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
packages/bruno-filestore/src/formats/yml/parseCollection.tspackages/bruno-electron/src/utils/transformBrunoConfig.jspackages/bruno-electron/src/app/collections.jspackages/bruno-app/src/components/Preferences/ProxySettings/index.jspackages/bruno-electron/src/app/collection-watcher.jspackages/bruno-electron/src/store/preferences.jspackages/bruno-electron/tests/store/proxy-preferences.spec.jspackages/bruno-filestore/src/formats/yml/stringifyCollection.tspackages/bruno-electron/tests/utils/transform-bruno-config.spec.jspackages/bruno-electron/src/ipc/network/cert-utils.jspackages/bruno-electron/src/ipc/collection.jspackages/bruno-filestore/src/formats/yml/common/auth-oauth2.tspackages/bruno-electron/src/utils/proxy-util.jspackages/bruno-app/src/components/CollectionSettings/ProxySettings/index.js
🧠 Learnings (4)
📚 Learning: 2025-12-02T07:24:50.311Z
Learnt from: bijin-bruno
Repo: usebruno/bruno PR: 6263
File: packages/bruno-requests/src/auth/oauth2-helper.ts:249-249
Timestamp: 2025-12-02T07:24:50.311Z
Learning: In OAuth2 Basic Auth headers for Bruno, clientSecret is optional and can be omitted. When constructing the Authorization header in `packages/bruno-requests/src/auth/oauth2-helper.ts`, use `clientSecret || ''` instead of `clientSecret!` to properly handle cases where only clientId is provided, per community requests.
Applied to files:
packages/bruno-filestore/src/formats/yml/parseCollection.tspackages/bruno-filestore/src/formats/yml/stringifyCollection.tspackages/bruno-electron/src/ipc/collection.jspackages/bruno-filestore/src/formats/yml/common/auth-oauth2.tspackages/bruno-electron/src/utils/proxy-util.js
📚 Learning: 2025-12-17T21:41:24.730Z
Learnt from: naman-bruno
Repo: usebruno/bruno PR: 6407
File: packages/bruno-app/src/components/Environments/ConfirmCloseEnvironment/index.js:5-41
Timestamp: 2025-12-17T21:41:24.730Z
Learning: Do not suggest PropTypes validation for React components in the Bruno codebase. The project does not use PropTypes, so reviews should avoid proposing PropTypes and rely on the existing typing/validation approach (e.g., TypeScript or alternative runtime checks) if applicable. This guideline applies broadly to all JavaScript/JSX components in the repo.
Applied to files:
packages/bruno-electron/src/utils/transformBrunoConfig.jspackages/bruno-electron/src/app/collections.jspackages/bruno-app/src/components/Preferences/ProxySettings/index.jspackages/bruno-electron/src/app/collection-watcher.jspackages/bruno-electron/src/store/preferences.jspackages/bruno-electron/tests/store/proxy-preferences.spec.jspackages/bruno-electron/tests/utils/transform-bruno-config.spec.jspackages/bruno-electron/src/ipc/network/cert-utils.jspackages/bruno-electron/src/ipc/collection.jspackages/bruno-electron/src/utils/proxy-util.jspackages/bruno-app/src/components/CollectionSettings/ProxySettings/index.js
📚 Learning: 2025-12-16T07:16:23.647Z
Learnt from: sanish-bruno
Repo: usebruno/bruno PR: 6090
File: tests/scripting/hooks/init-user-data/ui-state-snapshot.json:1-8
Timestamp: 2025-12-16T07:16:23.647Z
Learning: For e2e tests in the bruno repository: Collections that are shared between CLI and UI tests (comprehensive test suites testing core functionality) should be placed in `packages/bruno-tests/` to avoid duplication. The `tests/**/fixtures/collection` pattern should be used for test-specific collections that test particular UI behaviors or are specific to a single test file.
Applied to files:
packages/bruno-electron/src/app/collections.jspackages/bruno-electron/src/app/collection-watcher.jspackages/bruno-electron/tests/store/proxy-preferences.spec.jspackages/bruno-electron/tests/utils/transform-bruno-config.spec.jspackages/bruno-electron/src/ipc/collection.js
📚 Learning: 2025-12-05T20:31:33.005Z
Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-05T20:31:33.005Z
Learning: Applies to **/*.test.{js,jsx,ts,tsx} : Add tests for any new functionality or meaningful changes. If code is added, removed, or significantly modified, corresponding tests should be updated or created
Applied to files:
packages/bruno-electron/tests/store/proxy-preferences.spec.jspackages/bruno-electron/tests/utils/transform-bruno-config.spec.js
🧬 Code graph analysis (9)
packages/bruno-electron/src/app/collections.js (1)
packages/bruno-electron/src/app/collection-watcher.js (11)
require(5-11)require(12-18)require(19-19)require(21-21)require(22-22)require(23-23)require(24-24)require(25-25)require(28-28)require(29-29)require(30-30)
packages/bruno-app/src/components/Preferences/ProxySettings/index.js (1)
packages/bruno-app/src/components/CollectionSettings/ProxySettings/index.js (1)
passwordVisible(34-34)
packages/bruno-electron/src/store/preferences.js (2)
packages/bruno-cli/src/runner/prepare-request.js (1)
get(1-1)packages/bruno-electron/src/utils/transformBrunoConfig.js (2)
proxy(79-79)newProxy(85-97)
packages/bruno-electron/tests/store/proxy-preferences.spec.js (1)
packages/bruno-electron/src/store/preferences.js (2)
preferences(290-290)getPreferences(226-228)
packages/bruno-filestore/src/formats/yml/stringifyCollection.ts (4)
packages/bruno-electron/src/app/collection-watcher.js (2)
brunoConfig(212-212)brunoConfig(453-453)packages/bruno-electron/src/app/collections.js (1)
brunoConfig(103-103)packages/bruno-electron/src/ipc/network/cert-utils.js (1)
brunoConfig(49-49)packages/bruno-electron/src/utils/collection-import.js (1)
brunoConfig(100-100)
packages/bruno-electron/tests/utils/transform-bruno-config.spec.js (2)
packages/bruno-electron/src/app/collection-watcher.js (11)
require(5-11)require(12-18)require(19-19)require(21-21)require(22-22)require(23-23)require(24-24)require(25-25)require(28-28)require(29-29)require(30-30)packages/bruno-electron/src/app/collections.js (5)
require(3-3)require(5-5)require(6-6)require(7-7)require(8-8)
packages/bruno-electron/src/ipc/network/cert-utils.js (3)
packages/bruno-electron/src/app/collection-watcher.js (2)
brunoConfig(212-212)brunoConfig(453-453)packages/bruno-electron/src/app/collections.js (1)
brunoConfig(103-103)packages/bruno-electron/src/store/preferences.js (1)
preferencesUtil(244-299)
packages/bruno-electron/src/utils/proxy-util.js (1)
packages/bruno-cli/src/runner/run-single-request.js (2)
proxyConfig(290-290)proxyAuthEnabled(318-318)
packages/bruno-app/src/components/CollectionSettings/ProxySettings/index.js (2)
packages/bruno-electron/src/utils/proxy-util.js (2)
hostname(33-33)port(34-34)packages/bruno-app/src/components/Preferences/ProxySettings/index.js (1)
passwordVisible(87-87)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Unit Tests
- GitHub Check: CLI Tests
- GitHub Check: Playwright E2E Tests
- GitHub Check: SSL Tests - macOS
- GitHub Check: SSL Tests - Linux
- GitHub Check: SSL Tests - Windows
- GitHub Check: Playwright E2E Tests
- GitHub Check: SSL Tests - macOS
- GitHub Check: SSL Tests - Windows
🔇 Additional comments (19)
packages/bruno-filestore/package.json (1)
25-25: LGTM!The addition of
@opencollection/typesas a devDependency is consistent with the broader type system updates across the monorepo.packages/bruno-converters/package.json (1)
31-31: LGTM!The version bump to
@opencollection/types@~0.5.0is consistent with other packages in this PR.packages/bruno-filestore/src/formats/yml/common/auth-oauth2.ts (1)
544-547: LGTM!The inverse transformation correctly reads
pkce.disabledand inverts it to set the booleanpkcevalue in Bruno's internal format.package.json (1)
28-31: LGTM!The addition of Storybook 10.x tooling with Webpack 5 support is a development-time enhancement with no runtime impact.
Also applies to: 50-50
packages/bruno-electron/src/ipc/collection.js (1)
65-65: Good catch fixing the import path typo!Corrected
transfomBrunoConfig→transformBrunoConfig.packages/bruno-electron/src/app/collections.js (1)
7-7: Good catch fixing the import path typo!Corrected
transfomBrunoConfig→transformBrunoConfig.packages/bruno-electron/src/app/collection-watcher.js (1)
30-30: Good catch fixing the import path typo!Corrected
transfomBrunoConfig→transformBrunoConfig.packages/bruno-electron/src/utils/proxy-util.js (1)
321-322: Migration logic exists; default behavior change is intentional.The codebase includes migration logic in
transformBrunoConfig.jsandpreferences.jsthat transformsauth.enabled: falsetoauth.disabled: true. However, the behavior change for previously unconfigured setups is intentional:
- Old configs with explicit
auth.enabled: false→ correctly migrated toauth.disabled: true- Old configs with
auth.enabled: trueor undefined → default to auth enabled (changed from disabled by default)The migration comments indicate this behavior was intentional ("If auth.enabled is true or undefined, omit disabled").
packages/bruno-electron/tests/store/proxy-preferences.spec.js (1)
1-297: Solid test coverage for proxy migration scenarios.The test suite comprehensively covers both old formats (
enabledboolean andmodestring) and the new format (inherit/disabled/config). The mock setup withbeforeEachreset ensures proper test isolation.packages/bruno-electron/src/utils/transformBrunoConfig.js (1)
20-30: Clean proxy config cleanup logic.The before-save cleanup correctly removes optional
falsevalues (disabled: falseandauth.disabled: false) to keep the persisted config minimal.packages/bruno-electron/src/ipc/network/cert-utils.js (1)
101-144: Clean refactor of proxy derivation logic.The new disabled/inherit/config semantics are implemented correctly with proper fallback for backward compatibility (
get(collectionProxyConfig, 'config', collectionProxyConfig)). The cascading logic from collection → global → system is well-structured.packages/bruno-filestore/src/formats/yml/parseCollection.ts (1)
53-100: Proxy parsing correctly handles new nested format.The validation ensures only properly formatted proxy configs override the default. The fallback to
inherit: truefor invalid/missing configs is a safe default.Note: Same port type inconsistency as
stringifyCollection.ts- uses''instead ofnullon line 60 and 80.packages/bruno-electron/tests/utils/transform-bruno-config.spec.js (1)
3-569: Excellent test coverage for proxy transformation.The suite comprehensively tests:
- Old format migrations (
enabled: true|false|'global')- New format passthrough
- Edge cases (missing proxy, null ports, SOCKS protocols, missing auth)
- Before-save cleanup
- Round-trip transformations
This ensures the migration logic is robust.
packages/bruno-app/src/components/CollectionSettings/ProxySettings/index.js (2)
84-91: Shallow merge inupdateProxyworks correctly here.The handlers construct complete nested objects before calling
updateProxy, so the shallow spread merge is sufficient. For future maintainability, consider adding a comment clarifying that handlers must provide complete nested objects.
95-105: UI-to-model mapping is correct.The three-way mapping (
inherit,true,false) correctly translates to the newdisabled/inheritflag combinations:
inherit→ disabled: false, inherit: truetrue(enabled) → disabled: false, inherit: falsefalse(disabled) → disabled: true, inherit: falsepackages/bruno-app/src/components/Preferences/ProxySettings/index.js (1)
116-374: Form structure correctly implements new nested proxy model.The mode radio buttons, conditional rendering for system proxy vs custom proxy, and field bindings all correctly use the new
disabled/inherit/configstructure. The Formik integration with debounced auto-save is well-implemented.packages/bruno-electron/src/store/preferences.js (3)
32-44: LGTM!New proxy structure is consistent with the related implementation in
transformBrunoConfig.js.
82-96: LGTM!Schema properly validates the new nested proxy structure. Optional fields (
disabled,auth) and required fields (inherit,config) are correctly defined.
183-191: Clarify intent: Migration not persisted.The migration flag and
store.setare commented out, meaning proxy migration runs on everygetPreferences()call for users with legacy formats. This contrasts with the font size migration (lines 207-212) which persists the migration flag.If intentional (e.g., awaiting schema stabilization), consider adding a comment explaining why. If oversight, uncomment to persist migration state:
🔎 Suggested fix if persistence is intended
- // Mark migration as complete // ? - // if (!preferences._migrations) { - // preferences._migrations = {}; - // } - // preferences._migrations.proxyConfigFormat = true; - - // Save the migrated preferences back to the store - // this.store.set('preferences', preferences); + // Mark migration as complete + if (!preferences._migrations) { + preferences._migrations = {}; + } + preferences._migrations.proxyConfigFormat = true; + + // Save the migrated preferences back to the store + this.store.set('preferences', preferences);
| const systemProxyEnvVariables = useSelector((state) => state.app.systemProxyEnvVariables); | ||
| const { http_proxy, https_proxy, no_proxy } = systemProxyEnvVariables || {}; | ||
| const dispatch = useDispatch(); | ||
| console.log(preferences); |
There was a problem hiding this comment.
Remove debug console.log statement.
This debug statement should be removed before merging.
- console.log(preferences);📝 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.
| console.log(preferences); |
🤖 Prompt for AI Agents
In packages/bruno-app/src/components/Preferences/ProxySettings/index.js around
line 18, there is a leftover debug console.log(preferences); — remove this debug
statement entirely (delete that line) so no console output remains, and ensure
no other stray console.* calls are left in the file by scanning nearby lines.
| config: { | ||
| protocol: preferences.proxy.config?.protocol || 'http', | ||
| hostname: preferences.proxy.config?.hostname || '', | ||
| port: preferences.proxy.config?.port || 0, |
There was a problem hiding this comment.
Inconsistent initial port value: 0 vs ''.
initialValues sets port: 0 (line 48), but the useEffect sync sets port: '' (line 96). This inconsistency can cause unexpected form dirty state on first load or when preferences change.
initialValues: {
// ...
config: {
// ...
- port: preferences.proxy.config?.port || 0,
+ port: preferences.proxy.config?.port || '',Also applies to: 96-96
🤖 Prompt for AI Agents
In packages/bruno-app/src/components/Preferences/ProxySettings/index.js around
lines 48 and 96, the initialValues sets port to 0 while the useEffect sync sets
port to an empty string, creating inconsistent initial form state; make the two
values consistent (preferably change the initialValues port from 0 to '' to
match the effect), and ensure any submit/validation logic casts the string to a
number where needed so form dirtiness and validation behave predictably.
| if (newProxy.config.auth.disabled === false) { | ||
| delete newProxy.config.auth.disabled; | ||
| } |
There was a problem hiding this comment.
Dead code path.
newProxy.config.auth.disabled is only ever set to true at line 168 or remains undefined. This condition will never be true.
- // Omit auth.disabled: false (optional field)
- if (newProxy.config.auth.disabled === false) {
- delete newProxy.config.auth.disabled;
- }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/bruno-electron/src/store/preferences.js around lines 177 to 179, the
branch checking if newProxy.config.auth.disabled === false is dead (disabled is
only set to true or undefined), so remove this unreachable if block; delete
those lines or replace them with the intended logic (e.g., if you meant to clean
up when disabled === true, change the condition to === true), then run tests to
ensure behavior remains correct.
| // Migrate proxy configuration from old format to new format | ||
| if (brunoConfig.proxy) { | ||
| const proxy = brunoConfig.proxy; | ||
|
|
||
| // Check if this is an old format (has 'enabled' property) | ||
| if ('enabled' in proxy) { | ||
| const enabled = proxy.enabled; | ||
|
|
||
| let newProxy = { | ||
| inherit: true, | ||
| config: { | ||
| protocol: proxy.protocol || 'http', | ||
| hostname: proxy.hostname || '', | ||
| port: proxy.port || null, | ||
| auth: { | ||
| username: get(proxy, 'auth.username', ''), | ||
| password: get(proxy, 'auth.password', '') | ||
| }, | ||
| bypassProxy: proxy.bypassProxy || '' | ||
| } | ||
| }; | ||
|
|
||
| // Handle old format: enabled (true | false | 'global') | ||
| if (enabled === true) { | ||
| newProxy.disabled = false; | ||
| newProxy.inherit = false; | ||
| } else if (enabled === false) { | ||
| newProxy.disabled = true; | ||
| newProxy.inherit = false; | ||
| } else if (enabled === 'global') { | ||
| newProxy.disabled = false; | ||
| newProxy.inherit = true; | ||
| } | ||
|
|
||
| // Migrate auth.enabled to auth.disabled | ||
| if (get(proxy, 'auth.enabled') === false) { | ||
| newProxy.config.auth.disabled = true; | ||
| } | ||
| // If auth.enabled is true or undefined, omit disabled (defaults to false) | ||
|
|
||
| // Omit disabled: false at top level (optional field) | ||
| if (newProxy.disabled === false) { | ||
| delete newProxy.disabled; | ||
| } | ||
| // Omit auth.disabled: false (optional field) | ||
| if (newProxy.config.auth.disabled === false) { | ||
| delete newProxy.config.auth.disabled; | ||
| } | ||
|
|
||
| brunoConfig.proxy = newProxy; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's see the full transformBrunoConfig.js file around the migration logic
fd -e js -e ts transformBrunoConfigRepository: usebruno/bruno
Length of output: 115
🏁 Script executed:
#!/bin/bash
# Look for the test file mentioned
fd -e spec.js -e test.js proxy-preferencesRepository: usebruno/bruno
Length of output: 119
🏁 Script executed:
#!/bin/bash
# Search for 'mode' property references in proxy-related code
rg "mode.*['\"]off['\"]|mode.*['\"]on['\"]|mode.*['\"]system['\"]" --type js -A 2 -B 2Repository: usebruno/bruno
Length of output: 3338
🏁 Script executed:
#!/bin/bash
# Check the actual transformBrunoConfig.js file to see the full context
fd transformBrunoConfig -e js -x wc -l {}Repository: usebruno/bruno
Length of output: 121
🏁 Script executed:
#!/bin/bash
# Read the transformBrunoConfig.js file to see the full migration logic
cat -n packages/bruno-electron/src/utils/transformBrunoConfig.jsRepository: usebruno/bruno
Length of output: 5150
🏁 Script executed:
#!/bin/bash
# Read the preferences.js to see if mode migration is there
cat -n packages/bruno-electron/src/store/preferences.js | grep -A 30 "'mode' in proxy"Repository: usebruno/bruno
Length of output: 2225
Add mode property migration to transformBrunoConfig.js.
The mode migration ('off' | 'on' | 'system') is missing from transformBrunoConfigAfterRead. While this logic exists in preferences.js, transformBrunoConfig.js only handles the enabled property format (lines 82-109). If collection configurations use the mode format, they won't be migrated by this utility. Add an else if ('mode' in proxy) branch after line 109 to handle mode values the same way preferences.js does at lines 152-164.
🤖 Prompt for AI Agents
In packages/bruno-electron/src/utils/transformBrunoConfig.js around lines 77 to
128 (insert after the block handling enabled around line 109), add an else if
('mode' in proxy) branch that mirrors the preferences.js mapping: map mode ===
'on' to newProxy.disabled = false and newProxy.inherit = false; mode === 'off'
to newProxy.disabled = true and newProxy.inherit = false; mode === 'system' to
newProxy.disabled = false and newProxy.inherit = true. Keep the existing
auth.enabled -> auth.disabled migration and the subsequent deletion of false
optional fields (top-level disabled and config.auth.disabled) so behavior
matches the enabled-path.
packages/bruno-electron/tests/utils/transform-bruno-config.spec.js
Outdated
Show resolved
Hide resolved
| protocol: brunoConfig.proxy.config.protocol || 'http', | ||
| hostname: brunoConfig.proxy.config.hostname || '', | ||
| port: brunoConfig.proxy.config.port || '', | ||
| auth: { | ||
| username: brunoConfig.proxy.config.auth?.username || '', | ||
| password: brunoConfig.proxy.config.auth?.password || '' | ||
| }, | ||
| bypassProxy: brunoConfig.proxy.config.bypassProxy || '' | ||
| } |
There was a problem hiding this comment.
Port defaults to empty string, inconsistent with other files.
Here port defaults to '' (empty string), but parseCollection.ts and transformBrunoConfig.js use null for missing ports. This inconsistency could cause type mismatches during round-trip.
- port: brunoConfig.proxy.config.port || '',
+ port: brunoConfig.proxy.config.port || null,📝 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.
| protocol: brunoConfig.proxy.config.protocol || 'http', | |
| hostname: brunoConfig.proxy.config.hostname || '', | |
| port: brunoConfig.proxy.config.port || '', | |
| auth: { | |
| username: brunoConfig.proxy.config.auth?.username || '', | |
| password: brunoConfig.proxy.config.auth?.password || '' | |
| }, | |
| bypassProxy: brunoConfig.proxy.config.bypassProxy || '' | |
| } | |
| protocol: brunoConfig.proxy.config.protocol || 'http', | |
| hostname: brunoConfig.proxy.config.hostname || '', | |
| port: brunoConfig.proxy.config.port || null, | |
| auth: { | |
| username: brunoConfig.proxy.config.auth?.username || '', | |
| password: brunoConfig.proxy.config.auth?.password || '' | |
| }, | |
| bypassProxy: brunoConfig.proxy.config.bypassProxy || '' | |
| } |
🤖 Prompt for AI Agents
In packages/bruno-filestore/src/formats/yml/stringifyCollection.ts around lines
99 to 107, the proxy.port is defaulting to an empty string which is inconsistent
with parseCollection.ts and transformBrunoConfig.js that use null for missing
ports; change the default from '' to null so absent ports are serialized as null
(and update any typing/return shape if necessary) to ensure round-trip type
consistency.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/bruno-converters/src/opencollection/bruno-to-opencollection.ts (1)
35-41: Consider filtering empty/undefined proxy objects.When
brunoConfig.proxyexists but all its fields (disabled,inherit,config) areundefined, this creates an empty proxy object onconfig. Depending on the OpenCollection schema validation, this may be undesirable.Consider adding a guard:
if (brunoConfig.proxy) { + const proxy = { + disabled: brunoConfig.proxy.disabled, + inherit: brunoConfig.proxy.inherit, + config: brunoConfig.proxy.config + }; + // Only assign if at least one field is defined + if (proxy.disabled !== undefined || proxy.inherit !== undefined || proxy.config !== undefined) { + config.proxy = proxy; + } - config.proxy = { - disabled: brunoConfig.proxy.disabled, - inherit: brunoConfig.proxy.inherit, - config: brunoConfig.proxy.config - }; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/bruno-converters/src/opencollection/bruno-to-opencollection.tspackages/bruno-converters/src/opencollection/common/auth.tspackages/bruno-converters/src/opencollection/opencollection-to-bruno.tspackages/bruno-converters/src/opencollection/types.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation. No tabs, just spaces
Stick to single quotes for strings. For JSX/TSX attributes, use double quotes (e.g., )
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single params
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses
Space before and after the arrow in arrow functions.() => {}is good
No space between function name and parentheses.func()notfunc ()
Semicolons go at the end of the line, not on a new line
Names for functions need to be concise and descriptive
Add in JSDoc comments to add more details to the abstractions if needed
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
packages/bruno-converters/src/opencollection/bruno-to-opencollection.tspackages/bruno-converters/src/opencollection/common/auth.tspackages/bruno-converters/src/opencollection/opencollection-to-bruno.tspackages/bruno-converters/src/opencollection/types.ts
🧠 Learnings (1)
📚 Learning: 2025-12-02T07:24:50.311Z
Learnt from: bijin-bruno
Repo: usebruno/bruno PR: 6263
File: packages/bruno-requests/src/auth/oauth2-helper.ts:249-249
Timestamp: 2025-12-02T07:24:50.311Z
Learning: In OAuth2 Basic Auth headers for Bruno, clientSecret is optional and can be omitted. When constructing the Authorization header in `packages/bruno-requests/src/auth/oauth2-helper.ts`, use `clientSecret || ''` instead of `clientSecret!` to properly handle cases where only clientId is provided, per community requests.
Applied to files:
packages/bruno-converters/src/opencollection/bruno-to-opencollection.tspackages/bruno-converters/src/opencollection/common/auth.tspackages/bruno-converters/src/opencollection/opencollection-to-bruno.ts
🧬 Code graph analysis (1)
packages/bruno-converters/src/opencollection/common/auth.ts (1)
packages/bruno-electron/src/utils/oauth2.js (1)
oauth2(743-752)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Unit Tests
- GitHub Check: CLI Tests
- GitHub Check: Playwright E2E Tests
- GitHub Check: SSL Tests - Linux
- GitHub Check: SSL Tests - Windows
- GitHub Check: SSL Tests - macOS
🔇 Additional comments (4)
packages/bruno-converters/src/opencollection/common/auth.ts (1)
360-360: LGTM!Clean simplification—PKCE is now represented by presence of
{ method: 'S256' }when enabled,undefinedotherwise. Aligns with the broader move away from explicitenabledflags.packages/bruno-converters/src/opencollection/types.ts (2)
69-72: LGTM!Export reorganization correctly separates
EnvironmentandCollectionConfiginto their respective modules, and adds the new proxy-related types (ProxyConnectionConfig,ProxyConnectionAuth).
178-191: LGTM!The new nested proxy schema cleanly separates state flags (
disabled,inherit) from configuration (config). This structure supports both collection-level and app-level inheritance semantics as described in the PR objectives.packages/bruno-converters/src/opencollection/opencollection-to-bruno.ts (1)
39-45: Same consideration as bruno-to-opencollection.ts.The symmetric transformation is good for round-trip consistency. Apply the same empty-object guard if you address it in the other converter.
| clientSecret: auth.credentials?.clientSecret || null, | ||
| scope: auth.scope || null, | ||
| pkce: auth.pkce?.enabled || null, | ||
| pkce: (auth.pkce && !auth.pkce.disabled) || null, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the auth.ts file and check its location
find . -type f -name "auth.ts" | grep -E "opencollection|bruno-converters"Repository: usebruno/bruno
Length of output: 119
🏁 Script executed:
# Search for BrunoOAuth2 interface definition to determine pkce type
rg "interface BrunoOAuth2|type BrunoOAuth2" -A 20Repository: usebruno/bruno
Length of output: 40
🏁 Script executed:
# Check lines 127 and 360 in the specific file with context
rg -n "pkce" packages/bruno-converters/src/opencollection/common/auth.ts -B 3 -A 3Repository: usebruno/bruno
Length of output: 1207
🏁 Script executed:
# Get the full file to understand the types and context
wc -l packages/bruno-converters/src/opencollection/common/auth.tsRepository: usebruno/bruno
Length of output: 121
🏁 Script executed:
# Search for type definitions in the file
rg "type|interface" packages/bruno-converters/src/opencollection/common/auth.ts | head -20Repository: usebruno/bruno
Length of output: 319
🏁 Script executed:
# Get lines around 127 to understand the function context
sed -n '110,140p' packages/bruno-converters/src/opencollection/common/auth.tsRepository: usebruno/bruno
Length of output: 1522
🏁 Script executed:
# Get lines around 360 to understand the reverse conversion
sed -n '345,370p' packages/bruno-converters/src/opencollection/common/auth.tsRepository: usebruno/bruno
Length of output: 1116
🏁 Script executed:
# Find buildOAuth2Config function definition
rg "buildOAuth2Config|function buildOAuth2Config" packages/bruno-converters/src/opencollection/common/auth.ts -A 30 | head -50Repository: usebruno/bruno
Length of output: 2179
🏁 Script executed:
# Search for imports to find type definitions
head -30 packages/bruno-converters/src/opencollection/common/auth.tsRepository: usebruno/bruno
Length of output: 820
🏁 Script executed:
# Search for BrunoOAuth2 type in the entire codebase
rg "type BrunoOAuth2|interface BrunoOAuth2" -A 25Repository: usebruno/bruno
Length of output: 40
🏁 Script executed:
# Search for types file location
find . -type f -name "types.ts" -o -name "types.d.ts" | grep opencollectionRepository: usebruno/bruno
Length of output: 113
🏁 Script executed:
# Search more broadly for BrunoOAuth2 definition
rg "pkce" packages/bruno-requests/src/auth/ -B 3 -A 3 --type tsRepository: usebruno/bruno
Length of output: 40
🏁 Script executed:
# Check the oauth2-helper file mentioned in learnings
head -100 packages/bruno-requests/src/auth/oauth2-helper.ts | grep -A 20 -B 20 "pkce"Repository: usebruno/bruno
Length of output: 40
🏁 Script executed:
# Read the types file to find BrunoOAuth2 and AuthOAuth2 definitions
cat -n packages/bruno-converters/src/opencollection/types.tsRepository: usebruno/bruno
Length of output: 8181
🏁 Script executed:
# Search for OAuth2 type in schema-types or bruno-requests
find . -type f -name "*.ts" -path "*/schema-types/*" -o -name "*.ts" -path "*/bruno-requests/*" | xargs grep -l "OAuth2\|pkce" 2>/dev/null | head -10Repository: usebruno/bruno
Length of output: 192
🏁 Script executed:
# Look for pkce usage patterns in the codebase
rg "pkce" --type ts -B 2 -A 2 | head -100Repository: usebruno/bruno
Length of output: 4853
🏁 Script executed:
# Confirm the exact structure of auth.pkce in OpenCollection format
rg "auth.pkce" packages/bruno-converters/src/opencollection/common/auth.ts -B 5 -A 5 | head -50Repository: usebruno/bruno
Length of output: 690
🏁 Script executed:
# Check if there's a reverse conversion logic in bruno-filestore that shows the correct pattern
sed -n '1,50p' packages/bruno-filestore/src/formats/yml/common/auth-oauth2.tsRepository: usebruno/bruno
Length of output: 1495
Fix type mismatch: pkce must be boolean, not object.
The expression (auth.pkce && !auth.pkce.disabled) || null returns the auth.pkce object when truthy, but BrunoOAuth2.pkce expects boolean | null. This causes a type error downstream.
Use a ternary to explicitly return a boolean:
- pkce: (auth.pkce && !auth.pkce.disabled) || null,
+ pkce: (auth.pkce && !auth.pkce.disabled) ? true : null,📝 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.
| pkce: (auth.pkce && !auth.pkce.disabled) || null, | |
| pkce: (auth.pkce && !auth.pkce.disabled) ? true : null, |
🤖 Prompt for AI Agents
In packages/bruno-converters/src/opencollection/common/auth.ts around line 127,
the pkce assignment uses (auth.pkce && !auth.pkce.disabled) || null which can
return the auth.pkce object instead of a boolean; change it to explicitly return
a boolean or null (e.g., use a ternary that evaluates auth.pkce and returns
!auth.pkce.disabled ? true : false, or auth.pkce ? !auth.pkce.disabled : null)
so BrunoOAuth2.pkce gets a boolean | null as expected.
2 participants
jira
Description
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
Bug Fixes
Refactor
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.