fix(openapi-sync): simplify IPC calls, fix state priorities, and improve stored spec missing UX#7489
Merged
bijin-bruno merged 6 commits intousebruno:mainfrom Mar 16, 2026
Conversation
- Eliminated unnecessary sourceUrl prop from various components and hooks in the OpenAPISyncTab. - Improved pretty-printing logic in OpenAPISpecTab to handle non-JSON content gracefully. - Updated IPC calls to remove redundant parameters, enhancing code clarity and maintainability.
- Added onTabSelect prop to OpenAPISyncTab for improved tab navigation. - Updated color properties in StyledWrapper for better consistency with theme. - Replaced IconClock with IconAlertTriangle in CollectionStatusSection for clearer status indication. - Enhanced messaging in OverviewSection and SpecStatusSection to provide clearer user guidance. - Introduced handleRestoreSpec function in useSyncFlow for better spec restoration handling.
abhishek-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno and
naman-bruno
as code owners
Contributor
WalkthroughRefactors OpenAPI sync to a single-entry metadata model (brunoConfig.openapi[0]); removes Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Renderer UI
participant IPC as Renderer IPC
participant Main as Main Process (openapi-sync)
participant FS as FileSystem / brunoConfig
UI->>IPC: renderer:read-openapi-spec (collectionPath)
IPC->>Main: handle read with collectionPath
Main->>FS: load brunoConfig for collectionPath
FS-->>Main: brunoConfig (openapi[0].sourceUrl resolved)
Main-->>IPC: specContent + metadata (derived from openapi[0])
IPC-->>UI: return specContent
note right of Main: save flows resolve sourceUrl, posixify paths, and persist single openapi entry
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
- Changed button label from 'restore' to 'spec-details' for better context. - Updated the button text from 'View Details' to 'Go to Spec Updates' to enhance user understanding of navigation options.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
packages/bruno-electron/src/ipc/openapi-sync.js (1)
87-90: Add JSDoc forresolveSourceUrl.This helper is now central to path normalization behavior, so it should be documented as an abstraction contract.
As per coding guidelines "Add JSDoc comments to abstractions for additional details."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-electron/src/ipc/openapi-sync.js` around lines 87 - 90, Add a JSDoc block for the resolveSourceUrl function that documents its purpose (normalize/resolve a source URL or path), parameters (collectionPath: string - base path to resolve against; sourceUrl: string|null|undefined - URL or relative path), return type (string|undefined) and behavior: if sourceUrl is falsy or isValidHttpUrl(sourceUrl) it is returned unchanged, otherwise path.resolve(collectionPath, sourceUrl) is returned; include any thrown errors or side effects and a short usage note about accepting both HTTP URLs and filesystem-relative paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/bruno-app/src/components/OpenAPISyncTab/CollectionStatusSection/index.js`:
- Around line 117-120: Update the "What's tracked" notice text in the
CollectionStatusSection component
(packages/bruno-app/src/components/OpenAPISyncTab/CollectionStatusSection/index.js)
so it accurately reflects the full reset surface: include URL, HTTP method, and
documentation/comments in addition to parameters, headers, body, and auth; edit
the JSX span text (the one next to IconInfoCircle) to list all tracked items and
clarify that variables, scripts, tests, assertions, settings, etc. remain
untracked.
In `@packages/bruno-app/src/components/OpenAPISyncTab/hooks/useSyncFlow.js`:
- Around line 123-128: handleRestoreSpec currently invokes performSync(...,
'sync') which triggers a full sync; change it to call performSync with the
restore/apply action used by the storedSpecMissing flow (e.g., replace the
second argument 'sync' with 'restore' or the specific action your backend
expects) so the request goes through the stored-spec-missing apply path and
backend handles endpoints via the normal "added" merge path; keep localOnlyIds
and endpointDecisions as-is and ensure no modal is shown.
In `@packages/bruno-app/src/components/OpenAPISyncTab/OverviewSection/index.js`:
- Around line 138-145: The stored-spec-missing branch in OverviewSection uses
hasSpecUpdates but ignores delete-only upstream changes in
remoteDrift.localOnly, causing the banner to show when SpecStatusSection
considers there are updates; update the condition so the "stored spec missing"
banner only shows when there are truly no updates including remote deletions —
for example, change the check around specDrift?.storedSpecMissing to also verify
that hasSpecUpdates considers remoteDrift.localOnly (e.g. call hasSpecUpdates
with a flag/include that counts remoteDrift.localOnly or explicitly check
specDrift.remoteDrift?.localOnly === 0) so that delete-only changes prevent the
stored-spec-missing banner from appearing and keep OverviewSection consistent
with SpecStatusSection.
In `@packages/bruno-app/src/components/OpenAPISyncTab/SpecStatusSection/index.js`:
- Around line 35-39: The current hasRemoteUpdates calculation derives "spec
updated" from remoteDrift which misclassifies local-only edits; replace the
remoteDrift length-sum logic used when computing hasRemoteUpdates (and the
similar checks around where remoteDrift is used at functions/blocks referencing
hasRemoteUpdates) with the spec-level change flag (use specDrift.hasChanges or
the result.hasChanges from checkForUpdates) so the UI state is driven by
specDrift.hasChanges rather than remoteDrift; update all occurrences (including
the other two similar blocks that reference remoteDrift/hasRemoteUpdates) to use
specDrift.hasChanges instead.
In `@packages/bruno-electron/src/ipc/openapi-sync.js`:
- Around line 288-291: The relative sourceUrl written by path.relative can
contain Windows backslashes, so change the non-HTTP branch that sets sourceUrl
to produce a POSIX-style path: compute the relative path between collectionPath
and entry.sourceUrl (when isValidHttpUrl is false) and then normalize it to
POSIX separators (replace backslashes with '/') before assigning; update the
expression that constructs sourceUrl (referencing sourceUrl, isValidHttpUrl and
collectionPath) to use this POSIX-normalized relative path so saved configs are
cross-platform.
- Around line 135-137: getSpecEntryForUrl currently always returns the first
entry from getSpecEntriesForCollection(collectionPath) which breaks when
metadata.json contains older historical entries; update getSpecEntryForUrl to
handle legacy multi-entry arrays by: fetch entries via
getSpecEntriesForCollection(collectionPath), if entries.length === 0 return
null, if any entry has an explicit marker (e.g. entry.primary === true or
entry.current === true) return that entry, else if entries contain timestamps
(e.g. entry.savedAt or entry.updatedAt) pick the most recent by that field,
otherwise fall back to the last array item instead of index 0; reference
getSpecEntryForUrl and getSpecEntriesForCollection when making the change.
---
Nitpick comments:
In `@packages/bruno-electron/src/ipc/openapi-sync.js`:
- Around line 87-90: Add a JSDoc block for the resolveSourceUrl function that
documents its purpose (normalize/resolve a source URL or path), parameters
(collectionPath: string - base path to resolve against; sourceUrl:
string|null|undefined - URL or relative path), return type (string|undefined)
and behavior: if sourceUrl is falsy or isValidHttpUrl(sourceUrl) it is returned
unchanged, otherwise path.resolve(collectionPath, sourceUrl) is returned;
include any thrown errors or side effects and a short usage note about accepting
both HTTP URLs and filesystem-relative paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 76ff5eaf-78e9-40e7-8b6c-bac86a8510af
📒 Files selected for processing (10)
packages/bruno-app/src/components/OpenAPISpecTab/index.jspackages/bruno-app/src/components/OpenAPISyncTab/CollectionStatusSection/index.jspackages/bruno-app/src/components/OpenAPISyncTab/OverviewSection/index.jspackages/bruno-app/src/components/OpenAPISyncTab/SpecStatusSection/index.jspackages/bruno-app/src/components/OpenAPISyncTab/StyledWrapper.jspackages/bruno-app/src/components/OpenAPISyncTab/hooks/useOpenAPISync.jspackages/bruno-app/src/components/OpenAPISyncTab/hooks/useSyncFlow.jspackages/bruno-app/src/components/OpenAPISyncTab/index.jspackages/bruno-electron/src/ipc/collection.jspackages/bruno-electron/src/ipc/openapi-sync.js
packages/bruno-app/src/components/OpenAPISyncTab/CollectionStatusSection/index.js
Show resolved
Hide resolved
Comment on lines
+123
to
+128
| const handleRestoreSpec = () => { | ||
| const localOnlyIds = (remoteDrift?.localOnly || []) | ||
| .filter((ep) => specRemovedIds.has(ep.id)) | ||
| .map((ep) => ep.id); | ||
| performSync({ localOnlyIds, endpointDecisions: {} }, 'sync'); | ||
| }; |
Contributor
There was a problem hiding this comment.
handleRestoreSpec() is still doing a full sync.
This skips the modal but still calls performSync(..., 'sync'). In the stored-spec-missing flow that goes through the normal apply path and rewrites spec-managed request fields, so “Restore Spec File” can mutate the collection instead of only recreating the stored spec file.
💡 Suggested change
const handleRestoreSpec = () => {
- const localOnlyIds = (remoteDrift?.localOnly || [])
- .filter((ep) => specRemovedIds.has(ep.id))
- .map((ep) => ep.id);
- performSync({ localOnlyIds, endpointDecisions: {} }, 'sync');
+ performSync({ endpointDecisions: {} }, 'spec-only');
};Based on learnings: in the storedSpecMissing restore flow, the confirm modal is the only review step, and the backend applies those endpoints through the normal added merge path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/components/OpenAPISyncTab/hooks/useSyncFlow.js` around
lines 123 - 128, handleRestoreSpec currently invokes performSync(..., 'sync')
which triggers a full sync; change it to call performSync with the restore/apply
action used by the storedSpecMissing flow (e.g., replace the second argument
'sync' with 'restore' or the specific action your backend expects) so the
request goes through the stored-spec-missing apply path and backend handles
endpoints via the normal "added" merge path; keep localOnlyIds and
endpointDecisions as-is and ensure no modal is shown.
Comment on lines
+138
to
+145
| if (specDrift?.storedSpecMissing && lastSyncDate) { | ||
| return { | ||
| variant: 'warning', | ||
| title: 'Last synced spec not found', | ||
| subtitle: 'The last synced spec is missing in the storage. Restore the latest spec from the source to track collection changes.', | ||
| buttons: ['restore'] | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
Delete-only spec changes still fall through to the “stored spec missing” banner.
This new branch relies on hasSpecUpdates, but in the stored-spec-missing path that count still ignores the remoteDrift.localOnly bucket. If the only upstream change is an endpoint removal, Overview will show “Last synced spec not found” instead of the higher-priority update state. SpecStatusSection in this PR already treats that bucket as an update, so the two tabs can disagree about the same collection.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/components/OpenAPISyncTab/OverviewSection/index.js`
around lines 138 - 145, The stored-spec-missing branch in OverviewSection uses
hasSpecUpdates but ignores delete-only upstream changes in
remoteDrift.localOnly, causing the banner to show when SpecStatusSection
considers there are updates; update the condition so the "stored spec missing"
banner only shows when there are truly no updates including remote deletions —
for example, change the check around specDrift?.storedSpecMissing to also verify
that hasSpecUpdates considers remoteDrift.localOnly (e.g. call hasSpecUpdates
with a flag/include that counts remoteDrift.localOnly or explicitly check
specDrift.remoteDrift?.localOnly === 0) so that delete-only changes prevent the
stored-spec-missing banner from appearing and keep OverviewSection consistent
with SpecStatusSection.
Comment on lines
+35
to
+39
| const hasRemoteUpdates = remoteDrift && ( | ||
| (remoteDrift.missing?.length || 0) | ||
| + (remoteDrift.modified?.length || 0) | ||
| + (remoteDrift.localOnly?.length || 0) | ||
| ) > 0; |
Contributor
There was a problem hiding this comment.
Don’t derive “spec updated” from remoteDrift.
remoteDrift is a collection-vs-current-spec diff, so modified and localOnly also light up for purely local edits when the stored spec file is missing. That makes the new “No updates from the spec / Restore Spec File” state disappear even when the compare call found no upstream changes. checkForUpdates() already has that signal on result.hasChanges, so this should key off specDrift.hasChanges instead.
💡 Suggested change
- const hasRemoteUpdates = remoteDrift && (
- (remoteDrift.missing?.length || 0)
- + (remoteDrift.modified?.length || 0)
- + (remoteDrift.localOnly?.length || 0)
- ) > 0;
+ const hasRemoteUpdates = specDrift?.hasChanges === true;Also applies to: 51-56, 120-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/components/OpenAPISyncTab/SpecStatusSection/index.js`
around lines 35 - 39, The current hasRemoteUpdates calculation derives "spec
updated" from remoteDrift which misclassifies local-only edits; replace the
remoteDrift length-sum logic used when computing hasRemoteUpdates (and the
similar checks around where remoteDrift is used at functions/blocks referencing
hasRemoteUpdates) with the spec-level change flag (use specDrift.hasChanges or
the result.hasChanges from checkForUpdates) so the UI state is driven by
specDrift.hasChanges rather than remoteDrift; update all occurrences (including
the other two similar blocks that reference remoteDrift/hasRemoteUpdates) to use
specDrift.hasChanges instead.
Comment on lines
+135
to
137
| const getSpecEntryForUrl = (collectionPath) => { | ||
| return getSpecEntriesForCollection(collectionPath)[0] || null; | ||
| }; |
Contributor
There was a problem hiding this comment.
Handle legacy multi-entry metadata before forcing index 0.
Using (meta[collectionPath] || [])[0] as the canonical entry can select the wrong stored spec for users who already have multiple historical entries in metadata.json, causing incorrect diff/drift baselines until a re-save happens.
Suggested compatibility patch
-const getSpecEntryForUrl = (collectionPath) => {
- return getSpecEntriesForCollection(collectionPath)[0] || null;
-};
+const getSpecEntryForUrl = (collectionPath, sourceUrl) => {
+ const entries = getSpecEntriesForCollection(collectionPath);
+ if (!entries.length) return null;
+ if (!sourceUrl) return entries[0];
+
+ const resolvedSourceUrl = resolveSourceUrl(collectionPath, sourceUrl);
+ return entries.find((entry) => resolveSourceUrl(collectionPath, entry.sourceUrl) === resolvedSourceUrl) || entries[0];
+};📝 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.
Suggested change
| const getSpecEntryForUrl = (collectionPath) => { | |
| return getSpecEntriesForCollection(collectionPath)[0] || null; | |
| }; | |
| const getSpecEntryForUrl = (collectionPath, sourceUrl) => { | |
| const entries = getSpecEntriesForCollection(collectionPath); | |
| if (!entries.length) return null; | |
| if (!sourceUrl) return entries[0]; | |
| const resolvedSourceUrl = resolveSourceUrl(collectionPath, sourceUrl); | |
| return entries.find((entry) => resolveSourceUrl(collectionPath, entry.sourceUrl) === resolvedSourceUrl) || entries[0]; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-electron/src/ipc/openapi-sync.js` around lines 135 - 137,
getSpecEntryForUrl currently always returns the first entry from
getSpecEntriesForCollection(collectionPath) which breaks when metadata.json
contains older historical entries; update getSpecEntryForUrl to handle legacy
multi-entry arrays by: fetch entries via
getSpecEntriesForCollection(collectionPath), if entries.length === 0 return
null, if any entry has an explicit marker (e.g. entry.primary === true or
entry.current === true) return that entry, else if entries contain timestamps
(e.g. entry.savedAt or entry.updatedAt) pick the most recent by that field,
otherwise fall back to the last array item instead of index 0; reference
getSpecEntryForUrl and getSpecEntriesForCollection when making the change.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/bruno-app/src/components/OpenAPISyncTab/OverviewSection/index.js (1)
138-145:⚠️ Potential issue | 🟠 MajorStored-spec-missing can still appear for delete-only upstream drift.
At Line 138, this branch can still win when the only upstream change is deletions, because
hasSpecUpdatesis derived fromspecUpdatesPending, and the no-stored-spec path does not countremoteDrift.localOnly. That can still make Overview disagree with Spec Updates state priority.Proposed fix
- const specUpdatesPending = hasDriftData - ? (specDrift?.added?.length || 0) + (specDrift?.modified?.length || 0) + (specDrift?.removed?.length || 0) - : (remoteDrift?.modified?.length || 0) + (remoteDrift?.missing?.length || 0); + const specUpdatesPending = hasDriftData + ? (specDrift?.added?.length || 0) + (specDrift?.modified?.length || 0) + (specDrift?.removed?.length || 0) + : (remoteDrift?.modified?.length || 0) + (remoteDrift?.missing?.length || 0) + (remoteDrift?.localOnly?.length || 0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/OpenAPISyncTab/OverviewSection/index.js` around lines 138 - 145, The "Last synced spec not found" branch can wrongly win for delete-only upstream drift; update the conditional that renders this branch to also verify there are truly no spec updates and no local-only remote drift by adding checks against hasSpecUpdates and remoteDrift.localOnly (e.g., change the existing if (specDrift?.storedSpecMissing && lastSyncDate) to also require !hasSpecUpdates && !remoteDrift?.localOnly or alternatively adjust how hasSpecUpdates/specUpdatesPending are computed to account for remoteDrift.localOnly) so Overview and Spec Updates priority stays consistent; reference specDrift, hasSpecUpdates, specUpdatesPending, and remoteDrift.localOnly when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/bruno-app/src/components/OpenAPISyncTab/OverviewSection/index.js`:
- Around line 138-145: The "Last synced spec not found" branch can wrongly win
for delete-only upstream drift; update the conditional that renders this branch
to also verify there are truly no spec updates and no local-only remote drift by
adding checks against hasSpecUpdates and remoteDrift.localOnly (e.g., change the
existing if (specDrift?.storedSpecMissing && lastSyncDate) to also require
!hasSpecUpdates && !remoteDrift?.localOnly or alternatively adjust how
hasSpecUpdates/specUpdatesPending are computed to account for
remoteDrift.localOnly) so Overview and Spec Updates priority stays consistent;
reference specDrift, hasSpecUpdates, specUpdatesPending, and
remoteDrift.localOnly when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a4d342fd-3999-4068-8c05-8bd7249349de
📒 Files selected for processing (1)
packages/bruno-app/src/components/OpenAPISyncTab/OverviewSection/index.js
sanish-bruno
suggested changes
Mar 16, 2026
packages/bruno-app/src/components/OpenAPISyncTab/SpecStatusSection/index.js
Show resolved
Hide resolved
|
|
||
| const resolveSourceUrl = (collectionPath, sourceUrl) => { | ||
| if (!sourceUrl || isValidHttpUrl(sourceUrl)) return sourceUrl; | ||
| return path.resolve(collectionPath, sourceUrl); |
Collaborator
There was a problem hiding this comment.
Are we posixifying the path before storing them in bruno config!
Collaborator
sanish-bruno
left a comment
sanish-bruno
left a comment
There was a problem hiding this comment.
PR Review
Found 3 issue(s): 0 major, 0 minor, 3 nits.
Summary
- Solid simplification of the IPC interface — removing redundant
sourceUrl,brunoConfig, andoldSourceUrlparams is clean and enforces single-spec-per-collection semantics well. - The
mergeSpecIntoRequestfix is a genuine improvement — preserving user scripts, tests, vars, and assertions during non-fullReset sync is the correct behavior. - The state priority reordering in
OverviewSection(spec updates > stored spec missing) makes sense for user actionability. - The
resolveSourceUrl/loadBrunoConfig/saveBrunoConfiground-trip for path resolution is well-designed for git-shareability. - Three minor nits below.
| */ | ||
| const getSpecEntryForUrl = (collectionPath, sourceUrl) => { | ||
| return getSpecEntriesForCollection(collectionPath).find((e) => e.sourceUrl === sourceUrl) || null; | ||
| const getSpecEntryForUrl = (collectionPath) => { |
Collaborator
There was a problem hiding this comment.
Nit: The function name getSpecEntryForUrl is now misleading since it no longer accepts or uses a sourceUrl parameter. Consider renaming to getSpecEntry or getFirstSpecEntry to reflect the new single-spec-per-collection semantics.
Suggested fix:
const getSpecEntry = (collectionPath) => {
return getSpecEntriesForCollection(collectionPath)[0] || null;
};| brunoConfig.openapi = openapi; | ||
| if (brunoConfig.openapi?.[0]) { | ||
| brunoConfig.openapi[0] = { ...brunoConfig.openapi[0], lastSyncDate, specHash }; | ||
| }; |
Collaborator
There was a problem hiding this comment.
Nit: Stray semicolon after the closing } of the if block. Harmless (empty statement) but inconsistent with the rest of the codebase.
Suggested fix:
if (brunoConfig.openapi?.[0]) {
brunoConfig.openapi[0] = { ...brunoConfig.openapi[0], lastSyncDate, specHash };
}
packages/bruno-app/src/components/OpenAPISyncTab/CollectionStatusSection/index.js
Show resolved
Hide resolved
…t logic - Eliminated unnecessary props from OpenAPISyncTab, CollectionStatusSection, and SpecStatusSection for cleaner code. - Removed commented-out code in OverviewSection and SpecStatusSection to enhance readability. - Introduced posixifyPath utility function in filesystem.js to standardize path formatting.
…format - Modified the logic in loadBrunoConfig to handle openapi as an array, ensuring consistent resolution of source URLs for all entries. This change improves the configuration handling for OpenAPI specifications.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
packages/bruno-app/src/components/OpenAPISyncTab/OverviewSection/index.js (1)
71-74:⚠️ Potential issue | 🟠 MajorInclude the missing drift bucket when deriving pending spec updates in stored-spec-missing mode.
Lines 71–74 omit
remoteDrift.localOnly, sohasSpecUpdates(Line 99) can stay false for delete-only upstream changes and incorrectly fall through to the "Last synced spec not found" banner (Lines 138–145).Suggested fix
const specUpdatesPending = hasDriftData ? (specDrift?.added?.length || 0) + (specDrift?.modified?.length || 0) + (specDrift?.removed?.length || 0) - : (remoteDrift?.modified?.length || 0) + (remoteDrift?.missing?.length || 0); + : (remoteDrift?.modified?.length || 0) + (remoteDrift?.missing?.length || 0) + (remoteDrift?.localOnly?.length || 0);Also applies to: 99-99, 138-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/OpenAPISyncTab/OverviewSection/index.js` around lines 71 - 74, The computation of specUpdatesPending (symbol specUpdatesPending) in the stored-spec-missing branch omits the remoteDrift.localOnly bucket, so delete-only upstream changes yield zero and hasSpecUpdates stays false; update the ternary that builds specUpdatesPending to include (remoteDrift?.localOnly?.length || 0) in the stored-spec-missing path, and ensure hasSpecUpdates (the boolean derived from specUpdatesPending) then correctly triggers the existing "spec updates pending" banner logic instead of falling back to the "Last synced spec not found" banner.packages/bruno-app/src/components/OpenAPISyncTab/CollectionStatusSection/index.js (1)
113-113:⚠️ Potential issue | 🟡 MinorAlign “What’s tracked” text with actual sync/reset surface.
Line 113 currently lists only params/headers/body/auth, but the sync merge also manages URL, method, and docs. This understates what “Reset/Revert to Spec” can overwrite.
Suggested copy update
- <span><span className="whats-updated-title">What's tracked:</span> Changes to parameters, headers, body and auth compared to the synced spec. Your variables, scripts, tests, assertions, settings etc. are not tracked here.</span> + <span><span className="whats-updated-title">What's tracked:</span> Changes to URL, method, parameters, headers, body, auth and docs compared to the synced spec. Your variables, scripts, tests, assertions and settings are not tracked here.</span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/OpenAPISyncTab/CollectionStatusSection/index.js` at line 113, Update the "What's tracked" explanatory copy in the CollectionStatusSection component (the span containing className "whats-updated-title") to reflect all fields that the sync/reset affects — include URL, HTTP method, and documentation in addition to parameters, headers, body and auth — so the string accurately describes that Reset/Revert to Spec can overwrite URL, method, params, headers, body, auth and docs (while excluding variables, scripts, tests, assertions, settings).packages/bruno-app/src/components/OpenAPISyncTab/SpecStatusSection/index.js (1)
35-39:⚠️ Potential issue | 🟠 MajorDerive “spec updates available” from spec-level change signal, not
remoteDrift.Lines 35–39 currently treat collection-vs-spec drift as “remote updates.” That can misclassify local-only edits as upstream spec updates and suppress the new “No updates from the spec” restore UX (Lines 51–56, 112).
Suggested fix
- const hasRemoteUpdates = remoteDrift && ( - (remoteDrift.missing?.length || 0) - + (remoteDrift.modified?.length || 0) - + (remoteDrift.localOnly?.length || 0) - ) > 0; + const hasRemoteUpdates = specDrift?.hasChanges === true;Also applies to: 51-56, 112-112
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/OpenAPISyncTab/SpecStatusSection/index.js` around lines 35 - 39, The current hasRemoteUpdates boolean in SpecStatusSection is computed from remoteDrift, which misclassifies local-only edits as upstream spec updates; update hasRemoteUpdates to derive from the spec-level change signal (the prop/state that represents spec changes rather than collection drift) — replace the remoteDrift-based sum with a check against the spec change indicator (e.g., specChanged / specUpdateSignal / prop that signals upstream spec updates) and propagate that same change to the other usages mentioned (the UX branches around lines handling "No updates from the spec" at the two other spots) so those branches rely on the spec-level signal instead of remoteDrift. Ensure you update the variable name usage (hasRemoteUpdates) and any conditional checks in the SpecStatusSection component to reference the spec-level change symbol.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-electron/src/ipc/openapi-sync.js`:
- Around line 398-403: The saveSpecAndUpdateMetadata function currently calls
saveOpenApiSpecFile without ensuring a valid sourceUrl: use the loaded
brunoConfig (from loadBrunoConfig) to validate
brunoConfig?.openapi?.[0]?.sourceUrl before persisting; if missing or falsy,
throw or return a clear error (e.g., throw new Error with context) to fail fast
and avoid saving a spec with no source linkage, then only call
saveOpenApiSpecFile when sourceUrl is present and valid.
- Around line 285-291: The code assumes configToSave.openapi is always an array
and calls .map(), which crashes for legacy object-shaped configs; update the
logic in the openapi normalization to first check
Array.isArray(configToSave.openapi) and, if it's an object, convert it to a
single-element array (or otherwise normalize it to an array) before mapping;
keep the same transformation (using posixifyPath(path.relative(collectionPath,
entry.sourceUrl)) when sourceUrl exists and is not a valid HTTP URL) and apply
it whether openapi was originally an array or a single object so saves no longer
throw.
In `@packages/bruno-electron/src/utils/filesystem.js`:
- Line 504: The posixifyPath helper can throw when p is truthy but not a string;
update posixifyPath to first verify typeof p === 'string' before calling
.replace() and only perform the backslash-to-slash replace for string inputs,
otherwise return p (or an explicit fallback like null/empty string if preferred)
so non-string truthy payloads don't cause a runtime error in functions that call
posixifyPath.
---
Duplicate comments:
In
`@packages/bruno-app/src/components/OpenAPISyncTab/CollectionStatusSection/index.js`:
- Line 113: Update the "What's tracked" explanatory copy in the
CollectionStatusSection component (the span containing className
"whats-updated-title") to reflect all fields that the sync/reset affects —
include URL, HTTP method, and documentation in addition to parameters, headers,
body and auth — so the string accurately describes that Reset/Revert to Spec can
overwrite URL, method, params, headers, body, auth and docs (while excluding
variables, scripts, tests, assertions, settings).
In `@packages/bruno-app/src/components/OpenAPISyncTab/OverviewSection/index.js`:
- Around line 71-74: The computation of specUpdatesPending (symbol
specUpdatesPending) in the stored-spec-missing branch omits the
remoteDrift.localOnly bucket, so delete-only upstream changes yield zero and
hasSpecUpdates stays false; update the ternary that builds specUpdatesPending to
include (remoteDrift?.localOnly?.length || 0) in the stored-spec-missing path,
and ensure hasSpecUpdates (the boolean derived from specUpdatesPending) then
correctly triggers the existing "spec updates pending" banner logic instead of
falling back to the "Last synced spec not found" banner.
In `@packages/bruno-app/src/components/OpenAPISyncTab/SpecStatusSection/index.js`:
- Around line 35-39: The current hasRemoteUpdates boolean in SpecStatusSection
is computed from remoteDrift, which misclassifies local-only edits as upstream
spec updates; update hasRemoteUpdates to derive from the spec-level change
signal (the prop/state that represents spec changes rather than collection
drift) — replace the remoteDrift-based sum with a check against the spec change
indicator (e.g., specChanged / specUpdateSignal / prop that signals upstream
spec updates) and propagate that same change to the other usages mentioned (the
UX branches around lines handling "No updates from the spec" at the two other
spots) so those branches rely on the spec-level signal instead of remoteDrift.
Ensure you update the variable name usage (hasRemoteUpdates) and any conditional
checks in the SpecStatusSection component to reference the spec-level change
symbol.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 950160aa-a703-4b1e-b2e1-16f1bf01c48d
📒 Files selected for processing (7)
packages/bruno-app/src/components/OpenAPISyncTab/CollectionStatusSection/index.jspackages/bruno-app/src/components/OpenAPISyncTab/OpenAPISyncHeader/index.jspackages/bruno-app/src/components/OpenAPISyncTab/OverviewSection/index.jspackages/bruno-app/src/components/OpenAPISyncTab/SpecStatusSection/index.jspackages/bruno-app/src/components/OpenAPISyncTab/index.jspackages/bruno-electron/src/ipc/openapi-sync.jspackages/bruno-electron/src/utils/filesystem.js
💤 Files with no reviewable changes (1)
- packages/bruno-app/src/components/OpenAPISyncTab/OpenAPISyncHeader/index.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bruno-app/src/components/OpenAPISyncTab/index.js
Comment on lines
+285
to
+291
| if (configToSave?.openapi) { | ||
| configToSave.openapi = configToSave.openapi.map((entry) => ({ | ||
| ...entry, | ||
| sourceUrl: (entry.sourceUrl && !isValidHttpUrl(entry.sourceUrl)) | ||
| ? posixifyPath(path.relative(collectionPath, entry.sourceUrl)) | ||
| : entry.sourceUrl | ||
| })); |
Contributor
There was a problem hiding this comment.
Prevent runtime crash when openapi is not an array.
Line 286 assumes configToSave.openapi is always an array. Legacy/manual configs with object shape will throw on .map(), blocking saves.
Proposed fix
- if (configToSave?.openapi) {
- configToSave.openapi = configToSave.openapi.map((entry) => ({
+ if (configToSave?.openapi) {
+ const openapiEntries = Array.isArray(configToSave.openapi) ? configToSave.openapi : [configToSave.openapi];
+ configToSave.openapi = openapiEntries.map((entry = {}) => ({
...entry,
sourceUrl: (entry.sourceUrl && !isValidHttpUrl(entry.sourceUrl))
? posixifyPath(path.relative(collectionPath, entry.sourceUrl))
: entry.sourceUrl
}));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-electron/src/ipc/openapi-sync.js` around lines 285 - 291, The
code assumes configToSave.openapi is always an array and calls .map(), which
crashes for legacy object-shaped configs; update the logic in the openapi
normalization to first check Array.isArray(configToSave.openapi) and, if it's an
object, convert it to a single-element array (or otherwise normalize it to an
array) before mapping; keep the same transformation (using
posixifyPath(path.relative(collectionPath, entry.sourceUrl)) when sourceUrl
exists and is not a valid HTTP URL) and apply it whether openapi was originally
an array or a single object so saves no longer throw.
Comment on lines
+398
to
403
| const saveSpecAndUpdateMetadata = async ({ collectionPath, specContent }) => { | ||
| const { format, brunoConfig, collectionRoot } = loadBrunoConfig(collectionPath); | ||
| const sourceUrl = brunoConfig?.openapi?.[0]?.sourceUrl; | ||
|
|
||
| await saveOpenApiSpecFile({ collectionPath, content: specContent, sourceUrl }); | ||
|
|
Contributor
There was a problem hiding this comment.
Validate sourceUrl before persisting spec metadata.
If sourceUrl is missing at Line 400, this path can persist a stored spec with unusable linkage for source-dependent sync flows. Fail fast with a clear error.
Proposed fix
const saveSpecAndUpdateMetadata = async ({ collectionPath, specContent }) => {
const { format, brunoConfig, collectionRoot } = loadBrunoConfig(collectionPath);
const sourceUrl = brunoConfig?.openapi?.[0]?.sourceUrl;
+ if (!sourceUrl) {
+ throw new Error('OpenAPI sourceUrl is missing in bruno config');
+ }
await saveOpenApiSpecFile({ collectionPath, content: specContent, sourceUrl });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-electron/src/ipc/openapi-sync.js` around lines 398 - 403, The
saveSpecAndUpdateMetadata function currently calls saveOpenApiSpecFile without
ensuring a valid sourceUrl: use the loaded brunoConfig (from loadBrunoConfig) to
validate brunoConfig?.openapi?.[0]?.sourceUrl before persisting; if missing or
falsy, throw or return a clear error (e.g., throw new Error with context) to
fail fast and avoid saving a spec with no source linkage, then only call
saveOpenApiSpecFile when sourceUrl is present and valid.
| return brunoFolders; | ||
| }; | ||
|
|
||
| const posixifyPath = (p) => (p ? p.replace(/\\/g, '/') : p); |
Contributor
There was a problem hiding this comment.
Guard posixifyPath against non-string truthy inputs.
Line 504 can throw when p is truthy but not a string (e.g., malformed config payloads). Add a string check before .replace().
Proposed fix
-const posixifyPath = (p) => (p ? p.replace(/\\/g, '/') : p);
+const posixifyPath = (p) => (typeof p === 'string' ? p.replace(/\\/g, '/') : p);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-electron/src/utils/filesystem.js` at line 504, The
posixifyPath helper can throw when p is truthy but not a string; update
posixifyPath to first verify typeof p === 'string' before calling .replace() and
only perform the backslash-to-slash replace for string inputs, otherwise return
p (or an explicit fallback like null/empty string if preferred) so non-string
truthy payloads don't cause a runtime error in functions that call posixifyPath.
- Updated loadBrunoConfig to ensure openapi is treated as an array, enhancing source URL resolution. - Modified mergeWithUserValues to handle cases where specItems may be undefined, improving robustness in merging user values with specifications.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/bruno-electron/src/ipc/openapi-sync.js (2)
268-274:⚠️ Potential issue | 🟠 MajorNormalize
brunoConfig.openapito an array before downstream[0]access.Line 269 only handles array input. If a legacy/manual config still has object-shaped
openapi, later reads likebrunoConfig?.openapi?.[0](e.g., Line 400/913/1163) evaluate toundefinedand sync logic degrades silently.Proposed defensive normalization
const loadBrunoConfig = (collectionPath) => { const format = getCollectionFormat(collectionPath); let brunoConfig; let collectionRoot; @@ - // Resolve relative openapi sourceUrls to absolute so all callers get consistent paths - if (Array.isArray(brunoConfig?.openapi)) { - brunoConfig.openapi = brunoConfig.openapi.map((entry) => ({ + if (brunoConfig?.openapi && !Array.isArray(brunoConfig.openapi)) { + brunoConfig.openapi = [brunoConfig.openapi]; + } + + // Resolve relative openapi sourceUrls to absolute so all callers get consistent paths + if (Array.isArray(brunoConfig?.openapi)) { + brunoConfig.openapi = brunoConfig.openapi.map((entry = {}) => ({ ...entry, sourceUrl: resolveSourceUrl(collectionPath, entry.sourceUrl) })); }- if (Array.isArray(configToSave?.openapi)) { - configToSave.openapi = configToSave.openapi.map((entry) => ({ + if (configToSave?.openapi) { + const openapiEntries = Array.isArray(configToSave.openapi) ? configToSave.openapi : [configToSave.openapi]; + configToSave.openapi = openapiEntries.map((entry = {}) => ({ ...entry, sourceUrl: (entry.sourceUrl && !isValidHttpUrl(entry.sourceUrl)) ? posixifyPath(path.relative(collectionPath, entry.sourceUrl)) : entry.sourceUrl })); }Also applies to: 285-292
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-electron/src/ipc/openapi-sync.js` around lines 268 - 274, The code currently only maps when brunoConfig.openapi is an array, leaving object-shaped configs vulnerable to later brunoConfig?.openapi?.[0] accesses; change the normalization to ensure brunoConfig.openapi is always an array: if brunoConfig.openapi is an object (and not null), wrap it into an array before mapping; if it's undefined/null set to an empty array; apply the same defensive normalization wherever openapi entries are resolved (the existing resolveSourceUrl mapping block and the similar block around the other openapi handling) so all downstream code that uses brunoConfig.openapi[0] or similar will get a consistent array.
135-136:⚠️ Potential issue | 🟠 MajorAdd a compatibility path for legacy multi-entry metadata before forcing index
0.Line 135 and related reads/writes at Line 375 and Line 1585 always pick entry
[0]. For upgraded users with historical multi-entrymetadata.json, this can bind to the wrong stored spec and can leave old spec files orphaned on disconnect.Suggested migration-safe patch
-const getSpecEntryForUrl = (collectionPath) => { - return getSpecEntriesForCollection(collectionPath)[0] || null; -}; +const getSpecEntryForUrl = (collectionPath, sourceUrl) => { + const entries = getSpecEntriesForCollection(collectionPath); + if (!entries.length) return null; + if (!sourceUrl) return entries[entries.length - 1]; + + const resolved = resolveSourceUrl(collectionPath, sourceUrl); + return entries.find((entry) => resolveSourceUrl(collectionPath, entry.sourceUrl) === resolved) || entries[entries.length - 1]; +};-const existingEntry = (meta[collectionPath] || [])[0]; +const existingEntry = getSpecEntryForUrl(collectionPath, resolvedUrl);-const entry = (meta[collectionPath] || [])[0]; -if (entry && deleteSpecFile) { - const specPath = path.join(getSpecsDir(), entry.filename); - if (fs.existsSync(specPath)) fs.unlinkSync(specPath); -} +const entries = meta[collectionPath] || []; +if (deleteSpecFile) { + for (const entry of entries) { + const specPath = path.join(getSpecsDir(), entry.filename); + if (fs.existsSync(specPath)) fs.unlinkSync(specPath); + } +}Also applies to: 375-389, 1585-1591
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-electron/src/ipc/openapi-sync.js` around lines 135 - 136, The code currently assumes the collection metadata is a single-entry array by returning getSpecEntriesForCollection(collectionPath)[0], which breaks when legacy metadata.json contains multiple entries; update getSpecEntryForUrl to first scan the full array returned by getSpecEntriesForCollection(collectionPath) and find the correct entry by matching a stable key (e.g., entry.url, entry.id, or entry.specName), returning that match if found and only falling back to index 0/null if no match exists; apply the same safe-lookup logic to other places that directly index [0] in metadata reads/writes (the other call sites that assume the first entry) so they locate the right entry before reading or updating, and optionally write-back a normalized single-entry metadata record after a successful match to avoid orphaned spec files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/bruno-electron/src/ipc/openapi-sync.js`:
- Around line 268-274: The code currently only maps when brunoConfig.openapi is
an array, leaving object-shaped configs vulnerable to later
brunoConfig?.openapi?.[0] accesses; change the normalization to ensure
brunoConfig.openapi is always an array: if brunoConfig.openapi is an object (and
not null), wrap it into an array before mapping; if it's undefined/null set to
an empty array; apply the same defensive normalization wherever openapi entries
are resolved (the existing resolveSourceUrl mapping block and the similar block
around the other openapi handling) so all downstream code that uses
brunoConfig.openapi[0] or similar will get a consistent array.
- Around line 135-136: The code currently assumes the collection metadata is a
single-entry array by returning getSpecEntriesForCollection(collectionPath)[0],
which breaks when legacy metadata.json contains multiple entries; update
getSpecEntryForUrl to first scan the full array returned by
getSpecEntriesForCollection(collectionPath) and find the correct entry by
matching a stable key (e.g., entry.url, entry.id, or entry.specName), returning
that match if found and only falling back to index 0/null if no match exists;
apply the same safe-lookup logic to other places that directly index [0] in
metadata reads/writes (the other call sites that assume the first entry) so they
locate the right entry before reading or updating, and optionally write-back a
normalized single-entry metadata record after a successful match to avoid
orphaned spec files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6ddfa346-f12f-46f6-91cb-03f0cf7f3b18
📒 Files selected for processing (1)
packages/bruno-electron/src/ipc/openapi-sync.js
bijin-bruno
approved these changes
Mar 16, 2026
lohit-bruno
pushed a commit
to lohit-bruno/bruno
that referenced
this pull request
Mar 18, 2026
…ove stored spec missing UX (usebruno#7489) * refactor(OpenAPISyncTab): remove unused props and streamline IPC calls - Eliminated unnecessary sourceUrl prop from various components and hooks in the OpenAPISyncTab. - Improved pretty-printing logic in OpenAPISpecTab to handle non-JSON content gracefully. - Updated IPC calls to remove redundant parameters, enhancing code clarity and maintainability. * feat(OpenAPISyncTab): enhance user interaction and visual feedback - Added onTabSelect prop to OpenAPISyncTab for improved tab navigation. - Updated color properties in StyledWrapper for better consistency with theme. - Replaced IconClock with IconAlertTriangle in CollectionStatusSection for clearer status indication. - Enhanced messaging in OverviewSection and SpecStatusSection to provide clearer user guidance. - Introduced handleRestoreSpec function in useSyncFlow for better spec restoration handling. * fix(OpenAPISyncTab): update button labels for clarity in OverviewSection - Changed button label from 'restore' to 'spec-details' for better context. - Updated the button text from 'View Details' to 'Go to Spec Updates' to enhance user understanding of navigation options. * refactor(OpenAPISyncTab): remove unused props and streamline component logic - Eliminated unnecessary props from OpenAPISyncTab, CollectionStatusSection, and SpecStatusSection for cleaner code. - Removed commented-out code in OverviewSection and SpecStatusSection to enhance readability. - Introduced posixifyPath utility function in filesystem.js to standardize path formatting. * fix(OpenAPISyncTab): update openapi config handling to support array format - Modified the logic in loadBrunoConfig to handle openapi as an array, ensuring consistent resolution of source URLs for all entries. This change improves the configuration handling for OpenAPI specifications. * fix(OpenAPISyncTab): improve openapi config handling and merge logic - Updated loadBrunoConfig to ensure openapi is treated as an array, enhancing source URL resolution. - Modified mergeWithUserValues to handle cases where specItems may be undefined, improving robustness in merging user values with specifications.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
JIRA
sourceUrl,brunoConfig, andoldSourceUrlparams from IPC handlers — source URL is now always read frombrunoConfigon disk, enforcing single-spec-per-collection semanticsresolveSourceUrlhelper;loadBrunoConfigresolves relativesourceUrlto absolute internally,saveBrunoConfigconverts back to relative for git-shareabilityhandleRestoreSpecinuseSyncFlowbypasses the confirmation modal for a faster restore experienceupdateStoredSpecnow clears meta when spec is null, preventing stale title/version/count after spec file deletionsourceUrlparam from read-spec IPC calltheme.colors.text.subtext0/mutedwiththeme.textin StyledWrapperContribution 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
Style
Other