Conversation
|
Warning Rate limit exceeded@sanish-bruno has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 16 minutes and 14 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)
WalkthroughgRPC protobuf import-path resolution was moved from the renderer to the Electron main IPC handler; the renderer now sends Changes
Sequence Diagram(s)sequenceDiagram
participant App as bruno-app (Renderer)
participant IPC as bruno-electron (Main / IPC)
participant Grpc as bruno-requests (gRPC Client)
App->>IPC: invoke('grpc:load-methods-proto', { filePath, collection })
IPC->>IPC: getProtobufIncludeDirs(collection)
IPC->>Grpc: loadMethodsFromProtoFile({ filePath, includeDirs })
Grpc->>Grpc: resolve imports & parse proto using includeDirs
Grpc-->>IPC: return methods / error
IPC-->>App: return result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/bruno-electron/src/ipc/network/grpc-event-handlers.js (1)
93-101: Consider extracting duplicatedincludeDirslogic into a helper function.This logic is duplicated in the
grpc:load-methods-protohandler (lines 278-286). Extract into a reusable helper to reduce duplication.♻️ Suggested helper function
+/** + * Extracts and resolves include directories from collection's protobuf config + * @param {Object} collection - The collection object + * @returns {string[]} Resolved include directory paths + */ +const getIncludeDirs = (collection) => { + if (!collection) return []; + const protobufConfig = collection.draft?.brunoConfig?.protobuf || collection.brunoConfig?.protobuf; + if (!protobufConfig?.importPaths) return []; + return protobufConfig.importPaths + .filter((importPath) => importPath.enabled !== false) + .map((importPath) => normalizeAndResolvePath(path.resolve(collection.pathname, importPath.path))); +};Then replace both occurrences:
- // Extract import paths from collection's protobuf config (synchronous, no IPC needed) - let includeDirs = []; - const protobufConfig = collection.draft?.brunoConfig?.protobuf || collection.brunoConfig?.protobuf; - if (protobufConfig?.importPaths) { - includeDirs = protobufConfig.importPaths - .filter((importPath) => importPath.enabled !== false) - .map((importPath) => normalizeAndResolvePath(path.resolve(collection.pathname, importPath.path))); - } + const includeDirs = getIncludeDirs(collection);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/bruno-app/src/utils/network/index.jspackages/bruno-electron/src/ipc/network/grpc-event-handlers.jspackages/bruno-requests/src/grpc/grpc-client.js
🧰 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-requests/src/grpc/grpc-client.jspackages/bruno-app/src/utils/network/index.jspackages/bruno-electron/src/ipc/network/grpc-event-handlers.js
🧠 Learnings (1)
📚 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-requests/src/grpc/grpc-client.jspackages/bruno-app/src/utils/network/index.jspackages/bruno-electron/src/ipc/network/grpc-event-handlers.js
🧬 Code graph analysis (2)
packages/bruno-requests/src/grpc/grpc-client.js (1)
packages/bruno-cli/src/utils/bru.js (1)
protoPath(100-100)
packages/bruno-app/src/utils/network/index.js (2)
packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js (1)
window(2756-2756)packages/bruno-app/src/utils/network/ws-event-listeners.js (1)
window(8-8)
⏰ 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: SSL Tests - Linux
- GitHub Check: SSL Tests - Windows
- GitHub Check: SSL Tests - macOS
- GitHub Check: CLI Tests
- GitHub Check: Unit Tests
- GitHub Check: Playwright E2E Tests
🔇 Additional comments (8)
packages/bruno-app/src/utils/network/index.js (1)
142-147: LGTM!Clean migration of import path resolution to the IPC layer. The optional
collectionparameter withnulldefault maintains backward compatibility while enabling collection-based path resolution when available.packages/bruno-requests/src/grpc/grpc-client.js (4)
319-323: LGTM!JSDoc accurately documents the new
includeDirsparameter, and the signature update with default empty array is appropriate.
343-344: Correct use ofpath.resolvefor proto path resolution.Using
nodePath.resolveinstead ofpath.joincorrectly handles both relative and absolute proto paths. IfprotoPathis absolute, it's returned as-is; if relative, it's resolved againstcollectionPath.
467-479: LGTM!Signature update with
includeDirs = []maintains backward compatibility. JSDoc is properly updated.
515-516: LGTM!Proper propagation of
includeDirsto the fallback refresh path ensures consistent proto loading behavior.packages/bruno-electron/src/ipc/network/grpc-event-handlers.js (3)
11-11: LGTM!Import added for
normalizeAndResolvePathutility.
112-113: LGTM!
includeDirscorrectly passed to the gRPC client'sstartConnectionmethod.
275-288: Handler signature update aligns with client-side changes.The change from
{ filePath, includeDirs }to{ filePath, collection }correctly centralizes import path resolution in the IPC layer. The null-check oncollectionprovides graceful fallback to emptyincludeDirs.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/bruno-electron/src/ipc/network/grpc-event-handlers.js (1)
26-26: Consider aligning with the established pattern for accessing protobuf config.The codebase uses a different pattern for accessing protobuf configuration (see
bruno-app/src/components/CollectionSettings/index.js:58-60andbruno-app/src/hooks/useProtoFileManagement/index.js:23-25):const protobufConfig = collection.draft?.brunoConfig ? get(collection, 'draft.brunoConfig.protobuf', {}) : get(collection, 'brunoConfig.protobuf', {});While your current approach works, consistency across the codebase improves maintainability.
♻️ Proposed refactor for consistency
- const protobufConfig = collection.draft?.brunoConfig?.protobuf || collection.brunoConfig?.protobuf; + const protobufConfig = collection.draft?.brunoConfig + ? get(collection, 'draft.brunoConfig.protobuf', {}) + : get(collection, 'brunoConfig.protobuf', {});
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/bruno-electron/src/ipc/network/grpc-event-handlers.js
🧰 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-electron/src/ipc/network/grpc-event-handlers.js
🧠 Learnings (1)
📚 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/ipc/network/grpc-event-handlers.js
⏰ 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: SSL Tests - macOS
- GitHub Check: Playwright E2E Tests
- GitHub Check: CLI Tests
- GitHub Check: SSL Tests - Linux
- GitHub Check: SSL Tests - Windows
🔇 Additional comments (5)
packages/bruno-electron/src/ipc/network/grpc-event-handlers.js (5)
11-11: LGTM!The import of
normalizeAndResolvePathsupports cross-OS path normalization as intended by the PR objectives.
112-124: LGTM!The addition of
includeDirsresolution and passing it togrpcClient.startConnectionaligns perfectly with the PR objective to ensure import paths are respected during gRPC connection start.
288-290: LGTM!Computing
includeDirsfrom the collection and passing it toloadMethodsFromProtoFilecorrectly implements the import-path resolution in the IPC layer as intended.
286-286: All renderer calls togrpc:load-methods-protoare correctly updated.The single call site in
packages/bruno-app/src/utils/network/index.js(line 146) properly uses the new signature, passing{ filePath, collection }as expected. No instances of the oldincludeDirsparameter remain.
32-32: The double resolution is intentional and not redundant.The first
path.resolve(collection.pathname, importPath.path)resolves relative import paths against the collection directory. The subsequentnormalizeAndResolvePath()call applies cross-OS normalization (WSL handling), resolves symbolic links, and ensures the path is properly resolved. These are two distinct operations: context-aware resolution followed by platform-specific normalization.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @packages/bruno-electron/src/ipc/network/grpc-event-handlers.js:
- Around line 22-26: The getProtobufIncludeDirs function uses
collection.pathname later without guarding against it; add a defensive check for
collection.pathname (e.g., if (!collection.pathname) return [] or skip adding
pathname-based include dirs) after the existing null check for collection so
callers won't get runtime errors; update logic that builds include dirs
(referencing getProtobufIncludeDirs and protobufConfig) to only push or
reference collection.pathname when it is defined/truthy.
- Line 32: The map call currently double-resolves paths by passing
path.resolve(collection.pathname, importPath.path) into normalizeAndResolvePath;
change the mapping to join the segments instead and let normalizeAndResolvePath
perform resolution (e.g., use path.join(collection.pathname, importPath.path)
inside the map) or move the pathname combination logic into
normalizeAndResolvePath so the call becomes
normalizeAndResolvePath(collection.pathname, importPath.path); update the .map
callback that references importPath.path to use path.join or the new
normalizeAndResolvePath signature and remove the redundant path.resolve usage.
🧹 Nitpick comments (1)
packages/bruno-electron/src/ipc/network/grpc-event-handlers.js (1)
26-26: Consider matching the app layer's protobuf config access pattern.The app layer uses a different pattern to access protobuf config (see
packages/bruno-app/src/components/CollectionSettings/index.jsandpackages/bruno-app/src/hooks/useProtoFileManagement/index.js):const protobufConfig = collection.draft?.brunoConfig ? get(collection, 'draft.brunoConfig.protobuf', {}) : get(collection, 'brunoConfig.protobuf', {});Your implementation uses
||fallback, which behaves differently whendraft.brunoConfigexists butdraft.brunoConfig.protobufis undefined. For consistency and to avoid subtle bugs, consider matching the established pattern.♻️ Align with app layer pattern
- const protobufConfig = collection.draft?.brunoConfig?.protobuf || collection.brunoConfig?.protobuf; + const protobufConfig = collection.draft?.brunoConfig + ? get(collection, 'draft.brunoConfig.protobuf', {}) + : get(collection, 'brunoConfig.protobuf', {});Based on learnings, the project uses
lodash/getfor safe property access in similar scenarios.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/bruno-electron/src/ipc/network/grpc-event-handlers.js
🧰 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-electron/src/ipc/network/grpc-event-handlers.js
🧠 Learnings (1)
📚 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/ipc/network/grpc-event-handlers.js
🧬 Code graph analysis (1)
packages/bruno-electron/src/ipc/network/grpc-event-handlers.js (2)
packages/bruno-app/src/components/CollectionSettings/index.js (1)
protobufConfig(59-61)packages/bruno-app/src/hooks/useProtoFileManagement/index.js (1)
protobufConfig(24-26)
⏰ 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: SSL Tests - Windows
- GitHub Check: CLI Tests
- GitHub Check: Playwright E2E Tests
- GitHub Check: SSL Tests - Linux
- GitHub Check: Unit Tests
- GitHub Check: SSL Tests - macOS
🔇 Additional comments (4)
packages/bruno-electron/src/ipc/network/grpc-event-handlers.js (4)
11-11: LGTM: Import added for path normalization.The import is necessary and follows project conventions.
112-113: LGTM: includeDirs computation added correctly.Good placement to compute the include directories before starting the gRPC connection. This addresses the missing includeDirs issue mentioned in the PR objectives.
123-125: LGTM: includeDirs properly integrated into startConnection.The includeDirs parameter is correctly passed to
grpcClient.startConnection, completing the integration for the import path fix.
286-290: LGTM: Successfully moved import-path resolution to IPC layer.The handler signature change from
{ filePath, includeDirs }to{ filePath, collection }successfully moves the responsibility of computing include directories from the app/renderer layer to the IPC layer, achieving a key PR objective. This centralizes the path resolution logic and ensures consistent handling across gRPC operations.
| const getProtobufIncludeDirs = (collection) => { | ||
| if (!collection) { | ||
| return []; | ||
| } | ||
| const protobufConfig = collection.draft?.brunoConfig?.protobuf || collection.brunoConfig?.protobuf; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Validate collection.pathname exists.
The function uses collection.pathname at line 32 without validating it exists. Consider adding a check to prevent potential runtime errors.
🛡️ Proposed defensive check
const getProtobufIncludeDirs = (collection) => {
if (!collection) {
return [];
}
+ if (!collection.pathname) {
+ return [];
+ }
const protobufConfig = collection.draft?.brunoConfig?.protobuf || collection.brunoConfig?.protobuf;🤖 Prompt for AI Agents
In @packages/bruno-electron/src/ipc/network/grpc-event-handlers.js around lines
22 - 26, The getProtobufIncludeDirs function uses collection.pathname later
without guarding against it; add a defensive check for collection.pathname
(e.g., if (!collection.pathname) return [] or skip adding pathname-based include
dirs) after the existing null check for collection so callers won't get runtime
errors; update logic that builds include dirs (referencing
getProtobufIncludeDirs and protobufConfig) to only push or reference
collection.pathname when it is defined/truthy.
| } | ||
| return protobufConfig.importPaths | ||
| .filter((importPath) => Boolean(importPath.enabled)) | ||
| .map((importPath) => normalizeAndResolvePath(path.resolve(collection.pathname, importPath.path))); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for normalizeAndResolvePath function definition
rg -t js -t ts "normalizeAndResolvePath" --max-count 20Repository: usebruno/bruno
Length of output: 2149
🏁 Script executed:
#!/bin/bash
# View normalizeAndResolvePath implementation
sed -n '/^const normalizeAndResolvePath = /,/^};/p' packages/bruno-electron/src/utils/filesystem.js | head -50Repository: usebruno/bruno
Length of output: 566
Consider removing the redundant path.resolve() call before normalizeAndResolvePath().
The code currently resolves the path with path.resolve(collection.pathname, importPath.path) before passing it to normalizeAndResolvePath(). Since normalizeAndResolvePath() already calls path.resolve() internally (except for WSL and symlink cases), this creates redundant normalization in the common case.
Refactor to:
.map((importPath) => normalizeAndResolvePath(path.join(collection.pathname, importPath.path)))or move the pathname combination into normalizeAndResolvePath() if that function should handle path resolution.
🤖 Prompt for AI Agents
In @packages/bruno-electron/src/ipc/network/grpc-event-handlers.js at line 32,
The map call currently double-resolves paths by passing
path.resolve(collection.pathname, importPath.path) into normalizeAndResolvePath;
change the mapping to join the segments instead and let normalizeAndResolvePath
perform resolution (e.g., use path.join(collection.pathname, importPath.path)
inside the map) or move the pathname combination logic into
normalizeAndResolvePath so the call becomes
normalizeAndResolvePath(collection.pathname, importPath.path); update the .map
callback that references importPath.path to use path.join or the new
normalizeAndResolvePath signature and remove the redundant path.resolve usage.
Description
Fixes: #6724
Jira
const absoluteProtoPath = nodePath.join(collectionPath, protoPath)to get the absolute path , I think const absoluteProtoPath = nodePath.resolve(collectionPath, protoPath) suits better hereContribution 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
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.