Conversation
Add support for temporary "scratch" requests at the workspace level that are not tied to any collection. These requests are stored in a temp directory and displayed as tabs in the workspace home. Key changes: - Add IPC handlers for mounting scratch directory and creating requests - Add scratch directory watcher in collection-watcher.js - Extend workspaces Redux slice with scratch state and reducers - Add IPC listeners for scratch request events - Create ScratchRequestPane and CreateScratchRequest components - Update WorkspaceTabs with "+" button for creating scratch requests - Update WorkspaceHome to render scratch request tabs - Filter scratch collections from sidebar display Supports all request types: HTTP, GraphQL, gRPC, and WebSocket.
- Use Button component with ghost variant and primary color - Position button inside scrollable tab area - Vertically center button with tabs - Clean up unnecessary CSS properties
- Add centralized scratch path tracking in backend (scratchCollectionPaths Set) - Register scratch paths when created via renderer:mount-workspace-scratch - Set brunoConfig.type='scratch' based on registered paths instead of string pattern - Store scratchTempDirectory path in workspace state for frontend validation - Update schema to accept 'scratch' as valid collection type - Simplify frontend filtering to use brunoConfig.type or path comparison - Remove fragile 'bruno-scratch-' string pattern matching - Prevent scratch collections from being added to workspace.collections
…e tabs - Remove CreateScratchRequest component in favor of reusing CreateTransientRequest - Register scratch collection temp directory via addTransientDirectory for transient request creation - Add scratch collection item sync with workspace tabs - Display HTTP method with color on scratch request tabs
Remove separate WorkspaceTabs system and integrate workspace views (Overview, Environments) into the unified RequestTabs architecture using scratch collections. Key changes: - Remove WorkspaceTabs component and Redux slice - Add workspaceOverview and workspaceEnvironments as special tab types - Create WorkspaceHeader component to display workspace name in toolbar - Make workspace tabs non-closable and always present - Update tab creation on workspace switch to automatically add Overview and Environments tabs - Simplify WorkspaceHome component by removing redundant header - Update all references from WorkspaceTabs to unified tab system Benefits: - Single tab system for all content (collections and workspace views) - Consistent UX with unified navigation pattern - Reduced code complexity (~1000+ lines removed) - Easier maintenance and feature development
…equests - Add updateCollectionMountStatus to properly set scratch collection mount status to 'mounted' - Create new renderer:add-collection-watcher IPC handler for explicit watcher setup - Move workspace tab type checks before collection validation in RequestTabPanel - Update mountScratchCollection to use explicit watcher setup instead of open-multiple-collections This ensures the task middleware recognizes scratch collections as fully mounted, allowing transient requests to automatically open in tabs when created.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR removes the legacy workspace-tabs subsystem and replaces workspace-specific pages with tab-based workspaceOverview/workspaceEnvironments tabs; adds scratch-collection support (mounting, watchers, saving scratch requests) via new IPC handlers and Redux actions; introduces CollectionHeader and collection-picker UI for saving transient requests; and updates tests and teardown flow. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as CollectionHeader (UI)
participant Redux as Redux (workspaces/tabs/collections)
participant IPC as Electron IPC
participant FS as File System / Temp Dir
rect rgba(120, 200, 240, 0.5)
UI->>Redux: dispatch mountScratchCollection(workspaceUid)
Redux->>IPC: invoke renderer:mount-workspace-scratch({workspaceUid, workspacePath})
IPC->>FS: create temp directory, write opencollection.yml, register path
FS-->>IPC: tempPath
IPC-->>Redux: return tempPath
Redux->>Redux: create scratch collection entry, setWorkspaceScratchCollection(uid, tempPath)
Redux->>IPC: invoke renderer:add-collection-watcher({collectionPath, collectionUid})
IPC-->>Redux: watcher registered
Redux->>Redux: dispatch addTab({ type: 'workspaceOverview', uid: overviewUid, collectionUid: scratchUid })
UI-->>Redux: focusTab(overviewUid)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…requests Add multi-step save flow for scratch collection requests with collection selection before folder selection. Includes breadcrumb navigation showing "Collections > [Selected Collection] > [Folders...]" that allows users to navigate back to collection selector. Refactor scratch collection detection to use workspace.scratchCollectionUid instead of persisting type to brunoConfig, providing cleaner separation of concerns without disk persistence. Add backend support for automatic format conversion when saving from YAML scratch collections to BRU collections.
- Create dedicated openScratchCollectionEvent function for scratch collections - Revert openCollectionEvent to clean state without scratch-specific logic - Simplify closeTabs and closeAllCollectionTabs reducers in tabs slice - Remove unused isScratchCollectionPath helper function
chirag-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno and
naman-bruno
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bruno-app/src/components/RequestTabs/RequestTab/index.js (1)
175-179:⚠️ Potential issue | 🟠 MajorWorkspace tabs can be closed via middle-click despite being "non-closable".
The
StyledWrapperat line 179 hasonMouseUp={handleMouseUp}, which dispatchescloseTabson middle-click (button === 1). Since workspace tabs (workspaceOverview,workspaceEnvironments) are intended to be non-closable, middle-click should be suppressed for these tab types.Proposed fix
<StyledWrapper className={`flex items-center justify-between tab-container px-2 ${tab.preview ? 'italic' : ''}`} - onMouseUp={handleMouseUp} + onMouseUp={['workspaceOverview', 'workspaceEnvironments'].includes(tab.type) ? undefined : handleMouseUp} >
🤖 Fix all issues with AI agents
In `@packages/bruno-app/src/components/RequestTabPanel/index.js`:
- Around line 56-57: The new useSelector call (useSelector retrieving workspaces
and activeWorkspaceUid, and the derived activeWorkspace) is placed after an
early return guarded by typeof window == 'undefined', violating the Rules of
Hooks; move all hook calls for this component (including useSelector and any
other hooks used in RequestTabPanel) above the early-return conditional so hooks
run unconditionally, and refactor the SSR guard (the typeof window ==
'undefined' check) to use a precomputed variable or conditional rendering after
hooks (e.g., compute isServer before hooks or return null from render logic that
runs after hooks) to preserve SSR safety while keeping hooks at the top of the
component.
In `@packages/bruno-app/src/components/RequestTabs/WorkspaceHeader/index.js`:
- Around line 86-94: The validation in validateWorkspaceName checks name.length
before trimming, so inputs like 256 spaces are rejected for length instead of
being treated as empty; update validateWorkspaceName to operate on the trimmed
value (e.g., compute const trimmed = name?.trim() and use trimmed for the empty
and length checks) or reorder checks to trim first, then validate trimmed === ''
and trimmed.length > 255, returning the same error messages from the function.
In `@packages/bruno-app/src/components/SaveTransientRequest/index.js`:
- Around line 40-47: The fallback object in availableCollections uses uid: null
which leads handleSelectCollection (via selectedCollection.uid || uuid()) to
generate transient random UUIDs and can cause duplicate Redux entries when the
file-watcher later assigns a real UID; instead assign a stable deterministic UID
when building availableCollections (e.g., derive a hash or canonical string from
wc.path or wc.pathname) or cache/pre-assign a UID there so mountCollection
receives that same stable identifier; update availableCollections creation to
set uid to that deterministic value and ensure
handleSelectCollection/mountCollection use the provided uid rather than
generating a new uuid().
In `@packages/bruno-app/src/providers/App/useIpcEvents.js`:
- Around line 274-291: The addTab call in removeShowPreferencesListener
currently uses the explicit assignment "collectionUid: collectionUid"; change it
to ES6 shorthand by passing just "collectionUid" in the addTab payload (keep the
uid logic the same), so update the object passed to addTab inside
removeShowPreferencesListener to use the shorthand form for the collectionUid
property while leaving the rest unchanged.
In `@packages/bruno-app/src/providers/ReduxStore/slices/collections/actions.js`:
- Around line 2442-2474: The thunk openScratchCollectionEvent currently calls
ipcRenderer.invoke('renderer:get-collection-security-config', pathname) without
handling rejections so the outer Promise can hang; update
openScratchCollectionEvent to attach a .catch handler (or use async/await with
try/catch) to the ipcRenderer.invoke call and call reject(error) when it fails,
and ensure any errors from collectionSchema.validate or
dispatch(_createCollection(...)) still propagate to reject; also add a JSDoc
block above openScratchCollectionEvent describing parameters (uid, pathname,
brunoConfig), return value (Promise) and behavior.
In `@packages/bruno-app/src/providers/ReduxStore/slices/tabs.js`:
- Around line 208-210: When closing all tabs for a collection (handling
action.payload.collectionUid) the reducer currently resets state.activeTabUid
unconditionally; instead, capture the previous activeTabUid (state.activeTabUid)
before filtering state.tabs, then filter tabs as done now, and only reassign
state.activeTabUid if the previous active tab was removed (i.e., no tab in the
new state.tabs has uid === prevActive); otherwise keep prevActive; if prevActive
was removed set state.activeTabUid = state.tabs.length > 0 ?
last(state.tabs).uid : null.
In `@packages/bruno-electron/src/ipc/collection.js`:
- Around line 1921-1957: The save-scratch-request handler doesn't update the
example UID cache; after successfully writing the new scratch file (after
writeFile(targetPathname, content)) call syncExampleUidsCache for the affected
collection (e.g., syncExampleUidsCache(collectionPath)) so the example UID
mappings stay in sync; ensure you import/require syncExampleUidsCache if needed
and only call it on successful save before returning { newPathname:
targetPathname }.
- Around line 393-429: In the ipcMain handler for
'renderer:save-transient-request' ensure the chosen filename's extension matches
the targetFormat when targetFilename is omitted (and optionally enforce when
provided): use path.parse(sourcePathname) to get base name, map targetFormat ->
desired extension (e.g., 'bru' -> '.bru', 'yml' -> '.yml'), then set filename =
`${name}${desiredExt}` instead of using path.basename(sourcePathname) directly;
update references to filename/targetPathname so converted files get the correct
extension and watchers will pick them up (symbols: targetFilename, filename,
targetFormat, actualSourceFormat, targetPathname in the handler).
🧹 Nitpick comments (10)
packages/bruno-app/src/components/StatusBar/index.js (1)
42-48: Trailing comma on Line 46 violates coding guidelines.Proposed fix
dispatch( addTab({ type: 'preferences', uid: collectionUid ? `${collectionUid}-preferences` : 'preferences', - collectionUid: collectionUid + collectionUid }) );Also used shorthand property since key and value share the same name.
As per coding guidelines: "No trailing commas in code".
packages/bruno-app/src/hooks/useCollectionFolderTree/index.js (1)
66-70: Consider resetting via akeyprop instead ofuseEffect.This
useEffectduplicates the logic in the existingresetcallback (lines 147-150). More importantly, resetting state in response to a prop change is a well-known case where React'skey-based remounting is the idiomatic, effect-free alternative — the parent simply renders<Component key={collectionUid} ... />and the hook'suseStateinitializers handle the rest.If the
keyapproach isn't feasible here, at minimum deduplicate by callingreset()inside the effect rather than repeating the two setter calls.Based on learnings: "Avoid
useEffectunless absolutely needed. Prefer derived state and event handlers (MUST)."Option A – eliminate the effect entirely (preferred)
Remove the
useEffectfrom this hook and have the consumer add akey:// In the consuming component: <CollectionFolderTree key={collectionUid} collectionUid={collectionUid} />-import { useState, useMemo, useCallback, useEffect } from 'react'; +import { useState, useMemo, useCallback } from 'react';- - useEffect(() => { - setCurrentFolderPath([]); - setSelectedFolderUid(null); - }, [collectionUid]); -Option B – at least deduplicate with `reset`
useEffect(() => { - setCurrentFolderPath([]); - setSelectedFolderUid(null); + reset(); - }, [collectionUid]); + }, [collectionUid, reset]);packages/bruno-app/src/components/Sidebar/Collections/index.js (1)
19-29: Duplicated scratch-collection filtering logic across files.This filter (lines 22-26) is identical to the one in
CollectionsSection/index.js(lines 51-55). Consider extracting a shared utility likeisNonScratchCollection(collection, workspaces)to keep the logic DRY. Not blocking, but worth noting for maintainability.packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js (1)
859-943: Defer workspace scratch state updates until the mount succeeds.
This avoids leavingscratchCollectionUid/scratchTempDirectoryin Redux if any mount step fails.♻️ Suggested refactor
- dispatch(setWorkspaceScratchCollection({ - workspaceUid, - scratchCollectionUid, - scratchTempDirectory: tempDirectoryPath - })); @@ - await dispatch(openScratchCollectionEvent(scratchCollectionUid, tempDirectoryPath, brunoConfig)); + await dispatch(openScratchCollectionEvent(scratchCollectionUid, tempDirectoryPath, brunoConfig)); + dispatch(setWorkspaceScratchCollection({ + workspaceUid, + scratchCollectionUid, + scratchTempDirectory: tempDirectoryPath + }));packages/bruno-app/src/components/RequestTabs/index.js (1)
51-52: Consider memoizing the scratch-collection derivation.
isScratchCollectionandactiveWorkspaceare recomputed on every render with two separate array traversals. This is fine for a small workspace list, but sinceworkspacesselector returns the full array (any workspace mutation triggers re-render), you could wrap these in auseMemokeyed on[workspaces, activeCollection]to avoid redundant work as the feature scales.♻️ Optional memoization
- const isScratchCollection = activeCollection && workspaces.some((w) => w.scratchCollectionUid === activeCollection.uid); - const activeWorkspace = isScratchCollection ? workspaces.find((w) => w.scratchCollectionUid === activeCollection.uid) : null; + const { isScratchCollection, activeWorkspace } = useMemo(() => { + const ws = activeCollection ? workspaces.find((w) => w.scratchCollectionUid === activeCollection.uid) : null; + return { isScratchCollection: !!ws, activeWorkspace: ws }; + }, [workspaces, activeCollection]);This also removes the double traversal by doing a single
find.packages/bruno-app/src/components/SaveTransientRequest/CollectionListItem/index.js (1)
36-38: Use theme color instead of Tailwindtext-green-600.Per project conventions, Tailwind classes are for layout only — colors should come from the styled-component theme. The parent
StyledWrapperor an inline style using the theme should provide the green color.Based on learnings: "Tailwind classes are used specifically for layout-based styles, not colors" and "Use styled component's theme prop to manage CSS colors."
packages/bruno-app/src/components/RequestTabs/WorkspaceHeader/index.js (1)
25-38: MissinghandleCancelWorkspaceRenamein the effect's dependency array.The
handleClickOutsideclosure captureshandleCancelWorkspaceRename, but it's not listed in the[isRenamingWorkspace]dependency array. This works today because the function only calls stable state setters, but a linter would flag this. Either add it to deps (and wrap it inuseCallback), or extract the reset logic inline.packages/bruno-app/src/components/SaveTransientRequest/index.js (3)
76-82: Effect dependency is incomplete —selectedTargetCollectionfields aren't deeply tracked.The effect depends on
[selectedTargetCollectionPath, selectedTargetCollection], butselectedTargetCollectionis derived fromavailableCollectionswhich is a new object reference on each memo recomputation. This means the effect runs more often than necessary, though it's functionally harmless since the body is gated byselectedTargetCollectionPath && isFullyLoaded.Also, per project conventions, consider whether this can be handled as a derived-state check inside
handleSelectCollection+ the CollectionListItem's own state, rather than a separate effect.Based on learnings: "Avoid
useEffectunless absolutely needed. Prefer derived state and event handlers."
111-115:resetFormis not in the effect dependency array.
resetFormcapturesisScratchCollectionanditem.namevia closure but isn't listed as a dependency. IfisScratchCollectionchanges while the modal is open,resetFormwould use a stale value. This is unlikely to cause a user-visible bug since the modal is typically opened fresh, but it will trigger a linter warning.♻️ Quick fix: wrap resetForm in useCallback and add to deps
- const resetForm = () => { + const resetForm = useCallback(() => { setRequestName(item.name || ''); setSearchText(''); reset(); setShowNewFolderInput(false); setNewFolderName(''); setNewFolderDirectoryName(''); setShowFilesystemName(false); setIsEditingFolderFilename(false); setPendingFolderNavigation(null); setSelectedTargetCollectionPath(null); setIsSelectingCollection(isScratchCollection); - }; + }, [item?.name, isScratchCollection, reset]); useEffect(() => { if (isOpen && item) { resetForm(); } - }, [isOpen, item]); + }, [isOpen, item, resetForm]);
337-385: Breadcrumb rendering is duplicated between scratch and non-scratch paths.The breadcrumb JSX (chevrons + clickable names + root navigation) at lines 353-382 is nearly identical to lines 414-441. Extract a shared
BreadcrumbNavcomponent or helper to reduce duplication.Also applies to: 413-442
- Add tests for creating scratch requests (HTTP, GraphQL, gRPC, WebSocket) - Add tests for sending scratch requests and verifying response - Add tests for saving scratch requests to a collection - Add tests for multiple tabs and closing tabs - Handle "Don't Save" modal in playwright fixture during app close
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@playwright/index.ts`:
- Around line 123-131: The code uses dontSaveButton.isVisible({ timeout: 1000 })
but Playwright ignores the timeout option; replace that check with an explicit
wait on the locator (e.g., use dontSaveButton.waitFor({ state: 'visible',
timeout: 1000 }) or page.waitForSelector with the same selector) and handle the
timeout with .catch(() => false) before clicking; update the handleModal
function to await the locator.waitFor call and only click if it resolved to
visible to ensure the modal is reliably detected and clicked.
In `@tests/scratch-requests/scratch-requests.spec.ts`:
- Around line 168-171: The test step uses a macOS-only shortcut ('Meta+s') and
an unnecessary wait; update the step that calls page.keyboard.press inside the
'Trigger save action using keyboard shortcut' test.step to use a platform-aware
key (prefer 'Control+s' or conditionally choose 'Meta+s' on darwin via
process.platform) so the shortcut works on Linux/Windows CI, and remove the
extra await page.waitForTimeout(500) since the following step already waits for
the modal (5s). Locate the test.step block and modify the page.keyboard.press
call and delete the redundant waitForTimeout.
- Around line 250-259: The "Verify tab is closed" test step currently contains
no assertion; locate the test step named 'Verify tab is closed' and replace the
empty if block that checks isVisible (from activeTab =
page.locator('.request-tab.active') and isVisible = await activeTab.isVisible())
with a concrete assertion that the closed tab is no longer present — e.g.,
assert isVisible is false or assert the active tab count does not include the
closed tab id; ensure you use the existing activeTab locator
('.request-tab.active') or the closed tab identifier used earlier in the test to
make the assertion.
🧹 Nitpick comments (5)
tests/scratch-requests/scratch-requests.spec.ts (5)
39-42: Avoidpage.waitForTimeout(300)— prefer a meaningful wait condition.After asserting the active tab contains "Untitled" (line 41), the 300ms sleep is unnecessary. If there's a specific UI transition to wait for, use an
expectorwaitForon that element instead. As per coding guidelines: "Try to reduce usage ofpage.waitForTimeout();in code unless absolutely necessary and the locator cannot be found using existingexpect()playwright calls."Suggested fix
// Wait for the request tab to be active await page.locator('.request-tab.active').waitFor({ state: 'visible' }); await expect(page.locator('.request-tab.active')).toContainText('Untitled'); - await page.waitForTimeout(300); });
49-56: SamewaitForTimeoutconcern ingoToWorkspaceOverview.Replace
page.waitForTimeout(300)with a condition that confirms the workspace overview is actually visible (e.g., waiting for a workspace-specific element). As per coding guidelines, reduce usage ofpage.waitForTimeout().Suggested fix
const goToWorkspaceOverview = async (page: Page) => { await test.step('Navigate to workspace overview', async () => { const homeButton = page.locator('.titlebar-left .home-button'); await homeButton.click(); - await page.waitForTimeout(300); + // Wait for workspace overview to be visible + await page.locator('.workspace-title').waitFor({ state: 'visible' }); }); };
6-9:locatorsis built but barely used — prefer consistent locator usage.
buildCommonLocatorsis initialized inbeforeAllbut used only twice (lines 200, 203). Throughout the file, rawpage.locator('.request-tab.active')appears repeatedly whenlocators.tabs.activeRequestTab()already exists. Same forlocators.tabs.closeTab(...),locators.response.*, etc. Pick one pattern and stick with it. As per coding guidelines: "Use locator variables for locators."Also applies to: 69-71
223-230: Weak assertion for "multiple tabs" — doesn't verify two distinct scratch tabs.
toBeGreaterThanOrEqual(2)is fragile in a serial suite where prior tests may leave tabs. The comment says "different numbers" but there's no assertion on that. Consider filtering to scratch-specific tabs and asserting an exact count or verifying both tab labels.
58-79: Creation tests across all four request types are solid but repetitive.The four creation tests (HTTP, GraphQL, gRPC, WebSocket) follow the same pattern. They work fine, but consider whether a parameterized approach (e.g., a loop or
test.describewith a data array) would reduce duplication without sacrificing readability. Not blocking — just a thought for maintainability.Also applies to: 81-96, 98-113, 115-130
- Fix RequestTabPanel hooks violation by moving SSR guard after hooks - Fix validateWorkspaceName to trim before length check - Use stable deterministic UID in SaveTransientRequest - Use ES6 shorthand for collectionUid in useIpcEvents - Add JSDoc and error handling to openScratchCollectionEvent - Fix closeAllCollectionTabs to preserve activeTabUid when not removed - Add syncExampleUidsCache call to save-scratch-request handler - Use getCollectionFormat for save-transient-request extension handling - Fix Playwright modal handling with proper waitFor pattern - Make keyboard shortcut platform-aware in scratch tests - Remove flaky close tab test, handled by fixture cleanup - Extract isScratchCollection utility to reduce duplication - Memoize scratch collection derivation in RequestTabs - Use theme color instead of Tailwind class for success icon - Wrap resetForm and handleCancelWorkspaceRename in useCallback - Extract FolderBreadcrumbs into separate component - Call reset() inside useEffect in useCollectionFolderTree hook - Defer workspace scratch state updates until mount succeeds
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js`:
- Around line 934-940: The catch block can read stale `workspace` captured
earlier, so change the cleanup to read the current workspace from the Redux
state (via getState()) instead of using the local `workspace` variable;
specifically, in the catch of the async action that calls
setWorkspaceScratchCollection, addTransientDirectory, and
updateCollectionMountStatus, call getState() to obtain the latest workspace and
then use its scratchCollectionUid when dispatching updateCollectionMountStatus
(and any other cleanup) so orphaned scratch state is cleared even if errors
occur after setWorkspaceScratchCollection.
In `@packages/bruno-electron/src/ipc/collection.js`:
- Around line 1959-1961: The call to syncExampleUidsCache is passing
collectionPath (the collection root) instead of the saved request's filepath,
causing UID mappings to point to the wrong location; update the call in the
handler that currently uses collectionPath to pass targetPathname (the file path
of the saved request) as the first argument so
syncExampleUidsCache(collectionPath, request.examples) becomes
syncExampleUidsCache(targetPathname, request.examples), leaving the examples
argument unchanged and ensuring UID mapping matches the saved request file.
- Around line 429-433: syncExampleUidsCache is being called with sourcePathname
(transient path) instead of the actual save destination, so update the two calls
that pass sourcePathname to instead pass targetPathname (the variable used when
saving the file). Specifically, in the branch that sets finalContent =
stringifyRequest(mergedRequest, { format: targetFormat }) and the branch that
awaits stringifyRequestViaWorker(request, { format: targetFormat }), replace
syncExampleUidsCache(sourcePathname, ...) with
syncExampleUidsCache(targetPathname, ...) so the UID cache tracks the permanent
file location.
- Around line 424-425: Remove the redundant inner require that re-declares
parseRequest and stringifyRequest inside the needsConversion branch; instead
reference the top-level imports parseRequest and stringifyRequest already
declared at file scope so you don't shadow them—delete the inner "const {
parseRequest, stringifyRequest } = require('@usebruno/filestore')" and use
parseRequest/stringifyRequest directly within the needsConversion block.
🧹 Nitpick comments (9)
packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js (4)
864-873: Consider using a selector instead of a thunk for pure state reads.
getScratchCollectiononly reads state and returns a value — it never dispatches. A memoized selector (e.g., viareselect) would be more idiomatic and avoids the overhead of dispatching a thunk for a synchronous read.That said, this pattern exists elsewhere in the file, so it's consistent with the current codebase style.
887-894: Inconsistent return shapes.The early-return path (line 892) returns the full Redux collection object, while the fresh-mount path (line 933) returns
{ uid, pathname }. The caller only uses.uidtoday, so it works — but this inconsistency will bite if a future caller expects a uniform shape.Consider returning the same minimal shape from the early-return path:
Proposed fix
if (existingCollection) { - return existingCollection; + return { uid: existingCollection.uid, pathname: existingCollection.pathname }; }Also applies to: 933-933
897-900: Magic string'default'as fallback workspace path.Line 899 falls back to
'default'whenworkspace.pathnameis falsy. If this is a well-known sentinel for the default workspace, consider extracting it as a named constant to make the intent clear and avoid duplication if used elsewhere.
902-903: Dynamic import ofgenerateUidBasedOnHashcould be a static import.This module is loaded on every workspace switch. Since the bundler caches the module after the first load the runtime cost is negligible, but a static import at the top of the file would be simpler and make the dependency explicit.
packages/bruno-app/src/components/RequestTabPanel/index.js (1)
189-195:workspaceEnvironmentsrenders the same component asglobal-environment-settings.Both
focusedTab.type === 'global-environment-settings'(line 181) andfocusedTab.type === 'workspaceEnvironments'(line 193) render<GlobalEnvironmentSettings />. If these are intentionally the same view accessed via different tab types, consider extracting a shared set or adding a brief comment explaining why two distinct tab types map to the same component — helps future maintainers.packages/bruno-app/src/components/RequestTabs/index.js (1)
51-54: Inline scratch-collection check duplicates the sharedisScratchCollectionutility logic.The
isScratchCollectionutility fromutils/collectionschecksworkspaces.some((w) => w.scratchCollectionUid === collection.uid)— same concept as thefindhere. The difference is you also need the workspace object itself. Consider using the utility for the boolean and a separatefindfor the workspace, or extending the utility to return the matched workspace.packages/bruno-app/src/components/SaveTransientRequest/index.js (1)
69-85:selectedTargetCollectionis re-derived every render without memoization, used inuseEffectdeps.Lines 75-77 compute
selectedTargetCollectionvia afindon every render. WhileavailableCollectionsis memoized, the returned object reference can change whenallCollectionsupdates (which is the desired behavior for detecting mount status changes). This is functionally correct, but wrapping it in auseMemowould make the intent clearer and prevent the effect at lines 79-85 from firing on unrelated re-renders.♻️ Optional: memoize selectedTargetCollection
- const selectedTargetCollection = selectedTargetCollectionPath - ? availableCollections.find((c) => (c.path || c.pathname) === selectedTargetCollectionPath) - : null; + const selectedTargetCollection = useMemo(() => { + return selectedTargetCollectionPath + ? availableCollections.find((c) => (c.path || c.pathname) === selectedTargetCollectionPath) + : null; + }, [selectedTargetCollectionPath, availableCollections]);packages/bruno-electron/src/ipc/collection.js (2)
1906-1926: Consider extracting the async-load threshold logic into a shared helper.Lines 1912–1917 duplicate the same
shouldLoadCollectionAsynccomputation fromrenderer:mount-collection(lines 1856–1863). A small helper likeshouldLoadAsync(stats)would eliminate the copy-paste.Example helper extraction
+const shouldLoadAsync = ({ size, filesCount, maxFileSize }) => + (size > MAX_COLLECTION_SIZE_IN_MB) + || (filesCount > MAX_COLLECTION_FILES_COUNT) + || (maxFileSize > MAX_SINGLE_FILE_SIZE_IN_COLLECTION_IN_MB);Then replace both occurrences with
shouldLoadAsync(await getCollectionStats(collectionPath)).
1928-1968: Handler largely duplicatessave-transient-request— consider consolidating.The validation, filename normalization, and write logic in
save-scratch-request(lines 1928–1968) closely mirrorssave-transient-request(lines 393–441), minus the format conversion. If scratch requests always use the target collection's format directly, the two handlers could share a common utility to reduce duplication.
- Create CollectionHeader component that replaces WorkspaceHeader and CollectionToolBar - Add dropdown to switch between workspace and mounted collections - Display tab count badge next to collection/workspace name in header and dropdown - Remove unused WorkspaceHeader and CollectionToolBar components - Handle null/undefined values elegantly throughout
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/bruno-app/src/components/Dropdown/StyledWrapper.js`:
- Around line 88-97: The badge (.dropdown-tab-count) currently uses the same
theme token (props.theme.dropdown.hoverBg) as the dropdown item hover
(.dropdown-item:hover) which makes the badge blend in on hover; update the
styling for .dropdown-tab-count to use a distinct token or adjusted value (e.g.,
props.theme.dropdown.badgeBg or a darken/lighten variant of hoverBg) and/or add
a subtle outline (e.g., 1px solid props.theme.dropdown.badgeBorder or a soft
box-shadow) so the badge remains visually distinct when .dropdown-item:hover is
active.
In `@packages/bruno-app/src/components/RequestTabs/CollectionHeader/index.js`:
- Around line 225-246: The workspace name is being validated trimmed but saved
untrimmed; in handleSaveWorkspaceRename you should trim workspaceNameInput
before dispatching renameWorkspaceAction and before clearing/resetting state:
compute a const trimmed = workspaceNameInput.trim(), use trimmed when calling
dispatch(renameWorkspaceAction(...)) and when setting success messages or
resetting setWorkspaceNameInput(''), and also use trimmed for setting/clearing
setWorkspaceNameError if needed so stored workspace names never include
surrounding whitespace; keep validateWorkspaceName as-is but ensure rename uses
the trimmed value.
🧹 Nitpick comments (3)
packages/bruno-app/src/components/RequestTabs/index.js (1)
50-52: Consider reusing theisScratchCollectionutility fromutils/collections/index.js.There's already an exported utility that does exactly this check. Using it here avoids duplicating the logic and keeps the source of truth in one place.
♻️ Suggested refactor
+import { isScratchCollection as checkIsScratchCollection } from 'utils/collections'; - const isScratchCollection = useMemo(() => { - return activeCollection ? workspaces.some((w) => w.scratchCollectionUid === activeCollection.uid) : false; - }, [workspaces, activeCollection]); + const isScratchCollection = useMemo(() => { + return checkIsScratchCollection(activeCollection, workspaces); + }, [workspaces, activeCollection]);packages/bruno-app/src/components/RequestTabs/CollectionHeader/index.js (2)
82-90:mountedCollectionsfilter re-runs on every render.This filter iterates all collections and workspaces on each render. It's fine for small datasets, but given the learning to "memoize only when necessary," just flagging for awareness — if workspace/collection counts grow, wrapping this in
useMemowith[collections, workspaces, currentWorkspace]deps would be straightforward.
32-449: Large component — consider extracting workspace-specific logic.This component handles two distinct concerns: workspace header (rename, export, close, show in folder) and collection header (runner, variables, settings, environment). Extracting the workspace rename flow and workspace actions into a custom hook or sub-component would improve readability and testability. Not urgent, but worth considering as this feature matures.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/bruno-app/src/components/RequestTabs/CollectionHeader/index.js`:
- Around line 118-134: The tabs.find check in handleSwitchToCollection currently
matches any tab with the same collectionUid and can pick a request/other tab;
update the find predicate (used in the function handleSwitchToCollection) to
require t.type === 'collection-settings' as well so it only focuses an existing
collection-settings tab, and keep the existing addTab call (addTab({... type:
'collection-settings' })) and dispatch(focusTab(...)) behavior unchanged.
🧹 Nitpick comments (3)
packages/bruno-app/src/components/Dropdown/StyledWrapper.js (1)
88-97: Duplicated badge styles with.tab-countin CollectionHeader/StyledWrapper.js.The
font-size,padding,border-radius,min-width, andtext-alignvalues here are identical to the.tab-countclass in the newCollectionHeader/StyledWrapper.js(lines 36-43). Consider extracting a shared CSS mixin or a small reusable badge snippet to keep these in sync.packages/bruno-app/src/components/RequestTabs/CollectionHeader/index.js (2)
82-90: InlinemountedCollectionscomputation runs on every render.This filter chains through
collections,workspaces, andcurrentWorkspace.collectionson each render. For typical workload it's fine, but if the collection/workspace list grows, consider wrapping inuseMemokeyed on[collections, workspaces, currentWorkspace].
1-30: Large import surface — component handles many responsibilities.This component manages workspace switching, renaming, exporting, closing, collection tab navigation, and collection actions (runner, variables, settings). At ~450 lines, it's manageable today but could be a candidate for splitting into
WorkspaceSwitcher+CollectionActionssub-components if it grows further.
…h StyledWrapper - Introduced StyledWrapper component to encapsulate styling for transient request cards. - Updated SaveTransientRequest and ConfirmAppClose modals to utilize StyledWrapper for improved layout and styling consistency. - Enhanced transient request display with better class naming and structure for readability. - Added handling for transient requests in SaveRequestsModal, separating them from regular drafts for clarity.
- Revert playwright/index.ts modal handling hack - Add closeAllTabs helper to test utils for proper tab cleanup - Update scratch-requests test to use closeAllTabs in afterAll - Fix test assertion for new collection header dropdown structure
…uno into feature/scratch-requests
3 participants
JIRA
Description
This PR refactors the scratch requests feature by unifying the tab system and leveraging existing transient request infrastructure. The main goal was to simplify the architecture by removing parallel systems and reusing existing components.
Key Changes
Architecture Simplification
WorkspaceTabssystem and integrated workspace tabs (Overview, Environments) into the existingRequestTabssystemCreateTransientRequestcomponent instead of a customCreateScratchRequestcomponentScratch Collection Approach
/tmp/bruno-scratch-*)workspaceOverview,workspaceEnvironments) are non-closable tabs tied to this scratch collectionSaveTransientRequest Enhancement
Architecture Overview
Contribution Checklist:
scratch-request.mov
Summary by CodeRabbit
New Features
UI Improvements
Bug Fixes