fix(openAPI):multifile openAPI specs can be imported - #8946
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughChangesExternal API specification references
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to This change adds support for resolving external OpenAPI file references, and no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant ApiSpecLoader
participant resolveExternalApiSpecRefs
participant ReduxStore
participant SpecViewer
participant Swagger
ApiSpecLoader->>resolveExternalApiSpecRefs: Resolve local external $ref files
resolveExternalApiSpecRefs-->>ApiSpecLoader: Return resolvedJson or null
ApiSpecLoader->>ReduxStore: Store json and resolvedJson
ReduxStore->>SpecViewer: Pass content and resolvedSpec
SpecViewer->>Swagger: Render resolvedSpec when available
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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: 2
🧹 Nitpick comments (1)
tests/import/openapi/api-spec-panel-validation.spec.ts (1)
72-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove preview locators into the OpenAPI page module.
Expose locators for the rendered endpoint and resolver error through
tests/utils/page/openapi/*. Keep this spec limited to actions and assertions.As per path instructions: “Centralize locators and actions in page modules under tests/utils/page/*; do not inline raw selectors in specs.”
🤖 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/import/openapi/api-spec-panel-validation.spec.ts` around lines 72 - 73, Move the “Hello endpoint” and “Could not resolve reference” locators into the OpenAPI page module under tests/utils/page/openapi, exposing reusable locator properties or methods for the rendered endpoint and resolver error. Update the spec to use those page-module locators while keeping it limited to actions and assertions, with no inline text selectors.Source: Path instructions
🤖 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-electron/src/app/apiSpecsWatcher.js`:
- Line 33: Update the API specification watcher at
packages/bruno-electron/src/app/apiSpecsWatcher.js:33-33 to collect external
$ref dependency paths while adding the root specification and register those
dependencies with the watcher. At
packages/bruno-electron/src/app/apiSpecsWatcher.js:55-55, map changes to
dependency files back to their owning root specification, re-resolve it, and
publish the refreshed resolvedJson. Add watcher coverage that edits a referenced
file and verifies the root update contains the new resolved operation.
In `@tests/import/openapi/api-spec-panel-validation.spec.ts`:
- Around line 67-68: Update the OpenAPI external-$ref E2E test around
openApiSpecFromDialog to use the shared Playwright fixtures and create both the
root specification and referenced file inside a test-owned temporary directory.
Ensure the temporary directory is removed during cleanup, and pass the generated
root specification path instead of the repository fixture path to prevent
parallel-run collisions.
---
Nitpick comments:
In `@tests/import/openapi/api-spec-panel-validation.spec.ts`:
- Around line 72-73: Move the “Hello endpoint” and “Could not resolve reference”
locators into the OpenAPI page module under tests/utils/page/openapi, exposing
reusable locator properties or methods for the rendered endpoint and resolver
error. Update the spec to use those page-module locators while keeping it
limited to actions and assertions, with no inline text selectors.
🪄 Autofix
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: 2178052a-d227-48c6-9034-02481021ed0b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
packages/bruno-app/src/components/ApiSpecPanel/SpecViewer.jspackages/bruno-app/src/components/ApiSpecPanel/index.jspackages/bruno-app/src/providers/ReduxStore/slices/apiSpec.jspackages/bruno-electron/package.jsonpackages/bruno-electron/src/app/apiSpecs.jspackages/bruno-electron/src/app/apiSpecsWatcher.jspackages/bruno-electron/src/utils/apiSpecs.jspackages/bruno-electron/src/utils/tests/apiSpecs.spec.jspackages/bruno-electron/tests/app/apiSpecs.spec.jstests/import/openapi/api-spec-panel-validation.spec.tstests/import/openapi/fixtures/openapi-multifile-endpoint.yamltests/import/openapi/fixtures/openapi-multifile.yaml
| file.filename = basename; | ||
| file.pathname = pathname; | ||
| file.json = apiSpecContent; | ||
| file.resolvedJson = await resolveExternalApiSpecRefs(apiSpecContent, pathname); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Watch external $ref dependencies.
The watcher observes the root API specification only. If endpoint.yaml changes, it does not trigger a new resolved document. The preview then remains stale until the root specification changes.
packages/bruno-electron/src/app/apiSpecsWatcher.js#L33-L33: collect and register the external reference dependencies when the root specification is added.packages/bruno-electron/src/app/apiSpecsWatcher.js#L55-L55: map dependency-file changes back to the root specification and refresh itsresolvedJson.
Add watcher coverage that edits a referenced file and verifies that the root API-spec update contains the new resolved operation.
📍 Affects 1 file
packages/bruno-electron/src/app/apiSpecsWatcher.js#L33-L33(this comment)packages/bruno-electron/src/app/apiSpecsWatcher.js#L55-L55
🤖 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-electron/src/app/apiSpecsWatcher.js` at line 33, Update the
API specification watcher at
packages/bruno-electron/src/app/apiSpecsWatcher.js:33-33 to collect external
$ref dependency paths while adding the root specification and register those
dependencies with the watcher. At
packages/bruno-electron/src/app/apiSpecsWatcher.js:55-55, map changes to
dependency files back to their owning root specification, re-resolve it, and
publish the refreshed resolvedJson. Add watcher coverage that edits a referenced
file and verifies the root update contains the new resolved operation.
| const openApiFile = path.resolve(__dirname, 'fixtures', 'openapi-multifile.yaml'); | ||
| await openApiSpecFromDialog(page, electronApp, openApiFile); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use a test-owned temporary fixture directory.
Create the root specification and referenced file in an isolated temporary directory. Remove the directory during cleanup. This prevents fixture-path and persisted-spec collisions across parallel Electron test runs.
As per path instructions: “For the new OpenAPI external-$ref E2E test, use the shared Playwright fixtures and keep the test isolated with its own temporary directory and fixture files.”
🤖 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/import/openapi/api-spec-panel-validation.spec.ts` around lines 67 - 68,
Update the OpenAPI external-$ref E2E test around openApiSpecFromDialog to use
the shared Playwright fixtures and create both the root specification and
referenced file inside a test-owned temporary directory. Ensure the temporary
directory is removed during cleanup, and pass the generated root specification
path instead of the repository fixture path to prevent parallel-run collisions.
Source: Path instructions
| }; | ||
|
|
||
| const resolveExternalApiSpecRefs = async (json, apiSpecPath) => { | ||
| if (!containsExternalFileRef(json, path.dirname(apiSpecPath))) return null; |
There was a problem hiding this comment.
containsExternalFileRef here and new $RefParser() on the next line are outside the try below. The recursion can throw a RangeError on a deeply nested spec, and the constructor throws if the dep didn't install. Either one escapes this function, the watcher catch eats it, and the apispec-tree-updated send never runs, so a spec that opened fine before stops opening. Move the try up to cover both lines and return null on failure.
2 participants
Description
REF:BRU-3806
OpenApi Specs having external references ($ref) to different files can now be resolved and previewed.
Problem
Previously OpenAPI specs having external file references could not be resolved
Fix
Since Bruno runs locally, passing the Swagger URL directly to the renderer meant that external files could not be resolved. This PR introduces the external library apidevtools/json-schema-ref-parser(https://github.com/APIDevTools/json-schema-ref-parser), which resolves external files in the Electron app and sends the resolved spec to the renderer. The renderer then passes it to Swagger, which inlines the external references.
Few Reasons to use an External Library
1)Base Paths Change Per Spec File: An external reference is resolved relative to the file and not root , so for each spec its path has to be tracked
2)Repeated refs needs to be cached so that they are not fetched again and again
3)Visited files need to be tracked so that circular references do not crash the app
4)Updating Pointers after inlining content
5)It covers many edge cases that we would otherwise have to handle ourselves.
Screenshots
| Before | After |
|
|
|
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